While trying to repro another bug, I discovered an assertion failure in test_eme_persistent_sessions if I add a sleep to ClearKeyDecryptor::Release() (the idea here was to simulate when the Post(newDestroyTask()) there finished executing the task before the Post call returned. Assertion failure: mPromises.Contains(aId), at c:\Users\cpearce\src\mozilla\purple\dom\media\eme\MediaKeys.cpp:179 #01: mozilla::dom::MediaKeys::OnSessionLoaded (c:\users\cpearce\src\mozilla\purple\dom\media\eme\mediakeys.cpp:390) #02: mozilla::CDMProxy::OnResolveLoadSessionPromise (c:\users\cpearce\src\mozilla\purple\dom\media\eme\cdmproxy.cpp:405) #03: mozilla::LoadSessionTask::Run (c:\users\cpearce\src\mozilla\purple\dom\media\eme\cdmcallbackproxy.cpp:72) #04: nsThread::ProcessNextEvent (c:\users\cpearce\src\mozilla\purple\xpcom\threads\nsthread.cpp:835) #05: NS_ProcessNextEvent (c:\users\cpearce\src\mozilla\purple\xpcom\glue\nsthreadutils.cpp:265) #06: mozilla::ipc::MessagePump::Run (c:\users\cpearce\src\mozilla\purple\ipc\glue\messagepump.cpp:99) #07: MessageLoop::RunInternal (c:\users\cpearce\src\mozilla\purple\ipc\chromium\src\base\message_loop.cc:234) #08: MessageLoop::RunHandler (c:\users\cpearce\src\mozilla\purple\ipc\chromium\src\base\message_loop.cc:227) #09: MessageLoop::Run (c:\users\cpearce\src\mozilla\purple\ipc\chromium\src\base\message_loop.cc:201) #10: nsBaseAppShell::Run (c:\users\cpearce\src\mozilla\purple\widget\nsbaseappshell.cpp:166) #11: nsAppShell::Run (c:\users\cpearce\src\mozilla\purple\widget\windows\nsappshell.cpp:178) #12: nsAppStartup::Run (c:\users\cpearce\src\mozilla\purple\toolkit\components\startup\nsappstartup.cpp:281) #13: XREMain::XRE_mainRun (c:\users\cpearce\src\mozilla\purple\toolkit\xre\nsapprunner.cpp:4150) #14: XREMain::XRE_main (c:\users\cpearce\src\mozilla\purple\toolkit\xre\nsapprunner.cpp:4226) #15: XRE_main (c:\users\cpearce\src\mozilla\purple\toolkit\xre\nsapprunner.cpp:4446) #16: do_main (c:\users\cpearce\src\mozilla\purple\browser\app\nsbrowserapp.cpp:292) #17: NS_internal_main (c:\users\cpearce\src\mozilla\purple\browser\app\nsbrowserapp.cpp:661) #18: wmain (c:\users\cpearce\src\mozilla\purple\toolkit\xre\nswindowswmain.cpp:117) #19: __tmainCRTStartup (f:\dd\vctools\crt\crtw32\startup\crt0.c:255) #20: BaseThreadInitThunk[KERNEL32 +0x17c04] #21: RtlInitializeExceptionChain[ntdll +0x5b90f] #22: RtlInitializeExceptionChain[ntdll +0x5b8da] Maybe what's happening is we're marking the keys as usable before the load session completes, and then we're closing the session and thus the session load resolution is failing? Need to check up on this when I get back from PTO...

Comment on attachment 8561184 [details] [diff] [review] 1113474.patch Review of attachment 8561184 [details] [diff] [review]: ----------------------------------------------------------------- I'm a bit concerned that a misbehaving CDM that forgets to resolve all its promises would cause the MediaKeys to leak... Can we instead make CDMProxy::mKeys a strong pointer, and somehow ensure that the cycle we create by doing so gets broken?

Comment on attachment 8561184 [details] [diff] [review] 1113474.patch Review of attachment 8561184 [details] [diff] [review]: ----------------------------------------------------------------- r+ as per our discussion IRL; MediaKeys::Shutdown() will reject all the promises here, breaking cycles. Please add a comment to that effect when you addref.

MediaKeys::Shutdown() rejects the promises without decreasing the ref count of the MediaKeys object. It looks like a leak to me.

Nice find. I wonder why tbpl didn't pack a sad over that.

https://hg.mozilla.org/mozilla-central/rev/e3b66bc37706 https://hg.mozilla.org/mozilla-central/rev/7d5ff7ffa118

Comment on attachment 8562456 [details] [diff] [review] 1113474-fix.patch Review of attachment 8562456 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/eme/MediaKeys.cpp @@ +81,5 @@ > nsRefPtr<MediaKeySession>& aSession, > void* aClosure) > { > aSession->OnClosed(); > + ((MediaKeys*)aClosure)->Release(); Oops! This should go to RejectPromises() instead of CloseSessions()

https://hg.mozilla.org/releases/mozilla-beta/rev/c85410a124c6 https://hg.mozilla.org/releases/mozilla-beta/rev/706bf5c21e6d

Patch for beta branch as part of EME platform uplift.

Patch for beta branch as part of EME platform uplift.

Comment on attachment 8572360 [details] [diff] [review] Patch 1 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572359 [details] [diff] [review] Patch 2 - Beta patch Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment on attachment 8572360 [details] [diff] [review] Patch 1 - Beta patch Previously approved as part of the EME platform landing on Beta.