This will a remaining problem of fixing bug 185590. Got a fix for it already.
Created attachment 349759 [details] Patch
Created attachment 350012 [details] Patch
Comment on attachment 350012 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350012&action=review > LayoutTests/media/encrypted-media/mock-MediaKeySession-remove-expected.txt:40 > Promise resolved OK > RUN(promise = mediaKeySession.remove()) > +EXPECTED (event.messageType == 'license-request') OK This looks odd.
(In reply to Philippe Normand from comment #3) > Comment on attachment 350012 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350012&action=review > > > LayoutTests/media/encrypted-media/mock-MediaKeySession-remove-expected.txt:40 > > Promise resolved OK > > RUN(promise = mediaKeySession.remove()) > > +EXPECTED (event.messageType == 'license-request') OK > > This looks odd. Yes, it did to me too when I debugged it but the spec says that promise callbacks are run before any other event handler as they are scheduled as microtasks. Making this change (to make it conformant with the spec) has this side effect in this custom test. generateRequest returns a promise and an fires an license-request event so if you subscribe to the event handler in the promise callback, you're going to get it. If you think (unlike me) that it is a bad idea to have that check there, I can remove that check but the test has to be modified to expect that event before the other.
Phil, after our discussion, I think I managed to fix the issues and regressions we talked about. Please, test this if you can. I won't be able to land it until tomorrow unless you want to watch it.
(In reply to Xabier Rodríguez Calvar from comment #5) > Phil, after our discussion, I think I managed to fix the issues and > regressions we talked about. Please, test this if you can. I won't be able > to land it until tomorrow unless you want to watch it. Oops, wrong bug.
Created attachment 350209 [details] Patch This patch is still WIP, but I accept comments already
The problem here for the waiting-for-key test is that the event is arriving before the promise is resolved. Zan thinks we should have a queue for events and tasks and another for "run in parallel" steps but it looks like it blocks everything if I do that. This is the only thing, before any other investigation, that makes things work and besides that, it files a couple more tests.
Comment on attachment 350209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350209&action=review > Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:601 > - m_eventQueue.enqueueEvent(WTFMove(messageEvent)); > + m_eventQueue.enqueueTask([this, messageEvent = WTFMove(messageEvent), pendingActivity = makePendingActivity(*this)] { > + dispatchEvent(messageEvent); > + }); This really shouldn't be necessary. GenericEventQueue is implemented with a GenericTaskQueue; this just duplicates that work, without implementing things like ensuring the event's target is unset and without implementing event dispatch suspension. I suspect that some of the other parts of this patch are what's actually fixing this test. > Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:712 > + return m_eventQueue.hasPendingTasks() || ActiveDOMObject::hasPendingActivity(); This could just be m_eventQueue.hasPendingEvents()
(In reply to Xabier Rodríguez Calvar from comment #8) > The problem here for the waiting-for-key test is that the event is arriving > before the promise is resolved. Zan thinks we should have a queue for events > and tasks and another for "run in parallel" steps but it looks like it > blocks everything if I do that. This is the only thing, before any other > investigation, that makes things work and besides that, it files a couple > more tests. Yes, we have a general problem with event and promise ordering. We would need to fully implement the HTML Event Loop spec <https://html.spec.whatwg.org/multipage/webappapis.html#event-loops> to fix this generally.
Created attachment 350347 [details] Patch
Comment on attachment 350347 [details] Patch This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.
We are still failing following WPT Test, which this patch was fixing: https://wpt.fyi/results/encrypted-media/clearkey-mp4-waiting-for-a-key.https.html?label=master&label=experimental&aligned&q=encrypted-media%2Fclearkey-mp4 https://wpt.fyi/results/encrypted-media/clearkey-mp4-playback-temporary-events.https.html?label=master&label=experimental&aligned&q=encrypted-media%2Fclearkey-mp4 https://wpt.fyi/results/encrypted-media/clearkey-mp4-playback-persistent-license-events.https.html?label=experimental&label=master&aligned&q=encrypted-media%2Fclearkey-mp4