Set scrolls_viewport bits from viewport registration

The issue in this crash is that we compute the viewport registration and
the scrolls_inner_viewport|scrolls_outer_viewport bits separately, even
though they mean the same thing. However, in some cases they end up
not matching which leads to violating invariants in the compositor.

In some (I believe transient) cases, [1] will not have a
GlobalRootScroller so we don't have an outer viewport. Because of that,
we also avoid registering an inner viewport. This both-or-neither
viewport registration was added in https://crrev.com/c/1955860 to
simplify viewport existence.

However, setting the |scrolls_inner_viewport| bit on the ScrollNode
happens earlier, during pre-paint[2] at which point we don't yet know
whether the outer viewport will be registered. This means we end up in a
situation in the compositor where InnerViewportScrollNode() returns
nullptr (because it comes from the ViewportPropertyIds) but there exists
a node in the ScrollTree where |scrolls_inner_viewport| is true. The
crash happens because we assume that if we're looking at a node that
has this bit set to true, we must also have an outer viewport and so we
crash dereferencing OuterViewportScrollNode.

Ideally, we wouldn't need to duplicate this information but we don't
want to do a ScrollTree walk each time we want to get one of the
viewport nodes. So we need the ViewportPropertyIds registration. I've
considered removing the |scrolls_x_viewport| bits altogether,
compositor code can always do something like:

  scroll_node == InnerViewportScrollNode()

But there's a handful of cases inside property_trees.cc that check these
bits and they don't have access to the ViewportPropertyIds. We could
pass those down but that seems awkward.

This CL changes it so that we set these bits based on the nodes we
register to the ViewportPropertyIds. The information is duplicated but
at least it comes from the same source. This also means we don't need
the bit on the blink-side paint property node so the bit is removed
there.

[1] https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/frame/local_frame_view.cc;l=2813;drc=e085cd7f91fd90b436565329510b738afb436b19
[2] https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/frame/visual_viewport.cc;l=218;drc=4045478f9a1db6872c1527a8e725bb79ae90f5ce

Bug: 1037759
Change-Id: I1dc06d9c6541b4afcd351841e8e7544ec11e3eb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2006610
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733404}

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

@@ -215,7 +215,6 @@ PaintPropertyChangeType VisualViewport::UpdatePaintPropertyNodesIfNeeded(
     state.user_scrollable_horizontal =
         UserInputScrollable(kHorizontalScrollbar);
     state.user_scrollable_vertical = UserInputScrollable(kVerticalScrollbar);
-    state.scrolls_inner_viewport = true;
     state.max_scroll_offset_affected_by_page_scale = true;
     state.compositor_element_id = GetScrollElementId();
 

third_party/blink/renderer/core/paint/paint_property_tree_builder.cc

@@ -1823,8 +1823,6 @@ void FragmentPaintPropertyTreeBuilder::UpdateScrollAndScrollTranslation() {
       state.user_scrollable_vertical =
           scrollable_area->UserInputScrollable(kVerticalScrollbar);
 
-      state.scrolls_outer_viewport = box.IsGlobalRootScroller();
-
       // TODO(bokan): We probably don't need to pass ancestor reasons down the
       // scroll tree. On the compositor, in
       // LayerTreeHostImpl::FindScrollNodeForDeviceViewportPoint, we walk up

third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc

@@ -6509,65 +6509,6 @@ TEST_P(PaintPropertyTreeBuilderTest, NoPaintPropertyForSVGText) {
   EXPECT_FALSE(text->FirstFragment().PaintProperties());
 }
 
-TEST_P(PaintPropertyTreeBuilderTest, SetViewportScrollingBits) {
-  SetBodyInnerHTML(R"HTML(
-    <style>
-      body, html {
-        margin: 0;
-        width: 100%;
-        height: 100%;
-      }
-      #scroller {
-       width: 100%;
-       height: 200%;
-       overflow: auto;
-      }
-    </style>
-    <div id="scroller">
-      <div style="height: 3000px"></div>
-    </div>
-  )HTML");
-
-  const auto* scroller_node = PaintPropertiesForElement("scroller")->Scroll();
-  const auto* document_node = DocScroll();
-
-  // Ensure the LayoutView's ScrollNode is marked as scrolling the "outer" or
-  // "layout" viewport.
-  {
-    EXPECT_FALSE(scroller_node->ScrollsOuterViewport());
-    EXPECT_TRUE(document_node->ScrollsOuterViewport());
-  }
-
-  // Ensure the visual viewport is the only one that sets the inner scroll bit.
-  {
-    EXPECT_TRUE(GetDocument()
-                    .GetPage()
-                    ->GetVisualViewport()
-                    .GetScrollNode()
-                    ->ScrollsInnerViewport());
-    EXPECT_FALSE(scroller_node->ScrollsInnerViewport());
-    EXPECT_FALSE(document_node->ScrollsInnerViewport());
-  }
-
-  // Make the scroller fill the viewport. This will make it eligible for root
-  // scroller promotion. Ensure the outer viewport scrolling property is
-  // correctly recomputed, moving it from the LayoutView to the scroller.
-  {
-    Element* scroller = GetDocument().getElementById("scroller");
-    scroller->setAttribute(html_names::kStyleAttr, "height: 100%");
-    LocalFrameView* frame_view = GetDocument().View();
-    frame_view->UpdateAllLifecyclePhases(
-        DocumentLifecycle::LifecycleUpdateReason::kTest);
-    ASSERT_TRUE(scroller->GetLayoutObject()->IsGlobalRootScroller());
-
-    EXPECT_TRUE(scroller_node->ScrollsOuterViewport());
-
-    // Since the document is no longer scrollable and isn't the root scroller
-    // it shouldn't have a node.
-    EXPECT_FALSE(DocScroll());
-  }
-}
-
 TEST_P(PaintPropertyTreeBuilderTest, IsAffectedByOuterViewportBoundsDelta) {
   SetBodyInnerHTML(R"HTML(
     <style>div { will-change: transform; position: fixed; }</style>

third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.cc

@@ -1023,14 +1023,14 @@ static void UpdateCompositorViewportProperties(
             *properties.page_scale);
   }
   if (properties.inner_scroll_translation) {
-    ids.inner_scroll = property_tree_manager.EnsureCompositorScrollNode(
+    ids.inner_scroll = property_tree_manager.EnsureCompositorInnerScrollNode(
         *properties.inner_scroll_translation);
     if (properties.outer_clip) {
       ids.outer_clip = property_tree_manager.EnsureCompositorClipNode(
           *properties.outer_clip);
     }
     if (properties.outer_scroll_translation) {
-      ids.outer_scroll = property_tree_manager.EnsureCompositorScrollNode(
+      ids.outer_scroll = property_tree_manager.EnsureCompositorOuterScrollNode(
           *properties.outer_scroll_translation);
     }
   }

third_party/blink/renderer/platform/graphics/compositing/property_tree_manager.cc

@@ -418,7 +418,9 @@ int PropertyTreeManager::EnsureCompositorTransformNode(
     // cache a lookup of transform node to scroll translation transform node.
     const auto& scroll_ancestor = transform_node.NearestScrollTranslationNode();
     sticky_data.scroll_ancestor = EnsureCompositorScrollNode(scroll_ancestor);
-    if (scroll_ancestor.ScrollNode()->ScrollsOuterViewport())
+    const auto& scroll_ancestor_compositor_node =
+        *GetScrollTree().Node(sticky_data.scroll_ancestor);
+    if (scroll_ancestor_compositor_node.scrolls_outer_viewport)
       GetTransformTree().AddNodeAffectedByOuterViewportBoundsDelta(id);
     if (auto shifting_sticky_box_element_id =
             sticky_data.constraints.nearest_element_shifting_sticky_box) {
@@ -530,15 +532,9 @@ void PropertyTreeManager::CreateCompositorScrollNode(
       scroll_node.UserScrollableHorizontal();
   compositor_node.user_scrollable_vertical =
       scroll_node.UserScrollableVertical();
-  compositor_node.scrolls_inner_viewport = scroll_node.ScrollsInnerViewport();
-  compositor_node.scrolls_outer_viewport = scroll_node.ScrollsOuterViewport();
   compositor_node.prevent_viewport_scrolling_from_inner =
       scroll_node.PreventViewportScrollingFromInner();
 
-  // |scrolls_using_viewport| should only ever be set on the inner scroll node.
-  DCHECK(!compositor_node.prevent_viewport_scrolling_from_inner ||
-         compositor_node.scrolls_inner_viewport);
-
   compositor_node.max_scroll_offset_affected_by_page_scale =
       scroll_node.MaxScrollOffsetAffectedByPageScale();
   compositor_node.main_thread_scrolling_reasons =
@@ -577,6 +573,20 @@ int PropertyTreeManager::EnsureCompositorScrollNode(
   return id;
 }
 
+int PropertyTreeManager::EnsureCompositorInnerScrollNode(
+    const TransformPaintPropertyNode& scroll_offset_translation) {
+  int node_id = EnsureCompositorScrollNode(scroll_offset_translation);
+  GetScrollTree().Node(node_id)->scrolls_inner_viewport = true;
+  return node_id;
+}
+
+int PropertyTreeManager::EnsureCompositorOuterScrollNode(
+    const TransformPaintPropertyNode& scroll_offset_translation) {
+  int node_id = EnsureCompositorScrollNode(scroll_offset_translation);
+  GetScrollTree().Node(node_id)->scrolls_outer_viewport = true;
+  return node_id;
+}
+
 void PropertyTreeManager::EmitClipMaskLayer() {
   cc::EffectNode* mask_isolation = GetEffectTree().Node(current_.effect_id);
   DCHECK(mask_isolation);

third_party/blink/renderer/platform/graphics/compositing/property_tree_manager.h

@@ -99,6 +99,12 @@ class PropertyTreeManager {
   int EnsureCompositorScrollNode(
       const TransformPaintPropertyNode& scroll_offset_translation);
 
+  // Same as above but marks the scroll nodes as being the viewport.
+  int EnsureCompositorInnerScrollNode(
+      const TransformPaintPropertyNode& scroll_offset_translation);
+  int EnsureCompositorOuterScrollNode(
+      const TransformPaintPropertyNode& scroll_offset_translation);
+
   int EnsureCompositorPageScaleTransformNode(const TransformPaintPropertyNode&);
 
   // This function is expected to be invoked right before emitting each layer.

third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.cc

@@ -52,10 +52,6 @@ std::unique_ptr<JSONObject> ScrollPaintPropertyNode::ToJSON() const {
                         state_.main_thread_scrolling_reasons)
                         .c_str());
   }
-  if (state_.scrolls_inner_viewport)
-    json->SetString("scrollsInnerViewport", "true");
-  if (state_.scrolls_outer_viewport)
-    json->SetString("scrollsOuterViewport", "true");
   if (state_.max_scroll_offset_affected_by_page_scale)
     json->SetString("maxScrollOffsetAffectedByPageScale", "true");
   if (state_.compositor_element_id) {

third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.h

@@ -43,8 +43,6 @@ class PLATFORM_EXPORT ScrollPaintPropertyNode
     IntSize contents_size;
     bool user_scrollable_horizontal = false;
     bool user_scrollable_vertical = false;
-    bool scrolls_inner_viewport = false;
-    bool scrolls_outer_viewport = false;
 
     // This bit tells the compositor whether the inner viewport should be
     // scrolled using the full viewport mechanism (overscroll, top control
@@ -69,8 +67,6 @@ class PLATFORM_EXPORT ScrollPaintPropertyNode
           contents_size != other.contents_size ||
           user_scrollable_horizontal != other.user_scrollable_horizontal ||
           user_scrollable_vertical != other.user_scrollable_vertical ||
-          scrolls_inner_viewport != other.scrolls_inner_viewport ||
-          scrolls_outer_viewport != other.scrolls_outer_viewport ||
           prevent_viewport_scrolling_from_inner !=
               other.prevent_viewport_scrolling_from_inner ||
           max_scroll_offset_affected_by_page_scale !=
@@ -145,8 +141,6 @@ class PLATFORM_EXPORT ScrollPaintPropertyNode
   bool UserScrollableVertical() const {
     return state_.user_scrollable_vertical;
   }
-  bool ScrollsInnerViewport() const { return state_.scrolls_inner_viewport; }
-  bool ScrollsOuterViewport() const { return state_.scrolls_outer_viewport; }
   bool PreventViewportScrollingFromInner() const {
     return state_.prevent_viewport_scrolling_from_inner;
   }