Bug 185244

Summary: [EME][GStreamer] Crash when the mediaKeys are created before loading the media in debug conf
Product: WebKit Reporter: Yacine Bandou <bandou.yacine>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, commit-queue, olivier.blin, pnormand, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 185277    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Yacine Bandou 2018-05-03 02:29:10 PDT
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.
Comment 1 Philippe Normand 2018-05-03 02:39:04 PDT
Same issue as https://bugs.webkit.org/show_bug.cgi?id=185005 it seems.
Comment 2 Xabier Rodríguez Calvar 2018-05-03 03:06:53 PDT
We need to plainly remove that ASSERT.
Comment 3 Yacine Bandou 2018-05-03 16:43:18 PDT
Created attachment 339483 [details]
Patch
Comment 4 Yacine Bandou 2018-05-03 16:47:13 PDT
(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
Comment 5 Yacine Bandou 2018-05-03 18:42:50 PDT
Created attachment 339501 [details]
Patch
Comment 6 Zan Dobersek 2018-05-05 00:50:07 PDT
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?
Comment 7 Xabier Rodríguez Calvar 2018-05-07 01:24:51 PDT
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.
Comment 8 Yacine Bandou 2018-05-07 01:39:01 PDT
(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.
Comment 9 Yacine Bandou 2018-05-07 01:45:39 PDT
(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.
Comment 10 Xabier Rodríguez Calvar 2018-05-07 01:58:32 PDT
(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.
Comment 11 Yacine Bandou 2018-05-07 02:42:07 PDT
(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?
Comment 12 Yacine Bandou 2018-05-07 06:02:21 PDT
Created attachment 339714 [details]
Patch
Comment 13 Xabier Rodríguez Calvar 2018-05-08 00:09:16 PDT
(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.
Comment 14 Yacine Bandou 2018-05-08 05:31:34 PDT
Created attachment 339809 [details]
Patch
Comment 15 WebKit Commit Bot 2018-05-09 00:38:50 PDT
Comment on attachment 339809 [details]
Patch

Clearing flags on attachment: 339809

Committed r231551: <https://trac.webkit.org/changeset/231551>
Comment 16 WebKit Commit Bot 2018-05-09 00:38:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2018-05-09 00:39:18 PDT
<rdar://problem/40085390>