Reland r1081902 with additional geometry mapper cache invalidation

This patch is a reland of https://crrev.com/1081902 ("Only invalidate GeometryMapper cache if paint properties change") which caused several bugs (crbug.com/1400638) and was reverted in https://crrev.com/1083413. The root-cause of these bugs was a missing invalidation of the geometry mapper cache when SetParent was called on isolation/alias nodes. The geometry mapper cache stores ancestor information and needs to be updated when ancestors change, even if they are isolation/alias nodes. Prior to r1081902, this under invalidation was hidden because the PrePaint tree walk always invalidated the geometry mapper cache. Patchset 1 is the original patch, and the additional patchsets include the changes made on top of that. Change-Id: Icfe5f8e41f5dc064b716e6c4847075dfdefcb8bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4114275 Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/main@{#1084579}
chromium · Dec 16, 2022 · 10b6699 · 10b6699
1 parent c323927
commit 10b6699
Show file tree

Hide file tree

Showing 9 changed files with 103 additions and 74 deletions.
diff --git a/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc b/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
@@ -4240,10 +4240,6 @@ void PaintPropertyTreeBuilder::DirectlyUpdateTransformMatrix(
     const LayoutObject& object) {
   DCHECK(CanDoDeferredTransformNodeUpdate(object));
 
-  // GeometryMapper depends on paint properties. This is typically called from
-  // the PrePaintTreeWalk, but we may skip that for this direct update.
-  GeometryMapper::ClearCache();
-
   auto& box = To<LayoutBox>(object);
   PhysicalSize size = PhysicalSize(box.Size());
   FragmentData* fragment_data = &object.GetMutableForPainting().FirstFragment();

diff --git a/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc b/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
@@ -87,6 +87,12 @@ PaintPropertyTreeBuilderTest::PaintPropertiesForElement(const char* name) {
       .PaintProperties();
 }
 
+const GeometryMapperTransformCache&
+PaintPropertyTreeBuilderTest::GetTransformCache(
+    const TransformPaintPropertyNode& transform) {
+  return transform.GetTransformCache();
+}
+
 void PaintPropertyTreeBuilderTest::SetUp() {
   EnableCompositing();
   RenderingTest::SetUp();
@@ -7377,4 +7383,66 @@ TEST_P(PaintPropertyTreeBuilderTest, EffectOutputClipOfMissedOutOfFlow) {
   EXPECT_FALSE(properties->Effect()->OutputClip());
 }
 
+TEST_P(PaintPropertyTreeBuilderTest, TransformChangesInvalidateGeometryMapper) {
+  SetBodyInnerHTML(R"HTML(
+    <style>#div { width:10px; height:10px; transform:translateX(9px); }</style>
+    <div id="div" style="transform: translateX(5px);"></div>
+  )HTML");
+
+  const auto* properties = PaintPropertiesForElement("div");
+  const auto& transform_cache = GetTransformCache(*properties->Transform());
+  EXPECT_TRUE(transform_cache.IsValid());
+
+  // Change the transform and ensure the geometry mapper cache is invalidated.
+  auto* div = GetDocument().getElementById("div");
+  div->removeAttribute(html_names::kStyleAttr);
+  UpdateAllLifecyclePhasesExceptPaint();
+  EXPECT_FALSE(transform_cache.IsValid());
+
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_TRUE(transform_cache.IsValid());
+
+  // Make a color change and ensure the geometry mapper cache is not
+  // invalidated.
+  div->setAttribute(html_names::kStyleAttr, "background: green;");
+  UpdateAllLifecyclePhasesExceptPaint();
+  EXPECT_TRUE(transform_cache.IsValid());
+}
+
+TEST_P(PaintPropertyTreeBuilderTest,
+       GeometryMapperCacheInvalidationAcrossIsolationNodes) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #composited { transform: translate3d(1px, 2px, 3px); }
+      #container { contain: layout paint; width: 100px; height: 100px; }
+      #target { transform: translateX(1px); }
+    </style>
+    <div id='composited'>
+      <div id='container' style='transform: translate3d(1px, 2px, 3px);'>
+        <div id='target'></div>
+      </div>
+    </div>
+  )HTML");
+
+  LocalFrameView* frame_view = GetDocument().View();
+  frame_view->UpdateAllLifecyclePhasesForTest();
+
+  auto* container = GetLayoutObjectByElementId("container");
+  auto* container_properties = container->FirstFragment().PaintProperties();
+  auto* target = GetLayoutObjectByElementId("target");
+  auto* target_properties = target->FirstFragment().PaintProperties();
+  EXPECT_EQ(target_properties->Transform()->NearestDirectlyCompositedAncestor(),
+            container_properties->Transform());
+
+  // Remove the direct compositing reason from #container.
+  auto* container_element = GetDocument().getElementById("container");
+  container_element->setAttribute(html_names::kStyleAttr, "");
+  frame_view->UpdateAllLifecyclePhasesForTest();
+
+  auto* composited = GetLayoutObjectByElementId("composited");
+  auto* composited_properties = composited->FirstFragment().PaintProperties();
+  EXPECT_EQ(target_properties->Transform()->NearestDirectlyCompositedAncestor(),
+            composited_properties->Transform());
+}
+
 }  // namespace blink
diff --git a/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.h b/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.h
@@ -13,6 +13,7 @@
 namespace blink {
 
 class ClipPaintPropertyNode;
+class GeometryMapperTransformCache;
 class ScrollPaintPropertyNode;
 class TransformPaintPropertyNode;
 struct PhysicalOffset;
@@ -41,6 +42,9 @@ class PaintPropertyTreeBuilderTest : public PaintControllerPaintTest {
 
   const ObjectPaintProperties* PaintPropertiesForElement(const char* name);
 
+  const GeometryMapperTransformCache& GetTransformCache(
+      const TransformPaintPropertyNode&);
+
   static unsigned NumFragments(const LayoutObject* obj) {
     unsigned count = 0;
     auto* fragment = &obj->FirstFragment();

diff --git a/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc b/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc
@@ -27,7 +27,6 @@
 #include "third_party/blink/renderer/core/paint/object_paint_invalidator.h"
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/paint/paint_property_tree_printer.h"
-#include "third_party/blink/renderer/platform/graphics/paint/geometry_mapper.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 
 namespace blink {
@@ -56,11 +55,10 @@ void PrePaintTreeWalk::WalkTree(LocalFrameView& root_frame_view) {
 
   PrePaintTreeWalkContext context;
 
-  // GeometryMapper depends on paint properties.
-  bool needs_tree_builder_context_update =
+#if DCHECK_IS_ON()
+  bool needed_tree_builder_context_update =
       NeedsTreeBuilderContextUpdate(root_frame_view, context);
-  if (needs_tree_builder_context_update)
-    GeometryMapper::ClearCache();
+#endif
 
   VisualViewport& visual_viewport =
       root_frame_view.GetPage()->GetVisualViewport();
@@ -74,8 +72,9 @@ void PrePaintTreeWalk::WalkTree(LocalFrameView& root_frame_view) {
   paint_invalidator_.ProcessPendingDelayedPaintInvalidations();
 
 #if DCHECK_IS_ON()
-  if (needs_tree_builder_context_update && VLOG_IS_ON(1))
+  if (needed_tree_builder_context_update && VLOG_IS_ON(1)) {
     ShowAllPropertyTrees(root_frame_view);
+  }
 #endif
 
   bool was_opacity_updated = root_frame_view.UpdateAllPendingOpacityUpdates();

diff --git a/third_party/blink/renderer/platform/graphics/paint/clip_paint_property_node.h b/third_party/blink/renderer/platform/graphics/paint/clip_paint_property_node.h
@@ -52,6 +52,12 @@ class PLATFORM_EXPORT ClipPaintPropertyNodeOrAlias
 
   void ClearChangedToRoot(int sequence_number) const;
 
+  void AddChanged(PaintPropertyChangeType changed) {
+    DCHECK_NE(PaintPropertyChangeType::kUnchanged, changed);
+    GeometryMapperClipCache::ClearCache();
+    PaintPropertyNode::AddChanged(changed);
+  }
+
  protected:
   using PaintPropertyNode::PaintPropertyNode;
 };
@@ -63,12 +69,6 @@ class ClipPaintPropertyNodeAlias : public ClipPaintPropertyNodeOrAlias {
     return base::AdoptRef(new ClipPaintPropertyNodeAlias(parent));
   }
 
-  PaintPropertyChangeType SetParent(
-      const ClipPaintPropertyNodeOrAlias& parent) {
-    DCHECK(IsParentAlias());
-    return PaintPropertyNode::SetParent(parent);
-  }
-
  private:
   explicit ClipPaintPropertyNodeAlias(
       const ClipPaintPropertyNodeOrAlias& parent)
@@ -198,18 +198,6 @@ class PLATFORM_EXPORT ClipPaintPropertyNode
                         State&& state)
       : ClipPaintPropertyNodeOrAlias(parent), state_(std::move(state)) {}
 
-  void AddChanged(PaintPropertyChangeType changed) {
-    // TODO(crbug.com/814815): This is a workaround of the bug. When the bug is
-    // fixed, change the following condition to
-    //   DCHECK(!clip_cache_ || !clip_cache_->IsValid());
-    DCHECK_NE(PaintPropertyChangeType::kUnchanged, changed);
-    if (clip_cache_ && clip_cache_->IsValid()) {
-      DLOG(WARNING) << "Clip tree changed without invalidating the cache.";
-      GeometryMapperClipCache::ClearCache();
-    }
-    PaintPropertyNode::AddChanged(changed);
-  }
-
   // For access to GetClipCache();
   friend class GeometryMapper;
   friend class GeometryMapperClipCache;

diff --git a/third_party/blink/renderer/platform/graphics/paint/effect_paint_property_node.h b/third_party/blink/renderer/platform/graphics/paint/effect_paint_property_node.h
@@ -60,12 +60,6 @@ class EffectPaintPropertyNodeAlias : public EffectPaintPropertyNodeOrAlias {
     return base::AdoptRef(new EffectPaintPropertyNodeAlias(parent));
   }
 
-  PaintPropertyChangeType SetParent(
-      const EffectPaintPropertyNodeOrAlias& parent) {
-    DCHECK(IsParentAlias());
-    return PaintPropertyNode::SetParent(parent);
-  }
-
  private:
   explicit EffectPaintPropertyNodeAlias(
       const EffectPaintPropertyNodeOrAlias& parent)
@@ -327,8 +321,6 @@ class PLATFORM_EXPORT EffectPaintPropertyNode
                           State&& state)
       : EffectPaintPropertyNodeOrAlias(parent), state_(std::move(state)) {}
 
-  using EffectPaintPropertyNodeOrAlias::SetParent;
-
   State state_;
 };
 

diff --git a/third_party/blink/renderer/platform/graphics/paint/paint_property_node.h b/third_party/blink/renderer/platform/graphics/paint/paint_property_node.h
@@ -118,6 +118,18 @@ class PaintPropertyNode
       n->ClearChanged(sequence_number);
   }
 
+  PaintPropertyChangeType SetParent(const NodeTypeOrAlias& parent) {
+    DCHECK(!IsRoot());
+    DCHECK_NE(&parent, this);
+    if (&parent == parent_) {
+      return PaintPropertyChangeType::kUnchanged;
+    }
+    parent_ = &parent;
+    static_cast<NodeTypeOrAlias*>(this)->AddChanged(
+        PaintPropertyChangeType::kChangedOnlyValues);
+    return PaintPropertyChangeType::kChangedOnlyValues;
+  }
+
   // Returns true if this node is an alias for its parent. A parent alias is a
   // node which on its own does not contribute to the rendering output, and only
   // exists to enforce a particular structure of the paint property tree. Its
@@ -209,23 +221,6 @@ class PaintPropertyNode
         changed_(PaintPropertyChangeType::kNodeAddedOrRemoved),
         parent_(&parent) {}
 
-  PaintPropertyChangeType SetParent(const NodeTypeOrAlias& parent) {
-    DCHECK(!IsRoot());
-    DCHECK_NE(&parent, this);
-    if (&parent == parent_)
-      return PaintPropertyChangeType::kUnchanged;
-
-    parent_ = &parent;
-    if (IsParentAlias()) {
-      static_cast<NodeTypeOrAlias*>(this)->AddChanged(
-          PaintPropertyChangeType::kChangedOnlyValues);
-    } else {
-      static_cast<NodeType*>(this)->AddChanged(
-          PaintPropertyChangeType::kChangedOnlyValues);
-    }
-    return PaintPropertyChangeType::kChangedOnlyValues;
-  }
-
   void AddChanged(PaintPropertyChangeType changed) {
     DCHECK(!IsRoot());
     changed_ = std::max(changed_, changed);

diff --git a/third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.h b/third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.h
@@ -182,8 +182,6 @@ class PLATFORM_EXPORT ScrollPaintPropertyNode
     Validate();
   }
 
-  using PaintPropertyNode::SetParent;
-
   void Validate() const {
 #if DCHECK_IS_ON()
     DCHECK(!state_.compositor_element_id ||

diff --git a/third_party/blink/renderer/platform/graphics/paint/transform_paint_property_node.h b/third_party/blink/renderer/platform/graphics/paint/transform_paint_property_node.h
@@ -46,6 +46,13 @@ class PLATFORM_EXPORT TransformPaintPropertyNodeOrAlias
   bool Changed(PaintPropertyChangeType change,
                const TransformPaintPropertyNodeOrAlias& relative_to_node) const;
 
+  void AddChanged(PaintPropertyChangeType changed) {
+    DCHECK_NE(PaintPropertyChangeType::kUnchanged, changed);
+    GeometryMapperTransformCache::ClearCache();
+    GeometryMapperClipCache::ClearCache();
+    PaintPropertyNode::AddChanged(changed);
+  }
+
  protected:
   using PaintPropertyNode::PaintPropertyNode;
 };
@@ -58,12 +65,6 @@ class TransformPaintPropertyNodeAlias
     return base::AdoptRef(new TransformPaintPropertyNodeAlias(parent));
   }
 
-  PaintPropertyChangeType SetParent(
-      const TransformPaintPropertyNodeOrAlias& parent) {
-    DCHECK(IsParentAlias());
-    return PaintPropertyNode::SetParent(parent);
-  }
-
  private:
   explicit TransformPaintPropertyNodeAlias(
       const TransformPaintPropertyNodeOrAlias& parent)
@@ -425,24 +426,12 @@ class PLATFORM_EXPORT TransformPaintPropertyNode
 #endif
   }
 
-  void AddChanged(PaintPropertyChangeType changed) {
-    // TODO(crbug.com/814815): This is a workaround of the bug. When the bug is
-    // fixed, change the following condition to
-    //   DCHECK(!transform_cache_ || !transform_cache_->IsValid());
-    DCHECK_NE(PaintPropertyChangeType::kUnchanged, changed);
-    if (transform_cache_ && transform_cache_->IsValid()) {
-      DLOG(WARNING) << "Transform tree changed without invalidating the cache.";
-      GeometryMapperTransformCache::ClearCache();
-      GeometryMapperClipCache::ClearCache();
-    }
-    TransformPaintPropertyNodeOrAlias::AddChanged(changed);
-  }
-
   // For access to GetTransformCache() and SetCachedTransform.
   friend class GeometryMapper;
   friend class GeometryMapperTest;
   friend class GeometryMapperTransformCache;
   friend class GeometryMapperTransformCacheTest;
+  friend class PaintPropertyTreeBuilderTest;
 
   const GeometryMapperTransformCache& GetTransformCache() const {
     if (!transform_cache_)