Fix up cc::Layer scroll parent management

The previous code was trying to perform scroll parent pointer fixup during
LayerImpl destruction, which is problematic since ~LayerImpl() always runs
during tree synchronization and there is nothing to ensure that the scroll
parent's portion of the tree has be fully synchronized when a particular
scroll child is being destroyed.

Instead, this performs pointer fixup on the main thread and then just pushes
the values through to the LayerImpl tree without performing additional fixups.
So long as the main-thread (cc::Layer) tree stays structurally valid there's
no need for additional changes at layer destruction or tree manipulation time
on any LayerImpl trees.

BUG=347284

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@254244 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 113 insertions, 78 deletions

diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index 204d907..63b1288 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -957,22 +957,33 @@ void Layer::PushPropertiesTo(LayerImpl* layer) {
   layer->set_user_scrollable_vertical(user_scrollable_vertical_);
 
   LayerImpl* scroll_parent = NULL;
-  if (scroll_parent_)
+  if (scroll_parent_) {
     scroll_parent = layer->layer_tree_impl()->LayerById(scroll_parent_->id());
+    DCHECK(scroll_parent);
+  }
 
   layer->SetScrollParent(scroll_parent);
   if (scroll_children_) {
     std::set<LayerImpl*>* scroll_children = new std::set<LayerImpl*>;
     for (std::set<Layer*>::iterator it = scroll_children_->begin();
-        it != scroll_children_->end(); ++it)
-      scroll_children->insert(layer->layer_tree_impl()->LayerById((*it)->id()));
+         it != scroll_children_->end();
+         ++it) {
+      DCHECK_EQ((*it)->scroll_parent(), this);
+      LayerImpl* scroll_child =
+          layer->layer_tree_impl()->LayerById((*it)->id());
+      DCHECK(scroll_child);
+      scroll_children->insert(scroll_child);
+    }
     layer->SetScrollChildren(scroll_children);
+  } else {
+    layer->SetScrollChildren(NULL);
   }
 
   LayerImpl* clip_parent = NULL;
   if (clip_parent_) {
     clip_parent =
         layer->layer_tree_impl()->LayerById(clip_parent_->id());
+    DCHECK(clip_parent);
   }
 
   layer->SetClipParent(clip_parent);
@@ -980,11 +991,14 @@ void Layer::PushPropertiesTo(LayerImpl* layer) {
     std::set<LayerImpl*>* clip_children = new std::set<LayerImpl*>;
     for (std::set<Layer*>::iterator it = clip_children_->begin();
         it != clip_children_->end(); ++it) {
+      DCHECK_EQ((*it)->clip_parent(), this);
       LayerImpl* clip_child = layer->layer_tree_impl()->LayerById((*it)->id());
       DCHECK(clip_child);
       clip_children->insert(clip_child);
     }
     layer->SetClipChildren(clip_children);
+  } else {
+    layer->SetClipChildren(NULL);
   }
 
   // Adjust the scroll delta to be just the scrolls that have happened since
diff --git a/cc/layers/layer_impl.cc b/cc/layers/layer_impl.cc
index 2167341..06e14a7 100644
--- a/cc/layers/layer_impl.cc
+++ b/cc/layers/layer_impl.cc
@@ -93,24 +93,6 @@ LayerImpl::~LayerImpl() {
     layer_tree_impl()->RemoveLayerWithCopyOutputRequest(this);
   layer_tree_impl_->UnregisterLayer(this);
 
-  if (scroll_children_) {
-    for (std::set<LayerImpl*>::iterator it = scroll_children_->begin();
-        it != scroll_children_->end(); ++it)
-      (*it)->scroll_parent_ = NULL;
-  }
-
-  if (scroll_parent_)
-    scroll_parent_->RemoveScrollChild(this);
-
-  if (clip_children_) {
-    for (std::set<LayerImpl*>::iterator it = clip_children_->begin();
-        it != clip_children_->end(); ++it)
-      (*it)->clip_parent_ = NULL;
-  }
-
-  if (clip_parent_)
-    clip_parent_->RemoveClipChild(this);
-
   TRACE_EVENT_OBJECT_DELETED_WITH_ID(
       TRACE_DISABLED_BY_DEFAULT("cc.debug"), "cc::LayerImpl", this);
 }
@@ -173,8 +155,8 @@ void LayerImpl::SetScrollParent(LayerImpl* parent) {
   // Having both a scroll parent and a scroll offset delegate is unsupported.
   DCHECK(!scroll_offset_delegate_);
 
-  if (scroll_parent_)
-    scroll_parent_->RemoveScrollChild(this);
+  if (parent)
+    DCHECK_EQ(layer_tree_impl()->LayerById(parent->id()), parent);
 
   scroll_parent_ = parent;
   SetNeedsPushProperties();
@@ -193,21 +175,10 @@ void LayerImpl::SetScrollChildren(std::set<LayerImpl*>* children) {
   SetNeedsPushProperties();
 }
 
-void LayerImpl::RemoveScrollChild(LayerImpl* child) {
-  DCHECK(scroll_children_);
-  scroll_children_->erase(child);
-  if (scroll_children_->empty())
-    scroll_children_.reset();
-  SetNeedsPushProperties();
-}
-
 void LayerImpl::SetClipParent(LayerImpl* ancestor) {
   if (clip_parent_ == ancestor)
     return;
 
-  if (clip_parent_)
-    clip_parent_->RemoveClipChild(this);
-
   clip_parent_ = ancestor;
   SetNeedsPushProperties();
 }
@@ -219,14 +190,6 @@ void LayerImpl::SetClipChildren(std::set<LayerImpl*>* children) {
   SetNeedsPushProperties();
 }
 
-void LayerImpl::RemoveClipChild(LayerImpl* child) {
-  DCHECK(clip_children_);
-  clip_children_->erase(child);
-  if (clip_children_->empty())
-    clip_children_.reset();
-  SetNeedsPushProperties();
-}
-
 void LayerImpl::PassCopyRequests(ScopedPtrVector<CopyOutputRequest>* requests) {
   if (requests->empty())
     return;
@@ -568,22 +531,33 @@ void LayerImpl::PushPropertiesTo(LayerImpl* layer) {
   layer->SetSentScrollDelta(gfx::Vector2d());
 
   LayerImpl* scroll_parent = NULL;
-  if (scroll_parent_)
+  if (scroll_parent_) {
     scroll_parent = layer->layer_tree_impl()->LayerById(scroll_parent_->id());
+    DCHECK(scroll_parent);
+  }
 
   layer->SetScrollParent(scroll_parent);
   if (scroll_children_) {
     std::set<LayerImpl*>* scroll_children = new std::set<LayerImpl*>;
     for (std::set<LayerImpl*>::iterator it = scroll_children_->begin();
-        it != scroll_children_->end(); ++it)
-      scroll_children->insert(layer->layer_tree_impl()->LayerById((*it)->id()));
+         it != scroll_children_->end();
+         ++it) {
+      DCHECK_EQ((*it)->scroll_parent(), this);
+      LayerImpl* scroll_child =
+          layer->layer_tree_impl()->LayerById((*it)->id());
+      DCHECK(scroll_child);
+      scroll_children->insert(scroll_child);
+    }
     layer->SetScrollChildren(scroll_children);
+  } else {
+    layer->SetScrollChildren(NULL);
   }
 
   LayerImpl* clip_parent = NULL;
   if (clip_parent_) {
     clip_parent = layer->layer_tree_impl()->LayerById(
         clip_parent_->id());
+    DCHECK(clip_parent);
   }
 
   layer->SetClipParent(clip_parent);
@@ -593,6 +567,8 @@ void LayerImpl::PushPropertiesTo(LayerImpl* layer) {
         it != clip_children_->end(); ++it)
       clip_children->insert(layer->layer_tree_impl()->LayerById((*it)->id()));
     layer->SetClipChildren(clip_children);
+  } else {
+    layer->SetClipChildren(NULL);
   }
 
   layer->PassCopyRequests(&copy_requests_);
diff --git a/cc/layers/layer_impl.h b/cc/layers/layer_impl.h
index c86c554..e1fcfc1 100644
--- a/cc/layers/layer_impl.h
+++ b/cc/layers/layer_impl.h
@@ -111,7 +111,6 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   const LayerImpl* scroll_parent() const { return scroll_parent_; }
 
   void SetScrollChildren(std::set<LayerImpl*>* children);
-  void RemoveScrollChild(LayerImpl* child);
 
   std::set<LayerImpl*>* scroll_children() { return scroll_children_.get(); }
   const std::set<LayerImpl*>* scroll_children() const {
@@ -128,7 +127,6 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   }
 
   void SetClipChildren(std::set<LayerImpl*>* children);
-  void RemoveClipChild(LayerImpl* child);
 
   std::set<LayerImpl*>* clip_children() { return clip_children_.get(); }
   const std::set<LayerImpl*>* clip_children() const {
diff --git a/cc/layers/layer_impl_unittest.cc b/cc/layers/layer_impl_unittest.cc
index 016aa70..064b807 100644
--- a/cc/layers/layer_impl_unittest.cc
+++ b/cc/layers/layer_impl_unittest.cc
@@ -193,13 +193,9 @@ TEST(LayerImplTest, VerifyLayerChangesAreTrackedProperly) {
   EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
       root->SetScrollChildren(scroll_children));
   EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
-      root->RemoveScrollChild(scroll_child));
-  EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
       root->SetClipParent(clip_parent.get()));
   EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
       root->SetClipChildren(clip_children));
-  EXECUTE_AND_VERIFY_NEEDS_PUSH_PROPERTIES_AND_SUBTREE_DID_NOT_CHANGE(
-      root->RemoveClipChild(clip_child));
 
   // After setting all these properties already, setting to the exact same
   // values again should not cause any change.
diff --git a/cc/trees/tree_synchronizer.cc b/cc/trees/tree_synchronizer.cc
index 1c1c7e1..6d52f35 100644
--- a/cc/trees/tree_synchronizer.cc
+++ b/cc/trees/tree_synchronizer.cc
@@ -4,6 +4,8 @@
 
 #include "cc/trees/tree_synchronizer.h"
 
+#include <set>
+
 #include "base/containers/hash_tables.h"
 #include "base/containers/scoped_ptr_hash_map.h"
 #include "base/debug/trace_event.h"
@@ -234,11 +236,64 @@ void TreeSynchronizer::PushPropertiesInternal(
   *num_dependents_need_push_properties_for_parent += add_self_to_parent ? 1 : 0;
 }
 
+static void CheckScrollAndClipPointersRecursive(Layer* layer,
+                                                LayerImpl* layer_impl) {
+  DCHECK_EQ(!!layer, !!layer_impl);
+  if (!layer)
+    return;
+
+  DCHECK_EQ(!!layer->scroll_parent(), !!layer_impl->scroll_parent());
+  if (layer->scroll_parent())
+    DCHECK_EQ(layer->scroll_parent()->id(), layer_impl->scroll_parent()->id());
+
+  DCHECK_EQ(!!layer->clip_parent(), !!layer_impl->clip_parent());
+  if (layer->clip_parent())
+    DCHECK_EQ(layer->clip_parent()->id(), layer_impl->clip_parent()->id());
+
+  DCHECK_EQ(!!layer->scroll_children(), !!layer_impl->scroll_children());
+  if (layer->scroll_children()) {
+    for (std::set<Layer*>::iterator it = layer->scroll_children()->begin();
+         it != layer->scroll_children()->end();
+         ++it) {
+      DCHECK_EQ((*it)->scroll_parent(), layer);
+    }
+    for (std::set<LayerImpl*>::iterator it =
+             layer_impl->scroll_children()->begin();
+         it != layer_impl->scroll_children()->end();
+         ++it) {
+      DCHECK_EQ((*it)->scroll_parent(), layer_impl);
+    }
+  }
+
+  DCHECK_EQ(!!layer->clip_children(), !!layer_impl->clip_children());
+  if (layer->clip_children()) {
+    for (std::set<Layer*>::iterator it = layer->clip_children()->begin();
+         it != layer->clip_children()->end();
+         ++it) {
+      DCHECK_EQ((*it)->clip_parent(), layer);
+    }
+    for (std::set<LayerImpl*>::iterator it =
+             layer_impl->clip_children()->begin();
+         it != layer_impl->clip_children()->end();
+         ++it) {
+      DCHECK_EQ((*it)->clip_parent(), layer_impl);
+    }
+  }
+
+  for (size_t i = 0u; i < layer->children().size(); ++i) {
+    CheckScrollAndClipPointersRecursive(layer->child_at(i),
+                                        layer_impl->child_at(i));
+  }
+}
+
 void TreeSynchronizer::PushProperties(Layer* layer,
                                       LayerImpl* layer_impl) {
   size_t num_dependents_need_push_properties = 0;
   PushPropertiesInternal(
       layer, layer_impl, &num_dependents_need_push_properties);
+  if (DCHECK_IS_ON()) {
+    CheckScrollAndClipPointersRecursive(layer, layer_impl);
+  }
 }
 
 void TreeSynchronizer::PushProperties(LayerImpl* layer, LayerImpl* layer_impl) {
diff --git a/cc/trees/tree_synchronizer_unittest.cc b/cc/trees/tree_synchronizer_unittest.cc
index 827c366..2e853ea 100644
--- a/cc/trees/tree_synchronizer_unittest.cc
+++ b/cc/trees/tree_synchronizer_unittest.cc
@@ -8,6 +8,8 @@
 #include <set>
 #include <vector>
 
+#include "base/format_macros.h"
+#include "base/strings/stringprintf.h"
 #include "cc/animation/layer_animation_controller.h"
 #include "cc/layers/layer.h"
 #include "cc/layers/layer_impl.h"
@@ -110,12 +112,14 @@ void ExpectTreesAreIdentical(Layer* layer,
 
   ASSERT_EQ(!!layer->mask_layer(), !!layer_impl->mask_layer());
   if (layer->mask_layer()) {
+    SCOPED_TRACE("mask_layer");
     ExpectTreesAreIdentical(
         layer->mask_layer(), layer_impl->mask_layer(), tree_impl);
   }
 
   ASSERT_EQ(!!layer->replica_layer(), !!layer_impl->replica_layer());
   if (layer->replica_layer()) {
+    SCOPED_TRACE("replica_layer");
     ExpectTreesAreIdentical(
         layer->replica_layer(), layer_impl->replica_layer(), tree_impl);
   }
@@ -176,6 +180,7 @@ void ExpectTreesAreIdentical(Layer* layer,
   }
 
   for (size_t i = 0; i < layer_children.size(); ++i) {
+    SCOPED_TRACE(base::StringPrintf("child layer %" PRIuS, i).c_str());
     ExpectTreesAreIdentical(
         layer_children[i].get(), layer_impl_children[i], tree_impl);
   }
@@ -605,9 +610,12 @@ TEST_F(TreeSynchronizerTest, SynchronizeScrollParent) {
                                          host_impl->active_tree());
   TreeSynchronizer::PushProperties(layer_tree_root.get(),
                                    layer_impl_tree_root.get());
-  ExpectTreesAreIdentical(layer_tree_root.get(),
-                          layer_impl_tree_root.get(),
-                          host_impl->active_tree());
+  {
+    SCOPED_TRACE("case one");
+    ExpectTreesAreIdentical(layer_tree_root.get(),
+                            layer_impl_tree_root.get(),
+                            host_impl->active_tree());
+  }
 
   // Remove the first scroll child.
   layer_tree_root->children()[1]->RemoveFromParent();
@@ -617,9 +625,12 @@ TEST_F(TreeSynchronizerTest, SynchronizeScrollParent) {
                                          host_impl->active_tree());
   TreeSynchronizer::PushProperties(layer_tree_root.get(),
                                    layer_impl_tree_root.get());
-  ExpectTreesAreIdentical(layer_tree_root.get(),
-                          layer_impl_tree_root.get(),
-                          host_impl->active_tree());
+  {
+    SCOPED_TRACE("case two");
+    ExpectTreesAreIdentical(layer_tree_root.get(),
+                            layer_impl_tree_root.get(),
+                            host_impl->active_tree());
+  }
 
   // Add an additional scroll layer.
   scoped_refptr<Layer> additional_scroll_child = Layer::Create();
@@ -631,27 +642,12 @@ TEST_F(TreeSynchronizerTest, SynchronizeScrollParent) {
                                          host_impl->active_tree());
   TreeSynchronizer::PushProperties(layer_tree_root.get(),
                                    layer_impl_tree_root.get());
-  ExpectTreesAreIdentical(layer_tree_root.get(),
-                          layer_impl_tree_root.get(),
-                          host_impl->active_tree());
-
-  // Remove the scroll parent.
-  scroll_parent->RemoveFromParent();
-  scroll_parent = NULL;
-  layer_impl_tree_root =
-      TreeSynchronizer::SynchronizeTrees(layer_tree_root.get(),
-                                         layer_impl_tree_root.Pass(),
-                                         host_impl->active_tree());
-  TreeSynchronizer::PushProperties(layer_tree_root.get(),
-                                   layer_impl_tree_root.get());
-  ExpectTreesAreIdentical(layer_tree_root.get(),
-                          layer_impl_tree_root.get(),
-                          host_impl->active_tree());
-
-  // The scroll children should have been unhooked.
-  EXPECT_EQ(2u, layer_tree_root->children().size());
-  EXPECT_FALSE(layer_tree_root->children()[0]->scroll_parent());
-  EXPECT_FALSE(layer_tree_root->children()[1]->scroll_parent());
+  {
+    SCOPED_TRACE("case three");
+    ExpectTreesAreIdentical(layer_tree_root.get(),
+                            layer_impl_tree_root.get(),
+                            host_impl->active_tree());
+  }
 }
 
 TEST_F(TreeSynchronizerTest, SynchronizeClipParent) {