Bug 189619 – [EME] Queue a "message" Event algorithm should enqueue a task

• Component: WebCore Misc.
• Status: NEW

[Xabier Rodríguez Calvar]

This will a remaining problem of fixing bug 185590. Got a fix for it already.

[Xabier Rodríguez Calvar]

Created attachment 349759 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 350012 [details]
Patch

[Philippe Normand]

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.

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

Created attachment 350209 [details]
Patch

This patch is still WIP, but I accept comments already

[Xabier Rodríguez Calvar]

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.

[Jer Noble]

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()

[Jer Noble]

(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.

[Xabier Rodríguez Calvar]

Created attachment 350347 [details]
Patch

[Alex Christensen]

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.

[Ahmad Saleem]

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