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.

ex: http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/dom/media/eme/MediaKeys.cpp#452

Good catch.

Try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=e865cb79125d

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

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.

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

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