1222888 - [EME] Assertion failure: !aSessionId.IsEmpty() at GMPDecryptorParent.cpp:107

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase; causes assertion.

If script sends a MediaKeySession.update() before the MediaKeySession.generateRequest() has had a chance to run, we'll hit an assertion:

Assertion failure: !aSessionId.IsEmpty() && !aResponse.IsEmpty(), at c:/Users/cpearce/src/m-c/dom/media/gmp/GMPDecryptorParent.cpp:107
#01: mozilla::CDMProxy::gmp_UpdateSession (c:\users\cpearce\src\m-c\dom\media\eme\cdmproxy.cpp:377)
#02: nsRunnableMethodArguments<nsAutoPtr<mozilla::CDMProxy::UpdateSessionData> >::apply<mozilla::CDMProxy,void (__thiscall mozilla::CDMProxy::*)(nsAutoPtr<mozilla::CDMProxy::UpdateSessionData>)> (c:\u
sers\cpearce\src\m-c\objdir\dist\include\nsthreadutils.h:677)
#03: nsRunnableMethodImpl<void (__thiscall mozilla::CDMProxy::*)(nsAutoPtr<mozilla::CDMProxy::UpdateSessionData>),1,nsAutoPtr<mozilla::CDMProxy::UpdateSessionData> >::Run (c:\users\cpearce\src\m-c\obj
dir\dist\include\nsthreadutils.h:872)
#04: nsThread::ProcessNextEvent (c:\users\cpearce\src\m-c\xpcom\threads\nsthread.cpp:964)
#05: NS_ProcessNextEvent (c:\users\cpearce\src\m-c\xpcom\glue\nsthreadutils.cpp:297)
#06: mozilla::ipc::MessagePumpForNonMainThreads::Run (c:\users\cpearce\src\m-c\ipc\glue\messagepump.cpp:355)
#07: MessageLoop::RunInternal (c:\users\cpearce\src\m-c\ipc\chromium\src\base\message_loop.cc:235)
#08: MessageLoop::RunHandler (c:\users\cpearce\src\m-c\ipc\chromium\src\base\message_loop.cc:228)
#09: MessageLoop::Run (c:\users\cpearce\src\m-c\ipc\chromium\src\base\message_loop.cc:202)
#10: nsThread::ThreadFunc (c:\users\cpearce\src\m-c\xpcom\threads\nsthread.cpp:378)
#11: _PR_NativeRunThread (c:\users\cpearce\src\m-c\nsprpub\pr\src\threads\combined\pruthr.c:397)
#12: pr_root (c:\users\cpearce\src\m-c\nsprpub\pr\src\md\windows\w95thred.c:90)
#13: _get_flsindex[MSVCR120 +0x2c01d]
#14: _get_flsindex[MSVCR120 +0x2c001]
#15: BaseThreadInitThunk[kernel32 +0x1337a]
#16: RtlInitializeExceptionChain[ntdll +0x39882]
#17: RtlInitializeExceptionChain[ntdll +0x39855]

Test case attached.

Based on my testing, Netflix are not doing this.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

The solution I think is to implement the "callable" value on the MediaKeySession in the spec; that seems to in place to prevent the race between generateRequest/load setting the sessionId and subsequent update/close/remove actions on the session.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1: Mochitest for MediaKeySession "callable value"

Add a mochitest to verify that we can't call MediaKeySession.update() before the sessionId has been set.

Without patch 2, this hits a fatal assert.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: Implement MediaKeySession "callable value"

Implement the step:

"If this object's callable value is false, return a promise rejected with a new DOMException whose name is InvalidStateError."

in the MediaKeySession close, remove, and update method definitions.

Now we don't hit the assertion.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8684791 [details] [diff] [review]
Patch 1: Mochitest for MediaKeySession "callable value"

Review of attachment 8684791 [details] [diff] [review]:
-----------------------------------------------------------------

Failing test, my favourite. r+ with nit you can easily choose to ignore.

::: dom/media/test/test_eme_session_callable_value.html
@@ +13,5 @@
> +
> +function Test() {
> +  navigator.requestMediaKeySystemAccess('org.w3.clearkey', [{ initDataTypes: ['cenc'] }])
> +  .then(access => access.createMediaKeys())
> +  .then((mediaKeys) => {

No need for parens for single-argument arrow functions. Or alternatively add them to 'access' above to be consistent.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8684794 [details] [diff] [review]
Patch 2: Implement MediaKeySession "callable value"

Review of attachment 8684794 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nano-nit and negligible concern.

::: dom/media/eme/MediaKeySession.h
@@ +104,5 @@
> +
> +  bool IsCallable() const {
> +    // The EME spec sets the "callable value" to true whenever the CDM sets
> +    // the sessionId.
> +    return !mSessionId.IsEmpty();

To be explicit, shouldn't you also explain that when the session is initialized, sessionId is empty and callable is false? (so that having callable==!sessionId.empty makes sense)

My only concern would be that if the specs change one day to decouple these two variable (e.g. the session can become non-callable even when it still has a sessionId), we might miss it! But it seems low risk on both the specs changing that much, and us missing this detail.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9443b41710
https://hg.mozilla.org/integration/mozilla-inbound/rev/ade0ff7cb5dd

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf9443b41710
https://hg.mozilla.org/mozilla-central/rev/ade0ff7cb5dd