Cherry-pick: Fix RemoveFromScrollTree and RemoveFromClipTree

Clean cherry-pick of chromium
crrev.com/66133e86b6d79534605539aa684a248e6b6205bf

BUG: 17612259

Original description:

These functions should set needs commit, but didn't. This lead to stale
pointers in the impl tree.

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

Change-Id: I59ba963f2ad44aa6f64366ca30c7a64a97570fb3
Cr-Commit-Position: refs/heads/master@{#294749}
diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index e00d2c5..464714e 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -87,6 +87,9 @@
   layer_animation_controller_->RemoveValueObserver(this);
   layer_animation_controller_->remove_value_provider(this);
 
+  RemoveFromScrollTree();
+  RemoveFromClipTree();
+
   // Remove the parent reference from all children and dependents.
   RemoveAllChildren();
   if (mask_layer_.get()) {
@@ -97,9 +100,6 @@
     DCHECK_EQ(this, replica_layer_->parent());
     replica_layer_->RemoveFromParent();
   }
-
-  RemoveFromScrollTree();
-  RemoveFromClipTree();
 }
 
 void Layer::SetLayerTreeHost(LayerTreeHost* host) {
@@ -1159,28 +1159,24 @@
 
 void Layer::RemoveFromScrollTree() {
   if (scroll_children_.get()) {
-    for (std::set<Layer*>::iterator it = scroll_children_->begin();
-        it != scroll_children_->end(); ++it)
-      (*it)->scroll_parent_ = NULL;
+    std::set<Layer*> copy = *scroll_children_;
+    for (std::set<Layer*>::iterator it = copy.begin(); it != copy.end(); ++it)
+      (*it)->SetScrollParent(NULL);
   }
 
-  if (scroll_parent_)
-    scroll_parent_->RemoveScrollChild(this);
-
-  scroll_parent_ = NULL;
+  DCHECK(!scroll_children_);
+  SetScrollParent(NULL);
 }
 
 void Layer::RemoveFromClipTree() {
   if (clip_children_.get()) {
-    for (std::set<Layer*>::iterator it = clip_children_->begin();
-        it != clip_children_->end(); ++it)
-      (*it)->clip_parent_ = NULL;
+    std::set<Layer*> copy = *clip_children_;
+    for (std::set<Layer*>::iterator it = copy.begin(); it != copy.end(); ++it)
+      (*it)->SetClipParent(NULL);
   }
 
-  if (clip_parent_)
-    clip_parent_->RemoveClipChild(this);
-
-  clip_parent_ = NULL;
+  DCHECK(!clip_children_);
+  SetClipParent(NULL);
 }
 
 void Layer::RunMicroBenchmark(MicroBenchmark* benchmark) {
diff --git a/cc/layers/layer.h b/cc/layers/layer.h
index ab897a8..8b889bb 100644
--- a/cc/layers/layer.h
+++ b/cc/layers/layer.h
@@ -494,14 +494,6 @@
 
   bool IsPropertyChangeAllowed() const;
 
-  // If this layer has a scroll parent, it removes |this| from its list of
-  // scroll children.
-  void RemoveFromScrollTree();
-
-  // If this layer has a clip parent, it removes |this| from its list of clip
-  // children.
-  void RemoveFromClipTree();
-
   void reset_raster_scale_to_unknown() { raster_scale_ = 0.f; }
 
   // This flag is set when the layer needs to push properties to the impl
@@ -561,6 +553,14 @@
   virtual void OnAnimationWaitingForDeletion() OVERRIDE;
   virtual bool IsActive() const OVERRIDE;
 
+  // If this layer has a scroll parent, it removes |this| from its list of
+  // scroll children.
+  void RemoveFromScrollTree();
+
+  // If this layer has a clip parent, it removes |this| from its list of clip
+  // children.
+  void RemoveFromClipTree();
+
   LayerList children_;
   Layer* parent_;
 
diff --git a/cc/layers/layer_unittest.cc b/cc/layers/layer_unittest.cc
index ba77513..fde9dcb 100644
--- a/cc/layers/layer_unittest.cc
+++ b/cc/layers/layer_unittest.cc
@@ -337,6 +337,64 @@
   EXPECT_FALSE(child2_->parent());
 }
 
+TEST_F(LayerTest, DeleteRemovedScrollParent) {
+  scoped_refptr<Layer> parent = Layer::Create();
+  scoped_refptr<Layer> child1 = Layer::Create();
+  scoped_refptr<Layer> child2 = Layer::Create();
+
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, layer_tree_host_->SetRootLayer(parent));
+
+  ASSERT_EQ(0U, parent->children().size());
+
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, parent->InsertChild(child1, 0));
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, parent->InsertChild(child2, 1));
+
+  ASSERT_EQ(2U, parent->children().size());
+  EXPECT_EQ(child1, parent->children()[0]);
+  EXPECT_EQ(child2, parent->children()[1]);
+
+  EXPECT_SET_NEEDS_COMMIT(2, child1->SetScrollParent(child2.get()));
+
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, child2->RemoveFromParent());
+
+  child1->reset_needs_push_properties_for_testing();
+
+  EXPECT_SET_NEEDS_COMMIT(1, child2 = NULL);
+
+  EXPECT_TRUE(child1->needs_push_properties());
+
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, layer_tree_host_->SetRootLayer(NULL));
+}
+
+TEST_F(LayerTest, DeleteRemovedScrollChild) {
+  scoped_refptr<Layer> parent = Layer::Create();
+  scoped_refptr<Layer> child1 = Layer::Create();
+  scoped_refptr<Layer> child2 = Layer::Create();
+
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, layer_tree_host_->SetRootLayer(parent));
+
+  ASSERT_EQ(0U, parent->children().size());
+
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, parent->InsertChild(child1, 0));
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, parent->InsertChild(child2, 1));
+
+  ASSERT_EQ(2U, parent->children().size());
+  EXPECT_EQ(child1, parent->children()[0]);
+  EXPECT_EQ(child2, parent->children()[1]);
+
+  EXPECT_SET_NEEDS_COMMIT(2, child1->SetScrollParent(child2.get()));
+
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, child1->RemoveFromParent());
+
+  child2->reset_needs_push_properties_for_testing();
+
+  EXPECT_SET_NEEDS_COMMIT(1, child1 = NULL);
+
+  EXPECT_TRUE(child2->needs_push_properties());
+
+  EXPECT_SET_NEEDS_FULL_TREE_SYNC(1, layer_tree_host_->SetRootLayer(NULL));
+}
+
 TEST_F(LayerTest, ReplaceChildWithSameChild) {
   CreateSimpleTestTree();
 
diff --git a/cc/trees/tree_synchronizer.cc b/cc/trees/tree_synchronizer.cc
index a54bd22..1faf0e0 100644
--- a/cc/trees/tree_synchronizer.cc
+++ b/cc/trees/tree_synchronizer.cc
@@ -242,16 +242,23 @@
   if (!layer)
     return;
 
-  DCHECK_EQ(!!layer->scroll_parent(), !!layer_impl->scroll_parent());
-  if (layer->scroll_parent())
+  // Having a scroll parent on the impl thread implies having one the main
+  // thread, too. The main thread may have a scroll parent that is not in the
+  // tree because it's been removed but not deleted. In this case, the layer
+  // impl will have no scroll parent. Same argument applies for clip parents and
+  // scroll/clip children.
+  DCHECK(!layer_impl->scroll_parent() || !!layer->scroll_parent());
+  DCHECK(!layer_impl->clip_parent() || !!layer->clip_parent());
+  DCHECK(!layer_impl->scroll_children() || !!layer->scroll_children());
+  DCHECK(!layer_impl->clip_children() || !!layer->clip_children());
+
+  if (layer_impl->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())
+  if (layer_impl->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()) {
+  if (layer_impl->scroll_children()) {
     for (std::set<Layer*>::iterator it = layer->scroll_children()->begin();
          it != layer->scroll_children()->end();
          ++it) {
@@ -265,8 +272,7 @@
     }
   }
 
-  DCHECK_EQ(!!layer->clip_children(), !!layer_impl->clip_children());
-  if (layer->clip_children()) {
+  if (layer_impl->clip_children()) {
     for (std::set<Layer*>::iterator it = layer->clip_children()->begin();
          it != layer->clip_children()->end();
          ++it) {