794426 - Simultaneous calls to nsBuiltinDecoderStateMachine::StopAudioThread() cause problems

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Our code assumes that after the state machine thread has called StopAudioThread(), the audio thread is stopped. There is a situation where this is not true:
-- SetAudioCaptured gets called on the main thread, calling StopAudioThread from the main thread
-- main-thread StopAudioThread calls Shutdown() on the audio thread, and waits for it to shut down
-- meanwhile nsBuiltinDecoderStateMachine calls StopAudioThread()
-- state machine thread StopAudioThread calls Shutdown() on the audio thread, and returns immediately with an error because a thread can only be shut down by a single other thread
-- state machine thread carries on and cleans up stuff the audio thread is using --> crash

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 2

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

Comment on attachment 664897 [details] [diff] [review]
fix

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

I think we should avoid shutting down the audio thread from the main thread, as it will cause the JS context to block until the audio thread has shutdown, which may or may not be instantaneous; it'll introduce the possibility for jank.

How about in SetAudioCaptured() dispatching a runnable to the state machine thread to call StopAudioThread() instead? We can also have AudioLoop() check mAudioCaptured and exit if that's true to speed up the shutdown (similar to how we check for mState==SHUTDOWN).

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1517,5 @@
>    if (mDecodeThread) {
>      LOG(PR_LOG_DEBUG, ("%p Shutdown decode thread", mDecoder.get()));
>      {
>        ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> +      nsresult rv = mDecodeThread->Shutdown();

Does the same problem exist with the decode thread? Or are you just trying to prevent the same kind of error happening with the decode thread?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're right that we need to not wait for the audio thread to shut down from SetAudioCaptured on the main thread.

The problem is that if the audio thread is running, and we're trying to copy data from the audio queue into one or more MediaStreams, MediaDecoderStateMachine::SendStreamData and MediaDecoderStateMachine::AudioLoop both want to be in charge of popping data from the audio queue. So we can't allow that to happen. I think probably the solution is to not run MediaDecoderStateMachine::SendStreamData at all until the audio thread has shut down.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f30fd422239

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f30fd422239

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 688200 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): MediaStreams captured from media elements
User impact if declined: possible security issue
Testing completed: just laned on m-c
Risk to taking this patch (and alternatives if risky): fairly low risk since it should only affect a feature that is pretty much unused currently
String or UUID changes made by this patch: None

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Alex Keybl [:akeybl] from comment #8)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> > Bug caused by (feature/regressing bug #): MediaStreams captured from media
> > elements
> 
> When did this land? Is it enabled by default?

Landed April 30 so it's been around for a while. Enabled by default.

> > User impact if declined: possible security issue
> > Testing completed: just laned on m-c
> > Risk to taking this patch (and alternatives if risky): fairly low risk since
> > it should only affect a feature that is pretty much unused currently
> > String or UUID changes made by this patch: None
> 
> If this is a critical security bug, we should probably restrict it to the
> core-security group.

OK done.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 688200 [details] [diff] [review]
fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
esr17 and release

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be pretty easy to backport wherever needed.

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause noticeable regressions because it only affects a feature that's practically unused.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 688200 [details] [diff] [review]
fix

sec-approval+. We should figure out if we want to take this on Aurora, Beta, and ESR.

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> [Security approval request comment]

The idea of sec-approval? is to ask /before/ landing on m-c for potentially serious security bugs, so we don't force ourselves into landing stuff in the last beta or two before release that will end up going out without sufficient regression testing.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db3784061663
https://hg.mozilla.org/releases/mozilla-beta/rev/8fc480e6cfdc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8fc480e6cfdc

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/15b8772620ec