Tighten up conditions for IntersectionObserver tracking

The root cause of the linked bug is that IntersectionObserverController
continues to track an observer after the observer's root element is
deleted.

This CL tightens up the conditions for which objects are tracked:
  - Explicit root observers with observations, if the root is connected
  - Implicit root observations, if the target is connected

BUG=1046897
R=vmpstr@chromium.org

Change-Id: Ib98a0dd3c948e61200cd270011cfe18b8776d7fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068860Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744795}

third_party/blink/renderer/core/dom/element.cc

@@ -2734,7 +2734,7 @@ Node::InsertionNotificationRequest Element::InsertedInto(
   if (!insertion_point.IsInTreeScope())
     return kInsertionDone;
 
-  if (HasRareData()) {
+  if (isConnected() && HasRareData()) {
     ElementRareData* rare_data = GetElementRareData();
     if (ElementIntersectionObserverData* observer_data =
             rare_data->IntersectionObserverData()) {
@@ -2749,10 +2749,8 @@ Node::InsertionNotificationRequest Element::InsertedInto(
       }
     }
 
-    if (isConnected()) {
-      if (auto* context = rare_data->GetDisplayLockContext())
-        context->ElementConnected();
-    }
+    if (auto* context = rare_data->GetDisplayLockContext())
+      context->ElementConnected();
   }
 
   if (isConnected()) {

third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.cc

@@ -44,27 +44,18 @@ void ElementIntersectionObserverData::RemoveObserver(
 
 void ElementIntersectionObserverData::TrackWithController(
     IntersectionObserverController& controller) {
-  for (auto& entry : observations_) {
-    if (entry.key->RootIsImplicit()) {
-      controller.AddTrackedObservation(*entry.value,
-                                       entry.key->trackVisibility());
-    }
-  }
-  for (auto& observer : observers_) {
-    controller.AddTrackedObserver(*observer, observer->trackVisibility());
-  }
+  for (auto& entry : observations_)
+    controller.AddTrackedObservation(*entry.value);
+  for (auto& observer : observers_)
+    controller.AddTrackedObserver(*observer);
 }
 
 void ElementIntersectionObserverData::StopTrackingWithController(
     IntersectionObserverController& controller) {
-  for (auto& entry : observations_) {
-    if (entry.key->RootIsImplicit()) {
-      controller.RemoveTrackedObservation(*entry.value);
-    }
-  }
-  for (auto& observer : observers_) {
+  for (auto& entry : observations_)
+    controller.RemoveTrackedObservation(*entry.value);
+  for (auto& observer : observers_)
     controller.RemoveTrackedObserver(*observer);
-  }
 }
 
 bool ElementIntersectionObserverData::ComputeIntersectionsForTarget(

third_party/blink/renderer/core/intersection_observer/intersection_observation.cc

@@ -77,11 +77,7 @@ void IntersectionObservation::Disconnect() {
     ElementIntersectionObserverData* observer_data =
         target_->IntersectionObserverData();
     observer_data->RemoveObservation(*this);
-    // We track connected elements that are either the root of an explicit root
-    // observer, or the target of an implicit root observer. If the target was
-    // previously being tracked, but no longer needs to be tracked, then remove
-    // it.
-    if (target_->isConnected() && Observer()->RootIsImplicit()) {
+    if (target_->isConnected()) {
       IntersectionObserverController* controller =
           target_->GetDocument().GetIntersectionObserverController();
       if (controller)

third_party/blink/renderer/core/intersection_observer/intersection_observer.cc

@@ -290,9 +290,6 @@ IntersectionObserver::IntersectionObserver(
       DCHECK(root->IsElementNode());
       To<Element>(root)->EnsureIntersectionObserverData().AddObserver(*this);
     }
-    root->GetDocument()
-        .EnsureIntersectionObserverController()
-        .AddTrackedObserver(*this, track_visibility);
   }
 }
 
@@ -328,12 +325,16 @@ void IntersectionObserver::observe(Element* target,
       MakeGarbageCollected<IntersectionObservation>(*this, *target);
   target->EnsureIntersectionObserverData().AddObservation(*observation);
   observations_.insert(observation);
+  if (root() && root()->isConnected()) {
+    root()
+        ->GetDocument()
+        .EnsureIntersectionObserverController()
+        .AddTrackedObserver(*this);
+  }
   if (target->isConnected()) {
-    if (RootIsImplicit()) {
-      target->GetDocument()
-          .EnsureIntersectionObserverController()
-          .AddTrackedObservation(*observation, track_visibility_);
-    }
+    target->GetDocument()
+        .EnsureIntersectionObserverController()
+        .AddTrackedObservation(*observation);
     if (LocalFrameView* frame_view = target_frame->View()) {
       // The IntersectionObsever spec requires that at least one observation
       // be recorded after observe() is called, even if the frame is throttled.
@@ -362,12 +363,24 @@ void IntersectionObserver::unobserve(Element* target,
 
   observation->Disconnect();
   observations_.erase(observation);
+  if (root() && root()->isConnected() && observations_.IsEmpty()) {
+    root()
+        ->GetDocument()
+        .EnsureIntersectionObserverController()
+        .RemoveTrackedObserver(*this);
+  }
 }
 
 void IntersectionObserver::disconnect(ExceptionState& exception_state) {
   for (auto& observation : observations_)
     observation->Disconnect();
   observations_.clear();
+  if (root() && root()->isConnected()) {
+    root()
+        ->GetDocument()
+        .EnsureIntersectionObserverController()
+        .RemoveTrackedObserver(*this);
+  }
 }
 
 HeapVector<Member<IntersectionObserverEntry>> IntersectionObserver::takeRecords(

third_party/blink/renderer/core/intersection_observer/intersection_observer.h

@@ -129,6 +129,7 @@ class CORE_EXPORT IntersectionObserver final
   // root just because root_ is null.  Hence root_is_implicit_.
   bool RootIsImplicit() const { return root_is_implicit_; }
 
+  bool HasObservations() const { return !observations_.IsEmpty(); }
   bool AlwaysReportRootBounds() const { return always_report_root_bounds_; }
   bool NeedsOcclusionTracking() const {
     return trackVisibility() && !observations_.IsEmpty();

third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.cc

@@ -67,12 +67,15 @@ bool IntersectionObserverController::ComputeIntersections(unsigned flags) {
                  "IntersectionObserverController::"
                  "computeIntersections");
     HeapVector<Member<IntersectionObserver>> observers_to_process;
-    CopyToVector(explicit_root_observers_, observers_to_process);
+    CopyToVector(tracked_explicit_root_observers_, observers_to_process);
     for (auto& observer : observers_to_process) {
-      needs_occlusion_tracking_ |= observer->ComputeIntersections(flags);
+      if (observer->HasObservations())
+        needs_occlusion_tracking_ |= observer->ComputeIntersections(flags);
+      else
+        tracked_explicit_root_observers_.erase(observer);
     }
     HeapVector<Member<IntersectionObservation>> observations_to_process;
-    CopyToVector(implicit_root_observations_, observations_to_process);
+    CopyToVector(tracked_implicit_root_observations_, observations_to_process);
     for (auto& observation : observations_to_process) {
       observation->ComputeIntersection(flags);
       needs_occlusion_tracking_ |= observation->Observer()->trackVisibility();
@@ -82,57 +85,65 @@ bool IntersectionObserverController::ComputeIntersections(unsigned flags) {
 }
 
 void IntersectionObserverController::AddTrackedObserver(
-    IntersectionObserver& observer,
-    bool track_occlusion) {
-  DCHECK(!observer.RootIsImplicit());
-  explicit_root_observers_.insert(&observer);
-  if (!track_occlusion)
+    IntersectionObserver& observer) {
+  // We only track explicit-root observers that have active observations.
+  if (observer.RootIsImplicit() || !observer.HasObservations())
     return;
-  needs_occlusion_tracking_ = true;
-  if (LocalFrameView* frame_view = observer.root()->GetDocument().View()) {
-    if (FrameOwner* frame_owner = frame_view->GetFrame().Owner()) {
-      // Set this bit as early as possible, rather than waiting for a lifecycle
-      // update to recompute it.
-      frame_owner->SetNeedsOcclusionTracking(true);
+  tracked_explicit_root_observers_.insert(&observer);
+  if (observer.trackVisibility()) {
+    needs_occlusion_tracking_ = true;
+    if (LocalFrameView* frame_view = observer.root()->GetDocument().View()) {
+      if (FrameOwner* frame_owner = frame_view->GetFrame().Owner()) {
+        // Set this bit as early as possible, rather than waiting for a
+        // lifecycle update to recompute it.
+        frame_owner->SetNeedsOcclusionTracking(true);
+      }
     }
   }
 }
 
 void IntersectionObserverController::RemoveTrackedObserver(
     IntersectionObserver& observer) {
-  DCHECK(!observer.RootIsImplicit());
+  if (observer.RootIsImplicit())
+    return;
   // Note that we don't try to opportunistically turn off the 'needs occlusion
   // tracking' bit here, like the way we turn it on in AddTrackedObserver. The
   // bit will get recomputed on the next lifecycle update; there's no
   // compelling reason to do it here, so we avoid the iteration through
   // observers and observations here.
-  explicit_root_observers_.erase(&observer);
+  tracked_explicit_root_observers_.erase(&observer);
 }
 
 void IntersectionObserverController::AddTrackedObservation(
-    IntersectionObservation& observation,
-    bool track_occlusion) {
-  DCHECK(observation.Observer()->RootIsImplicit());
-  implicit_root_observations_.insert(&observation);
-  if (!track_occlusion)
+    IntersectionObservation& observation) {
+  IntersectionObserver* observer = observation.Observer();
+  DCHECK(observer);
+  if (!observer->RootIsImplicit())
     return;
-  needs_occlusion_tracking_ = true;
-  if (LocalFrameView* frame_view = observation.Target()->GetDocument().View()) {
-    if (FrameOwner* frame_owner = frame_view->GetFrame().Owner()) {
-      frame_owner->SetNeedsOcclusionTracking(true);
+  tracked_implicit_root_observations_.insert(&observation);
+  if (observer->trackVisibility()) {
+    needs_occlusion_tracking_ = true;
+    if (LocalFrameView* frame_view =
+            observation.Target()->GetDocument().View()) {
+      if (FrameOwner* frame_owner = frame_view->GetFrame().Owner()) {
+        frame_owner->SetNeedsOcclusionTracking(true);
+      }
     }
   }
 }
 
 void IntersectionObserverController::RemoveTrackedObservation(
     IntersectionObservation& observation) {
-  DCHECK(observation.Observer()->RootIsImplicit());
-  implicit_root_observations_.erase(&observation);
+  IntersectionObserver* observer = observation.Observer();
+  DCHECK(observer);
+  if (!observer->RootIsImplicit())
+    return;
+  tracked_implicit_root_observations_.erase(&observation);
 }
 
 void IntersectionObserverController::Trace(Visitor* visitor) {
-  visitor->Trace(explicit_root_observers_);
-  visitor->Trace(implicit_root_observations_);
+  visitor->Trace(tracked_explicit_root_observers_);
+  visitor->Trace(tracked_implicit_root_observations_);
   visitor->Trace(pending_intersection_observers_);
   ExecutionContextClient::Trace(visitor);
 }

third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.h

@@ -43,8 +43,8 @@ class IntersectionObserverController
 
   // The second argument indicates whether the Element is a target of any
   // observers for which observer->trackVisibility() is true.
-  void AddTrackedObserver(IntersectionObserver&, bool);
-  void AddTrackedObservation(IntersectionObservation&, bool);
+  void AddTrackedObserver(IntersectionObserver&);
+  void AddTrackedObservation(IntersectionObservation&);
   void RemoveTrackedObserver(IntersectionObserver&);
   void RemoveTrackedObservation(IntersectionObservation&);
 
@@ -56,20 +56,23 @@ class IntersectionObserverController
   }
 
   unsigned GetTrackedObserverCountForTesting() const {
-    return explicit_root_observers_.size();
+    return tracked_explicit_root_observers_.size();
   }
   unsigned GetTrackedObservationCountForTesting() const {
-    return implicit_root_observations_.size();
+    return tracked_implicit_root_observations_.size();
   }
 
  private:
   void PostTaskToDeliverNotifications();
 
  private:
-  // IntersectionObserver's with an explicit root in this document.
-  HeapHashSet<WeakMember<IntersectionObserver>> explicit_root_observers_;
-  // IntersectionObservations with an implicit root and target in this document.
-  HeapHashSet<WeakMember<IntersectionObservation>> implicit_root_observations_;
+  // IntersectionObserver's with a connected explicit root in this document.
+  HeapHashSet<WeakMember<IntersectionObserver>>
+      tracked_explicit_root_observers_;
+  // IntersectionObservations with an implicit root and connected target in this
+  // document.
+  HeapHashSet<WeakMember<IntersectionObservation>>
+      tracked_implicit_root_observations_;
   // IntersectionObservers for which this is the execution context of the
   // callback, and with unsent notifications.
   HeapHashSet<Member<IntersectionObserver>> pending_intersection_observers_;

third_party/blink/renderer/core/intersection_observer/intersection_observer_test.cc

@@ -488,19 +488,14 @@ TEST_F(IntersectionObserverTest, TrackedTargetBookkeeping) {
   Element* target = GetDocument().getElementById("target");
   ASSERT_TRUE(target);
   IntersectionObserverInit* observer_init = IntersectionObserverInit::Create();
-  DummyExceptionStateForTesting exception_state;
   TestIntersectionObserverDelegate* observer_delegate =
       MakeGarbageCollected<TestIntersectionObserverDelegate>(GetDocument());
-  IntersectionObserver* observer1 = IntersectionObserver::Create(
-      observer_init, *observer_delegate, exception_state);
-  ASSERT_FALSE(exception_state.HadException());
-  observer1->observe(target, exception_state);
-  ASSERT_FALSE(exception_state.HadException());
-  IntersectionObserver* observer2 = IntersectionObserver::Create(
-      observer_init, *observer_delegate, exception_state);
-  ASSERT_FALSE(exception_state.HadException());
-  observer2->observe(target, exception_state);
-  ASSERT_FALSE(exception_state.HadException());
+  IntersectionObserver* observer1 =
+      IntersectionObserver::Create(observer_init, *observer_delegate);
+  observer1->observe(target);
+  IntersectionObserver* observer2 =
+      IntersectionObserver::Create(observer_init, *observer_delegate);
+  observer2->observe(target);
 
   ElementIntersectionObserverData* target_data =
       target->IntersectionObserverData();
@@ -509,14 +504,17 @@ TEST_F(IntersectionObserverTest, TrackedTargetBookkeeping) {
       GetDocument().EnsureIntersectionObserverController();
   EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 2u);
   EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 0u);
-  observer1->unobserve(target, exception_state);
-  ASSERT_FALSE(exception_state.HadException());
+
+  target->remove();
+  EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 0u);
+  GetDocument().body()->AppendChild(target);
+  EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 2u);
+
+  observer1->unobserve(target);
   EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 1u);
-  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 0u);
-  observer2->unobserve(target, exception_state);
-  ASSERT_FALSE(exception_state.HadException());
+
+  observer2->unobserve(target);
   EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 0u);
-  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 0u);
 }
 
 TEST_F(IntersectionObserverTest, TrackedRootBookkeeping) {
@@ -544,15 +542,14 @@ TEST_F(IntersectionObserverTest, TrackedRootBookkeeping) {
   Persistent<IntersectionObserver> observer =
       IntersectionObserver::Create(observer_init, *observer_delegate);
 
-  // For an explicit-root observer, the root element is tracked, even when it
-  // has no observations.
+  // For an explicit-root observer, the root element is tracked only when it
+  // has observations and is connected. Target elements are not tracked.
   ElementIntersectionObserverData* root_data = root->IntersectionObserverData();
   ASSERT_TRUE(root_data);
   EXPECT_FALSE(root_data->IsEmpty());
-  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 1u);
+  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 0u);
   EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 0u);
 
-  // For an explicit-root observer, target elements are not tracked.
   observer->observe(target);
   ElementIntersectionObserverData* target_data =
       target->IntersectionObserverData();
@@ -561,9 +558,23 @@ TEST_F(IntersectionObserverTest, TrackedRootBookkeeping) {
   EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 1u);
   EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 0u);
 
-  // The existing observation should keep the observer alive and active, even
-  // when there are no other references to the observer.
-  observer = nullptr;
+  // Root should not be tracked if it's not connected.
+  root->remove();
+  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 0u);
+  GetDocument().body()->AppendChild(root);
+  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 1u);
+
+  // Root should not be tracked if it has no observations.
+  observer->disconnect();
+  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 0u);
+  observer->observe(target);
+  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 1u);
+  observer->unobserve(target);
+  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 0u);
+  observer->observe(target);
+  EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 1u);
+
+  // The existing observation should keep the observer alive and active.
   // Flush any pending notifications, which hold a hard reference to the
   // observer and can prevent it from being gc'ed. The observation will be the
   // only thing keeping the observer alive.
@@ -578,8 +589,8 @@ TEST_F(IntersectionObserverTest, TrackedRootBookkeeping) {
   EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 0u);
 
   // When the last observation is disconnected, as a result of the target being
-  // gc'ed, the observer should also get gc'ed and the root element should no
-  // longer be tracked.
+  // gc'ed, the root element should no longer be tracked after the next
+  // lifecycle update.
   target->remove();
   target = nullptr;
   target_data = nullptr;
@@ -590,10 +601,18 @@ TEST_F(IntersectionObserverTest, TrackedRootBookkeeping) {
   V8GCController::CollectAllGarbageForTesting(
       v8::Isolate::GetCurrent(),
       v8::EmbedderHeapTracer::EmbedderStackState::kEmpty);
-  EXPECT_TRUE(root_data->IsEmpty());
+  Compositor().BeginFrame();
   EXPECT_EQ(controller.GetTrackedObserverCountForTesting(), 0u);
   EXPECT_EQ(controller.GetTrackedObservationCountForTesting(), 0u);
 
+  // Removing the last reference to the observer should allow it to be dropeed
+  // from the root's ElementIntersectionObserverData.
+  observer = nullptr;
+  V8GCController::CollectAllGarbageForTesting(
+      v8::Isolate::GetCurrent(),
+      v8::EmbedderHeapTracer::EmbedderStackState::kEmpty);
+  EXPECT_TRUE(root_data->IsEmpty());
+
   target = GetDocument().getElementById("target2");
   observer = IntersectionObserver::Create(observer_init, *observer_delegate);
   observer->observe(target);