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.
Same issue as https://bugs.webkit.org/show_bug.cgi?id=185005 it seems.
We need to plainly remove that ASSERT.
Created attachment 339483 [details] Patch
(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
Created attachment 339501 [details] Patch
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 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.
(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.
(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.
(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.
(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?
Created attachment 339714 [details] Patch
(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.
Created attachment 339809 [details] Patch
Comment on attachment 339809 [details] Patch Clearing flags on attachment: 339809 Committed r231551: <https://trac.webkit.org/changeset/231551>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40085390>