Fix leak in GpuChannel

There may be remaining messages in the deferred queue on unclean teardown.
Delete those.

Also, a little bit of cleanup in the area:
- GpuChannel doesn't need to be refcounted
- It's ok to copy WeakPtr across threads, as long as they're only dereferenced
on the thread that the factory is destroyed. So no need for the extra
indirection.

BUG=374843

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

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

content/common/gpu/gpu_channel.cc

@@ -15,6 +15,7 @@
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/timer/timer.h"
 #include "content/common/gpu/devtools_gpu_agent.h"
@@ -71,8 +72,7 @@ const int64 kStopPreemptThresholdMs = kVsyncIntervalMs;
 // - it generates mailbox names for clients of the GPU process on the IO thread.
 class GpuChannelMessageFilter : public IPC::MessageFilter {
  public:
-  // Takes ownership of gpu_channel (see below).
-  GpuChannelMessageFilter(base::WeakPtr<GpuChannel>* gpu_channel,
+  GpuChannelMessageFilter(base::WeakPtr<GpuChannel> gpu_channel,
                           scoped_refptr<SyncPointManager> sync_point_manager,
                           scoped_refptr<base::MessageLoopProxy> message_loop)
       : preemption_state_(IDLE),
@@ -81,8 +81,7 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
         sync_point_manager_(sync_point_manager),
         message_loop_(message_loop),
         messages_forwarded_to_channel_(0),
-        a_stub_is_descheduled_(false) {
-  }
+        a_stub_is_descheduled_(false) {}
 
   virtual void OnFilterAdded(IPC::Channel* channel) OVERRIDE {
     DCHECK(!channel_);
@@ -153,10 +152,7 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
   }
 
  protected:
-  virtual ~GpuChannelMessageFilter() {
-    message_loop_->PostTask(FROM_HERE, base::Bind(
-        &GpuChannelMessageFilter::DeleteWeakPtrOnMainThread, gpu_channel_));
-  }
+  virtual ~GpuChannelMessageFilter() {}
 
  private:
   enum PreemptionState {
@@ -337,7 +333,7 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
   }
 
   static void InsertSyncPointOnMainThread(
-      base::WeakPtr<GpuChannel>* gpu_channel,
+      base::WeakPtr<GpuChannel> gpu_channel,
       scoped_refptr<SyncPointManager> manager,
       int32 routing_id,
       uint32 sync_point) {
@@ -346,30 +342,23 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
     // with it, but if that fails for any reason (channel or stub already
     // deleted, invalid routing id), we need to retire the sync point
     // immediately.
-    if (gpu_channel->get()) {
-      GpuCommandBufferStub* stub = gpu_channel->get()->LookupCommandBuffer(
-          routing_id);
+    if (gpu_channel) {
+      GpuCommandBufferStub* stub = gpu_channel->LookupCommandBuffer(routing_id);
       if (stub) {
         stub->AddSyncPoint(sync_point);
         GpuCommandBufferMsg_RetireSyncPoint message(routing_id, sync_point);
-        gpu_channel->get()->OnMessageReceived(message);
+        gpu_channel->OnMessageReceived(message);
         return;
       } else {
-        gpu_channel->get()->MessageProcessed();
+        gpu_channel->MessageProcessed();
       }
     }
     manager->RetireSyncPoint(sync_point);
   }
 
-  static void DeleteWeakPtrOnMainThread(
-      base::WeakPtr<GpuChannel>* gpu_channel) {
-    delete gpu_channel;
-  }
-
-  // NOTE: this is a pointer to a weak pointer. It is never dereferenced on the
-  // IO thread, it's only passed through - therefore the WeakPtr assumptions are
-  // respected.
-  base::WeakPtr<GpuChannel>* gpu_channel_;
+  // NOTE: this weak pointer is never dereferenced on the IO thread, it's only
+  // passed through - therefore the WeakPtr assumptions are respected.
+  base::WeakPtr<GpuChannel> gpu_channel_;
   IPC::Channel* channel_;
   scoped_refptr<SyncPointManager> sync_point_manager_;
   scoped_refptr<base::MessageLoopProxy> message_loop_;
@@ -411,6 +400,11 @@ GpuChannel::GpuChannel(GpuChannelManager* gpu_channel_manager,
   log_messages_ = command_line->HasSwitch(switches::kLogPluginMessages);
 }
 
+GpuChannel::~GpuChannel() {
+  STLDeleteElements(&deferred_messages_);
+  if (preempting_flag_.get())
+    preempting_flag_->Reset();
+}
 
 void GpuChannel::Init(base::MessageLoopProxy* io_message_loop,
                       base::WaitableEvent* shutdown_event) {
@@ -425,13 +419,10 @@ void GpuChannel::Init(base::MessageLoopProxy* io_message_loop,
       false,
       shutdown_event));
 
-  base::WeakPtr<GpuChannel>* weak_ptr(new base::WeakPtr<GpuChannel>(
-      weak_factory_.GetWeakPtr()));
-
-  filter_ = new GpuChannelMessageFilter(
-      weak_ptr,
-      gpu_channel_manager_->sync_point_manager(),
-      base::MessageLoopProxy::current());
+  filter_ =
+      new GpuChannelMessageFilter(weak_factory_.GetWeakPtr(),
+                                  gpu_channel_manager_->sync_point_manager(),
+                                  base::MessageLoopProxy::current());
   io_message_loop_ = io_message_loop;
   channel_->AddFilter(filter_.get());
 
@@ -632,11 +623,6 @@ void GpuChannel::MarkAllContextsLost() {
   }
 }
 
-void GpuChannel::DestroySoon() {
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE, base::Bind(&GpuChannel::OnDestroy, this));
-}
-
 bool GpuChannel::AddRoute(int32 route_id, IPC::Listener* listener) {
   return router_.AddRoute(route_id, listener);
 }
@@ -666,11 +652,6 @@ void GpuChannel::SetPreemptByFlag(
   }
 }
 
-GpuChannel::~GpuChannel() {
-  if (preempting_flag_.get())
-    preempting_flag_->Reset();
-}
-
 void GpuChannel::OnDestroy() {
   TRACE_EVENT0("gpu", "GpuChannel::OnDestroy");
   gpu_channel_manager_->RemoveChannel(client_id_);

content/common/gpu/gpu_channel.h

@@ -50,9 +50,7 @@ class GpuWatchdog;
 
 // Encapsulates an IPC channel between the GPU process and one renderer
 // process. On the renderer side there's a corresponding GpuChannelHost.
-class GpuChannel : public IPC::Listener,
-                   public IPC::Sender,
-                   public base::RefCountedThreadSafe<GpuChannel> {
+class GpuChannel : public IPC::Listener, public IPC::Sender {
  public:
   // Takes ownership of the renderer process handle.
   GpuChannel(GpuChannelManager* gpu_channel_manager,
@@ -61,6 +59,7 @@ class GpuChannel : public IPC::Listener,
              gpu::gles2::MailboxManager* mailbox_manager,
              int client_id,
              bool software);
+  virtual ~GpuChannel();
 
   void Init(base::MessageLoopProxy* io_message_loop,
             base::WaitableEvent* shutdown_event);
@@ -127,9 +126,6 @@ class GpuChannel : public IPC::Listener,
   void LoseAllContexts();
   void MarkAllContextsLost();
 
-  // Destroy channel and all contained contexts.
-  void DestroySoon();
-
   // Called to add a listener for a particular message routing ID.
   // Returns true if succeeded.
   bool AddRoute(int32 route_id, IPC::Listener* listener);
@@ -154,11 +150,7 @@ class GpuChannel : public IPC::Listener,
 
   uint64 GetMemoryUsage();
 
- protected:
-  virtual ~GpuChannel();
-
  private:
-  friend class base::RefCountedThreadSafe<GpuChannel>;
   friend class GpuChannelMessageFilter;
 
   void OnDestroy();

content/common/gpu/gpu_channel_manager.cc

@@ -98,7 +98,7 @@ GpuChannel* GpuChannelManager::LookupChannel(int32 client_id) {
   if (iter == gpu_channels_.end())
     return NULL;
   else
-    return iter->second.get();
+    return iter->second;
 }
 
 bool GpuChannelManager::OnMessageReceived(const IPC::Message& msg) {
@@ -133,14 +133,9 @@ void GpuChannelManager::OnEstablishChannel(int client_id, bool share_context) {
     mailbox_manager = mailbox_manager_.get();
   }
 
-  scoped_refptr<GpuChannel> channel = new GpuChannel(this,
-                                                     watchdog_,
-                                                     share_group,
-                                                     mailbox_manager,
-                                                     client_id,
-                                                     false);
+  scoped_ptr<GpuChannel> channel(new GpuChannel(
+      this, watchdog_, share_group, mailbox_manager, client_id, false));
   channel->Init(io_message_loop_.get(), shutdown_event_);
-  gpu_channels_[client_id] = channel;
   channel_handle.name = channel->GetChannelName();
 
 #if defined(OS_POSIX)
@@ -151,6 +146,8 @@ void GpuChannelManager::OnEstablishChannel(int client_id, bool share_context) {
   channel_handle.socket = base::FileDescriptor(renderer_fd, true);
 #endif
 
+  gpu_channels_.set(client_id, channel.Pass());
+
   Send(new GpuHostMsg_ChannelEstablished(channel_handle));
 }
 

content/common/gpu/gpu_channel_manager.h

@@ -9,7 +9,7 @@
 #include <string>
 #include <vector>
 
-#include "base/containers/hash_tables.h"
+#include "base/containers/scoped_ptr_hash_map.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ -105,7 +105,7 @@ class GpuChannelManager : public IPC::Listener,
     int32 sync_point;
     base::Closure callback;
   };
-  typedef base::hash_map<int, scoped_refptr<GpuChannel> > GpuChannelMap;
+  typedef base::ScopedPtrHashMap<int, GpuChannel> GpuChannelMap;
   typedef std::deque<ImageOperation*> ImageOperationQueue;
 
   // Message handlers.