SubtreeVisibility: Ensure not to recompute visual overflow in locked subtrees.

We already had code to ensure that we don't recurse down into locked
subtrees from within the visual overflow calculation. However, compositing
navigates a different tree that can end up calling into a visual overflow
recomputation on an object that is already inside a locked subtree (thus
bypassing the checks in that function).

This patch fixes the problem by blocking the compositing recursion on
locked subtrees. This should be a performance optimization as well, so
there's that.

Note that I've updated the code to check for prepaint block not paint
block. The difference is that prepaint can be forced. Since compositing
relies on the visual overflow calculations, and compositing itself can
be forced, the two need to align.

Note that there is already a TODO for adding a separate compositing phase
which should clear this up in the future.

R=chrishtr@chromium.org

Fixed: 1073452
Change-Id: I232427fef2093d30451323924bb80efe49832a72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2163409
Commit-Queue: vmpstr <vmpstr@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762550}

third_party/blink/renderer/core/display_lock/display_lock_context.cc

@@ -532,7 +532,6 @@ void DisplayLockContext::Unlock() {
   MarkForLayoutIfNeeded();
   MarkAncestorsForPrePaintIfNeeded();
   MarkPaintLayerNeedsRepaint();
-  MarkForCompositingUpdatesIfNeeded();
 }
 
 void DisplayLockContext::AddToWhitespaceReattachSet(Element& element) {
@@ -695,6 +694,11 @@ bool DisplayLockContext::MarkForCompositingUpdatesIfNeeded() {
     if (needs_compositing_requirements_update_)
       layout_box->Layer()->SetNeedsCompositingRequirementsUpdate();
     needs_compositing_requirements_update_ = false;
+
+    if (needs_compositing_dependent_flag_update_)
+      layout_box->Layer()->SetNeedsCompositingInputsUpdate();
+    needs_compositing_dependent_flag_update_ = false;
+
     return true;
   }
   return false;

third_party/blink/renderer/core/display_lock/display_lock_context.h

@@ -184,6 +184,9 @@ class CORE_EXPORT DisplayLockContext final
   void NotifyCompositingRequirementsUpdateWasBlocked() {
     needs_compositing_requirements_update_ = true;
   }
+  void NotifyCompositingDescendantDependentFlagUpdateWasBlocked() {
+    needs_compositing_dependent_flag_update_ = true;
+  }
 
   // Notify this element will be disconnected.
   void NotifyWillDisconnect();
@@ -345,6 +348,7 @@ class CORE_EXPORT DisplayLockContext final
   bool needs_prepaint_subtree_walk_ = false;
   bool needs_graphics_layer_collection_ = false;
   bool needs_compositing_requirements_update_ = false;
+  bool needs_compositing_dependent_flag_update_ = false;
 
   // Will be true if child traversal was blocked on a previous layout run on the
   // locked element. We need to keep track of this to ensure that on the next

third_party/blink/renderer/core/display_lock/display_lock_context_test.cc

@@ -24,6 +24,7 @@
 #include "third_party/blink/renderer/core/frame/frame_test_helpers.h"
 #include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
 #include "third_party/blink/renderer/core/html/html_template_element.h"
+#include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
 #include "third_party/blink/renderer/core/testing/core_unit_test_helper.h"
 #include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
@@ -1755,6 +1756,10 @@ class DisplayLockContextRenderingTest : public RenderingTest,
   bool IsObservingLifecycle(DisplayLockContext* context) const {
     return context->is_registered_for_lifecycle_notifications_;
   }
+  bool DescendantDependentFlagUpdateWasBlocked(
+      DisplayLockContext* context) const {
+    return context->needs_compositing_dependent_flag_update_;
+  }
   void LockImmediate(DisplayLockContext* context) {
     context->SetRequestedState(ESubtreeVisibility::kHidden);
   }
@@ -1785,6 +1790,58 @@ TEST_F(DisplayLockContextRenderingTest, FrameDocumentRemovedWhileAcquire) {
   LockImmediate(&target->EnsureDisplayLockContext());
 }
 
+TEST_F(DisplayLockContextRenderingTest,
+       VisualOverflowCalculateOnChildPaintLayer) {
+  SetHtmlInnerHTML(R"HTML(
+    <style>
+      .hidden { subtree-visibility: hidden }
+      .paint_layer { contain: paint }
+      .composited { will-change: transform }
+    </style>
+    <div id=lockable class=paint_layer>
+      <div id=parent class=paint_layer>
+        <div id=child class=paint_layer>
+          <span>content</span>
+          <span>content</span>
+          <span>content</span>
+        </div>
+      </div>
+    </div>
+  )HTML");
+
+  auto* parent = GetDocument().getElementById("parent");
+  auto* parent_box = ToLayoutBoxModelObject(parent->GetLayoutObject());
+  ASSERT_TRUE(parent_box);
+  EXPECT_TRUE(parent_box->Layer());
+  EXPECT_TRUE(parent_box->HasSelfPaintingLayer());
+
+  // Lock the container.
+  auto* lockable = GetDocument().getElementById("lockable");
+  lockable->classList().Add("hidden");
+  UpdateAllLifecyclePhasesForTest();
+
+  auto* child = GetDocument().getElementById("child");
+  auto* child_layer = ToLayoutBoxModelObject(child->GetLayoutObject())->Layer();
+  child_layer->SetNeedsVisualOverflowRecalc();
+  EXPECT_TRUE(child_layer->NeedsVisualOverflowRecalc());
+
+  // The following should not crash/DCHECK.
+  UpdateAllLifecyclePhasesForTest();
+
+  // Verify that the display lock knows that the descendant dependent flags
+  // update was blocked.
+  ASSERT_TRUE(lockable->GetDisplayLockContext());
+  EXPECT_TRUE(DescendantDependentFlagUpdateWasBlocked(
+      lockable->GetDisplayLockContext()));
+  EXPECT_TRUE(child_layer->NeedsVisualOverflowRecalc());
+
+  // After unlocking, we should process the pending visual overflow recalc.
+  lockable->classList().Remove("hidden");
+  UpdateAllLifecyclePhasesForTest();
+
+  EXPECT_FALSE(child_layer->NeedsVisualOverflowRecalc());
+}
+
 TEST_F(DisplayLockContextRenderingTest,
        SelectionOnAnonymousColumnSpannerDoesNotCrash) {
   SetHtmlInnerHTML(R"HTML(

third_party/blink/renderer/core/layout/layout_block.cc

@@ -30,6 +30,7 @@
 
 #include "base/memory/ptr_util.h"
 #include "third_party/blink/renderer/core/css/style_engine.h"
+#include "third_party/blink/renderer/core/display_lock/display_lock_utilities.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/element.h"
 #include "third_party/blink/renderer/core/editing/drag_caret.h"
@@ -434,7 +435,10 @@ void LayoutBlock::UpdateBlockLayout(bool) {
 }
 
 void LayoutBlock::AddVisualOverflowFromChildren() {
-  if (LayoutBlockedByDisplayLock(DisplayLockLifecycleTarget::kChildren))
+  // It is an error to call this function on a LayoutBlock that it itself inside
+  // a display-locked subtree.
+  DCHECK(!DisplayLockUtilities::NearestLockedExclusiveAncestor(*this));
+  if (PrePaintBlockedByDisplayLock(DisplayLockLifecycleTarget::kChildren))
     return;
 
   if (ChildrenInline())
@@ -2114,8 +2118,11 @@ bool LayoutBlock::RecalcChildLayoutOverflow() {
 
 void LayoutBlock::RecalcChildVisualOverflow() {
   DCHECK(!IsTable());
+  // It is an error to call this function on a LayoutBlock that it itself inside
+  // a display-locked subtree.
+  DCHECK(!DisplayLockUtilities::NearestLockedExclusiveAncestor(*this));
 
-  if (PaintBlockedByDisplayLock(DisplayLockLifecycleTarget::kChildren))
+  if (PrePaintBlockedByDisplayLock(DisplayLockLifecycleTarget::kChildren))
     return;
 
   if (ChildrenInline()) {

third_party/blink/renderer/core/paint/compositing/compositing_inputs_updater.cc

@@ -175,13 +175,14 @@ void CompositingInputsUpdater::UpdateSelfAndDescendantsRecursively(
           DisplayLockLifecycleTarget::kChildren);
 
   bool should_recurse = (layer->ChildNeedsCompositingInputsUpdate() ||
-                         update_type == kForceUpdate) &&
-                        !recursion_blocked_by_display_lock;
+                         update_type == kForceUpdate);
 
   layer->SetDescendantHasDirectOrScrollingCompositingReason(false);
   bool descendant_has_direct_compositing_reason = false;
-  for (PaintLayer* child = layer->FirstChild(); child;
-       child = child->NextSibling()) {
+
+  auto* first_child =
+      recursion_blocked_by_display_lock ? nullptr : layer->FirstChild();
+  for (PaintLayer* child = first_child; child; child = child->NextSibling()) {
     if (should_recurse)
       UpdateSelfAndDescendantsRecursively(child, update_type, info);
     descendant_has_direct_compositing_reason |=

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

@@ -641,8 +641,18 @@ void PaintLayer::UpdateDescendantDependentFlags() {
     bool can_contain_abs =
         GetLayoutObject().CanContainAbsolutePositionObjects();
 
-    for (PaintLayer* child = FirstChild(); child;
-         child = child->NextSibling()) {
+    auto* first_child = [this]() -> PaintLayer* {
+      if (GetLayoutObject().PrePaintBlockedByDisplayLock(
+              DisplayLockLifecycleTarget::kChildren)) {
+        GetLayoutObject()
+            .GetDisplayLockContext()
+            ->NotifyCompositingDescendantDependentFlagUpdateWasBlocked();
+        return nullptr;
+      }
+      return FirstChild();
+    }();
+
+    for (PaintLayer* child = first_child; child; child = child->NextSibling()) {
       const ComputedStyle& child_style = child->GetLayoutObject().StyleRef();
 
       child->UpdateDescendantDependentFlags();