GpuVideoDecodeAccelerator lifecycle fixes.

* GpuVideoDecodeAccelerator clears its reference to its parent
  GpuCommandBufferStub on GpuVideoDecodeAccelerator::OnWillDestroy(),
  then only removes its IPC channel route on destruction if the stub
  reference is valid.  This means the route is never actually removed,
  and so we leak the IPC route.  Modify OnWillDestroy() to immediately
  delete the GpuVideoDecodeAccelerator object and remove the route.
* GpuCommandBufferStub owns the GpuVideoDecodeAccelerator objects it
  creates, but does not destroy them when they are destroyed through the
  AcceleratedVideoDecoderMsg_Destroy IPC.  This causes a leak of
  GpuVDA objects as long as the GpuCommandBufferStub is alive.  Allow
  the OnDestroy() callback for AcceleratedVideoDecoderMsg_Destroy to
  destroy the GpuVDA directly, and remove the explicit owned list of
  GpuVDA objects from GpuCommandBufferStub to avoid double-deletion.

BUG=234465
TEST=local build, run on CrOS snow, desktop Linux

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

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

6 files changed, 23 insertions, 36 deletions

diff --git a/content/common/gpu/gpu_command_buffer_stub.cc b/content/common/gpu/gpu_command_buffer_stub.cc
index ceecbe4..9ebb287d 100644
--- a/content/common/gpu/gpu_command_buffer_stub.cc
+++ b/content/common/gpu/gpu_command_buffer_stub.cc
@@ -358,7 +358,7 @@ void GpuCommandBufferStub::Destroy() {
     have_context = decoder_->MakeCurrent();
   FOR_EACH_OBSERVER(DestructionObserver,
                     destruction_observers_,
-                    OnWillDestroyStub(this));
+                    OnWillDestroyStub());
 
   scoped_refptr<gfx::GLContext> context;
   if (decoder_) {
@@ -759,8 +759,9 @@ void GpuCommandBufferStub::OnCreateVideoDecoder(
   int decoder_route_id = channel_->GenerateRouteID();
   GpuVideoDecodeAccelerator* decoder =
       new GpuVideoDecodeAccelerator(decoder_route_id, this);
-  video_decoders_.push_back(decoder);
   decoder->Initialize(profile, reply_message);
+  // decoder is registered as a DestructionObserver of this stub and will
+  // self-delete during destruction of this stub.
 }
 
 void GpuCommandBufferStub::OnSetSurfaceVisible(bool visible) {
diff --git a/content/common/gpu/gpu_command_buffer_stub.h b/content/common/gpu/gpu_command_buffer_stub.h
index 4ff2237..c7f8195 100644
--- a/content/common/gpu/gpu_command_buffer_stub.h
+++ b/content/common/gpu/gpu_command_buffer_stub.h
@@ -53,7 +53,7 @@ class GpuCommandBufferStub
   class DestructionObserver {
    public:
     // Called in Destroy(), before the context/surface are released.
-    virtual void OnWillDestroyStub(GpuCommandBufferStub* stub) = 0;
+    virtual void OnWillDestroyStub() = 0;
 
    protected:
     virtual ~DestructionObserver() {}
@@ -236,10 +236,6 @@ class GpuCommandBufferStub
 
   GpuWatchdog* watchdog_;
 
-  // Zero or more video decoders owned by this stub.  Only used for lifecycle
-  // management (so the order within the vector is irrelevant).
-  ScopedVector<GpuVideoDecodeAccelerator> video_decoders_;
-
   ObserverList<DestructionObserver> destruction_observers_;
 
   // A queue of sync points associated with this stub.
diff --git a/content/common/gpu/media/gpu_video_decode_accelerator.cc b/content/common/gpu/media/gpu_video_decode_accelerator.cc
index 82be8bb..251e202 100644
--- a/content/common/gpu/media/gpu_video_decode_accelerator.cc
+++ b/content/common/gpu/media/gpu_video_decode_accelerator.cc
@@ -60,28 +60,28 @@ GpuVideoDecodeAccelerator::GpuVideoDecodeAccelerator(
     GpuCommandBufferStub* stub)
     : init_done_msg_(NULL),
       host_route_id_(host_route_id),
-      stub_(stub->AsWeakPtr()),
+      stub_(stub),
       video_decode_accelerator_(NULL),
       texture_target_(0) {
-  if (!stub_)
-    return;
+  DCHECK(stub_);
   stub_->AddDestructionObserver(this);
+  stub_->channel()->AddRoute(host_route_id_, this);
   make_context_current_ =
       base::Bind(&MakeDecoderContextCurrent, stub_->AsWeakPtr());
 }
 
 GpuVideoDecodeAccelerator::~GpuVideoDecodeAccelerator() {
-  if (stub_) {
-    stub_->channel()->RemoveRoute(host_route_id_);
-    stub_->RemoveDestructionObserver(this);
-  }
-
+  DCHECK(stub_);
   if (video_decode_accelerator_)
     video_decode_accelerator_.release()->Destroy();
+
+  stub_->channel()->RemoveRoute(host_route_id_);
+  stub_->RemoveDestructionObserver(this);
 }
 
 bool GpuVideoDecodeAccelerator::OnMessageReceived(const IPC::Message& msg) {
-  if (!stub_ || !video_decode_accelerator_)
+  DCHECK(stub_);
+  if (!video_decode_accelerator_)
     return false;
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(GpuVideoDecodeAccelerator, msg)
@@ -153,14 +153,11 @@ void GpuVideoDecodeAccelerator::NotifyError(
 void GpuVideoDecodeAccelerator::Initialize(
     const media::VideoCodecProfile profile,
     IPC::Message* init_done_msg) {
+  DCHECK(stub_);
   DCHECK(!video_decode_accelerator_.get());
   DCHECK(!init_done_msg_);
   DCHECK(init_done_msg);
   init_done_msg_ = init_done_msg;
-  if (!stub_)
-    return;
-
-  stub_->channel()->AddRoute(host_route_id_, this);
 
 #if !defined(OS_WIN)
   // Ensure we will be able to get a GL context at all before initializing
@@ -232,6 +229,7 @@ void GpuVideoDecodeAccelerator::OnAssignPictureBuffers(
       const std::vector<int32>& buffer_ids,
       const std::vector<uint32>& texture_ids,
       const std::vector<gfx::Size>& sizes) {
+  DCHECK(stub_);
   if (buffer_ids.size() != texture_ids.size() ||
       buffer_ids.size() != sizes.size()) {
     NotifyError(media::VideoDecodeAccelerator::INVALID_ARGUMENT);
@@ -308,7 +306,7 @@ void GpuVideoDecodeAccelerator::OnReset() {
 
 void GpuVideoDecodeAccelerator::OnDestroy() {
   DCHECK(video_decode_accelerator_.get());
-  video_decode_accelerator_.release()->Destroy();
+  delete this;
 }
 
 void GpuVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer(
@@ -339,14 +337,8 @@ void GpuVideoDecodeAccelerator::NotifyResetDone() {
     DLOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_ResetDone) failed";
 }
 
-void GpuVideoDecodeAccelerator::OnWillDestroyStub(GpuCommandBufferStub* stub) {
-  DCHECK_EQ(stub, stub_.get());
-  if (video_decode_accelerator_)
-    video_decode_accelerator_.release()->Destroy();
-  if (stub_) {
-    stub_->RemoveDestructionObserver(this);
-    stub_.reset();
-  }
+void GpuVideoDecodeAccelerator::OnWillDestroyStub() {
+  delete this;
 }
 
 bool GpuVideoDecodeAccelerator::Send(IPC::Message* message) {
diff --git a/content/common/gpu/media/gpu_video_decode_accelerator.h b/content/common/gpu/media/gpu_video_decode_accelerator.h
index 52d0417..91f1b7c 100644
--- a/content/common/gpu/media/gpu_video_decode_accelerator.h
+++ b/content/common/gpu/media/gpu_video_decode_accelerator.h
@@ -45,7 +45,7 @@ class GpuVideoDecodeAccelerator
   virtual void NotifyResetDone() OVERRIDE;
 
   // GpuCommandBufferStub::DestructionObserver implementation.
-  virtual void OnWillDestroyStub(GpuCommandBufferStub* stub) OVERRIDE;
+  virtual void OnWillDestroyStub() OVERRIDE;
 
   // Function to delegate sending to actual sender.
   virtual bool Send(IPC::Message* message) OVERRIDE;
@@ -79,7 +79,7 @@ class GpuVideoDecodeAccelerator
   int32 host_route_id_;
 
   // Unowned pointer to the underlying GpuCommandBufferStub.
-  base::WeakPtr<GpuCommandBufferStub> stub_;
+  GpuCommandBufferStub* stub_;
 
   // The underlying VideoDecodeAccelerator.
   scoped_ptr<media::VideoDecodeAccelerator> video_decode_accelerator_;
diff --git a/content/common/gpu/texture_image_transport_surface.cc b/content/common/gpu/texture_image_transport_surface.cc
index 8012da8..9710b35 100644
--- a/content/common/gpu/texture_image_transport_surface.cc
+++ b/content/common/gpu/texture_image_transport_surface.cc
@@ -173,10 +173,8 @@ void TextureImageTransportSurface::OnResize(gfx::Size size) {
   CreateBackTexture();
 }
 
-void TextureImageTransportSurface::OnWillDestroyStub(
-    GpuCommandBufferStub* stub) {
-  DCHECK(stub == helper_->stub());
-  stub->RemoveDestructionObserver(this);
+void TextureImageTransportSurface::OnWillDestroyStub() {
+  helper_->stub()->RemoveDestructionObserver(this);
 
   GpuHostMsg_AcceleratedSurfaceRelease_Params params;
   helper_->SendAcceleratedSurfaceRelease(params);
diff --git a/content/common/gpu/texture_image_transport_surface.h b/content/common/gpu/texture_image_transport_surface.h
index 8a36228..9ab9abc 100644
--- a/content/common/gpu/texture_image_transport_surface.h
+++ b/content/common/gpu/texture_image_transport_surface.h
@@ -59,7 +59,7 @@ class TextureImageTransportSurface
       const cc::LatencyInfo& latency_info) OVERRIDE;
 
   // GpuCommandBufferStub::DestructionObserver implementation.
-  virtual void OnWillDestroyStub(GpuCommandBufferStub* stub) OVERRIDE;
+  virtual void OnWillDestroyStub() OVERRIDE;
 
  private: