Fix active/pending id issues in cc::ScrollTimeline

There were two subtle bugs in the original implementation:

i. cc::ScrollTimeline was incorrectly initialized with active_id_ ==
   pending_id_. This is incorrect as the current active tree may not
   yet have the relevant ScrollNode. Instead only pending_id_ should be
   set, and it will be promoted to active via the existing logic when
   the pending tree activates.

ii. cc::ScrollTimeline::PushPropertiesTo incorrectly updated the impl
    thread active_id_ from the main side. This is incorrect, main never
    has a valid active_id_ value (since it never gets updated by pending
    tree activations). This should only push the pending_id_ value,
    which may change e.g. if the scroller went display:none and back.

Bug: 853231
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ib7bf4e82e779d11895c373f7c786727c1bcadb9d
Reviewed-on: https://chromium-review.googlesource.com/1110438
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569790}

cc/animation/scroll_timeline.cc

@@ -14,24 +14,15 @@ namespace cc {
 ScrollTimeline::ScrollTimeline(base::Optional<ElementId> scroller_id,
                                ScrollDirection orientation,
                                double time_range)
-    : active_id_(scroller_id),
+    : active_id_(),
       pending_id_(scroller_id),
       orientation_(orientation),
       time_range_(time_range) {}
 
-ScrollTimeline::ScrollTimeline(base::Optional<ElementId> active_id,
-                               base::Optional<ElementId> pending_id,
-                               ScrollDirection orientation,
-                               double time_range)
-    : active_id_(active_id),
-      pending_id_(pending_id),
-      orientation_(orientation),
-      time_range_(time_range) {}
-
 ScrollTimeline::~ScrollTimeline() {}
 
 std::unique_ptr<ScrollTimeline> ScrollTimeline::CreateImplInstance() const {
-  return std::make_unique<ScrollTimeline>(active_id_, pending_id_, orientation_,
+  return std::make_unique<ScrollTimeline>(pending_id_, orientation_,
                                           time_range_);
 }
 
@@ -76,7 +67,6 @@ double ScrollTimeline::CurrentTime(const ScrollTree& scroll_tree,
 
 void ScrollTimeline::PushPropertiesTo(ScrollTimeline* impl_timeline) {
   DCHECK(impl_timeline);
-  impl_timeline->active_id_ = active_id_;
   impl_timeline->pending_id_ = pending_id_;
 }
 

cc/animation/scroll_timeline.h

@@ -30,10 +30,6 @@ class CC_ANIMATION_EXPORT ScrollTimeline {
   ScrollTimeline(base::Optional<ElementId> scroller_id,
                  ScrollDirection orientation,
                  double time_range);
-  ScrollTimeline(base::Optional<ElementId> active_id,
-                 base::Optional<ElementId> pending_id,
-                 ScrollDirection orientation,
-                 double time_range);
   virtual ~ScrollTimeline();
 
   // Create a copy of this ScrollTimeline intended for the impl thread in the

cc/animation/scroll_timeline_unittest.cc

@@ -17,7 +17,7 @@ class ScrollTimelineTest : public ::testing::Test {
     // For simplicity we make the property_tree main thread; this avoids the
     // need to deal with the synced scroll offset code.
     property_trees_.is_main_thread = true;
-    property_trees_.is_active = true;
+    property_trees_.is_active = false;
 
     // We add a single node that is scrolling a 550x1100 contents inside a
     // 50x100 container.
@@ -55,16 +55,16 @@ TEST_F(ScrollTimelineTest, BasicCurrentTimeCalculations) {
 
   // Unscrolled, both timelines should read a current time of 0.
   scroll_tree().SetScrollOffset(scroller_id(), gfx::ScrollOffset());
-  EXPECT_FLOAT_EQ(0, vertical_timeline.CurrentTime(scroll_tree(), true));
-  EXPECT_FLOAT_EQ(0, horizontal_timeline.CurrentTime(scroll_tree(), true));
+  EXPECT_FLOAT_EQ(0, vertical_timeline.CurrentTime(scroll_tree(), false));
+  EXPECT_FLOAT_EQ(0, horizontal_timeline.CurrentTime(scroll_tree(), false));
 
   // Now do some scrolling and make sure that the ScrollTimelines update.
   scroll_tree().SetScrollOffset(scroller_id(), gfx::ScrollOffset(75, 50));
 
   // As noted above, we have mapped the time range such that current time should
   // just be the scroll offset.
-  EXPECT_FLOAT_EQ(50, vertical_timeline.CurrentTime(scroll_tree(), true));
-  EXPECT_FLOAT_EQ(75, horizontal_timeline.CurrentTime(scroll_tree(), true));
+  EXPECT_FLOAT_EQ(50, vertical_timeline.CurrentTime(scroll_tree(), false));
+  EXPECT_FLOAT_EQ(75, horizontal_timeline.CurrentTime(scroll_tree(), false));
 }
 
 TEST_F(ScrollTimelineTest, CurrentTimeIsAdjustedForTimeRange) {
@@ -75,7 +75,60 @@ TEST_F(ScrollTimelineTest, CurrentTimeIsAdjustedForTimeRange) {
   double halfwayY = (content_size().height() - container_size().height()) / 2.;
   scroll_tree().SetScrollOffset(scroller_id(), gfx::ScrollOffset(0, halfwayY));
 
-  EXPECT_FLOAT_EQ(50, timeline.CurrentTime(scroll_tree(), true));
+  EXPECT_FLOAT_EQ(50, timeline.CurrentTime(scroll_tree(), false));
+}
+
+// This test ensures that the ScrollTimeline's active scroller id is correct. We
+// had a few crashes caused by assuming that the id would be available in the
+// active tree before the activation happened; see http://crbug.com/853231
+TEST_F(ScrollTimelineTest, ActiveTimeIsSetOnlyAfterPromotion) {
+  PropertyTrees pending_tree;
+  PropertyTrees active_tree;
+
+  pending_tree.is_active = false;
+  active_tree.is_active = true;
+
+  // For simplicity we pretend the trees are main thread; this avoids the need
+  // to deal with the synced scroll offset code.
+  pending_tree.is_main_thread = true;
+  active_tree.is_main_thread = true;
+
+  // Initially only the pending tree has the scroll node.
+  ElementId scroller_id(1);
+  ScrollNode node;
+  node.scrollable = true;
+  node.bounds = content_size();
+  node.container_bounds = container_size();
+
+  int node_id = pending_tree.scroll_tree.Insert(node, 0);
+  pending_tree.element_id_to_scroll_node_index[scroller_id] = node_id;
+
+  double halfwayY = (content_size().height() - container_size().height()) / 2.;
+  pending_tree.scroll_tree.SetScrollOffset(scroller_id,
+                                           gfx::ScrollOffset(0, halfwayY));
+
+  ScrollTimeline main_timeline(scroller_id, ScrollTimeline::Vertical, 100);
+
+  // Now create an impl version of the ScrollTimeline. Initilly this should only
+  // have a pending scroller id, as the active tree may not yet have the
+  // scroller in it (as in this case).
+  std::unique_ptr<ScrollTimeline> impl_timeline =
+      main_timeline.CreateImplInstance();
+
+  EXPECT_TRUE(
+      std::isnan(impl_timeline->CurrentTime(active_tree.scroll_tree, true)));
+  EXPECT_FLOAT_EQ(50,
+                  impl_timeline->CurrentTime(pending_tree.scroll_tree, false));
+
+  // Now fake a tree activation; this should cause the ScrollTimeline to update
+  // its active scroller id. Note that we deliberately pass in the pending_tree
+  // and just claim it is the active tree; this avoids needing to properly
+  // implement tree swapping just for the test.
+  impl_timeline->PromoteScrollTimelinePendingToActive();
+  EXPECT_FLOAT_EQ(50,
+                  impl_timeline->CurrentTime(pending_tree.scroll_tree, true));
+  EXPECT_FLOAT_EQ(50,
+                  impl_timeline->CurrentTime(pending_tree.scroll_tree, false));
 }
 
 }  // namespace cc

third_party/WebKit/LayoutTests/TestExpectations

@@ -4720,7 +4720,6 @@ crbug.com/853360 [ Mac ] fast/forms/select/menulist-appearance-rtl.html [ Failur
 
 # Sheriff 2018-06-18
 crbug.com/853852 [ Linux ] virtual/threaded/external/wpt/feature-policy/experimental-features/vertical-scroll-wheel-block-manual.tentative.html [ Pass Timeout ]
-crbug.com/854053 [ Mac10.13 Debug ] virtual/threaded/fast/animationworklet/animation-worklet-scroll-timeline.html [ Pass Crash ]
 
 crbug.com/854538 [ Win7 ] http/tests/security/contentSecurityPolicy/1.1/form-action-src-default-ignored-with-redirect.html [ Skip ]