Closed Bug 1029372 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox33 --- unaffected
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

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
Depends on: 979104
Blocks: 1027270
Blocks: 1029317
Blocks: 1035157
Blocks: 1031590
Blocks: 1033089
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
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8483913 - Flags: review?(kinetik)
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 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()?
Attachment #8483913 - Flags: review?(kinetik) → review+
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().
https://hg.mozilla.org/mozilla-central/rev/5b59618f0c34
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Blocks: 1067933
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
Attachment #8492873 - Flags: review+
Attachment #8492873 - Flags: approval-mozilla-aurora?
Comment on attachment 8492873 [details] [diff] [review]
1029372_fix_reset_AudioQueue_before_stop_AudioThread-aurora.patch

Aurora+
Attachment #8492873 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify-
Duplicate of this bug: 1035157
You need to log in before you can comment on or make changes to this bug.