1129722 - [EME] Assertion failure in MediaKeyStatusMap::UpdateInternal on slow CDM shutdown

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Trying to reproduce bug 1113474, ran into the following assertion running test_eme_persistent_sessions.html.

Happens after inserting a sleep(2) at [1].

Looks like the CDM is racing the JS code to shutdown?


[1] http://hg.mozilla.org/mozilla-central/file/2297934585ab/media/gmp-clearkey/0.1/ClearKeySessionManager.cpp#l288


#0  0x00007f29e391f7bf in js::MapObject::clear (cx=0x7f29e8673690, obj=...) at /home/me/src/evil-central/js/src/builtin/MapObject.cpp:1557
1557	    MOZ_ASSERT(MapObject::is(obj));

(gdb) bt
#0  0x00007f29e391f7bf in js::MapObject::clear (cx=0x7f29e8673690, obj=...) at /home/me/src/evil-central/js/src/builtin/MapObject.cpp:1557
#1  0x00007f29e3921d63 in JS::MapClear (cx=0x7f29e8673690, obj=...) at /home/me/src/evil-central/js/src/builtin/MapObject.cpp:2142
#2  0x00007f29e1828a63 in mozilla::dom::MediaKeyStatusMap::UpdateInternal (this=0x7f29b16fcd80, keys=...) at /home/me/src/evil-central/dom/media/eme/MediaKeyStatusMap.cpp:219
#3  0x00007f29e1828d95 in mozilla::dom::MediaKeyStatusMap::Update (this=0x7f29b16fcd80, keys=...) at /home/me/src/evil-central/dom/media/eme/MediaKeyStatusMap.cpp:243
#4  0x00007f29e1826c35 in mozilla::dom::MediaKeySession::UpdateKeyStatusMap (this=0x7f29b1460f20) at /home/me/src/evil-central/dom/media/eme/MediaKeySession.cpp:127
#5  0x00007f29e1827869 in mozilla::dom::MediaKeySession::DispatchKeysChange (this=0x7f29b1460f20) at /home/me/src/evil-central/dom/media/eme/MediaKeySession.cpp:305
#6  0x00007f29e18244c2 in mozilla::CDMProxy::OnKeysChange (this=0x7f29b582f2e0, aSessionId=...) at /home/me/src/evil-central/dom/media/eme/CDMProxy.cpp:447
#7  0x00007f29e1834c64 in nsRunnableMethodImpl<void (mozilla::CDMProxy::*)(nsAString_internal const&), nsString, true>::Run (this=0x7f29b14b0d80) at ../../../dist/include/nsThreadUtils.h:361
#8  0x00007f29df43a50f in nsThread::ProcessNextEvent (this=0x7f29e8638a90, aMayWait=false, aResult=0x7fff074d739f) at /home/me/src/evil-central/xpcom/threads/nsThread.cpp:855
#9  0x00007f29df47e3fa in NS_ProcessNextEvent (aThread=0x7f29e8638a90, aMayWait=false) at /home/me/src/evil-central/xpcom/glue/nsThreadUtils.cpp:265
#10 0x00007f29df93f1be in mozilla::ipc::MessagePump::Run (this=0x7f29d6235440, aDelegate=0x7f29e866dd20) at /home/me/src/evil-central/ipc/glue/MessagePump.cpp:99
#11 0x00007f29df8eaf85 in MessageLoop::RunInternal (this=0x7f29e866dd20) at /home/me/src/evil-central/ipc/chromium/src/base/message_loop.cc:233
#12 0x00007f29df8eaf1a in MessageLoop::RunHandler (this=0x7f29e866dd20) at /home/me/src/evil-central/ipc/chromium/src/base/message_loop.cc:226
#13 0x00007f29df8eaeab in MessageLoop::Run (this=0x7f29e866dd20) at /home/me/src/evil-central/ipc/chromium/src/base/message_loop.cc:200
#14 0x00007f29e1db2490 in nsBaseAppShell::Run (this=0x7f29cd06a400) at /home/me/src/evil-central/widget/nsBaseAppShell.cpp:164
#15 0x00007f29e2a5d858 in nsAppStartup::Run (this=0x7f29cd068060) at /home/me/src/evil-central/toolkit/components/startup/nsAppStartup.cpp:281
#16 0x00007f29e2ae9259 in XREMain::XRE_mainRun (this=0x7fff074d7760) at /home/me/src/evil-central/toolkit/xre/nsAppRunner.cpp:4160
#17 0x00007f29e2ae95b9 in XREMain::XRE_main (this=0x7fff074d7760, argc=5, argv=0x7fff074d8c68, aAppData=0x7fff074d7980) at /home/me/src/evil-central/toolkit/xre/nsAppRunner.cpp:4236
#18 0x00007f29e2ae9853 in XRE_main (argc=5, argv=0x7fff074d8c68, aAppData=0x7fff074d7980, aFlags=0) at /home/me/src/evil-central/toolkit/xre/nsAppRunner.cpp:4456
#19 0x0000000000404ad7 in do_main (argc=5, argv=0x7fff074d8c68, xreDirectory=0x7f29e86516c0) at /home/me/src/evil-central/browser/app/nsBrowserApp.cpp:294
#20 0x0000000000404f56 in main (argc=5, argv=0x7fff074d8c68) at /home/me/src/evil-central/browser/app/nsBrowserApp.cpp:667

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Weird. Looks like the JS GC fails to traverse the MediaKeyStatusMap and just steamrolls mMap. Unsure why.

Comment 2

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

I see JSObject::group_ becomes null for the map object... it should be resulted from GC... but I wonder why MediaKeyStatusMap didn't keep mMap alive.

Comment 3

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

Another finding:

in mozilla::dom::MediaKeyStatusMap::UpdateInternal

(gdb) p * map.ptr->group_.value.clasp_
$14 = {
  name = 0x7fffc25f3e80 'K' <repeats 96 times>, "@?_\302\377\177",

The class name should be "Map".

Is it possible for MediaKeyStatusMap::mMap to point to another object due to moving GC?

Comment 4

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

As indicated by Edwin (https://developer.mozilla.org/en/docs/Interfacing_with_the_XPCOM_cycle_collector#Handling.C2.A0_JSObjects_fields) , it looks like we have to call |mozilla::HoldJSObjects(this)| in MediaKeyStatusMap constructor and |mozilla::DropJSObjects(this)| in the destructor.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1129722.patch

Comment 6

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

Comment on attachment 8559579 [details] [diff] [review]
1129722.patch

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

Include bz to make sure this is all we need to keep a JSObject from GC.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8559579 [details] [diff] [review]
1129722.patch

Yikes, yes.  I should have caught that....

>   printf("MediaKeyStatusMap dtor %p\n", this);

Please remove this line while you're here.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> >   printf("MediaKeyStatusMap dtor %p\n", this);
> 
> Please remove this line while you're here.

Whoops, that was from above this patch in my queue.

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/733843602df8

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/733843602df8

Comment 11

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

https://hg.mozilla.org/releases/mozilla-beta/rev/93c5dec5ad4b

Comment 12

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

Attachment: Beta patch

Patch for beta branch as part of EME platform uplift.

Comment 13

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

Comment on attachment 8572353 [details] [diff] [review]
Beta patch

Requesting retroactive approval for Beta landing as part of EME platform uplift.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8572353 [details] [diff] [review]
Beta patch

Approved for Beta as part of EME platform uplift.