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 fbff47b5b..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;
}
