Flash: Don't lose a reference to the Graphics3D from the VideoDecoder.

ppapi/shared_impl/ppb_video_decoder_shared.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 "ppapi/shared_impl/ppb_video_decoder_shared.h"
 
 #include "base/logging.h"
 #include "gpu/command_buffer/client/gles2_implementation.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/shared_impl/ppb_graphics_3d_shared.h"
@@ skipping 26 matching lines @@
     PP_Resource graphics_context,
     gpu::gles2::GLES2Implementation* gles2_impl) {
   DCHECK(graphics_context);
   DCHECK(!gles2_impl_ && !graphics_context_);
   gles2_impl_ = gles2_impl;
   PpapiGlobals::Get()->GetResourceTracker()->AddRefResource(graphics_context);
   graphics_context_ = graphics_context;
 }
 
 void PPB_VideoDecoder_Shared::Destroy() {
-  graphics_context_ = 0;
+  if (graphics_context_) {
+    PpapiGlobals::Get()->GetResourceTracker()->ReleaseResource(
+        graphics_context_);

[dmichael (off chromium), 2013/03/27 22:59:04]

What about either using ScopedPPResource, or actually getting the real
PPB_Graphics3D_Shared and holding a scoped_refptr to it?

[danakj, 2013/03/27 23:02:33]

Will ScopedPPResource work with this model where Destroy() is being called
manually? That would move the release to the object's destructor. But the proxy
seems to need to call this function directly.

[dmichael (off chromium), 2013/03/28 02:40:49]

Didn't see that ScopedPPResource has no public way to release a ref.

optional: having graphics_context_ be a scoped_refptr<PPB_Graphics3D_Shared>
instead might actually be pretty clean, and protects you from the plugin doing
stupid stuff with the refcount. But with this being a private API, it's not
important. It's kind of irrelevant except it guarantees the Graphics3D gets
released even if Destroy is not called.

[danakj, 2013/03/28 15:58:50]

I feel hesitant to make that change as it would be pretty invasive and this
needs to be backported to the branch. The graphics context resource id is
plumbed through at least 3 or 4 files at the moment..

I do like your point though, I will add a DCHECK to the destructor to help us
verify Destroy() continues to be called.

+    graphics_context_ = 0;
+  }
   gles2_impl_ = NULL;
-  PpapiGlobals::Get()->GetResourceTracker()->ReleaseResource(graphics_context_);
 }
 
 bool PPB_VideoDecoder_Shared::SetFlushCallback(
     scoped_refptr<TrackedCallback> callback) {
   if (TrackedCallback::IsPending(flush_callback_))
     return false;
   flush_callback_ = callback;
   return true;
 }
 
@@ skipping 37 matching lines @@
         graphics_context_, false);
     DCHECK(enter_g3d.succeeded());
     PPB_Graphics3D_Shared* graphics3d =
         static_cast<PPB_Graphics3D_Shared*>(enter_g3d.object());
     PPB_Graphics3D_Shared::ScopedNoLocking dont_lock(graphics3d);
     gles2_impl_->Flush();
   }
 }
 
 }  // namespace ppapi