Fix GpuChannelHost destruction and races

https://codereview.chromium.org/583043005 introduced
GpuChannelHost::DestroyChannel but it introduced a new state to GpuChannelHost
which was not captured everywhere and had races.

1- Ensure GpuChannelHost::DestroyChannel is always called on the main thread, before destruction.
2- properly handle the channel_ == NULL state.
3- Fix races (channel_ is used on any thread on windows, must be locked)

Also make sure contexts are destroyed before the channel, for sanity.

BUG=451456,471822

Review URL: https://codereview.chromium.org/1128233004

Cr-Commit-Position: refs/heads/master@{#328917}
diff --git a/content/browser/gpu/browser_gpu_channel_host_factory.cc b/content/browser/gpu/browser_gpu_channel_host_factory.cc
index 9edb967..4325c06 100644
--- a/content/browser/gpu/browser_gpu_channel_host_factory.cc
+++ b/content/browser/gpu/browser_gpu_channel_host_factory.cc
@@ -295,6 +295,10 @@
   for (size_t n = 0; n < established_callbacks_.size(); n++)
     established_callbacks_[n].Run();
   shutdown_event_->Signal();
+  if (gpu_channel_) {
+    gpu_channel_->DestroyChannel();
+    gpu_channel_ = NULL;
+  }
 }
 
 bool BrowserGpuChannelHostFactory::IsMainThread() {
@@ -396,6 +400,7 @@
   if (gpu_channel_.get() && gpu_channel_->IsLost()) {
     DCHECK(!pending_request_.get());
     // Recreate the channel if it has been lost.
+    gpu_channel_->DestroyChannel();
     gpu_channel_ = NULL;
   }
 
diff --git a/content/common/gpu/client/gpu_channel_host.cc b/content/common/gpu/client/gpu_channel_host.cc
index aa47e2f..475eeb15 100644
--- a/content/common/gpu/client/gpu_channel_host.cc
+++ b/content/common/gpu/client/gpu_channel_host.cc
@@ -68,6 +68,7 @@
 
 void GpuChannelHost::Connect(const IPC::ChannelHandle& channel_handle,
                              base::WaitableEvent* shutdown_event) {
+  DCHECK(factory_->IsMainThread());
   // Open a channel to the GPU process. We pass NULL as the main listener here
   // since we need to filter everything to route it to the right thread.
   scoped_refptr<base::MessageLoopProxy> io_loop = factory_->GetIOLoopProxy();
@@ -106,6 +107,12 @@
   // TODO: Can we just always use sync_filter_ since we setup the channel
   //       without a main listener?
   if (factory_->IsMainThread()) {
+    // channel_ is only modified on the main thread, so we don't need to take a
+    // lock here.
+    if (!channel_) {
+      DVLOG(1) << "GpuChannelHost::Send failed: Channel already destroyed";
+      return false;
+    }
     // http://crbug.com/125264
     base::ThreadRestrictions::ScopedAllowWait allow_wait;
     bool result = channel_->Send(message.release());
@@ -273,9 +280,8 @@
 }
 
 void GpuChannelHost::DestroyChannel() {
-  // channel_ must be destroyed on the main thread.
-  if (channel_.get() && !factory_->IsMainThread())
-    factory_->GetMainLoop()->DeleteSoon(FROM_HERE, channel_.release());
+  DCHECK(factory_->IsMainThread());
+  AutoLock lock(context_lock_);
   channel_.reset();
 }
 
@@ -305,8 +311,15 @@
 #if defined(OS_WIN)
   // Windows needs to explicitly duplicate the handle out to another process.
   base::SharedMemoryHandle target_handle;
+  base::ProcessId peer_pid;
+  {
+    AutoLock lock(context_lock_);
+    if (!channel_)
+      return base::SharedMemory::NULLHandle();
+    peer_pid = channel_->GetPeerPID();
+  }
   if (!BrokerDuplicateHandle(source_handle,
-                             channel_->GetPeerPID(),
+                             peer_pid,
                              &target_handle,
                              FILE_GENERIC_READ | FILE_GENERIC_WRITE,
                              0)) {
@@ -358,7 +371,11 @@
 }
 
 GpuChannelHost::~GpuChannelHost() {
-  DestroyChannel();
+#if DCHECK_IS_ON()
+  AutoLock lock(context_lock_);
+  DCHECK(!channel_)
+      << "GpuChannelHost::DestroyChannel must be called before destruction.";
+#endif
 }
 
 GpuChannelHost::MessageFilter::MessageFilter()
diff --git a/content/common/gpu/client/gpu_channel_host.h b/content/common/gpu/client/gpu_channel_host.h
index fbff47b..b692ec18 100644
--- a/content/common/gpu/client/gpu_channel_host.h
+++ b/content/common/gpu/client/gpu_channel_host.h
@@ -151,7 +151,8 @@
   // Destroy a command buffer created by this channel.
   void DestroyCommandBuffer(CommandBufferProxyImpl* command_buffer);
 
-  // Destroy this channel.
+  // Destroy this channel. Must be called on the main thread, before
+  // destruction.
   void DestroyChannel();
 
   // Add a route for the current message loop.
@@ -246,7 +247,6 @@
 
   const gpu::GPUInfo gpu_info_;
 
-  scoped_ptr<IPC::SyncChannel> channel_;
   scoped_refptr<MessageFilter> channel_filter_;
 
   gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager_;
@@ -263,8 +263,9 @@
   // Route IDs are allocated in sequence.
   base::AtomicSequenceNumber next_route_id_;
 
-  // Protects proxies_.
+  // Protects channel_ and proxies_.
   mutable base::Lock context_lock_;
+  scoped_ptr<IPC::SyncChannel> channel_;
   // Used to look up a proxy from its routing id.
   typedef base::hash_map<int, CommandBufferProxyImpl*> ProxyMap;
   ProxyMap proxies_;
diff --git a/content/renderer/render_thread_impl.cc b/content/renderer/render_thread_impl.cc
index 207724b..5d010b7 100644
--- a/content/renderer/render_thread_impl.cc
+++ b/content/renderer/render_thread_impl.cc
@@ -790,6 +790,7 @@
 
   // Context providers must be released prior to destroying the GPU channel.
   gpu_va_context_provider_ = nullptr;
+  shared_main_thread_contexts_ = nullptr;
 
   if (gpu_channel_.get())
     gpu_channel_->DestroyChannel();
@@ -1605,6 +1606,7 @@
       return gpu_channel_.get();
 
     // Recreate the channel if it has been lost.
+    gpu_channel_->DestroyChannel();
     gpu_channel_ = NULL;
   }