[scroll-timeline] only resolve scroll source at construction

Previously scroll source was resolved anytime it was accessed. However
this is problematic in two ways:
- The Document.scrollingElement can change so we could resolve to a different
  node if that happens. However Attach/Detach process expect the resolved node
  to be the same and violating this invariant can cause unexpected behavior.
- Resolution process can trigger layout which may not be allowed during
  detachment (see issue 890143)


This patch changes this behavior so that the scroll source is only resolved at
ScrollTimeline construction and the resolved node is used for the lifetime of
scroll timeline.

Bug: 890143
Change-Id: I8a745385e22e2c0718a5f9cade763f5eb2174c83
Reviewed-on: https://chromium-review.googlesource.com/c/1342677Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610187}

third_party/blink/renderer/core/animation/scroll_timeline.cc

@@ -60,6 +60,16 @@ bool StringToScrollOffset(String scroll_offset, CSSPrimitiveValue** result) {
   *result = value->IsIdentifierValue() ? nullptr : ToCSSPrimitiveValue(value);
   return true;
 }
+
+// Note that the resolution process may trigger document lifecycle to clean
+// style and layout.
+Node* ResolveScrollSource(Element* scroll_source) {
+  // When in quirks mode we need the style to be clean, so we don't use
+  // |ScrollingElementNoLayout|.
+  if (scroll_source == scroll_source->GetDocument().scrollingElement())
+    return &scroll_source->GetDocument();
+  return scroll_source;
+}
 }  // namespace
 
 ScrollTimeline* ScrollTimeline::Create(Document& document,
@@ -108,6 +118,7 @@ ScrollTimeline::ScrollTimeline(Element* scroll_source,
                                CSSPrimitiveValue* end_scroll_offset,
                                double time_range)
     : scroll_source_(scroll_source),
+      resolved_scroll_source_(ResolveScrollSource(scroll_source_)),
       orientation_(orientation),
       start_scroll_offset_(start_scroll_offset),
       end_scroll_offset_(end_scroll_offset),
@@ -119,7 +130,7 @@ double ScrollTimeline::currentTime(bool& is_null) {
   is_null = true;
   // 1. If scrollSource does not currently have a CSS layout box, or if its
   // layout box is not a scroll container, return an unresolved time value.
-  LayoutBox* layout_box = ResolvedScrollSource()->GetLayoutBox();
+  LayoutBox* layout_box = resolved_scroll_source_->GetLayoutBox();
   if (!layout_box || !layout_box->HasOverflowClip()) {
     return std::numeric_limits<double>::quiet_NaN();
   }
@@ -205,13 +216,6 @@ void ScrollTimeline::timeRange(DoubleOrScrollTimelineAutoKeyword& result) {
   result.SetDouble(time_range_);
 }
 
-Node* ScrollTimeline::ResolvedScrollSource() const {
-  // When in quirks mode we need the style to be clean, so we don't use
-  // |ScrollingElementNoLayout|.
-  if (scroll_source_ == scroll_source_->GetDocument().scrollingElement())
-    return &scroll_source_->GetDocument();
-  return scroll_source_;
-}
 
 void ScrollTimeline::GetCurrentAndMaxOffset(const LayoutBox* layout_box,
                                             double& current_offset,
@@ -288,11 +292,10 @@ void ScrollTimeline::ResolveScrollStartAndEnd(
 }
 
 void ScrollTimeline::AttachAnimation() {
-  Node* resolved_scroll_source = ResolvedScrollSource();
-  GetActiveScrollTimelineSet().insert(resolved_scroll_source);
-  if (resolved_scroll_source->IsElementNode())
-    ToElement(resolved_scroll_source)->SetNeedsCompositingUpdate();
-  resolved_scroll_source->GetDocument()
+  GetActiveScrollTimelineSet().insert(resolved_scroll_source_);
+  if (resolved_scroll_source_->IsElementNode())
+    ToElement(resolved_scroll_source_)->SetNeedsCompositingUpdate();
+  resolved_scroll_source_->GetDocument()
       .GetLayoutView()
       ->Compositor()
       ->SetNeedsCompositingUpdate(kCompositingUpdateRebuildTree);
@@ -304,11 +307,10 @@ void ScrollTimeline::AttachAnimation() {
 }
 
 void ScrollTimeline::DetachAnimation() {
-  Node* resolved_scroll_source = ResolvedScrollSource();
-  GetActiveScrollTimelineSet().erase(resolved_scroll_source);
-  if (resolved_scroll_source->IsElementNode())
-    ToElement(resolved_scroll_source)->SetNeedsCompositingUpdate();
-  auto* layout_view = resolved_scroll_source->GetDocument().GetLayoutView();
+  GetActiveScrollTimelineSet().erase(resolved_scroll_source_);
+  if (resolved_scroll_source_->IsElementNode())
+    ToElement(resolved_scroll_source_)->SetNeedsCompositingUpdate();
+  auto* layout_view = resolved_scroll_source_->GetDocument().GetLayoutView();
   if (layout_view && layout_view->Compositor()) {
     layout_view->Compositor()->SetNeedsCompositingUpdate(
         kCompositingUpdateRebuildTree);
@@ -323,6 +325,7 @@ void ScrollTimeline::DetachAnimation() {
 
 void ScrollTimeline::Trace(blink::Visitor* visitor) {
   visitor->Trace(scroll_source_);
+  visitor->Trace(resolved_scroll_source_);
   visitor->Trace(start_scroll_offset_);
   visitor->Trace(end_scroll_offset_);
   AnimationTimeline::Trace(visitor);

third_party/blink/renderer/core/animation/scroll_timeline.h

@@ -53,8 +53,7 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline {
   // Returns the Node that should actually have the ScrollableArea (if one
   // exists). This can differ from |scrollSource| when |scroll_source_| is the
   // Document's scrollingElement.
-  Node* ResolvedScrollSource() const;
-
+  Node* ResolvedScrollSource() const { return resolved_scroll_source_; }
   ScrollDirection GetOrientation() const { return orientation_; }
 
   void GetCurrentAndMaxOffset(const LayoutBox*,
@@ -86,7 +85,11 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline {
                  CSSPrimitiveValue*,
                  double);
 
+  // Use |scroll_source_| only to implement the web-exposed API but use
+  // resolved_scroll_source_ to actually access the scroll related properties.
   Member<Element> scroll_source_;
+  Member<Node> resolved_scroll_source_;
+
   ScrollDirection orientation_;
   Member<CSSPrimitiveValue> start_scroll_offset_;
   Member<CSSPrimitiveValue> end_scroll_offset_;

third_party/blink/renderer/core/animation/scroll_timeline_test.cc

@@ -159,4 +159,65 @@ TEST_F(ScrollTimelineTest,
   scroll_timeline->currentTime(current_time_is_null);
   EXPECT_TRUE(current_time_is_null);
 }
+
+TEST_F(ScrollTimelineTest,
+       UsingDocumentScrollingElementShouldCorrectlyResolveToDocument) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #content { width: 10000px; height: 10000px; }
+    </style>
+    <div id='content'></div>
+  )HTML");
+
+  EXPECT_EQ(GetDocument().documentElement(), GetDocument().scrollingElement());
+  // Create the ScrollTimeline with Document.scrollingElement() as source. The
+  // resolved scroll source should be the Document.
+  ScrollTimelineOptions* options = ScrollTimelineOptions::Create();
+  DoubleOrScrollTimelineAutoKeyword time_range =
+      DoubleOrScrollTimelineAutoKeyword::FromDouble(100);
+  options->setTimeRange(time_range);
+  options->setScrollSource(GetDocument().scrollingElement());
+  ScrollTimeline* scroll_timeline =
+      ScrollTimeline::Create(GetDocument(), options, ASSERT_NO_EXCEPTION);
+  EXPECT_EQ(&GetDocument(), scroll_timeline->ResolvedScrollSource());
+}
+
+TEST_F(ScrollTimelineTest,
+       ChangingDocumentScrollingElementShouldNotImpactScrollTimeline) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #body { overflow: scroll; width: 100px; height: 100px; }
+      #content { width: 10000px; height: 10000px; }
+    </style>
+    <div id='content'></div>
+  )HTML");
+
+  // In QuirksMode, the body is the scrolling element
+  GetDocument().SetCompatibilityMode(Document::kQuirksMode);
+  EXPECT_EQ(GetDocument().body(), GetDocument().scrollingElement());
+
+  // Create the ScrollTimeline with Document.scrollingElement() as source. The
+  // resolved scroll source should be the Document.
+  ScrollTimelineOptions* options = ScrollTimelineOptions::Create();
+  DoubleOrScrollTimelineAutoKeyword time_range =
+      DoubleOrScrollTimelineAutoKeyword::FromDouble(100);
+  options->setTimeRange(time_range);
+  options->setScrollSource(GetDocument().scrollingElement());
+  ScrollTimeline* scroll_timeline =
+      ScrollTimeline::Create(GetDocument(), options, ASSERT_NO_EXCEPTION);
+  EXPECT_EQ(&GetDocument(), scroll_timeline->ResolvedScrollSource());
+
+  // Now change the Document.scrollingElement(). In NoQuirksMode, the
+  // documentElement is the scrolling element and not the body.
+  GetDocument().SetCompatibilityMode(Document::kNoQuirksMode);
+  EXPECT_NE(GetDocument().documentElement(), GetDocument().body());
+  EXPECT_EQ(GetDocument().documentElement(), GetDocument().scrollingElement());
+
+  // Changing the scrollingElement should not impact the previously resolved
+  // scroll source. Note that at this point the scroll timeline's scroll source
+  // is still body element which is no longer the scrolling element. So if we
+  // were to re-resolve the scroll source, it would not map to Document.
+  EXPECT_EQ(&GetDocument(), scroll_timeline->ResolvedScrollSource());
+}
+
 }  //  namespace blink