Closed Bug 1113474 Opened 5 years ago Closed 5 years ago

[EME] Assertion failure intest_eme_persistent_sessions with slow GMP shutdown

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: cpearce, Assigned: eflores)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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...
Assignee: nobody → edwin
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?
Attachment #8561184 - Flags: review?(cpearce) → review-
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.
Attachment #8561184 - Flags: review- → review+
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.
Attachment #8562456 - Flags: review?(jwwang)
Attachment #8562456 - Flags: review?(jwwang) → review+
https://hg.mozilla.org/mozilla-central/rev/e3b66bc37706
https://hg.mozilla.org/mozilla-central/rev/7d5ff7ffa118
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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()
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.
Attachment #8572360 - Flags: approval-mozilla-beta?
Comment on attachment 8572359 [details] [diff] [review]
Patch 2 - Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.
Attachment #8572359 - Flags: approval-mozilla-beta?
Comment on attachment 8572360 [details] [diff] [review]
Patch 1 - Beta patch

Previously approved as part of the EME platform landing on Beta.
Attachment #8572360 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8572359 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.