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

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

When the Flash VideoDecoder is destroyed, it needs to drop the reference
it holds on the Graphics3D object.

Currently, the Destroy() method erases the id for the Graphics3D object
before trying to drop its reference, making the drop a no-op and leaking
the reference.

Also, if the PPB_VideoDecoder_Impl object fails to initialize and create
a platform_video_decoder_, it still holds onto its reference to the
Graphics3D since it does not call Destroy() even though it called
InitCommon() on the PPB_VideoDecoder_Shared base class.

Lastly, when the plugin proxy's VideoDecoder class is destroyed, it
should ensure that the base class' Destroy() has been called so that
its reference is not leaked.

BUG=chrome-os-partner:18331
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191176

[danakj, 2013-03-27 22:40:37]

Not sure who should review this, but dmichael, you're an OWNER in both places.
Please forward it along to someone better if I guessed wrong.

[piman, 2013-03-27 22:48:31]

https://codereview.chromium.org/13123003/diff/2001/ppapi/shared_impl/ppb_vide...
File ppapi/shared_impl/ppb_video_decoder_shared.cc (right):

https://codereview.chromium.org/13123003/diff/2001/ppapi/shared_impl/ppb_vide...
ppapi/shared_impl/ppb_video_decoder_shared.cc:51: graphics_context_ = 0;
nit: you can move this inside the if.

https://codereview.chromium.org/13123003/diff/2001/webkit/plugins/ppapi/ppb_v...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

https://codereview.chromium.org/13123003/diff/2001/webkit/plugins/ppapi/ppb_v...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:210:
PPB_VideoDecoder_Shared::Destroy();
Because this will release the context, I'd be somewhat more comfortable if we
still did it after the platform_video_decoder_.release()->Destroy()

e.g:
FlushCommandBuffer();
if (platform_video_decoder_.get())
  platform_video_decoder_.release()->Destroy();
PPB_VideoDecoder_Shared::Destroy();
ppp_videodecoder_ = NULL;

[danakj, 2013-03-27 22:54:52]

N

https://codereview.chromium.org/13123003/diff/2001/ppapi/shared_impl/ppb_vide...
File ppapi/shared_impl/ppb_video_decoder_shared.cc (right):

https://codereview.chromium.org/13123003/diff/2001/ppapi/shared_impl/ppb_vide...
ppapi/shared_impl/ppb_video_decoder_shared.cc:51: graphics_context_ = 0;
On 2013/03/27 22:48:31, piman wrote:
> nit: you can move this inside the if.

Done.

https://codereview.chromium.org/13123003/diff/2001/webkit/plugins/ppapi/ppb_v...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

https://codereview.chromium.org/13123003/diff/2001/webkit/plugins/ppapi/ppb_v...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:210:
PPB_VideoDecoder_Shared::Destroy();
On 2013/03/27 22:48:31, piman wrote:
> Because this will release the context, I'd be somewhat more comfortable if we
> still did it after the platform_video_decoder_.release()->Destroy()
> 
> e.g:
> FlushCommandBuffer();
> if (platform_video_decoder_.get())
>   platform_video_decoder_.release()->Destroy();
> PPB_VideoDecoder_Shared::Destroy();
> ppp_videodecoder_ = NULL;

Oh, sounds good! Done.

[danakj, 2013-03-27 22:56:04]

https://codereview.chromium.org/13123003/diff/7001/webkit/plugins/ppapi/ppb_v...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

https://codereview.chromium.org/13123003/diff/7001/webkit/plugins/ppapi/ppb_v...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:214: ppp_videodecoder_ = NULL;
I'll move this above too. "Clean up the subclass before the base class".

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

https://codereview.chromium.org/13123003/diff/7001/ppapi/shared_impl/ppb_vide...
File ppapi/shared_impl/ppb_video_decoder_shared.cc (right):

https://codereview.chromium.org/13123003/diff/7001/ppapi/shared_impl/ppb_vide...
ppapi/shared_impl/ppb_video_decoder_shared.cc:49: graphics_context_);
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]

https://codereview.chromium.org/13123003/diff/7001/ppapi/shared_impl/ppb_vide...
File ppapi/shared_impl/ppb_video_decoder_shared.cc (right):

https://codereview.chromium.org/13123003/diff/7001/ppapi/shared_impl/ppb_vide...
ppapi/shared_impl/ppb_video_decoder_shared.cc:49: graphics_context_);
On 2013/03/27 22:59:04, dmichael wrote:
> What about either using ScopedPPResource, or actually getting the real
> PPB_Graphics3D_Shared and holding a scoped_refptr to it?

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.

[danakj, 2013-03-27 23:20:58]

PTAL

New patch adds PPB_VideoDecoder_Shared::Destroy() to the proxy ~VideoDecoder()
destructor. This fixes the remaining missing dereferences from the linked bug.

Now it seems to always be cleaning up from a destructor. But there is still a
VideoDecoder::Destroy() that also does a destroy. So I'm super not clear if a
ScopedPPResource would be good here. Maybe it would, and the base classes could
stop calling Destroy(), except the VideoDecoder::Destroy() case which could
clear the scoped variable.

[piman, 2013-03-27 23:37:43]

https://codereview.chromium.org/13123003/diff/13001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

https://codereview.chromium.org/13123003/diff/13001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:213: FlushCommandBuffer();
I think this still needs to happen before the
platform_video_decoder_.release()->Destroy() (that's what guarantees ordering
between the command buffer and the video decoder IPCs)

[danakj, 2013-03-27 23:42:06]

https://codereview.chromium.org/13123003/diff/13001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

https://codereview.chromium.org/13123003/diff/13001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:213: FlushCommandBuffer();
On 2013/03/27 23:37:43, piman wrote:
> I think this still needs to happen before the
> platform_video_decoder_.release()->Destroy() (that's what guarantees ordering
> between the command buffer and the video decoder IPCs)

Ok i was kinda wondering about that one. Thanks. Done.

[piman, 2013-03-27 23:42:51]

lgtm

[danakj, 2013-03-28 00:09:57]

dmichael@ PTAL

[dmichael (off chromium), 2013-03-28 02:40:48]

lgtm with optional suggestion. piman knows this area better than I, so I'm
counting on his review.

https://codereview.chromium.org/13123003/diff/7001/ppapi/shared_impl/ppb_vide...
File ppapi/shared_impl/ppb_video_decoder_shared.cc (right):

https://codereview.chromium.org/13123003/diff/7001/ppapi/shared_impl/ppb_vide...
ppapi/shared_impl/ppb_video_decoder_shared.cc:49: graphics_context_);
On 2013/03/27 23:02:33, danakj wrote:
> On 2013/03/27 22:59:04, dmichael wrote:
> > What about either using ScopedPPResource, or actually getting the real
> > PPB_Graphics3D_Shared and holding a scoped_refptr to it?
> 
> 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.
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]

https://codereview.chromium.org/13123003/diff/7001/ppapi/shared_impl/ppb_vide...
File ppapi/shared_impl/ppb_video_decoder_shared.cc (right):

https://codereview.chromium.org/13123003/diff/7001/ppapi/shared_impl/ppb_vide...
ppapi/shared_impl/ppb_video_decoder_shared.cc:49: graphics_context_);
On 2013/03/28 02:40:49, dmichael wrote:
> On 2013/03/27 23:02:33, danakj wrote:
> > On 2013/03/27 22:59:04, dmichael wrote:
> > > What about either using ScopedPPResource, or actually getting the real
> > > PPB_Graphics3D_Shared and holding a scoped_refptr to it?
> > 
> > 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.
> 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.

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.

[commit-bot: I haz the power, 2013-03-28 16:00:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/13123003/27001

[commit-bot: I haz the power, 2013-03-28 18:12:06]

Change committed as 191176