viz: Fix destroying gfx::GpuMemoryBuffers.

The destruction callback of a GpuMemoryBufferImpls can be called on any
thread[1]. So make sure the destruction callbacks hops onto the correct
thread before mutating local data structures or sending mojo messages.

[1] See doc for GpuMemoryBufferImpl::CreateFromHandle()
https://cs.chromium.org/chromium/src/gpu/ipc/client/gpu_memory_buffer_impl.h?l=28

BUG=733482

Change-Id: Ifc3fecd6a8bb42aa61f3d2d9667bd0e931d7261c
Reviewed-on: https://chromium-review.googlesource.com/565858
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488793}

components/viz/host/server_gpu_memory_buffer_manager.cc

@@ -6,7 +6,7 @@
 
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
-#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "gpu/ipc/client/gpu_memory_buffer_impl.h"
@@ -18,6 +18,17 @@
 
 namespace viz {
 
+namespace {
+
+void OnGpuMemoryBufferDestroyed(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    const gpu::GpuMemoryBufferImpl::DestructionCallback& callback,
+    const gpu::SyncToken& sync_token) {
+  task_runner->PostTask(FROM_HERE, base::Bind(callback, sync_token));
+}
+
+}  // namespace
+
 ServerGpuMemoryBufferManager::BufferInfo::BufferInfo() = default;
 ServerGpuMemoryBufferManager::BufferInfo::~BufferInfo() = default;
 
@@ -27,8 +38,10 @@ ServerGpuMemoryBufferManager::ServerGpuMemoryBufferManager(
     : gpu_service_(gpu_service),
       client_id_(client_id),
       native_configurations_(gpu::GetNativeGpuMemoryBufferConfigurations()),
-      task_runner_(base::SequencedTaskRunnerHandle::Get()),
-      weak_factory_(this) {}
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_factory_(this) {
+  weak_ptr_ = weak_factory_.GetWeakPtr();
+}
 
 ServerGpuMemoryBufferManager::~ServerGpuMemoryBufferManager() {}
 
@@ -49,7 +62,7 @@ void ServerGpuMemoryBufferManager::AllocateGpuMemoryBuffer(
       gpu_service_->CreateGpuMemoryBuffer(
           id, size, format, usage, client_id, surface_handle,
           base::Bind(&ServerGpuMemoryBufferManager::OnGpuMemoryBufferAllocated,
-                     weak_factory_.GetWeakPtr(), client_id,
+                     weak_ptr_, client_id,
                      gfx::BufferSizeForBufferFormat(size, format),
                      base::Passed(std::move(callback))));
       return;
@@ -108,10 +121,15 @@ ServerGpuMemoryBufferManager::CreateGpuMemoryBuffer(
   wait_event.Wait();
   if (handle.is_null())
     return nullptr;
+  // The destruction callback can be called on any thread. So use an
+  // intermediate callback here as the destruction callback, which bounces off
+  // onto the |task_runner_| thread to do the real work.
   return gpu::GpuMemoryBufferImpl::CreateFromHandle(
       handle, size, format, usage,
-      base::Bind(&ServerGpuMemoryBufferManager::DestroyGpuMemoryBuffer,
-                 weak_factory_.GetWeakPtr(), id, client_id_));
+      base::Bind(
+          &OnGpuMemoryBufferDestroyed, task_runner_,
+          base::Bind(&ServerGpuMemoryBufferManager::DestroyGpuMemoryBuffer,
+                     weak_ptr_, id, client_id_)));
 }
 
 void ServerGpuMemoryBufferManager::SetDestructionSyncToken(

components/viz/host/server_gpu_memory_buffer_manager.h

@@ -9,7 +9,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/sequenced_task_runner.h"
+#include "base/single_thread_task_runner.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "components/viz/host/viz_host_export.h"
@@ -92,7 +92,8 @@ class VIZ_HOST_EXPORT ServerGpuMemoryBufferManager
   std::unordered_set<int> pending_buffers_;
 
   const gpu::GpuMemoryBufferConfigurationSet native_configurations_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  base::WeakPtr<ServerGpuMemoryBufferManager> weak_ptr_;
   base::WeakPtrFactory<ServerGpuMemoryBufferManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ServerGpuMemoryBufferManager);

components/viz/host/server_gpu_memory_buffer_manager_unittest.cc

@@ -160,6 +160,28 @@ class ServerGpuMemoryBufferManagerTest : public ::testing::Test {
   ServerGpuMemoryBufferManagerTest() = default;
   ~ServerGpuMemoryBufferManagerTest() override = default;
 
+  std::unique_ptr<gfx::GpuMemoryBuffer> AllocateGpuMemoryBufferSync(
+      ServerGpuMemoryBufferManager* manager) {
+    base::Thread diff_thread("TestThread");
+    diff_thread.Start();
+    std::unique_ptr<gfx::GpuMemoryBuffer> buffer;
+    base::RunLoop run_loop;
+    diff_thread.task_runner()->PostTask(
+        FROM_HERE, base::Bind(
+                       [](ServerGpuMemoryBufferManager* manager,
+                          std::unique_ptr<gfx::GpuMemoryBuffer>* out_buffer,
+                          const base::Closure& callback) {
+                         *out_buffer = manager->CreateGpuMemoryBuffer(
+                             gfx::Size(64, 64), gfx::BufferFormat::YVU_420,
+                             gfx::BufferUsage::GPU_READ,
+                             gpu::kNullSurfaceHandle);
+                         callback.Run();
+                       },
+                       manager, &buffer, run_loop.QuitClosure()));
+    run_loop.Run();
+    return buffer;
+  }
+
   // ::testing::Test:
   void SetUp() override {
     gfx::ClientNativePixmapFactory::ResetInstance();
@@ -265,24 +287,25 @@ TEST_F(ServerGpuMemoryBufferManagerTest, GpuMemoryBufferDestroyed) {
   gfx::ClientNativePixmapFactory::ResetInstance();
   TestGpuService gpu_service;
   ServerGpuMemoryBufferManager manager(&gpu_service, 1);
-  base::Thread diff_thread("TestThread");
-  ASSERT_TRUE(diff_thread.Start());
-  std::unique_ptr<gfx::GpuMemoryBuffer> buffer;
-  base::RunLoop run_loop;
-  ASSERT_TRUE(diff_thread.task_runner()->PostTask(
-      FROM_HERE, base::Bind(
-                     [](ServerGpuMemoryBufferManager* manager,
-                        std::unique_ptr<gfx::GpuMemoryBuffer>* out_buffer,
-                        const base::Closure& callback) {
-                       *out_buffer = manager->CreateGpuMemoryBuffer(
-                           gfx::Size(64, 64), gfx::BufferFormat::YVU_420,
-                           gfx::BufferUsage::GPU_READ, gpu::kNullSurfaceHandle);
-                       callback.Run();
-                     },
-                     &manager, &buffer, run_loop.QuitClosure())));
-  run_loop.Run();
+  auto buffer = AllocateGpuMemoryBufferSync(&manager);
   EXPECT_TRUE(buffer);
   buffer.reset();
 }
 
+TEST_F(ServerGpuMemoryBufferManagerTest,
+       GpuMemoryBufferDestroyedOnDifferentThread) {
+  gfx::ClientNativePixmapFactory::ResetInstance();
+  TestGpuService gpu_service;
+  ServerGpuMemoryBufferManager manager(&gpu_service, 1);
+  auto buffer = AllocateGpuMemoryBufferSync(&manager);
+  EXPECT_TRUE(buffer);
+  // Destroy the buffer in a different thread.
+  base::Thread diff_thread("DestroyThread");
+  ASSERT_TRUE(diff_thread.Start());
+  diff_thread.task_runner()->PostTask(
+      FROM_HERE, base::Bind([](std::unique_ptr<gfx::GpuMemoryBuffer> buffer) {},
+                            base::Passed(&buffer)));
+  diff_thread.Stop();
+}
+
 }  // namespace viz