Stop using SyncChannel in GpuChannelHost

After https://chromium-review.googlesource.com/666478 SyncChannel
functionality isn't really used except to setup the underlying
IPC::Channel and attaching a message filter.

With this CL we explicitly setup an IPC::ChannelMojo instead that we
directly use from the IO thread. The MessageFilter becomes a direct
listener. This has 3 benefits:
1- Decouples GpuChannelHost from the "main thread" (SyncChannel listener
thread)
2- Slightly faster startup, because we don't need to wait for the
channel connection (peer pid) before sending messages
3- Less code, simpler invariants.

This also cleans up mojo::ScopedMessagePipeHandle <-> IPC::ChannelHandle
back-and-forth conversions.

Bug: 566273
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I8745cdf7d4fe13c61de39a152982a63d59692d0e
Reviewed-on: https://chromium-review.googlesource.com/762144
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515613}

content/browser/gpu/browser_gpu_channel_host_factory.cc

@@ -26,9 +26,6 @@
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
 #include "gpu/command_buffer/service/gpu_switches.h"
-#include "gpu/ipc/common/gpu_messages.h"
-#include "ipc/ipc_channel_handle.h"
-#include "ipc/message_filter.h"
 #include "services/resource_coordinator/public/interfaces/memory_instrumentation/constants.mojom.h"
 #include "services/service_manager/runner/common/client_util.h"
 
@@ -56,7 +53,9 @@ class BrowserGpuChannelHostFactory::EstablishRequest
   void Wait();
   void Cancel();
 
-  IPC::ChannelHandle& channel_handle() { return channel_handle_; }
+  mojo::ScopedMessagePipeHandle TakeChannelHandle() {
+    return std::move(channel_handle_);
+  }
   const gpu::GPUInfo& gpu_info() const { return gpu_info_; }
   const gpu::GpuFeatureInfo& gpu_feature_info() const {
     return gpu_feature_info_;
@@ -68,7 +67,7 @@ class BrowserGpuChannelHostFactory::EstablishRequest
   ~EstablishRequest() {}
   void RestartTimeout();
   void EstablishOnIO();
-  void OnEstablishedOnIO(const IPC::ChannelHandle& channel_handle,
+  void OnEstablishedOnIO(mojo::ScopedMessagePipeHandle channel_handle,
                          const gpu::GPUInfo& gpu_info,
                          const gpu::GpuFeatureInfo& gpu_feature_info,
                          GpuProcessHost::EstablishChannelStatus status);
@@ -78,7 +77,7 @@ class BrowserGpuChannelHostFactory::EstablishRequest
   base::WaitableEvent event_;
   const int gpu_client_id_;
   const uint64_t gpu_client_tracing_id_;
-  IPC::ChannelHandle channel_handle_;
+  mojo::ScopedMessagePipeHandle channel_handle_;
   gpu::GPUInfo gpu_info_;
   gpu::GpuFeatureInfo gpu_feature_info_;
   bool finished_;
@@ -138,11 +137,11 @@ void BrowserGpuChannelHostFactory::EstablishRequest::EstablishOnIO() {
 }
 
 void BrowserGpuChannelHostFactory::EstablishRequest::OnEstablishedOnIO(
-    const IPC::ChannelHandle& channel_handle,
+    mojo::ScopedMessagePipeHandle channel_handle,
     const gpu::GPUInfo& gpu_info,
     const gpu::GpuFeatureInfo& gpu_feature_info,
     GpuProcessHost::EstablishChannelStatus status) {
-  if (!channel_handle.mojo_handle.is_valid() &&
+  if (!channel_handle.is_valid() &&
       status == GpuProcessHost::EstablishChannelStatus::GPU_HOST_INVALID) {
     DVLOG(1) << "Failed to create channel on existing GPU process. Trying to "
                 "restart GPU process.";
@@ -154,7 +153,7 @@ void BrowserGpuChannelHostFactory::EstablishRequest::OnEstablishedOnIO(
     EstablishOnIO();
     return;
   }
-  channel_handle_ = channel_handle;
+  channel_handle_ = std::move(channel_handle);
   gpu_info_ = gpu_info;
   gpu_feature_info_ = gpu_feature_info;
   FinishOnIO();
@@ -223,9 +222,6 @@ BrowserGpuChannelHostFactory::BrowserGpuChannelHostFactory()
     : gpu_client_id_(ChildProcessHostImpl::GenerateChildProcessUniqueId()),
       gpu_client_tracing_id_(
           memory_instrumentation::mojom::kServiceTracingProcessId),
-      shutdown_event_(new base::WaitableEvent(
-          base::WaitableEvent::ResetPolicy::MANUAL,
-          base::WaitableEvent::InitialState::NOT_SIGNALED)),
       gpu_memory_buffer_manager_(
           new BrowserGpuMemoryBufferManager(gpu_client_id_,
                                             gpu_client_tracing_id_)) {
@@ -245,20 +241,15 @@ BrowserGpuChannelHostFactory::BrowserGpuChannelHostFactory()
 }
 
 BrowserGpuChannelHostFactory::~BrowserGpuChannelHostFactory() {
-  DCHECK(IsMainThread());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (pending_request_.get())
     pending_request_->Cancel();
-  shutdown_event_->Signal();
   if (gpu_channel_) {
     gpu_channel_->DestroyChannel();
     gpu_channel_ = nullptr;
   }
 }
 
-bool BrowserGpuChannelHostFactory::IsMainThread() {
-  return BrowserThread::CurrentlyOn(BrowserThread::UI);
-}
-
 scoped_refptr<base::SingleThreadTaskRunner>
 BrowserGpuChannelHostFactory::GetIOThreadTaskRunner() {
   return BrowserThread::GetTaskRunnerForThread(BrowserThread::IO);
@@ -277,7 +268,7 @@ void BrowserGpuChannelHostFactory::EstablishGpuChannel(
 #if defined(USE_AURA)
   DCHECK_EQ(aura::Env::Mode::LOCAL, aura::Env::GetInstance()->mode());
 #endif
-  DCHECK(IsMainThread());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (gpu_channel_.get() && gpu_channel_->IsLost()) {
     DCHECK(!pending_request_.get());
     // Recreate the channel if it has been lost.
@@ -332,16 +323,16 @@ gpu::GpuChannelHost* BrowserGpuChannelHostFactory::GetGpuChannel() {
 }
 
 void BrowserGpuChannelHostFactory::GpuChannelEstablished() {
-  DCHECK(IsMainThread());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(pending_request_.get());
-  if (!pending_request_->channel_handle().mojo_handle.is_valid()) {
+  mojo::ScopedMessagePipeHandle handle(pending_request_->TakeChannelHandle());
+  if (!handle.is_valid()) {
     DCHECK(!gpu_channel_.get());
   } else {
     GetContentClient()->SetGpuInfo(pending_request_->gpu_info());
-    gpu_channel_ = gpu::GpuChannelHost::Create(
+    gpu_channel_ = base::MakeRefCounted<gpu::GpuChannelHost>(
         this, gpu_client_id_, pending_request_->gpu_info(),
-        pending_request_->gpu_feature_info(),
-        pending_request_->channel_handle(), shutdown_event_.get(),
+        pending_request_->gpu_feature_info(), std::move(handle),
         gpu_memory_buffer_manager_.get());
   }
   pending_request_ = nullptr;
@@ -354,7 +345,7 @@ void BrowserGpuChannelHostFactory::GpuChannelEstablished() {
 }
 
 void BrowserGpuChannelHostFactory::RestartTimeout() {
-  DCHECK(IsMainThread());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 // Only implement timeout on Android, which does not have a software fallback.
 #if defined(OS_ANDROID)
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(

content/browser/gpu/browser_gpu_channel_host_factory.h

@@ -32,7 +32,6 @@ class CONTENT_EXPORT BrowserGpuChannelHostFactory
   static BrowserGpuChannelHostFactory* instance() { return instance_; }
 
   // Overridden from gpu::GpuChannelHostFactory:
-  bool IsMainThread() override;
   scoped_refptr<base::SingleThreadTaskRunner> GetIOThreadTaskRunner() override;
   std::unique_ptr<base::SharedMemory> AllocateSharedMemory(
       size_t size) override;
@@ -68,7 +67,6 @@ class CONTENT_EXPORT BrowserGpuChannelHostFactory
 
   const int gpu_client_id_;
   const uint64_t gpu_client_tracing_id_;
-  std::unique_ptr<base::WaitableEvent> shutdown_event_;
   scoped_refptr<gpu::GpuChannelHost> gpu_channel_;
   std::unique_ptr<BrowserGpuMemoryBufferManager> gpu_memory_buffer_manager_;
   scoped_refptr<EstablishRequest> pending_request_;

content/browser/gpu/gpu_client.cc

@@ -40,27 +40,17 @@ void GpuClient::OnError() {
 
 void GpuClient::OnEstablishGpuChannel(
     const EstablishGpuChannelCallback& callback,
-    const IPC::ChannelHandle& channel,
+    mojo::ScopedMessagePipeHandle channel_handle,
     const gpu::GPUInfo& gpu_info,
     const gpu::GpuFeatureInfo& gpu_feature_info,
     GpuProcessHost::EstablishChannelStatus status) {
-  if (status == GpuProcessHost::EstablishChannelStatus::GPU_ACCESS_DENIED) {
-    // GPU access is not allowed. Notify the client immediately.
-    DCHECK(!channel.mojo_handle.is_valid());
-    callback.Run(render_process_id_, mojo::ScopedMessagePipeHandle(), gpu_info,
-                 gpu_feature_info);
-    return;
-  }
-
+  DCHECK_EQ(channel_handle.is_valid(),
+            status == GpuProcessHost::EstablishChannelStatus::SUCCESS);
   if (status == GpuProcessHost::EstablishChannelStatus::GPU_HOST_INVALID) {
     // GPU process may have crashed or been killed. Try again.
-    DCHECK(!channel.mojo_handle.is_valid());
     EstablishGpuChannel(callback);
     return;
   }
-  DCHECK(channel.mojo_handle.is_valid());
-  mojo::ScopedMessagePipeHandle channel_handle;
-  channel_handle.reset(channel.mojo_handle);
   callback.Run(render_process_id_, std::move(channel_handle), gpu_info,
                gpu_feature_info);
 }
@@ -76,7 +66,8 @@ void GpuClient::EstablishGpuChannel(
   GpuProcessHost* host = GpuProcessHost::Get();
   if (!host) {
     OnEstablishGpuChannel(
-        callback, IPC::ChannelHandle(), gpu::GPUInfo(), gpu::GpuFeatureInfo(),
+        callback, mojo::ScopedMessagePipeHandle(), gpu::GPUInfo(),
+        gpu::GpuFeatureInfo(),
         GpuProcessHost::EstablishChannelStatus::GPU_ACCESS_DENIED);
     return;
   }

content/browser/gpu/gpu_client.h

@@ -7,7 +7,6 @@
 
 #include "base/memory/weak_ptr.h"
 #include "content/browser/gpu/gpu_process_host.h"
-#include "ipc/ipc_channel_handle.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "services/ui/public/interfaces/gpu.mojom.h"
 
@@ -23,7 +22,7 @@ class GpuClient : public ui::mojom::Gpu {
  private:
   void OnError();
   void OnEstablishGpuChannel(const EstablishGpuChannelCallback& callback,
-                             const IPC::ChannelHandle& channel,
+                             mojo::ScopedMessagePipeHandle channel_handle,
                              const gpu::GPUInfo& gpu_info,
                              const gpu::GpuFeatureInfo& gpu_feature_info,
                              GpuProcessHost::EstablishChannelStatus status);

content/browser/gpu/gpu_process_host.cc

@@ -57,8 +57,6 @@
 #include "gpu/config/gpu_driver_bug_workaround_type.h"
 #include "gpu/ipc/host/shader_disk_cache.h"
 #include "gpu/ipc/service/switches.h"
-#include "ipc/ipc_channel_handle.h"
-#include "ipc/message_filter.h"
 #include "media/base/media_switches.h"
 #include "media/media_features.h"
 #include "mojo/edk/embedder/embedder.h"
@@ -690,7 +688,8 @@ void GpuProcessHost::EstablishGpuChannel(
   // If GPU features are already blacklisted, no need to establish the channel.
   if (!GpuDataManagerImpl::GetInstance()->GpuAccessAllowed(nullptr)) {
     DVLOG(1) << "GPU blacklisted, refusing to open a GPU channel.";
-    callback.Run(IPC::ChannelHandle(), gpu::GPUInfo(), gpu::GpuFeatureInfo(),
+    callback.Run(mojo::ScopedMessagePipeHandle(), gpu::GPUInfo(),
+                 gpu::GpuFeatureInfo(),
                  EstablishChannelStatus::GPU_ACCESS_DENIED);
     return;
   }
@@ -788,15 +787,15 @@ void GpuProcessHost::OnChannelEstablished(
   if (channel_handle.is_valid() &&
       !gpu_data_manager->GpuAccessAllowed(nullptr)) {
     gpu_service_ptr_->CloseChannel(client_id);
-    callback.Run(IPC::ChannelHandle(), gpu::GPUInfo(), gpu::GpuFeatureInfo(),
+    callback.Run(mojo::ScopedMessagePipeHandle(), gpu::GPUInfo(),
+                 gpu::GpuFeatureInfo(),
                  EstablishChannelStatus::GPU_ACCESS_DENIED);
     RecordLogMessage(logging::LOG_WARNING, "WARNING",
                      "Hardware acceleration is unavailable.");
     return;
   }
 
-  callback.Run(IPC::ChannelHandle(channel_handle.release()),
-               gpu_data_manager->GetGPUInfo(),
+  callback.Run(std::move(channel_handle), gpu_data_manager->GetGPUInfo(),
                gpu_data_manager->GetGpuFeatureInfo(),
                EstablishChannelStatus::SUCCESS);
 }
@@ -1099,7 +1098,8 @@ void GpuProcessHost::SendOutstandingReplies() {
   while (!channel_requests_.empty()) {
     auto callback = channel_requests_.front();
     channel_requests_.pop();
-    callback.Run(IPC::ChannelHandle(), gpu::GPUInfo(), gpu::GpuFeatureInfo(),
+    callback.Run(mojo::ScopedMessagePipeHandle(), gpu::GPUInfo(),
+                 gpu::GpuFeatureInfo(),
                  EstablishChannelStatus::GPU_HOST_INVALID);
   }
 

content/browser/gpu/gpu_process_host.h

@@ -29,7 +29,6 @@
 #include "gpu/config/gpu_info.h"
 #include "gpu/ipc/common/surface_handle.h"
 #include "ipc/ipc_sender.h"
-#include "ipc/message_filter.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "services/viz/privileged/interfaces/compositing/frame_sink_manager.mojom.h"
 #include "services/viz/privileged/interfaces/gl/gpu_host.mojom.h"
@@ -43,10 +42,6 @@ namespace base {
 class Thread;
 }
 
-namespace IPC {
-struct ChannelHandle;
-}
-
 namespace gpu {
 class ShaderDiskCache;
 struct SyncToken;
@@ -74,7 +69,7 @@ class GpuProcessHost : public BrowserChildProcessHostDelegate,
     SUCCESS
   };
   using EstablishChannelCallback =
-      base::Callback<void(const IPC::ChannelHandle&,
+      base::Callback<void(mojo::ScopedMessagePipeHandle channel_handle,
                           const gpu::GPUInfo&,
                           const gpu::GpuFeatureInfo&,
                           EstablishChannelStatus status)>;

content/renderer/render_thread_impl.h

@@ -46,6 +46,7 @@
 #include "content/renderer/layout_test_dependencies.h"
 #include "content/renderer/media/audio_ipc_factory.h"
 #include "gpu/ipc/client/gpu_channel_host.h"
+#include "ipc/ipc_sync_channel.h"
 #include "media/media_features.h"
 #include "mojo/public/cpp/bindings/associated_binding.h"
 #include "mojo/public/cpp/bindings/binding.h"

content/renderer/render_widget.cc

@@ -68,6 +68,7 @@
 #include "content/renderer/resizing_mode_selector.h"
 #include "ipc/ipc_message_start.h"
 #include "ipc/ipc_sync_message.h"
+#include "ipc/ipc_sync_message_filter.h"
 #include "ppapi/features/features.h"
 #include "skia/ext/platform_canvas.h"
 #include "third_party/WebKit/public/platform/FilePathConversion.h"

content/renderer/renderer_blink_platform_impl.cc

@@ -85,6 +85,7 @@
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/config/gpu_info.h"
 #include "gpu/ipc/client/gpu_channel_host.h"
+#include "ipc/ipc_sync_channel.h"
 #include "ipc/ipc_sync_message_filter.h"
 #include "media/audio/audio_output_device.h"
 #include "media/blink/webcontentdecryptionmodule_impl.h"