Fix GpuChannelHost destruction and races

content/common/gpu/client/gpu_channel_host.cc

 // Copyright (c) 2012 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 "content/common/gpu/client/gpu_channel_host.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 50 matching lines @@
     : factory_(factory),
       gpu_info_(gpu_info),
       gpu_memory_buffer_manager_(gpu_memory_buffer_manager) {
   next_transfer_buffer_id_.GetNext();
   next_image_id_.GetNext();
   next_route_id_.GetNext();
 }
 
 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();
   channel_ = IPC::SyncChannel::Create(channel_handle,
                                       IPC::Channel::MODE_CLIENT,
                                       NULL,
                                       io_loop.get(),
                                       true,
                                       shutdown_event);
 
@@ skipping 18 matching lines @@
 
   // Currently we need to choose between two different mechanisms for sending.
   // On the main thread we use the regular channel Send() method, on another
   // thread we use SyncMessageFilter. We also have to be careful interpreting
   // IsMainThread() since it might return false during shutdown,
   // impl we are actually calling from the main thread (discard message then).
   //
   // 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());
     if (!result)
       DVLOG(1) << "GpuChannelHost::Send failed: Channel::Send failed";
     return result;
   }
 
   bool result = sync_filter_->Send(message.release());
   return result;
@@ skipping 147 matching lines @@
 
   AutoLock lock(context_lock_);
   proxies_.erase(route_id);
   if (flush_info_.flush_pending && flush_info_.route_id == route_id)
     flush_info_.flush_pending = false;
 
   delete command_buffer;
 }
 
 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();
 }
 
 void GpuChannelHost::AddRoute(
     int route_id, base::WeakPtr<IPC::Listener> listener) {
   DCHECK(MessageLoopProxy::current().get());
 
   scoped_refptr<base::MessageLoopProxy> io_loop = factory_->GetIOLoopProxy();
   io_loop->PostTask(FROM_HERE,
                     base::Bind(&GpuChannelHost::MessageFilter::AddRoute,
                                channel_filter_.get(), route_id, listener,
                                MessageLoopProxy::current()));
 }
 
 void GpuChannelHost::RemoveRoute(int route_id) {
   scoped_refptr<base::MessageLoopProxy> io_loop = factory_->GetIOLoopProxy();
   io_loop->PostTask(FROM_HERE,
                     base::Bind(&GpuChannelHost::MessageFilter::RemoveRoute,
                                channel_filter_.get(), route_id));
 }
 
 base::SharedMemoryHandle GpuChannelHost::ShareToGpuProcess(
     base::SharedMemoryHandle source_handle) {
   if (IsLost())
     return base::SharedMemory::NULLHandle();
 
 #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)) {
     return base::SharedMemory::NULLHandle();
   }
 
   return target_handle;
 #else
   int duped_handle = HANDLE_EINTR(dup(source_handle.fd));
   if (duped_handle < 0)
@@ skipping 31 matching lines @@
 
 int32 GpuChannelHost::ReserveImageId() {
   return next_image_id_.GetNext();
 }
 
 int32 GpuChannelHost::GenerateRouteID() {
   return next_route_id_.GetNext();
 }
 
 GpuChannelHost::~GpuChannelHost() {
-  DestroyChannel();
+#if DCHECK_IS_ON()
+  AutoLock lock(context_lock_);
+  DCHECK(!channel_)
+      << "GpuChannelHost::DestroyChannel must be called before destruction.";
+#endif
 }
 
 GpuChannelHost::MessageFilter::MessageFilter()
     : lost_(false) {
 }
 
 GpuChannelHost::MessageFilter::~MessageFilter() {}
 
 void GpuChannelHost::MessageFilter::AddRoute(
     int route_id,
@@ skipping 54 matching lines @@
 
   listeners_.clear();
 }
 
 bool GpuChannelHost::MessageFilter::IsLost() const {
   AutoLock lock(lock_);
   return lost_;
 }
 
 }  // namespace content