[IntersectionObserver] Deliver non-js callbacks synchronously

For the javascript IntersectionObserver class, when new notifications
are generated, a task is posted to deliver the notifications (i.e.,
run the javascript callback). Because the task is put in the regular
task queue, there's no guarantee when it will run.

For blink-internal users of IntersectionObserver, it doesn't make
sense to post a task to deliver the observations -- and this may
even lead to correctness issues or other bugs. The bug link on this CL
refers to an old bug of this kind which was previously fixed using a
gross but necessary hack.

This patch changes IntersectionObserver so that non-javascript observers
will deliver notifications synchronously as they are generated.

R=chrishtr@chromium.org,mlamouri@chromium.org

Bug: 590856
Change-Id: I2f91f23a01288214209f901bc582717678312dd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1501655
Commit-Queue: Stefan Zager <szager@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637919}

third_party/blink/renderer/bindings/core/v8/v8_intersection_observer_delegate.h

@@ -31,6 +31,10 @@ class V8IntersectionObserverDelegate final
 
   void Trace(blink::Visitor*) override;
 
+  IntersectionObserver::DeliveryBehavior GetDeliveryBehavior() const override {
+    return IntersectionObserver::kPostTaskToDeliver;
+  }
+
   void Deliver(const HeapVector<Member<IntersectionObserverEntry>>&,
                IntersectionObserver&) override;
 

third_party/blink/renderer/core/frame/local_frame_view.cc

@@ -1072,6 +1072,7 @@ void LocalFrameView::RunIntersectionObserverSteps() {
 #if DCHECK_IS_ON()
   DCHECK(was_dirty || !NeedsLayout());
 #endif
+  DeliverSynchronousIntersectionObservations();
 }
 
 LayoutSVGRoot* LocalFrameView::EmbeddedReplacedContent() const {
@@ -3661,6 +3662,9 @@ void LocalFrameView::PaintOutsideOfLifecycle(
     const CullRect& cull_rect) {
   DCHECK(PaintOutsideOfLifecycleIsAllowed(context, *this));
 
+  base::AutoReset<bool> past_layout_lifecycle_resetter(
+      &past_layout_lifecycle_update_, true);
+
   ForAllNonThrottledLocalFrameViews([](LocalFrameView& frame_view) {
     frame_view.Lifecycle().AdvanceTo(DocumentLifecycle::kInPaint);
   });
@@ -3678,6 +3682,9 @@ void LocalFrameView::PaintContentsOutsideOfLifecycle(
     const CullRect& cull_rect) {
   DCHECK(PaintOutsideOfLifecycleIsAllowed(context, *this));
 
+  base::AutoReset<bool> past_layout_lifecycle_resetter(
+      &past_layout_lifecycle_update_, true);
+
   ForAllNonThrottledLocalFrameViews([](LocalFrameView& frame_view) {
     frame_view.Lifecycle().AdvanceTo(DocumentLifecycle::kInPaint);
   });
@@ -3906,6 +3913,17 @@ void LocalFrameView::UpdateViewportIntersectionsForSubtree(
   }
 }
 
+void LocalFrameView::DeliverSynchronousIntersectionObservations() {
+  if (IntersectionObserverController* controller =
+          GetFrame().GetDocument()->GetIntersectionObserverController()) {
+    controller->DeliverIntersectionObservations(
+        IntersectionObserver::kDeliverDuringPostLifecycleSteps);
+  }
+  ForAllChildLocalFrameViews([](LocalFrameView& frame_view) {
+    frame_view.DeliverSynchronousIntersectionObservations();
+  });
+}
+
 void LocalFrameView::UpdateThrottlingStatusForSubtree() {
   if (!GetFrame().GetDocument()->IsActive())
     return;
@@ -3926,10 +3944,6 @@ void LocalFrameView::UpdateThrottlingStatusForSubtree() {
   });
 }
 
-void LocalFrameView::UpdateRenderThrottlingStatusForTesting() {
-  visibility_observer_->DeliverObservationsForTesting();
-}
-
 void LocalFrameView::CrossOriginStatusChanged() {
   // Cross-domain status is not stored as a dirty bit within LocalFrameView,
   // so force-invalidate throttling status when it changes regardless of

third_party/blink/renderer/core/frame/local_frame_view.h

@@ -604,10 +604,6 @@ class CORE_EXPORT LocalFrameView final
   bool IsHiddenForThrottling() const { return hidden_for_throttling_; }
   void SetupRenderThrottling();
 
-  // For testing, run pending intersection observer notifications for this
-  // frame.
-  void UpdateRenderThrottlingStatusForTesting();
-
   void BeginLifecycleUpdates();
 
   // Shorthands of LayoutView's corresponding methods.
@@ -868,6 +864,7 @@ class CORE_EXPORT LocalFrameView final
   void ForAllNonThrottledLocalFrameViews(const Function&);
 
   void UpdateViewportIntersectionsForSubtree(unsigned parent_flags) override;
+  void DeliverSynchronousIntersectionObservations();
 
   void UpdateThrottlingStatusForSubtree();
 

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

@@ -33,8 +33,10 @@ void ElementIntersectionObserverData::RemoveObservation(
 }
 
 void ElementIntersectionObserverData::ComputeObservations(unsigned flags) {
-  for (auto& observation : intersection_observations_)
-    observation.value->Compute(flags);
+  HeapVector<Member<IntersectionObservation>> observations_to_process;
+  CopyValuesToVector(intersection_observations_, observations_to_process);
+  for (auto& observation : observations_to_process)
+    observation->Compute(flags);
 }
 
 void ElementIntersectionObserverData::Trace(blink::Visitor* visitor) {

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

@@ -40,8 +40,12 @@ class IntersectionObserverDelegateImpl final
                                    IntersectionObserver::EventCallback callback)
       : context_(context), callback_(std::move(callback)) {}
 
+  IntersectionObserver::DeliveryBehavior GetDeliveryBehavior() const override {
+    return IntersectionObserver::kDeliverDuringPostLifecycleSteps;
+  }
+
   void Deliver(const HeapVector<Member<IntersectionObserverEntry>>& entries,
-               IntersectionObserver&) override {
+               IntersectionObserver& observer) override {
     callback_.Run(entries);
   }
 
@@ -379,6 +383,11 @@ void IntersectionObserver::SetNeedsDelivery() {
       .ScheduleIntersectionObserverForDelivery(*this);
 }
 
+IntersectionObserver::DeliveryBehavior
+IntersectionObserver::GetDeliveryBehavior() const {
+  return delegate_->GetDeliveryBehavior();
+}
+
 void IntersectionObserver::Deliver() {
   if (!needs_delivery_)
     return;

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

@@ -57,6 +57,15 @@ class CORE_EXPORT IntersectionObserver final
   //                         ////////////////////
   enum ThresholdInterpretation { kFractionOfTarget, kFractionOfRoot };
 
+  // Used to specify when callbacks should be invoked with new notifications.
+  // Blink-internal users of IntersectionObserver will have their callbacks
+  // invoked synchronously at the end of a lifecycle update. Javascript
+  // observers will PostTask to invoke their callbacks.
+  enum DeliveryBehavior {
+    kDeliverDuringPostLifecycleSteps,
+    kPostTaskToDeliver
+  };
+
   static IntersectionObserver* Create(const IntersectionObserverInit*,
                                       IntersectionObserverDelegate&,
                                       ExceptionState&);
@@ -125,6 +134,7 @@ class CORE_EXPORT IntersectionObserver final
   const Length& BottomMargin() const { return bottom_margin_; }
   const Length& LeftMargin() const { return left_margin_; }
   void SetNeedsDelivery();
+  DeliveryBehavior GetDeliveryBehavior() const;
   void Deliver();
 
   // Returns false if this observer has an explicit root element which has been

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

@@ -38,16 +38,20 @@ void IntersectionObserverController::PostTaskToDeliverObservations() {
           FROM_HERE,
           WTF::Bind(
               &IntersectionObserverController::DeliverIntersectionObservations,
-              WrapWeakPersistent(this)));
+              WrapWeakPersistent(this),
+              IntersectionObserver::kPostTaskToDeliver));
 }
 
 void IntersectionObserverController::ScheduleIntersectionObserverForDelivery(
     IntersectionObserver& observer) {
   pending_intersection_observers_.insert(&observer);
-  PostTaskToDeliverObservations();
+  if (observer.GetDeliveryBehavior() ==
+      IntersectionObserver::kPostTaskToDeliver)
+    PostTaskToDeliverObservations();
 }
 
-void IntersectionObserverController::DeliverIntersectionObservations() {
+void IntersectionObserverController::DeliverIntersectionObservations(
+    IntersectionObserver::DeliveryBehavior behavior) {
   ExecutionContext* context = GetExecutionContext();
   if (!context) {
     pending_intersection_observers_.clear();
@@ -56,10 +60,14 @@ void IntersectionObserverController::DeliverIntersectionObservations() {
   // TODO(yukishiino): Remove this CHECK once https://crbug.com/809784 gets
   // resolved.
   CHECK(!context->IsContextDestroyed());
-  CopyToVector(pending_intersection_observers_,
-               intersection_observers_being_invoked_);
-  for (auto& observer : intersection_observers_being_invoked_)
+  for (auto& observer : pending_intersection_observers_) {
+    if (observer->GetDeliveryBehavior() == behavior)
+      intersection_observers_being_invoked_.push_back(observer);
+  }
+  for (auto& observer : intersection_observers_being_invoked_) {
+    pending_intersection_observers_.erase(observer);
     observer->Deliver();
+  }
   intersection_observers_being_invoked_.clear();
 }
 
@@ -69,7 +77,9 @@ void IntersectionObserverController::ComputeTrackedIntersectionObservations(
     TRACE_EVENT0("blink",
                  "IntersectionObserverController::"
                  "computeTrackedIntersectionObservations");
-    for (auto& element : tracked_observation_targets_)
+    HeapVector<Member<Element>> elements_to_process;
+    CopyToVector(tracked_observation_targets_, elements_to_process);
+    for (auto& element : elements_to_process)
       element->ComputeIntersectionObservations(flags);
   }
 }

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

@@ -32,7 +32,11 @@ class IntersectionObserverController
   virtual ~IntersectionObserverController();
 
   void ScheduleIntersectionObserverForDelivery(IntersectionObserver&);
-  void DeliverIntersectionObservations();
+
+  // Immediately deliver all notifications for all observers for which
+  // (observer->GetDeliveryBehavior() == behavior).
+  void DeliverIntersectionObservations(
+      IntersectionObserver::DeliveryBehavior behavior);
 
   // The flags argument is composed of values from
   // IntersectionObservation::ComputeFlags. They are dirty bits that control

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

@@ -5,6 +5,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_INTERSECTION_OBSERVER_INTERSECTION_OBSERVER_DELEGATE_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_INTERSECTION_OBSERVER_INTERSECTION_OBSERVER_DELEGATE_H_
 
+#include "third_party/blink/renderer/core/intersection_observer/intersection_observer.h"
 #include "third_party/blink/renderer/platform/bindings/name_client.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 
@@ -19,6 +20,10 @@ class IntersectionObserverDelegate
       public NameClient {
  public:
   virtual ~IntersectionObserverDelegate() = default;
+
+  virtual IntersectionObserver::DeliveryBehavior GetDeliveryBehavior()
+      const = 0;
+
   virtual void Deliver(const HeapVector<Member<IntersectionObserverEntry>>&,
                        IntersectionObserver&) = 0;
   virtual ExecutionContext* GetExecutionContext() const = 0;

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

@@ -3,9 +3,9 @@
 // found in the LICENSE file.
 
 #include "third_party/blink/renderer/core/intersection_observer/intersection_observer_entry.h"
-#include "third_party/blink/renderer/core/geometry/dom_rect_read_only.h"
 
 #include "third_party/blink/renderer/core/dom/element.h"
+#include "third_party/blink/renderer/core/geometry/dom_rect_read_only.h"
 
 namespace blink {
 

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

@@ -26,6 +26,12 @@ class TestIntersectionObserverDelegate : public IntersectionObserverDelegate {
  public:
   TestIntersectionObserverDelegate(Document& document)
       : document_(document), call_count_(0) {}
+  // TODO(szager): Add tests for the synchronous delivery code path. There is
+  // already some indirect coverage by unit tests exercising features that rely
+  // on it, but we should have some direct coverage in here.
+  IntersectionObserver::DeliveryBehavior GetDeliveryBehavior() const override {
+    return IntersectionObserver::kPostTaskToDeliver;
+  }
   void Deliver(const HeapVector<Member<IntersectionObserverEntry>>& entries,
                IntersectionObserver&) override {
     call_count_++;

third_party/blink/renderer/core/scheduler/frame_throttling_test.cc

@@ -1041,57 +1041,21 @@ TEST_P(FrameThrottlingTest, ThrottleSubtreeAtomically) {
       "<iframe id=child-frame sandbox src=child-iframe.html></iframe>");
   child_frame_resource.Complete("");
 
-  // Move both frames offscreen, but don't run the intersection observers yet.
+  // Move both frames offscreen. IntersectionObservers will run during
+  // post-lifecycle steps and synchronously update throttling status.
   auto* frame_element =
       ToHTMLIFrameElement(GetDocument().getElementById("frame"));
   auto* child_frame_element = ToHTMLIFrameElement(
       frame_element->contentDocument()->getElementById("child-frame"));
   frame_element->setAttribute(kStyleAttr, "transform: translateY(480px)");
   Compositor().BeginFrame();
-  EXPECT_FALSE(
-      frame_element->contentDocument()->View()->CanThrottleRendering());
-  EXPECT_FALSE(
-      child_frame_element->contentDocument()->View()->CanThrottleRendering());
-
-  // Only run the intersection observer for the parent frame. Both frames
-  // should immediately become throttled. This simulates the case where a task
-  // such as BeginMainFrame runs in the middle of dispatching intersection
-  // observer notifications.
-  frame_element->contentDocument()
-      ->View()
-      ->UpdateRenderThrottlingStatusForTesting();
-  EXPECT_TRUE(frame_element->contentDocument()->View()->CanThrottleRendering());
-  EXPECT_TRUE(
-      child_frame_element->contentDocument()->View()->CanThrottleRendering());
-
-  // Both frames should still be throttled after the second notification.
-  child_frame_element->contentDocument()
-      ->View()
-      ->UpdateRenderThrottlingStatusForTesting();
   EXPECT_TRUE(frame_element->contentDocument()->View()->CanThrottleRendering());
   EXPECT_TRUE(
       child_frame_element->contentDocument()->View()->CanThrottleRendering());
 
-  // Move the frame back on screen but don't update throttling yet.
+  // Move the frame back on screen.
   frame_element->setAttribute(kStyleAttr, "transform: translateY(0px)");
   Compositor().BeginFrame();
-  EXPECT_TRUE(frame_element->contentDocument()->View()->CanThrottleRendering());
-  EXPECT_TRUE(
-      child_frame_element->contentDocument()->View()->CanThrottleRendering());
-
-  // Update throttling for the child. It should remain throttled because the
-  // parent is still throttled.
-  child_frame_element->contentDocument()
-      ->View()
-      ->UpdateRenderThrottlingStatusForTesting();
-  EXPECT_TRUE(frame_element->contentDocument()->View()->CanThrottleRendering());
-  EXPECT_TRUE(
-      child_frame_element->contentDocument()->View()->CanThrottleRendering());
-
-  // Updating throttling on the parent should unthrottle both frames.
-  frame_element->contentDocument()
-      ->View()
-      ->UpdateRenderThrottlingStatusForTesting();
   EXPECT_FALSE(
       frame_element->contentDocument()->View()->CanThrottleRendering());
   EXPECT_FALSE(

third_party/blink/renderer/core/testing/sim/sim_compositor.cc

@@ -57,9 +57,14 @@ SimCanvas::Commands SimCompositor::BeginFrame(double time_delta_in_seconds) {
 
   last_frame_time_ += base::TimeDelta::FromSecondsD(time_delta_in_seconds);
 
+  SimCanvas::Commands commands;
+  paint_commands_ = &commands;
+
   layer_tree_view_->layer_tree_host()->Composite(last_frame_time_,
                                                  /*raster=*/false);
-  return PaintFrame();
+
+  paint_commands_ = nullptr;
+  return commands;
 }
 
 SimCanvas::Commands SimCompositor::PaintFrame() {
@@ -96,6 +101,7 @@ void SimCompositor::BeginMainFrame(base::TimeTicks frame_time) {
   web_view_->MainFrameWidget()->BeginFrame(last_frame_time_, false);
   web_view_->MainFrameWidget()->UpdateAllLifecyclePhases(
       WebWidget::LifecycleUpdateReason::kTest);
+  *paint_commands_ = PaintFrame();
 }
 
 }  // namespace blink

third_party/blink/renderer/core/testing/sim/sim_compositor.h

@@ -107,6 +107,8 @@ class SimCompositor final : public content::StubLayerTreeViewDelegate {
 
   base::TimeTicks last_frame_time_;
 
+  SimCanvas::Commands* paint_commands_;
+
   std::unique_ptr<cc::ScopedDeferMainFrameUpdate>
       scoped_defer_main_frame_update_;
 };