1111391 - [EME] Enable keyMessages to be sent before create/load session promise is resolved

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Currently we don't allow implementations of GMPDecryptor::LoadSession() and CreateSession() to send a message to JS before they resolve their promise.

In the createSession case this is because the session Id is set in the ResolveNewSessionPromise() callback, and our SessionMessage() callback requires a session Id. Also, the MediaKeySession object is held in MediaKeys::mPendingSessions, so it can't be accessed by the usual CDMProxy::OnSessionMessage path.

In the loadSession case Gecko already knows the session Id, but again the MediaKeySession is held in mPendingSessions.

Adobe need in some cases to send some messages before resolving the load/create session promises, so we should support this.

Comment 1

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

This is probably a good opportunity to rename GMPDecryptor::CreateSession to GenerateKeyRequest, to match the JS method name.

Comment 2

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

Attachment: Patch v1

Inside the GMPDecryptor API, separate the the response to a GMPDecryptor::CreateSession() call into two discrete steps; setting the sessionId on the MediaKeySession on which generateRequest() was called, and resolving the promise that generateRequest() returns.

This means that the GMPDecryptor can call GMPDecryptorCallback::SessionMessage() to send a message out and to the license and individualization servers if needed in order before resolving the promise returned by generateRequest().

This means that the GMP can reject the promise if the messages it sends over the wire timeout, or the license server rejects the request or somesuch.

I also make it possible to send messages before resolving the promise passed to GMPDecryptor::LoadSession(). This is for the same reason; the GMP may need to send a message to a license server before it knows whether it can load a session.

I had to re-arrange how the MediaKeys stores pending sessions. Now we track the sessions awaiting a sessionId in the MediaKeys object, and as soon as we determine the sessionId we move the MediaKeySession into the MediaKeys::mKeySessions set, so that GMPDecryptorCallback::SessionMessage() calls work.

The sending messages in the LoadSession() case is tested by the change in ClearKeyDecryptionManager::PersistentSessionDataLoaded(). The change to make CreateSession() able to send messages before resolving its promise is harder to test, as we can only really test that from the JS API, we can't write a meaningful gtest for that.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8544316 [details] [diff] [review]
Patch v1

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

Looks great to me!

::: dom/media/eme/MediaKeys.cpp
@@ +217,5 @@
> +  if (!mPendingSessions.Contains(aSession->Token())) {
> +    NS_WARNING("MediaKeySession made ready when it wasn't waiting to be ready!");
> +    return;
> +  }
> +  if (!aSession) {

This check should come first before you can deref it.

@@ +402,5 @@
>  }
>  
> +already_AddRefed<MediaKeySession>
> +MediaKeys::GetPendingSession(uint32_t aToken)
> +{

This can be simplified as follows:

nsRefPtr<MediaKeySession> session;
mPendingSessions.Get(aToken, getter_AddRefs(session));
mPendingSessions.Remove(aToken);
return session.forget();

if aToken is not in mPendingSessions, session should remain unchanged which is nullptr.
if aToken is in mPendingSessions, session should be updated and shouldn't be nullptr.

Comment 4

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

Comment on attachment 8544316 [details] [diff] [review]
Patch v1

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

::: dom/media/eme/MediaKeys.cpp
@@ +213,5 @@
> +  if (mKeySessions.Contains(aSession->GetSessionId())) {
> +    NS_WARNING("MediaKeySession's made ready multiple times!");
> +    return;
> +  }
> +  if (!mPendingSessions.Contains(aSession->Token())) {

Ur, the "!" here needs to be inverted...

@@ +221,5 @@
> +  if (!aSession) {
> +    NS_WARNING("Invalid MediaKeySession passed to OnSessionIdReady()");
> +    return;
> +  }
> +  if (!aSession->GetSessionId().IsEmpty()) {

... and the "!" here needs to be inverted too... :(

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a74e49e232f9

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8544316 [details] [diff] [review]
Patch v1

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

::: dom/media/eme/MediaKeys.cpp
@@ +213,5 @@
> +  if (mKeySessions.Contains(aSession->GetSessionId())) {
> +    NS_WARNING("MediaKeySession's made ready multiple times!");
> +    return;
> +  }
> +  if (!mPendingSessions.Contains(aSession->Token())) {

This would allow a session that is in neither mPendingSessions nor mKeySessions to be added to mKeySessions successfully. Since a session could be made ready on when it was in mPendingSessions, it would be better to move a session from mPendingSessions to mKeySessions in OnSessionIdReady() to enfore the invariant and GetPendingSession() will not change mPendingSessions.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a74e49e232f9

Comment 8

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

Mass update firefox-status to track EME uplift.