[SPv175+] Don't hold references to paint properties from CompositedLayerRasterInvalidator

CompositedLayerRasterInvalidator::PaintChunkInfo::property_tree_state
no longer holds references to the property nodes which may be
freed after this structure is created. We never dereference the
pointers and newly created property nodes always have Changed()
flag set, so this is good for property change detection.

This avoids the structure holding the whole out-dated paint property
tree (in the worst case).

Bug: 833496
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: If16082f7371dbc59b816e274ee02a87158952b08
Reviewed-on: https://chromium-review.googlesource.com/1017768Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553363}

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

@@ -47,6 +47,7 @@ size_t CompositedLayerRasterInvalidator::MatchNewChunkToOldChunk(
 
 PaintInvalidationReason
 CompositedLayerRasterInvalidator::ChunkPropertiesChanged(
+    const RefCountedPropertyTreeState& new_chunk_state,
     const PaintChunkInfo& new_chunk,
     const PaintChunkInfo& old_chunk,
     const PropertyTreeState& layer_state) const {
@@ -60,9 +61,7 @@ CompositedLayerRasterInvalidator::ChunkPropertiesChanged(
   // Treat the chunk property as changed if the effect node pointer is
   // different, or the effect node's value changed between the layer state and
   // the chunk state.
-  const auto& new_chunk_state = new_chunk.property_tree_state;
-  const auto& old_chunk_state = old_chunk.property_tree_state;
-  if (new_chunk_state.Effect() != old_chunk_state.Effect() ||
+  if (new_chunk_state.Effect() != old_chunk.effect_state ||
       new_chunk_state.Effect()->Changed(*layer_state.Effect()))
     return PaintInvalidationReason::kPaintProperty;
 
@@ -81,7 +80,7 @@ CompositedLayerRasterInvalidator::ChunkPropertiesChanged(
   // Otherwise treat the chunk property as changed if the clip node pointer is
   // different, or the clip node's value changed between the layer state and the
   // chunk state.
-  if (new_chunk_state.Clip() != old_chunk_state.Clip() ||
+  if (new_chunk_state.Clip() != old_chunk.clip_state ||
       new_chunk_state.Clip()->Changed(*layer_state.Clip()))
     return PaintInvalidationReason::kPaintProperty;
 
@@ -138,8 +137,8 @@ void CompositedLayerRasterInvalidator::GenerateRasterInvalidations(
     PaintInvalidationReason reason =
         matched_old_index < max_matched_old_index
             ? PaintInvalidationReason::kChunkReordered
-            : ChunkPropertiesChanged(new_chunk_info, old_chunk_info,
-                                     layer_state);
+            : ChunkPropertiesChanged(new_chunk.properties, new_chunk_info,
+                                     old_chunk_info, layer_state);
 
     if (IsFullPaintInvalidationReason(reason)) {
       // Invalidate both old and new bounds of the chunk if the chunk's paint

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

@@ -63,7 +63,8 @@ class PLATFORM_EXPORT CompositedLayerRasterInvalidator {
                    const ChunkToLayerMapper& mapper,
                    const PaintChunk& chunk)
         : id(chunk.id),
-          property_tree_state(chunk.properties),
+          clip_state(chunk.properties.Clip()),
+          effect_state(chunk.properties.Effect()),
           is_cacheable(chunk.is_cacheable),
           bounds_in_layer(invalidator.ClipByLayerBounds(
               mapper.MapVisualRect(chunk.bounds))),
@@ -76,7 +77,12 @@ class PLATFORM_EXPORT CompositedLayerRasterInvalidator {
     }
 
     PaintChunk::Id id;
-    RefCountedPropertyTreeState property_tree_state;
+    // These two pointers are for property change detection. The pointed
+    // property nodes can be freed after this structure is created. As newly
+    // created property nodes always have Changed() flag set, it's not a problem
+    // that a new node is created at the address pointed by these pointers.
+    const void* clip_state;
+    const void* effect_state;
     bool is_cacheable;
     IntRect bounds_in_layer;
     FloatClipRect chunk_to_layer_clip;
@@ -105,6 +111,7 @@ class PLATFORM_EXPORT CompositedLayerRasterInvalidator {
                                            PaintInvalidationReason,
                                            const String* debug_name = nullptr);
   PaintInvalidationReason ChunkPropertiesChanged(
+      const RefCountedPropertyTreeState& new_chunk_state,
       const PaintChunkInfo& new_chunk,
       const PaintChunkInfo& old_chunk,
       const PropertyTreeState& layer_state) const;

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

@@ -116,7 +116,7 @@ class PaintPropertyNode : public RefCounted<NodeType> {
 
  protected:
   PaintPropertyNode(scoped_refptr<const NodeType> parent)
-      : parent_(std::move(parent)), changed_(false) {}
+      : parent_(std::move(parent)) {}
 
   bool SetParent(scoped_refptr<const NodeType> parent) {
     DCHECK(!IsRoot());
@@ -133,7 +133,7 @@ class PaintPropertyNode : public RefCounted<NodeType> {
 
  private:
   scoped_refptr<const NodeType> parent_;
-  mutable bool changed_;
+  mutable bool changed_ = true;
 
 #if DCHECK_IS_ON()
   String debug_name_;

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

@@ -43,6 +43,15 @@ class PaintPropertyNodeTest : public testing::Test {
   }
 
   void ExpectInitialState() {
+    EXPECT_FALSE(root->Changed(*root));
+    EXPECT_TRUE(node->Changed(*root));
+    EXPECT_TRUE(child1->Changed(*node));
+    EXPECT_TRUE(child2->Changed(*node));
+    EXPECT_TRUE(grandchild1->Changed(*child1));
+    EXPECT_TRUE(grandchild2->Changed(*child2));
+  }
+
+  void ExpectUnchangedState() {
     EXPECT_FALSE(root->Changed(*root));
     EXPECT_FALSE(node->Changed(*root));
     EXPECT_FALSE(child1->Changed(*root));
@@ -80,10 +89,11 @@ TEST_F(PaintPropertyNodeTest, LowestCommonAncestor) {
 TEST_F(PaintPropertyNodeTest, InitialStateAndReset) {
   ExpectInitialState();
   ResetAllChanged();
-  ExpectInitialState();
+  ExpectUnchangedState();
 }
 
 TEST_F(PaintPropertyNodeTest, ChangeNode) {
+  ResetAllChanged();
   Update(node, root, FloatRoundedRect(1, 2, 3, 4));
   EXPECT_TRUE(node->Changed(*root));
   EXPECT_FALSE(node->Changed(*node));
@@ -96,10 +106,11 @@ TEST_F(PaintPropertyNodeTest, ChangeNode) {
   EXPECT_FALSE(grandchild1->Changed(*grandchild2));
 
   ResetAllChanged();
-  ExpectInitialState();
+  ExpectUnchangedState();
 }
 
 TEST_F(PaintPropertyNodeTest, ChangeOneChild) {
+  ResetAllChanged();
   Update(child1, node, FloatRoundedRect(1, 2, 3, 4));
   EXPECT_FALSE(node->Changed(*root));
   EXPECT_FALSE(node->Changed(*node));
@@ -120,10 +131,11 @@ TEST_F(PaintPropertyNodeTest, ChangeOneChild) {
   EXPECT_TRUE(grandchild2->Changed(*grandchild1));
 
   ResetAllChanged();
-  ExpectInitialState();
+  ExpectUnchangedState();
 }
 
 TEST_F(PaintPropertyNodeTest, Reparent) {
+  ResetAllChanged();
   Update(child1, child2, FloatRoundedRect(1, 2, 3, 4));
   EXPECT_FALSE(node->Changed(*root));
   EXPECT_TRUE(child1->Changed(*node));
@@ -134,7 +146,7 @@ TEST_F(PaintPropertyNodeTest, Reparent) {
   EXPECT_TRUE(grandchild1->Changed(*child2));
 
   ResetAllChanged();
-  ExpectInitialState();
+  ExpectUnchangedState();
 }
 
 }  // namespace blink