viz: Re-arrange the threads in ui::GpuMain.

This reverts commit 81ed4437.

Create a separate thread for the compositor. Use the thread the GpuMain
instance is created on as the thread for the GpuService. This allows
using it in content (in GpuChildThread), where the main thread is used
for the gpu service.

For the mus window server, until the gpu process-split happens, create
a separate thread for the gpu service from the DefaultGpuHost. After
the process split, the main thread of the new process can be used for
the gpu service (like GpuChildThread in content).

BUG=730213
TBR=kylechar@chromium.org,fsamuel@chromium.org

Change-Id: I96b5dc9e74e1c14db62659c686e537a3c131e2d7
Reviewed-on: https://chromium-review.googlesource.com/704158Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506976}

services/ui/gpu/gpu_main.cc

@@ -44,24 +44,9 @@ std::unique_ptr<base::MessagePump> CreateMessagePumpMac() {
 }
 #endif  // defined(OS_MACOSX)
 
-}  // namespace
-
-namespace ui {
-
-GpuMain::GpuMain(mojom::GpuMainRequest request)
-    : gpu_thread_("GpuThread"),
-      io_thread_("GpuIOThread"),
-      binding_(this) {
-  // TODO: crbug.com/609317: Remove this when Mus Window Server and GPU are
-  // split into separate processes. Until then this is necessary to be able to
-  // run Mushrome (chrome --mus) with Mus running in the browser process.
-  if (!base::PowerMonitor::Get()) {
-    power_monitor_ = base::MakeUnique<base::PowerMonitor>(
-        base::MakeUnique<base::PowerMonitorDeviceSource>());
-  }
-
+std::unique_ptr<base::Thread> CreateAndStartCompositorThread() {
+  auto thread = std::make_unique<base::Thread>("CompositorThread");
   base::Thread::Options thread_options;
-
 #if defined(OS_WIN)
   thread_options.message_loop_type = base::MessageLoop::TYPE_DEFAULT;
 #elif defined(USE_X11)
@@ -82,12 +67,31 @@ GpuMain::GpuMain(mojom::GpuMainRequest request)
 #if defined(OS_ANDROID) || defined(OS_CHROMEOS)
   thread_options.priority = base::ThreadPriority::DISPLAY;
 #endif
-  CHECK(gpu_thread_.StartWithOptions(thread_options));
-  gpu_thread_task_runner_ = gpu_thread_.task_runner();
+  CHECK(thread->StartWithOptions(thread_options));
+  return thread;
+}
+
+}  // namespace
+
+namespace ui {
+
+GpuMain::GpuMain(mojom::GpuMainRequest request)
+    : io_thread_("GpuIOThread"),
+      gpu_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      compositor_thread_(CreateAndStartCompositorThread()),
+      compositor_thread_task_runner_(compositor_thread_->task_runner()),
+      binding_(this, std::move(request)) {
+  // TODO(crbug.com/609317): Remove this when Mus Window Server and GPU are
+  // split into separate processes. Until then this is necessary to be able to
+  // run Mushrome (chrome --mus) with Mus running in the browser process.
+  if (!base::PowerMonitor::Get()) {
+    power_monitor_ = base::MakeUnique<base::PowerMonitor>(
+        base::MakeUnique<base::PowerMonitorDeviceSource>());
+  }
 
   // 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);
+  base::Thread::Options 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
@@ -96,29 +100,50 @@ GpuMain::GpuMain(mojom::GpuMainRequest request)
 #endif
   CHECK(io_thread_.StartWithOptions(thread_options));
 
-  compositor_thread_task_runner_ = base::ThreadTaskRunnerHandle::Get();
-
-  // |this| will outlive the gpu thread and so it's safe to use
-  // base::Unretained here.
-  gpu_thread_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&GpuMain::InitOnGpuThread, base::Unretained(this),
-                 io_thread_.task_runner(), compositor_thread_task_runner_));
-  gpu_thread_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&GpuMain::BindOnGpu, base::Unretained(this),
-                            base::Passed(std::move(request))));
+  gpu_init_ = std::make_unique<gpu::GpuInit>();
+  gpu_init_->set_sandbox_helper(this);
+  // TODO(crbug.com/609317): Use InitializeAndStartSandbox() when gpu-mus is
+  // split into a separate process.
+  gpu_init_->InitializeInProcess(base::CommandLine::ForCurrentProcess());
+  gpu_service_ = base::MakeUnique<viz::GpuServiceImpl>(
+      gpu_init_->gpu_info(), gpu_init_->TakeWatchdogThread(),
+      io_thread_.task_runner(), gpu_init_->gpu_feature_info());
 }
 
 GpuMain::~GpuMain() {
-  DCHECK(compositor_thread_task_runner_->BelongsToCurrentThread());
-  // Tear down the compositor first because it blocks on the gpu service.
-  TearDownOnCompositorThread();
+  DCHECK(gpu_thread_task_runner_->BelongsToCurrentThread());
+  io_thread_.Stop();
+}
 
+void GpuMain::TearDown() {
+  DCHECK(!gpu_thread_task_runner_->BelongsToCurrentThread());
+  DCHECK(!compositor_thread_task_runner_->BelongsToCurrentThread());
+  // The compositor holds on to some resources from gpu service. So destroy the
+  // compositor first, before destroying the gpu service. However, before the
+  // compositor is destroyed, close the binding, so that the gpu service doesn't
+  // need to process commands from the compositor as it is shutting down.
+  base::WaitableEvent binding_wait(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
   gpu_thread_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&GpuMain::TearDownOnGpuThread, base::Unretained(this)));
-  gpu_thread_.Stop();
-  io_thread_.Stop();
+      FROM_HERE, base::Bind(&GpuMain::CloseGpuMainBindingOnGpuThread,
+                            base::Unretained(this), &binding_wait));
+  binding_wait.Wait();
+
+  base::WaitableEvent compositor_wait(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  compositor_thread_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&GpuMain::TearDownOnCompositorThread,
+                            base::Unretained(this), &compositor_wait));
+  compositor_wait.Wait();
+
+  base::WaitableEvent gpu_wait(base::WaitableEvent::ResetPolicy::MANUAL,
+                               base::WaitableEvent::InitialState::NOT_SIGNALED);
+  gpu_thread_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&GpuMain::TearDownOnGpuThread,
+                            base::Unretained(this), &gpu_wait));
+  gpu_wait.Wait();
 }
 
 void GpuMain::CreateGpuService(viz::mojom::GpuServiceRequest request,
@@ -126,9 +151,17 @@ void GpuMain::CreateGpuService(viz::mojom::GpuServiceRequest request,
                                const gpu::GpuPreferences& preferences,
                                mojo::ScopedSharedBufferHandle activity_flags) {
   DCHECK(gpu_thread_task_runner_->BelongsToCurrentThread());
-  CreateGpuServiceOnGpuThread(
-      std::move(request), std::move(gpu_host), preferences,
+  gpu_service_->UpdateGPUInfoFromPreferences(preferences);
+  gpu_service_->InitializeWithHost(
+      std::move(gpu_host),
       gpu::GpuProcessActivityFlags(std::move(activity_flags)));
+  gpu_service_->Bind(std::move(request));
+
+  if (pending_frame_sink_manager_request_.is_pending()) {
+    CreateFrameSinkManagerInternal(
+        std::move(pending_frame_sink_manager_request_),
+        std::move(pending_frame_sink_manager_client_info_));
+  }
 }
 
 void GpuMain::CreateFrameSinkManager(
@@ -143,23 +176,6 @@ void GpuMain::CreateFrameSinkManager(
   CreateFrameSinkManagerInternal(std::move(request), client.PassInterface());
 }
 
-void GpuMain::BindOnGpu(mojom::GpuMainRequest request) {
-  binding_.Bind(std::move(request));
-}
-
-void GpuMain::InitOnGpuThread(
-    scoped_refptr<base::SingleThreadTaskRunner> io_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> compositor_runner) {
-  gpu_init_.reset(new gpu::GpuInit());
-  gpu_init_->set_sandbox_helper(this);
-  // TODO(crbug.com/609317): Use InitializeAndStartSandbox() when gpu-mus is
-  // split into a separate process.
-  gpu_init_->InitializeInProcess(base::CommandLine::ForCurrentProcess());
-  gpu_service_ = base::MakeUnique<viz::GpuServiceImpl>(
-      gpu_init_->gpu_info(), gpu_init_->TakeWatchdogThread(), io_runner,
-      gpu_init_->gpu_feature_info());
-}
-
 void GpuMain::CreateFrameSinkManagerInternal(
     viz::mojom::FrameSinkManagerRequest request,
     viz::mojom::FrameSinkManagerClientPtrInfo client_info) {
@@ -194,33 +210,23 @@ void GpuMain::CreateFrameSinkManagerOnCompositorThread(
                                         std::move(client));
 }
 
-void GpuMain::TearDownOnCompositorThread() {
+void GpuMain::CloseGpuMainBindingOnGpuThread(base::WaitableEvent* wait) {
+  binding_.Close();
+  wait->Signal();
+}
+
+void GpuMain::TearDownOnCompositorThread(base::WaitableEvent* wait) {
   frame_sink_manager_.reset();
   display_provider_.reset();
+  wait->Signal();
 }
 
-void GpuMain::TearDownOnGpuThread() {
-  binding_.Close();
+void GpuMain::TearDownOnGpuThread(base::WaitableEvent* wait) {
+  gpu_command_service_ = nullptr;
   gpu_service_.reset();
   gpu_memory_buffer_factory_.reset();
   gpu_init_.reset();
-}
-
-void GpuMain::CreateGpuServiceOnGpuThread(
-    viz::mojom::GpuServiceRequest request,
-    mojom::GpuHostPtr gpu_host,
-    const gpu::GpuPreferences& preferences,
-    gpu::GpuProcessActivityFlags activity_flags) {
-  gpu_service_->UpdateGPUInfoFromPreferences(preferences);
-  gpu_service_->InitializeWithHost(std::move(gpu_host),
-                                   std::move(activity_flags));
-  gpu_service_->Bind(std::move(request));
-
-  if (pending_frame_sink_manager_request_.is_pending()) {
-    CreateFrameSinkManagerInternal(
-        std::move(pending_frame_sink_manager_request_),
-        std::move(pending_frame_sink_manager_client_info_));
-  }
+  wait->Signal();
 }
 
 void GpuMain::PreSandboxStartup() {

services/ui/gpu/gpu_main.h

@@ -30,6 +30,12 @@ class GpuMain : public gpu::GpuSandboxHelper, public mojom::GpuMain {
   explicit GpuMain(mojom::GpuMainRequest request);
   ~GpuMain() override;
 
+  // Calling this from the gpu or compositor thread can lead to crash/deadlock.
+  // So this must be called from a different thread.
+  // TODO(crbug.com/609317): After the process split, we should revisit to make
+  // this cleaner.
+  void TearDown();
+
   // mojom::GpuMain implementation:
   void CreateGpuService(viz::mojom::GpuServiceRequest request,
                         mojom::GpuHostPtr gpu_host,
@@ -39,35 +45,28 @@ class GpuMain : public gpu::GpuSandboxHelper, public mojom::GpuMain {
       viz::mojom::FrameSinkManagerRequest request,
       viz::mojom::FrameSinkManagerClientPtr client) override;
 
-  void OnStart();
-
   viz::GpuServiceImpl* gpu_service() { return gpu_service_.get(); }
 
  private:
-  void BindOnGpu(mojom::GpuMainRequest request);
-  void InitOnGpuThread(
-      scoped_refptr<base::SingleThreadTaskRunner> io_runner,
-      scoped_refptr<base::SingleThreadTaskRunner> compositor_runner);
-
   void CreateFrameSinkManagerInternal(
       viz::mojom::FrameSinkManagerRequest request,
       viz::mojom::FrameSinkManagerClientPtrInfo client_info);
   void CreateFrameSinkManagerOnCompositorThread(
       viz::mojom::FrameSinkManagerRequest request,
       viz::mojom::FrameSinkManagerClientPtrInfo client_info);
-  void CreateGpuServiceOnGpuThread(viz::mojom::GpuServiceRequest request,
-                                   mojom::GpuHostPtr gpu_host,
-                                   const gpu::GpuPreferences& preferences,
-                                   gpu::GpuProcessActivityFlags activity_flags);
 
-  void TearDownOnCompositorThread();
-  void TearDownOnGpuThread();
+  void CloseGpuMainBindingOnGpuThread(base::WaitableEvent* wait);
+  void TearDownOnCompositorThread(base::WaitableEvent* wait);
+  void TearDownOnGpuThread(base::WaitableEvent* wait);
 
   // gpu::GpuSandboxHelper:
   void PreSandboxStartup() override;
   bool EnsureSandboxInitialized(gpu::GpuWatchdogThread* watchdog_thread,
                                 const gpu::GPUInfo* gpu_info) override;
 
+  // The thread that handles IO events for Gpu.
+  base::Thread io_thread_;
+
   std::unique_ptr<gpu::GpuInit> gpu_init_;
   std::unique_ptr<viz::GpuServiceImpl> gpu_service_;
 
@@ -86,14 +85,12 @@ class GpuMain : public gpu::GpuSandboxHelper, public mojom::GpuMain {
 
   std::unique_ptr<gpu::GpuMemoryBufferFactory> gpu_memory_buffer_factory_;
 
-  // The main thread for Gpu.
-  base::Thread gpu_thread_;
-  scoped_refptr<base::SingleThreadTaskRunner> gpu_thread_task_runner_;
-
-  // The thread that handles IO events for Gpu.
-  base::Thread io_thread_;
+  const scoped_refptr<base::SingleThreadTaskRunner> gpu_thread_task_runner_;
 
-  scoped_refptr<base::SingleThreadTaskRunner> compositor_thread_task_runner_;
+  // The main thread for the display compositor.
+  std::unique_ptr<base::Thread> compositor_thread_;
+  const scoped_refptr<base::SingleThreadTaskRunner>
+      compositor_thread_task_runner_;
 
   std::unique_ptr<base::PowerMonitor> power_monitor_;
   mojo::Binding<mojom::GpuMain> binding_;

services/ui/ws/gpu_host.cc

@@ -37,10 +37,17 @@ DefaultGpuHost::DefaultGpuHost(GpuHostDelegate* delegate)
     : delegate_(delegate),
       next_client_id_(kInternalGpuChannelClientId + 1),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      gpu_host_binding_(this) {
+      gpu_host_binding_(this),
+      gpu_thread_("GpuThread"),
+      gpu_main_wait_(base::WaitableEvent::ResetPolicy::MANUAL,
+                     base::WaitableEvent::InitialState::NOT_SIGNALED) {
   // TODO(sad): Once GPU process is split, this would look like:
   //   connector->BindInterface("gpu", &gpu_main_);
-  gpu_main_impl_ = base::MakeUnique<GpuMain>(MakeRequest(&gpu_main_));
+  gpu_thread_.Start();
+  gpu_thread_.task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&DefaultGpuHost::InitializeGpuMain, base::Unretained(this),
+                     base::Passed(MakeRequest(&gpu_main_))));
 
   // TODO(sad): Correctly initialize gpu::GpuPreferences (like it is initialized
   // in GpuProcessHost::Init()).
@@ -55,7 +62,14 @@ DefaultGpuHost::DefaultGpuHost(GpuHostDelegate* delegate)
                                                           next_client_id_++);
 }
 
-DefaultGpuHost::~DefaultGpuHost() {}
+DefaultGpuHost::~DefaultGpuHost() {
+  // Make sure |gpu_main_impl_| has been successfully created (i.e. the task
+  // posted in the constructor to run InitializeGpuMain() has actually run).
+  gpu_main_wait_.Wait();
+  gpu_main_impl_->TearDown();
+  gpu_thread_.task_runner()->DeleteSoon(FROM_HERE, std::move(gpu_main_impl_));
+  gpu_thread_.Stop();
+}
 
 void DefaultGpuHost::Add(mojom::GpuRequest request) {
   AddInternal(std::move(request));
@@ -96,6 +110,11 @@ void DefaultGpuHost::OnBadMessageFromGpu() {
   NOTIMPLEMENTED();
 }
 
+void DefaultGpuHost::InitializeGpuMain(mojom::GpuMainRequest request) {
+  gpu_main_impl_ = std::make_unique<GpuMain>(std::move(request));
+  gpu_main_wait_.Signal();
+}
+
 void DefaultGpuHost::DidInitialize(
     const gpu::GPUInfo& gpu_info,
     const gpu::GpuFeatureInfo& gpu_feature_info) {

services/ui/ws/gpu_host.h

@@ -63,6 +63,9 @@ class DefaultGpuHost : public GpuHost, public mojom::GpuHost {
   GpuClient* AddInternal(mojom::GpuRequest request);
   void OnBadMessageFromGpu();
 
+  // TODO(crbug.com/611505): this goes away after the gpu proces split in mus.
+  void InitializeGpuMain(mojom::GpuMainRequest request);
+
   // GpuHost:
   void Add(mojom::GpuRequest request) override;
   void OnAcceleratedWidgetAvailable(gfx::AcceleratedWidget widget) override;
@@ -103,7 +106,11 @@ class DefaultGpuHost : public GpuHost, public mojom::GpuHost {
 
   // TODO(fsamuel): GpuHost should not be holding onto |gpu_main_impl|
   // because that will live in another process soon.
+  base::Thread gpu_thread_;
   std::unique_ptr<GpuMain> gpu_main_impl_;
+  // This is used to make sure that the |gpu_main_impl_| has been set up
+  // correctly, before we start tearing it down.
+  base::WaitableEvent gpu_main_wait_;
 
   mojo::StrongBindingSet<mojom::Gpu> gpu_bindings_;