Keep reference to CDM to avoid it's premature destruction

Keep reference to CDM to avoid it's premature destruction

Occassionally the CDM object is destroyed before the pipeline that
uses it is stopped, so have WebMediaPlayerImpl keep a reference to
it to prevent this from happening.

This worked in the past as the pipeline was destroyed from
HTMLMediaElement::stop() while the CDM was destroyed later from
MediaKeys::contextDestroyed(). A recent change merged stop()
and contextDestroyed(), so the CDM can be freed before
WebMediaPlayerImpl is done with it.

BUG=652978, 653001
TEST=EME layout tests pass

Committed: https://crrev.com/e616ee95642ecc0b168a8e807abc86ff3633cda8
Cr-Commit-Position: refs/heads/master@{#424046}

[jrummell, 2016-10-07 01:50:30]

jrummell@chromium.org changed reviewers:
+ xhwang@chromium.org

[jrummell, 2016-10-07 01:50:30]

PTAL. I do get crashes on ToT without this.

[xhwang, 2016-10-07 05:02:14]

Thanks! Please see the comments.

Also, could you please explain in the CL description why it was safe before but
starts to crash now (e.g. quick teardown etc)

https://chromiumcodereview.appspot.com/2402563002/diff/1/media/blink/webconte...
File media/blink/webcontentdecryptionmodule_impl.h (right):

https://chromiumcodereview.appspot.com/2402563002/diff/1/media/blink/webconte...
media/blink/webcontentdecryptionmodule_impl.h:61: CdmSessionAdapter*
GetCdmAdapterReference();
This returns a raw pointer, not a reference... Ah, CdmSessionAdapter is
refcounted. But usually for refcounted object, we put it in a scoped_refptr
immediately after it's created, and then we pretty much should never use the raw
pointer anywhere.

Actually, instead of letting WMPI hold a ref of the CdmSessionAdapter, can we
just let WIMPI hold a ref of the |cdm_|? That'll be consistent with AVDA and
MojoRendererService. Please see the comment here:

https://cs.chromium.org/chromium/src/media/base/media_keys.h?rcl=0&l=45

https://chromiumcodereview.appspot.com/2402563002/diff/1/media/blink/webmedia...
File media/blink/webmediaplayer_impl.h (right):

https://chromiumcodereview.appspot.com/2402563002/diff/1/media/blink/webmedia...
media/blink/webmediaplayer_impl.h:497: scoped_refptr<CdmSessionAdapter>
cdm_adapter_;
We have two cdm_adapter already, this variable name seems a little bit
misleading...

[jrummell, 2016-10-07 20:15:30]

Updated to save MediaKeys.

https://codereview.chromium.org/2402563002/diff/1/media/blink/webcontentdecry...
File media/blink/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/2402563002/diff/1/media/blink/webcontentdecry...
media/blink/webcontentdecryptionmodule_impl.h:61: CdmSessionAdapter*
GetCdmAdapterReference();
On 2016/10/07 05:02:14, xhwang wrote:
> This returns a raw pointer, not a reference... Ah, CdmSessionAdapter is
> refcounted. But usually for refcounted object, we put it in a scoped_refptr
> immediately after it's created, and then we pretty much should never use the
raw
> pointer anywhere.
> 
> Actually, instead of letting WMPI hold a ref of the CdmSessionAdapter, can we
> just let WIMPI hold a ref of the |cdm_|? That'll be consistent with AVDA and
> MojoRendererService. Please see the comment here:
> 
> https://cs.chromium.org/chromium/src/media/base/media_keys.h?rcl=0&l=45

Done.

https://codereview.chromium.org/2402563002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2402563002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.h:497: scoped_refptr<CdmSessionAdapter>
cdm_adapter_;
On 2016/10/07 05:02:14, xhwang wrote:
> We have two cdm_adapter already, this variable name seems a little bit
> misleading...

Changed.

[jrummell, 2016-10-07 20:21:50]

Description was changed from

==========
Keep reference to CDM to avoid it's premature destruction

Occassionally the CDM object is destroyed before the pipeline that
uses it is stopped, so have WebMediaPlayerImpl keep a reference to
it to prevent this from happening.

BUG=652978,653001
TEST=EME layout tests pass
==========

to

==========
Keep reference to CDM to avoid it's premature destruction

Occassionally the CDM object is destroyed before the pipeline that
uses it is stopped, so have WebMediaPlayerImpl keep a reference to
it to prevent this from happening.

This worked in the past as the pipeline was destroyed from
HTMLMediaElement::stop() while the CDM was destroyed later from
MediaKeys::contextDestroyed(). A recent change merged stop()
and contextDestroyed(), so the CDM can be freed before
WebMediaPlayerImpl is done with it.

BUG=652978,653001
TEST=EME layout tests pass
==========

[xhwang, 2016-10-07 20:44:31]

Thanks! The general approach is good and thanks for carching the bug. More
comments :)

Also, please follow up on the bug with blink owners to find the root cause that
changed our assumption. I wonder whether that could have wider impact than our
use case here.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/cdm_session...
File media/blink/cdm_session_adapter.h (right):

https://codereview.chromium.org/2402563002/diff/20001/media/blink/cdm_session...
media/blink/cdm_session_adapter.h:94: scoped_refptr<MediaKeys>
GetCdmReference();
CDM (MediaKeys) is already ref-counted so it seems "Reference" is not necessary.
In other words, the caller won't do anything different whether this is called
GetCdmReference() or GetCdm().

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:248: SetCdm(params.initial_cdm());
Thanks for catching this bug!

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:945: void
WebMediaPlayerImpl::SetCdm(blink::WebContentDecryptionModule* cdm) {
nit: DCHECK this is called on the main thread.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:949: if (!cdm_reference) {
Shall we NOTREACHED() here as well?

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:962: cdm_ = std::move(cdm_reference);
I wonder whether the following could happen:

If we already have a CDM set (cdm_ != null), and that MediaKeys is destroyed by
JS so that |cdm_| holds the last ref of the old CDM. Then  SetCdm() is called
again, we could end up releasing the ref-count of the old CDM here, causing the
old CDM to be destroyed. But the media pipeline could still be using the old
CDM.

Currently we prevent resetting the CDM in RendererImpl, such that different
Renderer implementations could choose whether resetting CDM is supported or not.

Maybe we could have a |pending_cdm_| variable, to hold the CDM being set, while
|cdm_| is for a CDM that's already set. WDYT?

We could also bind the pending CDM in a callback, but callback lifetime is
tricky and I am scared of it...

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:964: // If CDM initialization succeeded, tell
the pipeline about it.
I don't really know what this comment means now :)

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:966:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnCdmAttached));
This BIND_TO_RENDER_LOOP is not needed now that the main/media logic should be
fully buried within PipelineImpl:

https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=14758535...

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:969: void
WebMediaPlayerImpl::OnCdmAttached(bool success) {
DCHECK this is called on the main thread.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:971: // promise (|set_cdm_result_|) to
fulfill.
DCHECK(cdm_) here to make assumptions more clear

[jrummell, 2016-10-07 23:46:46]

Updated.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/cdm_session...
File media/blink/cdm_session_adapter.h (right):

https://codereview.chromium.org/2402563002/diff/20001/media/blink/cdm_session...
media/blink/cdm_session_adapter.h:94: scoped_refptr<MediaKeys>
GetCdmReference();
On 2016/10/07 20:44:31, xhwang wrote:
> CDM (MediaKeys) is already ref-counted so it seems "Reference" is not
necessary.
> In other words, the caller won't do anything different whether this is called
> GetCdmReference() or GetCdm().

Done.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:248: SetCdm(params.initial_cdm());
On 2016/10/07 20:44:31, xhwang wrote:
> Thanks for catching this bug!

Acknowledged.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:945: void
WebMediaPlayerImpl::SetCdm(blink::WebContentDecryptionModule* cdm) {
On 2016/10/07 20:44:31, xhwang wrote:
> nit: DCHECK this is called on the main thread.

Done.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:949: if (!cdm_reference) {
On 2016/10/07 20:44:31, xhwang wrote:
> Shall we NOTREACHED() here as well?

Sure. It shouldn't happen.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:962: cdm_ = std::move(cdm_reference);
On 2016/10/07 20:44:31, xhwang wrote:
> I wonder whether the following could happen:
> 
> If we already have a CDM set (cdm_ != null), and that MediaKeys is destroyed
by
> JS so that |cdm_| holds the last ref of the old CDM. Then  SetCdm() is called
> again, we could end up releasing the ref-count of the old CDM here, causing
the
> old CDM to be destroyed. But the media pipeline could still be using the old
> CDM.
> 
> Currently we prevent resetting the CDM in RendererImpl, such that different
> Renderer implementations could choose whether resetting CDM is supported or
not.
> 
> Maybe we could have a |pending_cdm_| variable, to hold the CDM being set,
while
> |cdm_| is for a CDM that's already set. WDYT?
> 
> We could also bind the pending CDM in a callback, but callback lifetime is
> tricky and I am scared of it...

I thought we didn't allow the CDM to be changed, but it appears that the check
in setContentDecryptionModule() only prevents clearing the CDM. Done.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:964: // If CDM initialization succeeded, tell
the pipeline about it.
On 2016/10/07 20:44:31, xhwang wrote:
> I don't really know what this comment means now :)

Removed.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:966:
BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnCdmAttached));
On 2016/10/07 20:44:31, xhwang wrote:
> This BIND_TO_RENDER_LOOP is not needed now that the main/media logic should be
> fully buried within PipelineImpl:
> 
>
https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=14758535...

Done.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:969: void
WebMediaPlayerImpl::OnCdmAttached(bool success) {
On 2016/10/07 20:44:31, xhwang wrote:
> DCHECK this is called on the main thread.

Done.

https://codereview.chromium.org/2402563002/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:971: // promise (|set_cdm_result_|) to
fulfill.
On 2016/10/07 20:44:31, xhwang wrote:
> DCHECK(cdm_) here to make assumptions more clear

Done.

[xhwang, 2016-10-08 00:16:00]

Thanks!

lgtm with nits

https://codereview.chromium.org/2402563002/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2402563002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:963: // after the pipeline is done with it.
nit s/it/the |cdm_context|/

https://codereview.chromium.org/2402563002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:974: // promise (|set_cdm_result_|) to
fulfill.
nit: reflow this comment

https://codereview.chromium.org/2402563002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:976: cdm_ = std::move(pending_cdm_);
Add a comment that this could release an older attached CDM.

[jrummell, 2016-10-08 00:32:18]

Thanks for the reviews.

https://codereview.chromium.org/2402563002/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2402563002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:963: // after the pipeline is done with it.
On 2016/10/08 00:16:00, xhwang wrote:
> nit s/it/the |cdm_context|/

Done.

https://codereview.chromium.org/2402563002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:974: // promise (|set_cdm_result_|) to
fulfill.
On 2016/10/08 00:16:00, xhwang wrote:
> nit: reflow this comment

I did it to keep the variable name with "promise", but done anyway.

https://codereview.chromium.org/2402563002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:976: cdm_ = std::move(pending_cdm_);
On 2016/10/08 00:16:00, xhwang wrote:
> Add a comment that this could release an older attached CDM.

Done.

[jrummell, 2016-10-08 00:32:23]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-10-08 00:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2402563002/#ps60001
(title: "nits")

[commit-bot: I haz the power, 2016-10-08 00:33:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-08 02:16:02]

Description was changed from

==========
Keep reference to CDM to avoid it's premature destruction

Occassionally the CDM object is destroyed before the pipeline that
uses it is stopped, so have WebMediaPlayerImpl keep a reference to
it to prevent this from happening.

This worked in the past as the pipeline was destroyed from
HTMLMediaElement::stop() while the CDM was destroyed later from
MediaKeys::contextDestroyed(). A recent change merged stop()
and contextDestroyed(), so the CDM can be freed before
WebMediaPlayerImpl is done with it.

BUG=652978,653001
TEST=EME layout tests pass
==========

to

==========
Keep reference to CDM to avoid it's premature destruction

Occassionally the CDM object is destroyed before the pipeline that
uses it is stopped, so have WebMediaPlayerImpl keep a reference to
it to prevent this from happening.

This worked in the past as the pipeline was destroyed from
HTMLMediaElement::stop() while the CDM was destroyed later from
MediaKeys::contextDestroyed(). A recent change merged stop()
and contextDestroyed(), so the CDM can be freed before
WebMediaPlayerImpl is done with it.

BUG=652978,653001
TEST=EME layout tests pass
==========

[commit-bot: I haz the power, 2016-10-08 02:16:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-08 02:18:17]

Description was changed from

==========
Keep reference to CDM to avoid it's premature destruction

Occassionally the CDM object is destroyed before the pipeline that
uses it is stopped, so have WebMediaPlayerImpl keep a reference to
it to prevent this from happening.

This worked in the past as the pipeline was destroyed from
HTMLMediaElement::stop() while the CDM was destroyed later from
MediaKeys::contextDestroyed(). A recent change merged stop()
and contextDestroyed(), so the CDM can be freed before
WebMediaPlayerImpl is done with it.

BUG=652978,653001
TEST=EME layout tests pass
==========

to

==========
Keep reference to CDM to avoid it's premature destruction

Occassionally the CDM object is destroyed before the pipeline that
uses it is stopped, so have WebMediaPlayerImpl keep a reference to
it to prevent this from happening.

This worked in the past as the pipeline was destroyed from
HTMLMediaElement::stop() while the CDM was destroyed later from
MediaKeys::contextDestroyed(). A recent change merged stop()
and contextDestroyed(), so the CDM can be freed before
WebMediaPlayerImpl is done with it.

BUG=652978,653001
TEST=EME layout tests pass

Committed: https://crrev.com/e616ee95642ecc0b168a8e807abc86ff3633cda8
Cr-Commit-Position: refs/heads/master@{#424046}
==========

[commit-bot: I haz the power, 2016-10-08 02:18:18]

Patchset 4 (id:??) landed as
https://crrev.com/e616ee95642ecc0b168a8e807abc86ff3633cda8
Cr-Commit-Position: refs/heads/master@{#424046}