cc: Fix LayerTree deserialization for commit.

When we receive a commit protobuf from the engine, we collect all the
existing Layers in a map and proceed with deserializing the new LayerTree
in the commit protobuf, re-using any existing Layers if possible.

When deserializing a Layer, Layer::FromLayerNodeProto creates a copy of
the existing children, in order to clear the parent pointers for these
layers, if they have now been added to the children of another layer.
However, since we only traverse the layers from the updated LayerTree
during deserialization, if a Layer has been destroyed on the engine, it will
not be traversed during the derserialization step, and will still hold a
reference to all its children. When this Layer is being destroyed on the
client, it will call Layer::RemoveFromParent for all its children, which
may now have been added to the children list of a different Layer.

This patch clears all the properties that are sent with a Layer Hierarchy
update in Layer::ClearLayerHierarchyPropertiesForDeserializationAndAddToMap,
which is run for the existing LayerTree before parsing the new LayerTree from
the commit protobuf.

BUG=590102
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1804703004

Cr-Commit-Position: refs/heads/master@{#382953}

2 files changed, 49 insertions, 14 deletions

diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
index 16197c3..64622e4 100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc
@@ -1504,19 +1504,11 @@ void LayerTreeHost::FromProtobufForCommit(const proto::LayerTreeHost& proto) {
   // Layer hierarchy.
   scoped_refptr<Layer> new_root_layer =
       LayerProtoConverter::DeserializeLayerHierarchy(root_layer_,
-                                                     proto.root_layer());
+                                                     proto.root_layer(), this);
   if (root_layer_ != new_root_layer) {
-    if (root_layer_)
-      root_layer_->SetLayerTreeHost(nullptr);
     root_layer_ = new_root_layer;
-    root_layer_->SetLayerTreeHost(this);
   }
 
-  // Populate layer_id_map_ with the new layers.
-  layer_id_map_.clear();
-  LayerTreeHostCommon::CallFunctionForSubtree(
-      root_layer(),
-      [this](Layer* layer) { layer_id_map_[layer->id()] = layer; });
   for (auto layer_id : proto.layers_that_should_push_properties())
     layers_that_should_push_properties_.insert(layer_id_map_[layer_id]);
 
diff --git a/cc/trees/layer_tree_host_unittest_serialization.cc b/cc/trees/layer_tree_host_unittest_serialization.cc
index 3e3e0a1..cae21ca 100644
--- a/cc/trees/layer_tree_host_unittest_serialization.cc
+++ b/cc/trees/layer_tree_host_unittest_serialization.cc
@@ -40,13 +40,19 @@ class LayerTreeHostSerializationTest : public testing::Test {
     layer_tree_host_src_->in_paint_layer_contents_ = false;
     layer_tree_host_dst_->in_paint_layer_contents_ = false;
 
-    // Need to reset root layer and LayerTreeHost pointers before tear down.
-    layer_tree_host_src_->SetRootLayer(nullptr);
+    // Need to reset LayerTreeHost pointers before tear down.
     layer_tree_host_src_ = nullptr;
-    layer_tree_host_dst_->SetRootLayer(nullptr);
     layer_tree_host_dst_ = nullptr;
   }
 
+  void VerifyHostHasAllExpectedLayersInTree(Layer* root_layer) {
+    LayerTreeHostCommon::CallFunctionForSubtree(
+        root_layer, [root_layer](Layer* layer) {
+          DCHECK(layer->layer_tree_host());
+          EXPECT_EQ(layer, layer->layer_tree_host()->LayerById(layer->id()));
+        });
+  }
+
   void VerifySerializationAndDeserialization() {
     proto::LayerTreeHost proto;
 
@@ -269,7 +275,7 @@ class LayerTreeHostSerializationTest : public testing::Test {
     VerifySerializationAndDeserialization();
   }
 
-  void LayersChangedMultipleSerializations() {
+  void RunLayersChangedMultipleSerializations() {
     // Just fake setup a layer for both source and dest.
     scoped_refptr<Layer> root_layer_src = Layer::Create();
     layer_tree_host_src_->SetRootLayer(root_layer_src);
@@ -307,6 +313,39 @@ class LayerTreeHostSerializationTest : public testing::Test {
     VerifySerializationAndDeserialization();
   }
 
+  void RunAddAndRemoveNodeFromLayerTree() {
+    /* Testing serialization when the tree hierarchy changes like this:
+         root                  root
+        /    \                /    \
+       a      b        =>    a      c
+               \                     \
+                c                     d
+    */
+    scoped_refptr<Layer> layer_src_root = Layer::Create();
+    layer_tree_host_src_->SetRootLayer(layer_src_root);
+
+    scoped_refptr<Layer> layer_src_a = Layer::Create();
+    scoped_refptr<Layer> layer_src_b = Layer::Create();
+    scoped_refptr<Layer> layer_src_c = Layer::Create();
+    scoped_refptr<Layer> layer_src_d = Layer::Create();
+
+    layer_src_root->AddChild(layer_src_a);
+    layer_src_root->AddChild(layer_src_b);
+    layer_src_b->AddChild(layer_src_c);
+
+    VerifySerializationAndDeserialization();
+    VerifyHostHasAllExpectedLayersInTree(layer_tree_host_dst_->root_layer());
+
+    // Now change the Layer Hierarchy
+    layer_src_c->RemoveFromParent();
+    layer_src_b->RemoveFromParent();
+    layer_src_root->AddChild(layer_src_c);
+    layer_src_c->AddChild(layer_src_d);
+
+    VerifySerializationAndDeserialization();
+    VerifyHostHasAllExpectedLayersInTree(layer_tree_host_dst_->root_layer());
+  }
+
  private:
   TestTaskGraphRunner task_graph_runner_src_;
   FakeLayerTreeHostClient client_src_;
@@ -326,7 +365,11 @@ TEST_F(LayerTreeHostSerializationTest, LayersChanged) {
 }
 
 TEST_F(LayerTreeHostSerializationTest, LayersChangedMultipleSerializations) {
-  LayersChangedMultipleSerializations();
+  RunLayersChangedMultipleSerializations();
+}
+
+TEST_F(LayerTreeHostSerializationTest, AddAndRemoveNodeFromLayerTree) {
+  RunAddAndRemoveNodeFromLayerTree();
 }
 
 }  // namespace cc