Fix GpuChannelHost destruction races.

GpuChannelHost::MessageFilter can outlive GpuChannelHost, so make sure it
doesn't use a raw pointer to it.
Also ensure the BrowserGpuChannelHostFactory outlives the IO thread.

BUG=168282


Review URL: https://chromiumcodereview.appspot.com/11886005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@176557 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 28 insertions, 21 deletions

diff --git a/chrome/test/gpu/gpu_feature_browsertest.cc b/chrome/test/gpu/gpu_feature_browsertest.cc
index 2234f79..81f430b 100644
--- a/chrome/test/gpu/gpu_feature_browsertest.cc
+++ b/chrome/test/gpu/gpu_feature_browsertest.cc
@@ -214,9 +214,8 @@ class AcceleratedCompositingBlockedTest : public GpuFeatureTest {
   }
 };
 
-#if (defined(OS_WIN) && defined(USE_AURA)) || defined(OS_CHROMEOS)
+#if (defined(OS_WIN) && defined(USE_AURA))
 // Compositing is always on for Windows Aura.
-// Disabled on chrome os due to http://crbug.com/167806
 #define MAYBE_AcceleratedCompositingBlocked DISABLED_AcceleratedCompositingBlocked
 #else
 #define MAYBE_AcceleratedCompositingBlocked AcceleratedCompositingBlocked
@@ -385,12 +384,7 @@ IN_PROC_BROWSER_TEST_F(WebGLMultisamplingTest, MultisamplingDisabled) {
   RunTest(url, "\"FALSE\"", true);
 }
 
-#if defined(OS_LINUX)
-#define MAYBE_Canvas2DAllowed DISABLED_Canvas2DAllowed
-#else
-#define MAYBE_Canvas2DAllowed Canvas2DAllowed
-#endif
-IN_PROC_BROWSER_TEST_F(GpuFeatureTest, MAYBE_Canvas2DAllowed) {
+IN_PROC_BROWSER_TEST_F(GpuFeatureTest, Canvas2DAllowed) {
   // Accelerated canvas 2D is not supported on XP.
   if (GPUTestBotConfig::CurrentConfigMatches("XP"))
     return;
diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc
index 55f8cf1..255e63a 100644
--- a/content/browser/browser_main_loop.cc
+++ b/content/browser/browser_main_loop.cc
@@ -522,7 +522,6 @@ void BrowserMainLoop::ShutdownThreadsAndCleanUp() {
 #if defined(USE_AURA)
   ImageTransportFactory::Terminate();
 #endif
-  BrowserGpuChannelHostFactory::Terminate();
 
   // The device monitors are using |system_monitor_| as dependency, so delete
   // them before |system_monitor_| goes away.
@@ -627,6 +626,10 @@ void BrowserMainLoop::ShutdownThreadsAndCleanUp() {
   BrowserThreadImpl::ShutdownThreadPool();
 
 #if !defined(OS_IOS)
+  // Must happen after the IO thread is shutdown since this may be accessed from
+  // it.
+  BrowserGpuChannelHostFactory::Terminate();
+
   // Must happen after the I/O thread is shutdown since this class lives on the
   // I/O thread and isn't threadsafe.
   GamepadService::GetInstance()->Terminate();
diff --git a/content/common/gpu/client/gpu_channel_host.cc b/content/common/gpu/client/gpu_channel_host.cc
index ade330d..31108b8 100644
--- a/content/common/gpu/client/gpu_channel_host.cc
+++ b/content/common/gpu/client/gpu_channel_host.cc
@@ -52,7 +52,7 @@ void GpuChannelHost::Connect(
 
   channel_->AddFilter(sync_filter_.get());
 
-  channel_filter_ = new MessageFilter(this);
+  channel_filter_ = new MessageFilter(AsWeakPtr(), factory_);
 
   // Install the filter last, because we intercept all leftover
   // messages.
@@ -316,8 +316,11 @@ int32 GpuChannelHost::ReserveTransferBufferId() {
 GpuChannelHost::~GpuChannelHost() {}
 
 
-GpuChannelHost::MessageFilter::MessageFilter(GpuChannelHost* parent)
-    : parent_(parent) {
+GpuChannelHost::MessageFilter::MessageFilter(
+    base::WeakPtr<GpuChannelHost> parent,
+    GpuChannelHostFactory* factory)
+    : parent_(parent),
+      factory_(factory) {
 }
 
 GpuChannelHost::MessageFilter::~MessageFilter() {}
@@ -326,7 +329,7 @@ void GpuChannelHost::MessageFilter::AddRoute(
     int route_id,
     base::WeakPtr<IPC::Listener> listener,
     scoped_refptr<MessageLoopProxy> loop) {
-  DCHECK(parent_->factory_->IsIOThread());
+  DCHECK(factory_->IsIOThread());
   DCHECK(listeners_.find(route_id) == listeners_.end());
   GpuListenerInfo info;
   info.listener = listener;
@@ -335,7 +338,7 @@ void GpuChannelHost::MessageFilter::AddRoute(
 }
 
 void GpuChannelHost::MessageFilter::RemoveRoute(int route_id) {
-  DCHECK(parent_->factory_->IsIOThread());
+  DCHECK(factory_->IsIOThread());
   ListenerMap::iterator it = listeners_.find(route_id);
   if (it != listeners_.end())
     listeners_.erase(it);
@@ -343,14 +346,14 @@ void GpuChannelHost::MessageFilter::RemoveRoute(int route_id) {
 
 bool GpuChannelHost::MessageFilter::OnMessageReceived(
     const IPC::Message& message) {
-  DCHECK(parent_->factory_->IsIOThread());
+  DCHECK(factory_->IsIOThread());
 
   // Never handle sync message replies or we will deadlock here.
   if (message.is_reply())
     return false;
 
   if (message.routing_id() == MSG_ROUTING_CONTROL) {
-    MessageLoop* main_loop = parent_->factory_->GetMainLoop();
+    MessageLoop* main_loop = factory_->GetMainLoop();
     main_loop->PostTask(FROM_HERE,
                         base::Bind(&GpuChannelHost::OnMessageReceived,
                                    parent_,
@@ -374,12 +377,12 @@ bool GpuChannelHost::MessageFilter::OnMessageReceived(
 }
 
 void GpuChannelHost::MessageFilter::OnChannelError() {
-  DCHECK(parent_->factory_->IsIOThread());
+  DCHECK(factory_->IsIOThread());
 
   // Post the task to signal the GpuChannelHost before the proxies. That way, if
   // they themselves post a task to recreate the context, they will not try to
   // re-use this channel host before it has a chance to mark itself lost.
-  MessageLoop* main_loop = parent_->factory_->GetMainLoop();
+  MessageLoop* main_loop = factory_->GetMainLoop();
   main_loop->PostTask(FROM_HERE,
                       base::Bind(&GpuChannelHost::OnChannelError, parent_));
   // Inform all the proxies that an error has occurred. This will be reported
diff --git a/content/common/gpu/client/gpu_channel_host.h b/content/common/gpu/client/gpu_channel_host.h
index ea37e28..13fce10 100644
--- a/content/common/gpu/client/gpu_channel_host.h
+++ b/content/common/gpu/client/gpu_channel_host.h
@@ -78,7 +78,8 @@ class CONTENT_EXPORT GpuChannelHostFactory {
 // Encapsulates an IPC channel between the client and one GPU process.
 // On the GPU process side there's a corresponding GpuChannel.
 class GpuChannelHost : public IPC::Sender,
-                       public base::RefCountedThreadSafe<GpuChannelHost> {
+                       public base::RefCountedThreadSafe<GpuChannelHost>,
+                       public base::SupportsWeakPtr<GpuChannelHost> {
  public:
   enum State {
     // Not yet connected.
@@ -185,7 +186,8 @@ class GpuChannelHost : public IPC::Sender,
   // to the correct message loop.
   class MessageFilter : public IPC::ChannelProxy::MessageFilter {
    public:
-    explicit MessageFilter(GpuChannelHost* parent);
+    MessageFilter(base::WeakPtr<GpuChannelHost> parent,
+                  GpuChannelHostFactory* factory);
 
     void AddRoute(int route_id,
                   base::WeakPtr<IPC::Listener> listener,
@@ -199,7 +201,12 @@ class GpuChannelHost : public IPC::Sender,
    private:
     virtual ~MessageFilter();
 
-    GpuChannelHost* parent_;
+    // Note: this reference can only be used to post tasks back to the
+    // GpuChannelHost, it is illegal to dereference on the IO thread where the
+    // MessageFilter lives.
+    base::WeakPtr<GpuChannelHost> parent_;
+
+    GpuChannelHostFactory* factory_;
 
     typedef base::hash_map<int, GpuListenerInfo> ListenerMap;
     ListenerMap listeners_;