Revert 204442 "Split Layer::SetScrollOffset to two functions for..."

webkit_unit_tests gets broken after 20442.
Logs:
 http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/11442
 http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/4910

> Split Layer::SetScrollOffset to two functions for different use
> 
> Currently there are two callers to Layer::SetScrollOffset. One is from
> the Blink ScrollingCoordinator to setup scrolling parameters. The other
> one is from the threaded compositing commits to apply scroll deltas.
> 
> After the patch we have Layer::SetScrollOffset for Blink. This version
> will setNeedsCommit(), but will not notify ScrollableArea (who initiated
> the scroll anyway) for the change. And a new function
> Layer::SetScrollOffsetFromImplSide for commits. This version will notify
> ScrollableArea about the change, but won't setNeedsCommit() for no reason.
> 
> This fixes a use-after-free bug in Layer::SetScrollOffset, due to
> ScrollableArea destroying the layer during the process then the layer
> attempts to setNeedsCommit().
> 
> R=jamesr@chromium.org
> BUG=245713,245987
> NOTRY=true
> TEST=cc_unittests:LayerTreeHostScrollTestLayerStructureChange.*
> 
> Review URL: https://chromiumcodereview.appspot.com/15984005

TBR=trchen@chromium.org

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

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

6 files changed, 14 insertions, 118 deletions

diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index 5ec0621..09e1a46 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -515,19 +515,9 @@ void Layer::SetScrollOffset(gfx::Vector2d scroll_offset) {
   if (scroll_offset_ == scroll_offset)
     return;
   scroll_offset_ = scroll_offset;
-  SetNeedsCommit();
-}
-
-void Layer::SetScrollOffsetFromImplSide(gfx::Vector2d scroll_offset) {
-  DCHECK(IsPropertyChangeAllowed());
-  DCHECK(layer_tree_host_ && layer_tree_host_->CommitRequested());
-  if (scroll_offset_ == scroll_offset)
-    return;
-  scroll_offset_ = scroll_offset;
   if (layer_scroll_client_)
     layer_scroll_client_->didScroll();
-  // Note: didScroll() could potentially change the layer structure.
-  //       "this" may have been destroyed during the process.
+  SetNeedsCommit();
 }
 
 void Layer::SetMaxScrollOffset(gfx::Vector2d max_scroll_offset) {
diff --git a/cc/layers/layer.h b/cc/layers/layer.h
index 91972b3..fc7113d 100644
--- a/cc/layers/layer.h
+++ b/cc/layers/layer.h
@@ -208,7 +208,6 @@ class CC_EXPORT Layer : public base::RefCounted<Layer>,
 
   void SetScrollOffset(gfx::Vector2d scroll_offset);
   gfx::Vector2d scroll_offset() const { return scroll_offset_; }
-  void SetScrollOffsetFromImplSide(gfx::Vector2d scroll_offset);
 
   void SetMaxScrollOffset(gfx::Vector2d max_scroll_offset);
   gfx::Vector2d max_scroll_offset() const { return max_scroll_offset_; }
diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
index 4ba7d59..108a711 100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc
@@ -300,7 +300,7 @@ void LayerTreeHost::FinishCommitOnImplThread(LayerTreeHostImpl* host_impl) {
   sync_tree->set_needs_full_tree_sync(needs_full_tree_sync_);
   needs_full_tree_sync_ = false;
 
-  if (hud_layer_.get()) {
+  if (root_layer_.get() && hud_layer_.get()) {
     LayerImpl* hud_impl = LayerTreeHostCommon::FindLayerInSubtree(
         sync_tree->root_layer(), hud_layer_->id());
     sync_tree->set_hud_layer(static_cast<HeadsUpDisplayLayerImpl*>(hud_impl));
@@ -963,12 +963,11 @@ void LayerTreeHost::ApplyScrollAndScale(const ScrollAndScaleSet& info) {
                                                 info.scrolls[i].layer_id);
     if (!layer)
       continue;
-    if (layer == root_scroll_layer) {
+    if (layer == root_scroll_layer)
       root_scroll_delta += info.scrolls[i].scroll_delta;
-    } else {
-      layer->SetScrollOffsetFromImplSide(layer->scroll_offset() +
-                                         info.scrolls[i].scroll_delta);
-    }
+    else
+      layer->SetScrollOffset(layer->scroll_offset() +
+                             info.scrolls[i].scroll_delta);
   }
   if (!root_scroll_delta.IsZero() || info.page_scale_delta != 1.f)
     client_->ApplyScrollAndScale(root_scroll_delta, info.page_scale_delta);
diff --git a/cc/trees/layer_tree_host_common.h b/cc/trees/layer_tree_host_common.h
index 165cd5e..7712db2 100644
--- a/cc/trees/layer_tree_host_common.h
+++ b/cc/trees/layer_tree_host_common.h
@@ -115,9 +115,6 @@ bool LayerTreeHostCommon::RenderSurfaceContributesToTarget(
 template <typename LayerType>
 LayerType* LayerTreeHostCommon::FindLayerInSubtree(LayerType* root_layer,
                                                    int layer_id) {
-  if (!root_layer)
-    return NULL;
-
   if (root_layer->id() == layer_id)
     return root_layer;
 
diff --git a/cc/trees/layer_tree_host_unittest_scroll.cc b/cc/trees/layer_tree_host_unittest_scroll.cc
index febf23f..a840cf5 100644
--- a/cc/trees/layer_tree_host_unittest_scroll.cc
+++ b/cc/trees/layer_tree_host_unittest_scroll.cc
@@ -752,98 +752,5 @@ TEST(LayerTreeHostFlingTest, DidStopFlingingThread) {
   EXPECT_TRUE(received_stop_flinging);
 }
 
-class LayerTreeHostScrollTestLayerStructureChange
-    : public LayerTreeHostScrollTest {
- public:
-  LayerTreeHostScrollTestLayerStructureChange()
-      : scroll_destroy_whole_tree_(false) {}
-
-  virtual void SetupTree() OVERRIDE {
-    scoped_refptr<Layer> root_layer = Layer::Create();
-    root_layer->SetBounds(gfx::Size(10, 10));
-
-    Layer* root_scroll_layer = CreateScrollLayer(root_layer,
-        &root_scroll_layer_client_);
-    CreateScrollLayer(root_layer, &sibling_scroll_layer_client_);
-    CreateScrollLayer(root_scroll_layer, &child_scroll_layer_client_);
-
-    layer_tree_host()->SetRootLayer(root_layer);
-    LayerTreeHostScrollTest::SetupTree();
-  }
-
-  virtual void BeginTest() OVERRIDE {
-    PostSetNeedsCommitToMainThread();
-  }
-
-  virtual void DrawLayersOnThread(LayerTreeHostImpl* impl) OVERRIDE {
-    LayerImpl* root = impl->active_tree()->root_layer();
-    switch (impl->active_tree()->source_frame_number()) {
-      case 0:
-        root->child_at(0)->SetScrollDelta(gfx::Vector2dF(5, 5));
-        root->child_at(0)->child_at(0)->SetScrollDelta(gfx::Vector2dF(5, 5));
-        root->child_at(1)->SetScrollDelta(gfx::Vector2dF(5, 5));
-        PostSetNeedsCommitToMainThread();
-        break;
-      case 1:
-        EndTest();
-        break;
-    }
-  }
-
-  virtual void AfterTest() OVERRIDE {}
-
-  virtual void DidScroll(Layer* layer) {
-    if (scroll_destroy_whole_tree_) {
-      layer_tree_host()->SetRootLayer(NULL);
-      EndTest();
-      return;
-    }
-    layer->RemoveFromParent();
-  }
-
- protected:
-  class FakeWebLayerScrollClient : public WebKit::WebLayerScrollClient {
-   public:
-    virtual void didScroll() OVERRIDE {
-      owner_->DidScroll(layer_);
-    }
-    LayerTreeHostScrollTestLayerStructureChange* owner_;
-    Layer* layer_;
-  };
-
-  Layer* CreateScrollLayer(Layer* parent, FakeWebLayerScrollClient* client) {
-    scoped_refptr<Layer> scroll_layer =
-        ContentLayer::Create(&fake_content_layer_client_);
-    scroll_layer->SetBounds(gfx::Size(110, 110));
-    scroll_layer->SetPosition(gfx::Point(0, 0));
-    scroll_layer->SetAnchorPoint(gfx::PointF());
-    scroll_layer->SetIsDrawable(true);
-    scroll_layer->SetScrollable(true);
-    scroll_layer->SetMaxScrollOffset(gfx::Vector2d(100, 100));
-    scroll_layer->set_layer_scroll_client(client);
-    client->owner_ = this;
-    client->layer_ = scroll_layer;
-    parent->AddChild(scroll_layer);
-    return scroll_layer.get();
-  }
-
-  FakeWebLayerScrollClient root_scroll_layer_client_;
-  FakeWebLayerScrollClient sibling_scroll_layer_client_;
-  FakeWebLayerScrollClient child_scroll_layer_client_;
-
-  FakeContentLayerClient fake_content_layer_client_;
-
-  bool scroll_destroy_whole_tree_;
-};
-
-TEST_F(LayerTreeHostScrollTestLayerStructureChange, ScrollDestroyLayer) {
-    RunTest(true, false, false);
-}
-
-TEST_F(LayerTreeHostScrollTestLayerStructureChange, ScrollDestroyWholeTree) {
-    scroll_destroy_whole_tree_ = true;
-    RunTest(true, false, false);
-}
-
 }  // namespace
 }  // namespace cc
diff --git a/cc/trees/layer_tree_impl.cc b/cc/trees/layer_tree_impl.cc
index 6bc370c..f006032 100644
--- a/cc/trees/layer_tree_impl.cc
+++ b/cc/trees/layer_tree_impl.cc
@@ -79,7 +79,7 @@ void LayerTreeImpl::FindRootScrollLayer() {
         root_layer_scroll_offset_delegate_);
   }
 
-  if (scrolling_layer_id_from_previous_tree_) {
+  if (root_layer_ && scrolling_layer_id_from_previous_tree_) {
     currently_scrolling_layer_ = LayerTreeHostCommon::FindLayerInSubtree(
         root_layer_.get(),
         scrolling_layer_id_from_previous_tree_);
@@ -341,9 +341,13 @@ void LayerTreeImpl::UnregisterLayer(LayerImpl* layer) {
 }
 
 void LayerTreeImpl::PushPersistedState(LayerTreeImpl* pending_tree) {
-  pending_tree->SetCurrentlyScrollingLayer(
-      LayerTreeHostCommon::FindLayerInSubtree(pending_tree->root_layer(),
-          currently_scrolling_layer_ ? currently_scrolling_layer_->id() : 0));
+  int id = currently_scrolling_layer_ ? currently_scrolling_layer_->id() : 0;
+  LayerImpl* pending_scrolling_layer_twin = NULL;
+  if (pending_tree->root_layer()) {
+    pending_scrolling_layer_twin =
+        LayerTreeHostCommon::FindLayerInSubtree(pending_tree->root_layer(), id);
+  }
+  pending_tree->SetCurrentlyScrollingLayer(pending_scrolling_layer_twin);
 }
 
 static void MarkActive(LayerImpl* layer) {