1311904 - !IsShutdown() assertion failure in MediaDecoder::NotifySuspendedStatusChanged()

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

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

Attachment: Testcase

STR:
1. Open attachment.
2. CTRL+Click link.
3. Do not switch tab, just wait.
4. Observe assertion failure.


Assertion failure: !IsShutdown(), at c:/Users/cpearce/src/mozilla/purple/dom/media/MediaDecoder.cpp:1183
#01: mozilla::MediaDecoder::FirstFrameLoaded (c:\users\cpearce\src\mozilla\purple\dom\media\mediadecoder.cpp:1018)
#02: mozilla::detail::ListenerHelper<1,mozilla::AbstractThread,<lambda_b598f49334d1a4c730b6255e2d2fb618> >::R<nsAutoPtr<mozilla::MediaInfo>,enum mozilla::MediaDecoderEventVisibilit
y>::Run (c:\users\cpearce\src\mozilla\purple\opt-objdir\dist\include\mediaeventsource.h:185)
#03: mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run (c:\users\cpearce\src\mozilla\purple\opt-objdir\dist\include\mozilla\taskdispatcher.h:195)
#04: nsThread::ProcessNextEvent (c:\users\cpearce\src\mozilla\purple\xpcom\threads\nsthread.cpp:1082)
#05: NS_ProcessNextEvent (c:\users\cpearce\src\mozilla\purple\xpcom\glue\nsthreadutils.cpp:290)
#06: mozilla::ipc::MessagePump::Run (c:\users\cpearce\src\mozilla\purple\ipc\glue\messagepump.cpp:124)
#07: MessageLoop::RunInternal (c:\users\cpearce\src\mozilla\purple\ipc\chromium\src\base\message_loop.cc:232)
#08: MessageLoop::RunHandler (c:\users\cpearce\src\mozilla\purple\ipc\chromium\src\base\message_loop.cc:226)
#09: MessageLoop::Run (c:\users\cpearce\src\mozilla\purple\ipc\chromium\src\base\message_loop.cc:206)
#10: nsBaseAppShell::Run (c:\users\cpearce\src\mozilla\purple\widget\nsbaseappshell.cpp:158)
#11: nsAppShell::Run (c:\users\cpearce\src\mozilla\purple\widget\windows\nsappshell.cpp:264)
#12: nsAppStartup::Run (c:\users\cpearce\src\mozilla\purple\toolkit\components\startup\nsappstartup.cpp:284)
#13: XREMain::XRE_mainRun (c:\users\cpearce\src\mozilla\purple\toolkit\xre\nsapprunner.cpp:4450)
#14: XREMain::XRE_main (c:\users\cpearce\src\mozilla\purple\toolkit\xre\nsapprunner.cpp:4583)
#15: XRE_main (c:\users\cpearce\src\mozilla\purple\toolkit\xre\nsapprunner.cpp:4674)
#16: do_main (c:\users\cpearce\src\mozilla\purple\browser\app\nsbrowserapp.cpp:282)
#17: NS_internal_main (c:\users\cpearce\src\mozilla\purple\browser\app\nsbrowserapp.cpp:415)
#18: wmain (c:\users\cpearce\src\mozilla\purple\toolkit\xre\nswindowswmain.cpp:118)
#19: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
#20: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x162c4]
#21: RtlSubscribeWnfStateChangeNotification[C:\WINDOWS\SYSTEM32\ntdll.dll +0x60609]
#22: RtlSubscribeWnfStateChangeNotification[C:\WINDOWS\SYSTEM32\ntdll.dll +0x605d4]


The page being opened in the background is an EME Web Platform Test.

Comment 1

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

#0  mozilla::MediaDecoder::Shutdown (this=0x7fffbdc82c00) at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/media/MediaDecoder.cpp:585
#1  0x00007fffeb14518d in mozilla::dom::HTMLMediaElement::ShutdownDecoder (this=0x7fffb861a800) at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/html/HTMLMediaElement.cpp:910
#2  0x00007fffeb1593e3 in mozilla::dom::HTMLMediaElement::SuspendOrResumeElement (this=0x7fffb861a800, aPauseElement=<optimized out>, aSuspendEvents=<optimized out>)
    at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/html/HTMLMediaElement.cpp:5243
#3  0x00007fffeb15ad12 in mozilla::dom::HTMLMediaElement::WindowSuspendChanged (this=0x7fffb861a800, aSuspend=2)
    at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/html/HTMLMediaElement.cpp:5842
#4  0x00007fffeb14c81b in mozilla::dom::HTMLMediaElement::NotifyAudioChannelAgent (this=0x7fffb861a800, aPlaying=<optimized out>)
    at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/html/HTMLMediaElement.cpp:5798
#5  0x00007fffeb14d2ff in mozilla::dom::HTMLMediaElement::ChangeReadyState (this=this@entry=0x7fffb861a800, aState=<optimized out>)
    at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/html/HTMLMediaElement.cpp:4814
#6  0x00007fffeb15668e in mozilla::dom::HTMLMediaElement::UpdateReadyStateInternal (this=this@entry=0x7fffb861a800)
    at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/html/HTMLMediaElement.cpp:4791
#7  0x00007fffeb1569db in mozilla::dom::HTMLMediaElement::FirstFrameLoaded (this=0x7fffb861a800) at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/html/HTMLMediaElement.cpp:4361
#8  0x00007fffeb20df63 in mozilla::MediaDecoder::FirstFrameLoaded (this=0x7fffbdc82c00, aInfo=..., aEventVisibility=<optimized out>)
    at /media/jwwang/DATA/codebase/mozilla-central/firefox/dom/media/MediaDecoder.cpp:1001

MediaDecoder::Shutdown happens in between
http://searchfox.org/mozilla-central/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/dom/media/MediaDecoder.cpp#991
and
http://searchfox.org/mozilla-central/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/dom/media/MediaDecoder.cpp#1017

So IsShutdown() swith from false to true and fail the assertion.

It is bad that mOwner->FirstFrameLoaded() results in reentrant calls to MediaDecoder which causes unexpected call flows.

I think the fix is to call |mOwner->FirstFrameLoaded()| asynchronously to avoid reentrant calls or move it to the end of FirstFrameLoaded().

Comment 2

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

Is there any reason for mOwner->FirstFrameLoaded() to be called in the middle of MediaDecoder::FirstFrameLoaded()? Or can it be called at the end of the function?

Comment 3

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

Attachment: Bug 1311904 - move |mOwner->FirstFrameLoaded()| to the bottom of FirstFrameLoaded() to prevent shutdown from happening in the middle of FirstFrameLoaded().

Review commit: https://reviewboard.mozilla.org/r/87950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87950/

Comment 4

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

We should review all mOwner->Blah() calls which could be reentrant too.

Comment 5

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

Comment on attachment 8803792 [details]
Bug 1311904 - move |mOwner->FirstFrameLoaded()| to the bottom of FirstFrameLoaded() to prevent shutdown from happening in the middle of FirstFrameLoaded().

https://reviewboard.mozilla.org/r/87950/#review87146

Comment 6

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

Thanks for the review!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe1bae8e454e
move |mOwner->FirstFrameLoaded()| to the bottom of FirstFrameLoaded() to prevent shutdown from happening in the middle of FirstFrameLoaded(). r=cpearce

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fe1bae8e454e