Bug 185244 – [EME][GStreamer] Crash when the mediaKeys are created before loading the media in debug conf

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Yacine Bandou]

The function "MediaPlayerPrivateGStreamerBase::cdmInstanceAttached" is expected to be called once, so there is an ASSERT(!m_cdmInstance).
But when the MediaKeys created before loading the media, the cdminstance is created and attached to the MediaPlayerPrivate via
"MediaPlayerPrivateGStreamerBase::cdmInstanceAttached" before laoding the media.

Then when the media is loading, the function "MediaPlayerPrivateGStreamerBase::cdmInstanceAttached" will be called several times via
the function "mediaEngineWasUpdated" wich is called for each change in the state of the MediaElement, thus the WebProcess
crashes in the ASSERT(!m_cdmInstance).

To Avoid the crash we replace the Assert with a simple check.

[Philippe Normand]

Same issue as https://bugs.webkit.org/show_bug.cgi?id=185005 it seems.

[Xabier Rodríguez Calvar]

We need to plainly remove that ASSERT.

[Yacine Bandou]

Created attachment 339483 [details]
Patch

[Yacine Bandou]

(In reply to Philippe Normand from comment #1)
> Same issue as https://bugs.webkit.org/show_bug.cgi?id=185005 it seems.

Not exactly, but this issue is a part of this one 185005

I'll update the WPE TestExpectations when all patches are landed

[Yacine Bandou]

Created attachment 339501 [details]
Patch

[Zan Dobersek]

Comment on attachment 339501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339501&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1222
> +    if (m_cdmInstance.get() == &instance) {

.get() required here?

[Xabier Rodríguez Calvar]

Comment on attachment 339501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339501&action=review

> Source/WebCore/ChangeLog:11
> +        to the MediaPlayerPrivate via "MediaPlayerPrivateGStreamerBase::cdmInstanceAttached" before laoding

loading

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1222
>> +    if (m_cdmInstance.get() == &instance) {
> 
> .get() required here?

It looks like it is not. Besides, I don't think this chunk of code is necessary as detach is not called multiple times as attach is. Unless you think I am wrong, please remove this chunk and leave the code as it was.

[Yacine Bandou]

(In reply to Zan Dobersek from comment #6)
> Comment on attachment 339501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339501&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1222
> > +    if (m_cdmInstance.get() == &instance) {
> 
> .get() required here?

No, is just an oversight.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #7)
> Comment on attachment 339501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339501&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        to the MediaPlayerPrivate via "MediaPlayerPrivateGStreamerBase::cdmInstanceAttached" before laoding
> 
> loading
> 
> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1222
> >> +    if (m_cdmInstance.get() == &instance) {
> > 
> > .get() required here?
> 
> It looks like it is not. Besides, I don't think this chunk of code is
> necessary as detach is not called multiple times as attach is. Unless you
> think I am wrong, please remove this chunk and leave the code as it was.

I added it because I had a crash in some test.
Otherwise, If we remove the assert from attach, I don't see the inconvenience of removing it from dettach.

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #9)
> I added it because I had a crash in some test.
> Otherwise, If we remove the assert from attach, I don't see the
> inconvenience of removing it from dettach.

It had the same idea but different consequences. In the case of attach, we do get it called multiple times and nothing happens if it is the same CDM instance. Actually, we might want to add an assert to check if we are assigned a different instance.

In the case of detach, we should still assert because we are getting a request to detach an instance we don't know.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #10)
> (In reply to Yacine Bandou from comment #9)
> > I added it because I had a crash in some test.
> > Otherwise, If we remove the assert from attach, I don't see the
> > inconvenience of removing it from dettach.
> 
> It had the same idea but different consequences. In the case of attach, we
> do get it called multiple times and nothing happens if it is the same CDM
> instance. Actually, we might want to add an assert to check if we are
> assigned a different instance.
> 
> In the case of detach, we should still assert because we are getting a
> request to detach an instance we don't know.


With the assert in the cdmInstanceDetached function the following test "imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-setmediakeys-multiple-times-with-different-mediakeys.https.html" crashes.

Without the assert the test passes.

We have two solutions, we remove the Assert or we leave it but we will note that the test will be crashed in TestExpectations.

What do you suggest?

[Yacine Bandou]

Created attachment 339714 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #11)
> With the assert in the cdmInstanceDetached function the following test
> "imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-setmediakeys-
> multiple-times-with-different-mediakeys.https.html" crashes.
> 
> Without the assert the test passes.
> 
> We have two solutions, we remove the Assert or we leave it but we will note
> that the test will be crashed in TestExpectations.
> 
> What do you suggest?

I suggest we remove the ASSERT. If there is something that triggers it it is better to fix it. So you were right.

[Yacine Bandou]

Created attachment 339809 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339809 [details]
Patch

Clearing flags on attachment: 339809

Committed r231551: <https://trac.webkit.org/changeset/231551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40085390>