cc: Fix use-after-frees for LayerTreeHostAnimationTest.

cc/test/layer_tree_test.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/test/layer_tree_test.h"
 
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
@@ skipping 706 matching lines @@
         this, mode_, client_.get(), nullptr, shared_bitmap_manager_.get(),
         gpu_memory_buffer_manager_.get(), task_graph_runner_.get(), settings_,
         base::ThreadTaskRunnerHandle::Get(),
         impl_thread_ ? impl_thread_->task_runner() : NULL,
         std::move(external_begin_frame_source),
         image_serialization_processor_.get());
   }
 
   ASSERT_TRUE(layer_tree_host_);
 
+  main_task_runner_ =
+      layer_tree_host_->task_runner_provider()->MainThreadTaskRunner();

[vmpstr, 2016/07/18 23:57:26]

Also, this is probably a minor point, but this is creating a scoped_refptr out
of a raw pointer, which is something that I'd want to ideally avoid (since it
makes transition to shared_ptr harder in the future). 

I'm not sure whether task_runner_provider() should be returning scoped_refptrs
or what, but can you at least leave a TODO(vmpstr) here?

[danakj, 2016/07/19 00:03:27]

Ya cc is a mess of raw pointers for task runners all over the place that become
scoped_refptrs. I think some of them could stay raw pointers, and with some work
more of them could stay that way forever.

+  impl_task_runner_ =
+      layer_tree_host_->task_runner_provider()->ImplThreadTaskRunner();
+  if (!impl_task_runner_) {
+    // For tests, if there's no impl thread, make things easier by just giving
+    // the main thread task runner.
+    impl_task_runner_ = main_task_runner_;
+  }
+
+  if (timeout_seconds_) {
+    timeout_.Reset(base::Bind(&LayerTreeTest::Timeout, base::Unretained(this)));
+    main_task_runner_->PostDelayedTask(
+        FROM_HERE, timeout_.callback(),
+        base::TimeDelta::FromSeconds(timeout_seconds_));
+  }
+
   started_ = true;
   beginning_ = true;
   SetupTree();
   WillBeginTest();
   BeginTest();
   beginning_ = false;
   if (end_when_begin_returns_)
     RealEndTest();
 
   // Allow commits to happen once BeginTest() has had a chance to post tasks
@@ skipping 42 matching lines @@
         base::Bind(&LayerTreeTest::RealEndTest, main_thread_weak_ptr_));
     return;
   }
 
   base::MessageLoop::current()->QuitWhenIdle();
 }
 
 void LayerTreeTest::DispatchAddAnimationToPlayer(
     AnimationPlayer* player_to_receive_animation,
     double animation_duration) {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   if (player_to_receive_animation) {
     AddOpacityTransitionToPlayer(player_to_receive_animation,
                                  animation_duration, 0, 0.5, true);
   }
 }
 
 void LayerTreeTest::DispatchSetDeferCommits(bool defer_commits) {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
-
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     layer_tree_host_->SetDeferCommits(defer_commits);
 }
 
 void LayerTreeTest::DispatchSetNeedsCommit() {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
-
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     layer_tree_host_->SetNeedsCommit();
 }
 
 void LayerTreeTest::DispatchSetNeedsUpdateLayers() {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
-
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     layer_tree_host_->SetNeedsUpdateLayers();
 }
 
 void LayerTreeTest::DispatchSetNeedsRedraw() {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
-
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     layer_tree_host_->SetNeedsRedraw();
 }
 
 void LayerTreeTest::DispatchSetNeedsRedrawRect(const gfx::Rect& damage_rect) {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
-
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     layer_tree_host_->SetNeedsRedrawRect(damage_rect);
 }
 
 void LayerTreeTest::DispatchSetVisible(bool visible) {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     SetVisibleOnLayerTreeHost(visible);
 }
 
 void LayerTreeTest::DispatchSetNextCommitForcesRedraw() {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
-
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     layer_tree_host_->SetNextCommitForcesRedraw();
 }
 
 void LayerTreeTest::DispatchCompositeImmediately() {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     layer_tree_host_->Composite(base::TimeTicks::Now());
 }
 
 void LayerTreeTest::DispatchNextCommitWaitsForActivation() {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread());
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   if (layer_tree_host_)
     layer_tree_host_->SetNextCommitWaitsForActivation();
 }
 
 void LayerTreeTest::RunTest(CompositorMode mode, bool delegating_renderer) {
   mode_ = mode;
   if (mode_ == CompositorMode::THREADED || mode_ == CompositorMode::REMOTE) {
     impl_thread_.reset(new base::Thread("Compositor"));
     ASSERT_TRUE(impl_thread_->Start());
   }
 
-  main_task_runner_ = base::ThreadTaskRunnerHandle::Get();
-
   shared_bitmap_manager_.reset(new TestSharedBitmapManager);
   gpu_memory_buffer_manager_.reset(new TestGpuMemoryBufferManager);
   task_graph_runner_.reset(new TestTaskGraphRunner);
 
   delegating_renderer_ = delegating_renderer;
 
   // Spend less time waiting for BeginFrame because the output is
   // mocked out.
   settings_.renderer_settings.refresh_rate = 200.0;
   settings_.background_animation_rate = 200.0;
   settings_.verify_clip_tree_calculations = true;
   settings_.verify_transform_tree_calculations = true;
   InitializeSettings(&settings_);
 
-  main_task_runner_->PostTask(
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(&LayerTreeTest::DoBeginTest, base::Unretained(this)));
 
-  if (timeout_seconds_) {
-    timeout_.Reset(base::Bind(&LayerTreeTest::Timeout, base::Unretained(this)));
-    main_task_runner_->PostDelayedTask(
-        FROM_HERE,
-        timeout_.callback(),
-        base::TimeDelta::FromSeconds(timeout_seconds_));
-  }
-
   base::RunLoop().Run();
   DestroyLayerTreeHost();
 
   timeout_.Cancel();
 
   ASSERT_FALSE(layer_tree_host_.get());
   client_ = nullptr;
   if (timed_out_) {
     FAIL() << "Test timed out";
     return;
@@ skipping 10 matching lines @@
 }
 
 std::unique_ptr<OutputSurface> LayerTreeTest::CreateOutputSurface() {
   return delegating_renderer_ ? FakeOutputSurface::CreateDelegating3d()
                               : FakeOutputSurface::Create3d();
 }
 
 void LayerTreeTest::DestroyLayerTreeHost() {
   if (layer_tree_host_ && layer_tree_host_->root_layer())
     layer_tree_host_->root_layer()->SetLayerTreeHost(NULL);
-  layer_tree_host_ = nullptr;
+
+  // Destroy the LayerTreeHost after making the member variable null to ensure
+  // that if tests try to use the layer_tree_host_ while we are destroying it
+  // they will for sure see null, as some std::unique_ptr implementations don't
+  // do this for us (*ahem* MSVC) since it is not required by the standard.

[vmpstr, 2016/07/18 23:26:12]

Is the concern that something might access layer_tree_host_ in its dtor?

Also, FWIW here's what unique_ptr::reset does according to cppreference:

... Given current_ptr, the pointer that was managed by *this, performs the
following actions, in this order:
- Saves a copy of the current pointer old_ptr = current_ptr
- Overwrites the current pointer with the argument current_ptr = ptr
- If the old pointer was non-empty, deletes the previously managed object
if(old_ptr != nullptr) get_deleter()(old_ptr).

I _think_ the code you have here is doing just that (unless I'm missing
something obvious)? And if so, is that equivalent to layer_tree_host_ = nullptr?
:) (since operator=(nullptr_t) is defined as "Effectively the same as calling
reset()"?

[danakj, 2016/07/19 00:03:27]

Yes. The LTH destructor joins the compositor thread which could go here, or
could call things that end up in TestHooks and tests could naively get back in
here. It's happened at least twice before.
Yep you're right, I continue to be amazed by the crash then.

+  {
+    auto temp = std::move(layer_tree_host_);
+    temp = nullptr;
+  }
 
   DCHECK(!remote_proto_channel_bridge_.channel_main.HasReceiver());
 
   // Destroying the LayerTreeHost should destroy the remote client
   // LayerTreeHost.
   DCHECK(!remote_client_layer_tree_host_);
 }
 
 void LayerTreeTest::DestroyRemoteClientHost() {
   DCHECK(IsRemoteTest());
@@ skipping 27 matching lines @@
 
 TaskGraphRunner* LayerTreeTest::task_graph_runner() const {
   return task_graph_runner_.get();
 }
 
 TaskRunnerProvider* LayerTreeTest::task_runner_provider() const {
   // All LayerTreeTests can use the task runner provider to access the impl
   // thread. In the remote mode, the impl thread of the compositor lives on
   // the client, so return the task runner provider owned by the remote client
   // LayerTreeHost.
-  if (IsRemoteTest()) {
-    return remote_client_layer_tree_host_
-               ? remote_client_layer_tree_host_->task_runner_provider()
-               : nullptr;
-  }
-  return layer_tree_host_ ? layer_tree_host_->task_runner_provider() : nullptr;
+  LayerTreeHost* host = IsRemoteTest() ? remote_client_layer_tree_host_.get()
+                                       : layer_tree_host_.get();
+
+  // If this fails, the test has ended and there is no task runners to find
+  // anymore.
+  DCHECK(host);
+
+  return host->task_runner_provider();
 }
 
 LayerTreeHost* LayerTreeTest::layer_tree_host() {
-  // We check for a null task_runner_provider here as we sometimes ask for the
-  // layer tree host when the task_runner_provider does not exist, often for
-  // checking settings after a test has completed. For example,
-  // LTHPixelResourceTest::RunPixelResourceTest. See elsewhere in this file for
-  // other examples.
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread() ||
+  DCHECK(task_runner_provider()->IsMainThread() ||
          task_runner_provider()->IsMainThreadBlocked());
   return layer_tree_host_.get();
 }
 
 LayerTreeHost* LayerTreeTest::remote_client_layer_tree_host() {
-  DCHECK(!task_runner_provider() || task_runner_provider()->IsMainThread() ||
+  DCHECK(IsRemoteTest());
+  DCHECK(task_runner_provider()->IsMainThread() ||
          task_runner_provider()->IsMainThreadBlocked());
-  DCHECK(IsRemoteTest());
   return remote_client_layer_tree_host_.get();
 }
 
 ProxyMainForTest* LayerTreeTest::GetProxyMainForTest() const {
   DCHECK(HasImplThread());
   return static_cast<ProxyMainForTest*>(proxy());
 }
 
 ProxyImplForTest* LayerTreeTest::GetProxyImplForTest() const {
   DCHECK(HasImplThread());
@@ skipping 19 matching lines @@
 
 RemoteChannelImplForTest* LayerTreeTest::GetRemoteChannelImplForTest() const {
   DCHECK(IsRemoteTest());
   DCHECK(remote_client_layer_tree_host_);
 
   return static_cast<RemoteChannelImplForTest*>(
       remote_client_layer_tree_host_->proxy());
 }
 
 }  // namespace cc