[EME] Getting wrong pending MediaKeySession when promise is resolved.

RESOLVED FIXED in Firefox 52

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kikuo, Assigned: kikuo)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Session Token is put into map, but trying to remove it by a promiseId while rejecting / resolving promise. [2]

[1] http://searchfox.org/mozilla-central/source/dom/media/eme/MediaKeys.cpp#452
[2] http://searchfox.org/mozilla-central/source/dom/media/eme/MediaKeys.cpp#270-280

That would result to failures (org.w3.clearkey test if MediaKeySession generateRequest() resolves for various sessions) in http://www.w3c-test.org/encrypted-media/clearkey-mp4-syntax-mediakeysession.html.

I'd provide a mapping for promiseId to token, and let session in mPendingSessions be found by token only.
Please use Permalink to avoid referencing wrong lines of the code.
Good catch.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8802489 - Flags: review?(cpearce)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8802489 [details]
Bug 1310936- Provide a map to get pending MediaKeySession by promise Id correctly.

https://reviewboard.mozilla.org/r/86876/#review86064

::: dom/media/eme/MediaKeys.h:161
(Diff revision 1)
>    RefPtr<nsIPrincipal> mTopLevelPrincipal;
>  
>    const bool mDistinctiveIdentifierRequired;
>    const bool mPersistentStateRequired;
> +
> +  std::map<uint32_t, uint32_t> mPromiseIdToken;

You should use nsDataHashtable<nsUint32HashKey, uint32_t>; we're not supposed to use std containers in mozilla code.

ns*Hashtable is optimized for our use cases, and has an easier to understand API.

::: dom/media/eme/MediaKeys.cpp:205
(Diff revision 1)
> +MediaKeys::ConnectPendingPromiseIdWithToken(PromiseId aId, uint32_t aToken)
> +{
> +  // Should only be called from MediaKeySession::GenerateRequest and
> +  // MediaKeySession::Load.
> +  mPromiseIdToken.insert(std::pair<uint32_t, uint32_t>(aId, aToken));
> +  EME_LOG("MediaKeys[%p]::ConnectPendingPromiseIdWithToken() id=%d => token(%d)", this, aId, aToken);

Use %u instead of %d for unsigned.

::: dom/media/eme/MediaKeys.cpp:236
(Diff revision 1)
> -  if (mPendingSessions.Contains(aId)) {
> +
> -    // This promise could be a createSession or loadSession promise,
> +  // This promise could be a createSession or loadSession promise,
> -    // so we might have a pending session waiting to be resolved into
> +  // so we might have a pending session waiting to be resolved into
> -    // the promise on success. We've been directed to reject to promise,
> +  // the promise on success. We've been directed to reject to promise,
> -    // so we can throw away the corresponding session object.
> +  // so we can throw away the corresponding session object.
> -    mPendingSessions.Remove(aId);
> +  auto iter = mPromiseIdToken.find(aId);

Using nsDataHashtable, this should be able to become:

uint32_t token = 0;
if (mPromiseIdToken.Get(aId, &token)) {
  MOZ_ASSERT(mPendingSessions.Contains(token));
  mPendingSessions.Remove(token);
  mPromiseIdToken.Remove(aId);
}

::: dom/media/eme/MediaKeys.cpp:285
(Diff revision 1)
>    if (!promise) {
>      return;
>    }
> -  if (mPendingSessions.Contains(aId)) {
> +
> +  auto iter = mPromiseIdToken.find(aId);
> +  if (iter != mPromiseIdToken.end()) {

uint32_t token = 0;
if (!mPromiseIdToken.Get(aId, &token))) {
  promise->MaybeResolveWithUndefined();
  return;
}


Then you don't need to indent the "found token" block below, making the code simpler.

::: dom/media/eme/MediaKeys.cpp:288
(Diff revision 1)
> -  if (mPendingSessions.Contains(aId)) {
> +
> +  auto iter = mPromiseIdToken.find(aId);
> +  if (iter != mPromiseIdToken.end()) {
> +    uint32_t token = iter->second;
> +    mPromiseIdToken.erase(iter);
> +    if (mPendingSessions.Contains(token)) {

We could just call mPendingSessions.Get() here instead of mPendingSessions.Contains(), and if it returns true then mPendingSessions contains the session.

Then we can immediately call mPendingSessions.Remove(token) once, and we don't need to call it on two different paths below.

::: dom/media/eme/MediaKeys.cpp:309
(Diff revision 1)
> -    promise->MaybeResolveWithUndefined();
> +      promise->MaybeResolveWithUndefined();
> -  }
> +    }
> +  } else {
> +    promise->MaybeResolveWithUndefined();
> +  }
>    MOZ_ASSERT(!mPromises.Contains(aId));

This "MOZ_ASSERT(!mPromises.Contains(aId));" assert is asserting that RetrievePromise removes the promise from the map, so we can move it up to right after RetrievePromise(). Then we don't need to worry about not asserting on all code paths.

Comment 7

2 years ago
mozreview-review
Comment on attachment 8802489 [details]
Bug 1310936- Provide a map to get pending MediaKeySession by promise Id correctly.

https://reviewboard.mozilla.org/r/86876/#review86070

r+ with comments addressed. Ping me when you've pushed to review with the comments addressed, and I'll push it for you.
Attachment #8802489 - Flags: review?(cpearce) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8802489 [details]
Bug 1310936- Provide a map to get pending MediaKeySession by promise Id correctly.

https://reviewboard.mozilla.org/r/86876/#review86070

Thanks !  Comments were addressed.
(Assignee)

Comment 10

2 years ago
Another try, https://treeherder.mozilla.org/#/jobs?repo=try&revision=57c00ff5239d
In case Chris is not available, then I'll mark it "checkin-needed".

Comment 11

2 years ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b8ad0c0bac6
Provide a map to get pending MediaKeySession by promise Id correctly. r=cpearce

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b8ad0c0bac6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1311848
You need to log in before you can comment on or make changes to this bug.