Mus: Avoid deadlock during teardown

services/ui/gpu/gpu_main.cc

 // Copyright 2016 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 "services/ui/gpu/gpu_main.h"
 
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
 #include "gpu/ipc/common/gpu_memory_buffer_support.h"
 #include "gpu/ipc/service/gpu_memory_buffer_factory.h"
@@ skipping 23 matching lines @@
 std::unique_ptr<base::MessagePump> CreateMessagePumpMac() {
   return base::MakeUnique<base::MessagePumpCFRunLoop>();
 }
 #endif  // defined(OS_MACOSX)
 
 }  // namespace
 
 namespace ui {
 
 GpuMain::GpuMain()
-    : gpu_thread_("GpuThread"), io_thread_("GpuIOThread"), weak_factory_(this) {
+    : gpu_thread_("GpuThread"),
+      io_thread_("GpuIOThread"),
+      compositor_thread_("DisplayCompositorThread"),
+      weak_factory_(this) {
   base::Thread::Options thread_options;
 
 #if defined(OS_WIN)
   thread_options.message_pump_factory = base::Bind(&CreateMessagePumpWin);
 #elif defined(USE_X11)
   thread_options.message_pump_factory = base::Bind(&CreateMessagePumpX11);
 #elif defined(USE_OZONE)
   thread_options.message_loop_type = base::MessageLoop::TYPE_UI;
 #elif defined(OS_LINUX)
   thread_options.message_loop_type = base::MessageLoop::TYPE_DEFAULT;
@@ skipping 11 matching lines @@
   // TODO(sad): We do not need the IO thread once gpu has a separate process. It
   // should be possible to use |main_task_runner_| for doing IO tasks.
   thread_options = base::Thread::Options(base::MessageLoop::TYPE_IO, 0);
   thread_options.priority = base::ThreadPriority::NORMAL;
 #if defined(OS_ANDROID) || defined(OS_CHROMEOS)
   // TODO(reveman): Remove this in favor of setting it explicitly for each type
   // of process.
   thread_options.priority = base::ThreadPriority::DISPLAY;
 #endif
   CHECK(io_thread_.StartWithOptions(thread_options));
+
+  // Start the compositor thread.
+  compositor_thread_.Start();
 }
 
 GpuMain::~GpuMain() {
   // Unretained() is OK here since the thread/task runner is owned by |this|.
+  compositor_thread_.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&GpuMain::TearDownOnCompositorThread, base::Unretained(this)));
+  // Block the main thread until the compositor thread terminates which blocks
+  // on the gpu thread. The Stop must be initiated from here instead of the gpu
+  // thread to avoid deadlock.
+  compositor_thread_.Stop();
+
   gpu_thread_.task_runner()->PostTask(
       FROM_HERE,
       base::Bind(&GpuMain::TearDownOnGpuThread, base::Unretained(this)));
   gpu_thread_.Stop();
   io_thread_.Stop();
 }
 
 void GpuMain::OnStart() {
   gpu_thread_.task_runner()->PostTask(
       FROM_HERE,
@@ skipping 12 matching lines @@
   gpu_init_->set_sandbox_helper(this);
   bool success = gpu_init_->InitializeAndStartSandbox(
       *base::CommandLine::ForCurrentProcess());
   if (success) {
     if (gpu::GetNativeGpuMemoryBufferType() != gfx::EMPTY_BUFFER) {
       gpu_memory_buffer_factory_ =
           gpu::GpuMemoryBufferFactory::CreateNativeType();
     }
     gpu_service_internal_.reset(new GpuServiceInternal(
         gpu_init_->gpu_info(), gpu_init_->TakeWatchdogThread(),
-        gpu_memory_buffer_factory_.get(), io_thread_.task_runner()));
+        gpu_memory_buffer_factory_.get(), io_thread_.task_runner(),
+        compositor_thread_.task_runner()));
   }
 }
 
+void GpuMain::TearDownOnCompositorThread() {
+  gpu_service_internal_->DestroyDisplayCompositor();

[sadrul, 2016/11/24 05:07:17]

It is possible to be here without having the |gpu_service_internal_| been
created at all, I think? (e.g. gpu-thread hasn't run InitOnGpuThread() yet,
which means gpu_service_internal_ is still uninitialized)

Can we move DisplayCompositor ownership out of GpuServiceInternal, and into here
in GpuMain instead?

[Fady Samuel, 2016/11/24 05:21:55]

The problem is "CreateDisplayCompositor" is part of the GpuServiceInternal
interface. I have filed a bug to pull that out:
https://bugs.chromium.org/p/chromium/issues/detail?id=668357

I'll check that gpu_service_internal_ is not null here, and do the right thing
tomorrow or Friday.

+}
+
 void GpuMain::TearDownOnGpuThread() {
   gpu_service_internal_.reset();
   gpu_memory_buffer_factory_.reset();
   gpu_init_.reset();
 }
 
 void GpuMain::CreateOnGpuThread(mojom::GpuServiceInternalRequest request) {
   if (gpu_service_internal_)
     gpu_service_internal_->Add(std::move(request));
 }
 
 void GpuMain::PreSandboxStartup() {
   // TODO(sad): https://crbug.com/645602
 }
 
 bool GpuMain::EnsureSandboxInitialized(
     gpu::GpuWatchdogThread* watchdog_thread) {
   // TODO(sad): https://crbug.com/645602
   return true;
 }
 
 }  // namespace ui