1029372 - PROCESS-CRASH | /tests/content/media/test/test_bug448534.html | application crashed [@ mozilla::MediaDecoderStateMachine::PlayFromAudioQueue(unsigned long, unsigned int)]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

https://tbpl.mozilla.org/php/getParsedLog.php?id=42328868&tree=Try&full=1#error3

22:51:44     INFO -  [Parent 1783] WARNING: Should be playing: 'IsPlaying()', file /builds/slave/try-l64-d-00000000000000000000/build/content/media/MediaDecoderStateMachine.cpp, line 1354
22:51:44     INFO -  [Parent 1783] ###!!! ASSERTION: You can't dereference a NULL nsAutoPtr with operator->().: 'mRawPtr != 0', file ../../dist/include/nsAutoPtr.h, line 166
22:51:44     INFO -  mozilla::MediaDecoderStateMachine::PlayFromAudioQueue(unsigned long, unsigned int) [content/media/MediaDecoderStateMachine.cpp:1361]
22:51:44     INFO -  mozilla::MediaDecoderStateMachine::AudioLoop() [content/media/MediaDecoderStateMachine.cpp:1263]
22:51:44     INFO -  nsRunnableMethodImpl<void (mozilla::MediaDecoderStateMachine::*)(), void, true>::Run() [obj-firefox/dist/include/nsThreadUtils.h:389]
22:51:44     INFO -  nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:766]
22:51:44     INFO -  NS_ProcessNextEvent(nsIThread*, bool) [xpcom/glue/nsThreadUtils.cpp:263]
22:51:44     INFO -  mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:308]
22:51:44     INFO -  MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:230]
22:51:44     INFO -  MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:504]
22:51:44     INFO -  nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:355]
22:51:44     INFO -  _pt_root [nsprpub/pr/src/pthreads/ptthread.c:215]
22:51:44     INFO -  libpthread.so.0 + 0x7e9a
22:51:44     INFO -  libc.so.6 + 0xf3dbd

ResetPlayback() [1] is called before StopAudioThread(). It is possible for AudioQueue to become empty when PlayFromAudioQueue() is being called and |audio| [2] will be null.

[1] http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/content/media/MediaDecoderStateMachine.cpp#l2574
[2] http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/content/media/MediaDecoderStateMachine.cpp#l1331

Comment 1

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

Attachment: 1029372_fix_reset_AudioQueue_before_stop_AudioThread-v1.patch

For audio thread related code has been moved to AudioSink, I will tell the story again.

Since AudioSink::PlayFromAudioQueue() is running outside the decoder monitor, it is possible for AudioQueue().PopFront() to return null [1] if MediaDecoderStateMachine::ResetPlayback() [2] just happens before the pop action.

Fix: We should not clear the audio queue until audio thread is stopped.

[1] http://hg.mozilla.org/mozilla-central/file/5e9826980be5/content/media/AudioSink.cpp#l296
[2] http://hg.mozilla.org/mozilla-central/file/5e9826980be5/content/media/MediaDecoderStateMachine.cpp#l1460

Comment 2

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

Comment on attachment 8483913 [details] [diff] [review]
1029372_fix_reset_AudioQueue_before_stop_AudioThread-v1.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +2284,5 @@
>        if (IsPlaying()) {
>          StopPlayback();
>        }
>  
> +      StopAudioThread();

By the time  StopAudioThread() is returned, mAudioSink should be completed and cleared. Otherwise, there will be a race in clear the audio queue while AudioSink is popping audio samples.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8483913 [details] [diff] [review]
1029372_fix_reset_AudioQueue_before_stop_AudioThread-v1.patch

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

Looks good, thanks.

::: content/media/MediaDecoderStateMachine.cpp
@@ +1567,5 @@
>      // That may have been waiting for the audio thread to stop.
>      SendStreamData();
>    }
> +  // Wake up those waiting for audio sink to finish.
> +  mDecoder->GetReentrantMonitor().NotifyAll();

Why do we need a second NotifyAll()?

Comment 4

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

Comment on attachment 8483913 [details] [diff] [review]
1029372_fix_reset_AudioQueue_before_stop_AudioThread-v1.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1567,5 @@
>      // That may have been waiting for the audio thread to stop.
>      SendStreamData();
>    }
> +  // Wake up those waiting for audio sink to finish.
> +  mDecoder->GetReentrantMonitor().NotifyAll();

When thread 1 is at |mAudioSink->Shutdown()| and thread 2 is at |if (mStopAudioThread)|, thread 2 will get stuck at |mDecoder->GetReentrantMonitor().Wait()| if thread 1 doesn't issue a 2nd NotifyAll().

Comment 5

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=d63a52ae699b
Most green.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b59618f0c34

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b59618f0c34

Comment 8

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

Attachment: 1029372_fix_reset_AudioQueue_before_stop_AudioThread-aurora.patch

Approval Request Comment
[Feature/regressing bug #]:unknown
[User impact if declined]:Occasional crash in test case might be experienced on Aurora (see bug 1035157).
[Describe test coverage new/current, TBPL]:
Try: https://tbpl.mozilla.org/?tree=Try&rev=fbec78925e95
[Risks and why]: Low. The cause is well known and has been exercised on Central for weeks.
[String/UUID change made/needed]:none

Comment 9

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

Comment on attachment 8492873 [details] [diff] [review]
1029372_fix_reset_AudioQueue_before_stop_AudioThread-aurora.patch

Aurora+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e7c9193980f