Improve InProcessGpuMemoryBufferManager thread safety.

Don't pass GpuChannelManager to InProcessGpuMemoryBufferManager. Most of
GpuChannelManager should only be accessed from the GPU thread so there
is the potential for unsafe operations. Both SyncPointManager and
GpuMemoryBufferFactory are thread safe so they should be fine.

Also ensure that GpuMemoryBuffer destruction callbacks dereference their
WeakPtr on the correct thread. GpuMemoryBuffers can be created and used
on any thread, even though InProcessGpuMemoryBufferManager isn't used
on multiple threads currently.

Bug: 902163
Change-Id: I8118def4fabd0307f0a58d403b9f9c08bc72fc6b
Reviewed-on: https://chromium-review.googlesource.com/c/1330030
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607761}

cc/test/pixel_test.cc

@@ -317,16 +317,13 @@ void PixelTest::SetUpSkiaRendererDDL() {
   renderer_->SetVisible(true);
 
   // Set up the client side context provider, etc
-  auto* gpu_channel_manager = gpu_service_->gpu_channel_manager();
   gpu_memory_buffer_manager_ =
       std::make_unique<viz::InProcessGpuMemoryBufferManager>(
-          gpu_channel_manager);
-  gpu::ImageFactory* image_factory = nullptr;
-  if (gpu_channel_manager->gpu_memory_buffer_factory()) {
-    image_factory =
-        gpu_channel_manager->gpu_memory_buffer_factory()->AsImageFactory();
-  }
-  auto* gpu_channel_manager_delegate = gpu_channel_manager->delegate();
+          gpu_service_->gpu_memory_buffer_factory(),
+          gpu_service_->sync_point_manager());
+  gpu::ImageFactory* image_factory = gpu_service_->gpu_image_factory();
+  auto* gpu_channel_manager_delegate =
+      gpu_service_->gpu_channel_manager()->delegate();
   child_context_provider_ =
       base::MakeRefCounted<viz::VizProcessContextProvider>(
           task_executor_, gpu::kNullSurfaceHandle,

components/viz/service/display_embedder/in_process_gpu_memory_buffer_manager.cc

@@ -5,25 +5,43 @@
 #include "components/viz/service/display_embedder/in_process_gpu_memory_buffer_manager.h"
 
 #include "base/bind.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "gpu/command_buffer/service/sync_point_manager.h"
 #include "gpu/ipc/common/gpu_client_ids.h"
 #include "gpu/ipc/common/gpu_memory_buffer_impl.h"
-#include "gpu/ipc/common/gpu_memory_buffer_support.h"
 #include "gpu/ipc/in_process_command_buffer.h"
-#include "gpu/ipc/service/gpu_channel_manager.h"
 #include "gpu/ipc/service/gpu_memory_buffer_factory.h"
 
 namespace viz {
+namespace {
+
+void DestroyOnThread(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    const gpu::GpuMemoryBufferImpl::DestructionCallback& callback,
+    const gpu::SyncToken& sync_token) {
+  if (task_runner->BelongsToCurrentThread()) {
+    std::move(callback).Run(sync_token);
+  } else {
+    task_runner->PostTask(FROM_HERE, base::BindOnce(callback, sync_token));
+  }
+}
+
+}  // namespace
 
 InProcessGpuMemoryBufferManager::InProcessGpuMemoryBufferManager(
-    gpu::GpuChannelManager* channel_manager)
-    : gpu_memory_buffer_support_(new gpu::GpuMemoryBufferSupport()),
-      client_id_(gpu::kInProcessCommandBufferClientId),
-      channel_manager_(channel_manager),
-      weak_factory_(this) {
-  weak_ptr_ = weak_factory_.GetWeakPtr();
+    gpu::GpuMemoryBufferFactory* gpu_memory_buffer_factory,
+    gpu::SyncPointManager* sync_point_manager)
+    : client_id_(gpu::kInProcessCommandBufferClientId),
+      gpu_memory_buffer_factory_(gpu_memory_buffer_factory),
+      sync_point_manager_(sync_point_manager),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_ptr_factory_(this) {
+  weak_ptr_ = weak_ptr_factory_.GetWeakPtr();
 }
 
-InProcessGpuMemoryBufferManager::~InProcessGpuMemoryBufferManager() {}
+InProcessGpuMemoryBufferManager::~InProcessGpuMemoryBufferManager() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+}
 
 std::unique_ptr<gfx::GpuMemoryBuffer>
 InProcessGpuMemoryBufferManager::CreateGpuMemoryBuffer(
@@ -33,12 +51,15 @@ InProcessGpuMemoryBufferManager::CreateGpuMemoryBuffer(
     gpu::SurfaceHandle surface_handle) {
   gfx::GpuMemoryBufferId id(next_gpu_memory_id_++);
   gfx::GpuMemoryBufferHandle buffer_handle =
-      channel_manager_->gpu_memory_buffer_factory()->CreateGpuMemoryBuffer(
+      gpu_memory_buffer_factory_->CreateGpuMemoryBuffer(
           id, size, format, usage, client_id_, surface_handle);
-  return gpu_memory_buffer_support_->CreateGpuMemoryBufferImplFromHandle(
+
+  auto callback = base::BindRepeating(
+      &InProcessGpuMemoryBufferManager::ShouldDestroyGpuMemoryBuffer, weak_ptr_,
+      id);
+  return gpu_memory_buffer_support_.CreateGpuMemoryBufferImplFromHandle(
       std::move(buffer_handle), size, format, usage,
-      base::Bind(&InProcessGpuMemoryBufferManager::DestroyGpuMemoryBuffer,
-                 weak_ptr_, id, client_id_));
+      base::BindRepeating(&DestroyOnThread, task_runner_, std::move(callback)));
 }
 
 void InProcessGpuMemoryBufferManager::SetDestructionSyncToken(
@@ -48,11 +69,28 @@ void InProcessGpuMemoryBufferManager::SetDestructionSyncToken(
       sync_token);
 }
 
-void InProcessGpuMemoryBufferManager::DestroyGpuMemoryBuffer(
+void InProcessGpuMemoryBufferManager::ShouldDestroyGpuMemoryBuffer(
     gfx::GpuMemoryBufferId id,
-    int client_id,
     const gpu::SyncToken& sync_token) {
-  channel_manager_->DestroyGpuMemoryBuffer(id, client_id, sync_token);
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  auto callback = base::BindOnce(
+      &InProcessGpuMemoryBufferManager::DestroyGpuMemoryBuffer, weak_ptr_, id);
+  // This is equivalent to calling SyncPointerManager::WaitOutOfOrder() except
+  // |callback| will be run on this thread instead of the thread where the sync
+  // point is released.
+  bool will_run = sync_point_manager_->WaitNonThreadSafe(
+      sync_token, gpu::SequenceId(), UINT32_MAX, task_runner_,
+      std::move(callback));
+
+  // No sync token or invalid sync token, destroy immediately.
+  if (!will_run)
+    DestroyGpuMemoryBuffer(id);
+}
+
+void InProcessGpuMemoryBufferManager::DestroyGpuMemoryBuffer(
+    gfx::GpuMemoryBufferId id) {
+  gpu_memory_buffer_factory_->DestroyGpuMemoryBuffer(id, client_id_);
 }
 
 }  // namespace viz

components/viz/service/display_embedder/in_process_gpu_memory_buffer_manager.h

@@ -8,20 +8,26 @@
 #include "base/memory/weak_ptr.h"
 #include "components/viz/service/viz_service_export.h"
 #include "gpu/command_buffer/client/gpu_memory_buffer_manager.h"
+#include "gpu/ipc/common/gpu_memory_buffer_support.h"
 
 namespace gpu {
-class GpuChannelManager;
-class GpuMemoryBufferSupport;
+class GpuMemoryBufferFactory;
+class SyncPointManager;
 }
 
 namespace viz {
 
+// GpuMemoryBufferManager implementation usable from any thread in the GPU
+// process. Must be created and destroyed on the same thread.
 class VIZ_SERVICE_EXPORT InProcessGpuMemoryBufferManager
     : public gpu::GpuMemoryBufferManager {
  public:
-  explicit InProcessGpuMemoryBufferManager(
-      gpu::GpuChannelManager* channel_manager);
-
+  // |gpu_memory_buffer_factory| and |sync_point_manager| must outlive |this|.
+  InProcessGpuMemoryBufferManager(
+      gpu::GpuMemoryBufferFactory* gpu_memory_buffer_factory,
+      gpu::SyncPointManager* sync_point_manager);
+  // Note: Any GpuMemoryBuffers that haven't been destroyed yet will be leaked
+  // until the GpuMemoryBufferFactory is destroyed.
   ~InProcessGpuMemoryBufferManager() override;
 
   // gpu::GpuMemoryBufferManager:
@@ -34,15 +40,22 @@ class VIZ_SERVICE_EXPORT InProcessGpuMemoryBufferManager
                                const gpu::SyncToken& sync_token) override;
 
  private:
-  void DestroyGpuMemoryBuffer(gfx::GpuMemoryBufferId id,
-                              int client_id,
-                              const gpu::SyncToken& sync_token);
-  std::unique_ptr<gpu::GpuMemoryBufferSupport> gpu_memory_buffer_support_;
+  // Provided as callback when a GpuMemoryBuffer should be destroyed.
+  void ShouldDestroyGpuMemoryBuffer(gfx::GpuMemoryBufferId id,
+                                    const gpu::SyncToken& sync_token);
+  void DestroyGpuMemoryBuffer(gfx::GpuMemoryBufferId id);
+
+  gpu::GpuMemoryBufferSupport gpu_memory_buffer_support_;
   const int client_id_;
   int next_gpu_memory_id_ = 1;
-  gpu::GpuChannelManager* channel_manager_;
+
+  gpu::GpuMemoryBufferFactory* const gpu_memory_buffer_factory_;
+  gpu::SyncPointManager* const sync_point_manager_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   base::WeakPtr<InProcessGpuMemoryBufferManager> weak_ptr_;
-  base::WeakPtrFactory<InProcessGpuMemoryBufferManager> weak_factory_;
+
+  base::WeakPtrFactory<InProcessGpuMemoryBufferManager> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(InProcessGpuMemoryBufferManager);
 };
 

components/viz/service/main/viz_compositor_thread_runner.cc

@@ -77,17 +77,13 @@ void VizCompositorThreadRunner::CreateFrameSinkManager(
       FROM_HERE,
       base::BindOnce(
           &VizCompositorThreadRunner::CreateFrameSinkManagerOnCompositorThread,
-          base::Unretained(this), std::move(params), nullptr, nullptr, nullptr,
-          nullptr));
+          base::Unretained(this), std::move(params), nullptr, nullptr));
 }
 
 void VizCompositorThreadRunner::CreateFrameSinkManager(
     mojom::FrameSinkManagerParamsPtr params,
     scoped_refptr<gpu::CommandBufferTaskExecutor> task_executor,
     GpuServiceImpl* gpu_service) {
-  auto* gpu_channel_manager = gpu_service->gpu_channel_manager();
-  auto* image_factory = gpu_service->gpu_image_factory();
-
   // All of the unretained objects are owned on the GPU thread and destroyed
   // after VizCompositorThread has been shutdown.
   task_runner_->PostTask(
@@ -95,8 +91,7 @@ void VizCompositorThreadRunner::CreateFrameSinkManager(
       base::BindOnce(
           &VizCompositorThreadRunner::CreateFrameSinkManagerOnCompositorThread,
           base::Unretained(this), std::move(params), std::move(task_executor),
-          base::Unretained(gpu_service), base::Unretained(image_factory),
-          base::Unretained(gpu_channel_manager)));
+          base::Unretained(gpu_service)));
 }
 
 #if defined(USE_VIZ_DEVTOOLS)
@@ -125,9 +120,7 @@ void VizCompositorThreadRunner::CleanupForShutdown(
 void VizCompositorThreadRunner::CreateFrameSinkManagerOnCompositorThread(
     mojom::FrameSinkManagerParamsPtr params,
     scoped_refptr<gpu::CommandBufferTaskExecutor> task_executor,
-    GpuServiceImpl* gpu_service,
-    gpu::ImageFactory* image_factory,
-    gpu::GpuChannelManager* gpu_channel_manager) {
+    GpuServiceImpl* gpu_service) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!frame_sink_manager_);
 
@@ -142,11 +135,17 @@ void VizCompositorThreadRunner::CreateFrameSinkManagerOnCompositorThread(
       command_line->HasSwitch(switches::kRunAllCompositorStagesBeforeDraw);
 
   if (task_executor) {
+    DCHECK(gpu_service);
     // Create DisplayProvider usable for GPU + software compositing.
+    auto gpu_memory_buffer_manager =
+        std::make_unique<InProcessGpuMemoryBufferManager>(
+            gpu_service->gpu_memory_buffer_factory(),
+            gpu_service->sync_point_manager());
+    auto* image_factory = gpu_service->gpu_image_factory();
     display_provider_ = std::make_unique<GpuDisplayProvider>(
         params->restart_id, gpu_service, std::move(task_executor), gpu_service,
-        std::make_unique<InProcessGpuMemoryBufferManager>(gpu_channel_manager),
-        image_factory, server_shared_bitmap_manager_.get(), headless,
+        std::move(gpu_memory_buffer_manager), image_factory,
+        server_shared_bitmap_manager_.get(), headless,
         run_all_compositor_stages_before_draw);
   } else {
     // Create DisplayProvider usable for software compositing only.

components/viz/service/main/viz_compositor_thread_runner.h

@@ -24,8 +24,6 @@ class Thread;
 
 namespace gpu {
 class CommandBufferTaskExecutor;
-class GpuChannelManager;
-class ImageFactory;
 }  // namespace gpu
 
 namespace ui_devtools {
@@ -86,9 +84,7 @@ class VizCompositorThreadRunner {
   void CreateFrameSinkManagerOnCompositorThread(
       mojom::FrameSinkManagerParamsPtr params,
       scoped_refptr<gpu::CommandBufferTaskExecutor> task_executor,
-      GpuServiceImpl* gpu_service,
-      gpu::ImageFactory* image_factory,
-      gpu::GpuChannelManager* gpu_channel_manager);
+      GpuServiceImpl* gpu_service);
 #if defined(USE_VIZ_DEVTOOLS)
   void CreateVizDevToolsOnCompositorThread(mojom::VizDevToolsParamsPtr params);
   void InitVizDevToolsOnCompositorThread(mojom::VizDevToolsParamsPtr params);

gpu/ipc/service/gpu_memory_buffer_factory.h

@@ -37,8 +37,8 @@ class GPU_IPC_SERVICE_EXPORT GpuMemoryBufferFactory {
       int client_id,
       SurfaceHandle surface_handle) = 0;
 
-  // Destroys GPU memory buffer identified by |id|.
-  // It can be called on any thread.
+  // Destroys GPU memory buffer identified by |id|. It can be called on any
+  // thread.
   virtual void DestroyGpuMemoryBuffer(gfx::GpuMemoryBufferId id,
                                       int client_id) = 0;