589071 - nsBuiltinDecoderStateMachine::GetAudioClock() does not hold audio monitor

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

Since GetAudioClock does not hold the audio monitor, it's possible that both it and the audio thread are calling nsAudioStream at the same time.  Even if the sydneyaudio API was thread-safe, this would still be unsafe because either caller may call nsAudioStream::Shutdown if an error occurs.

Either we need to hold the audio monitor in GetAudioClock, use some kind of ref/call counter for nsAudioStream::Shutdown to make sure it's only shut down when safe to do so, or have nsAudioStream return errors and force the callers to call Shutdown() explicitly.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Looking at this a bit further:

- Code is designed assuming nsAudioStream::GetPosition is safe to call if another thread is calling other nsAudioStream functions (mainly nsAudioStream::Write())
- We don't want to take the audio monitor in GetAudioClock because it'll cause it to block (and callers expect it to be fast) when the audio thread is blocked in a write.
- Write(), Drain(), and SetVolume() may call Shutdown() if they fail.  This is unsafe if another thread is calling GetPosition().

We can make this safer by removing the Shutdown() calls from Write(), Drain(), and SetVolume().  This guarantees that Shutdown() is only called explicitly, which also guarantees (via code inspection) that it's only called from the state machine thread.  Since GetAudioClock() is also only called on the state machine thread, this makes the code safe in the face of nsAudioStream::Shutdown() vs calls to GetAudioClock().

My proposed change means that an error occurring in nsAudioStream will delay shutting down the in-error stream until we shut down the owning decoder.  This isn't ideal, but probably doesn't matter much.  The presence of an mInError flag added to nsAudioStream allows early-error-exit for calls after the stream has been marked in-error.  It's possible for callers to race checking mInError vs it being set by another thread, but this already happens in the current code with a race vs Shutdown().

The attached patch doesn't solve all of the thread-safety problems, I'm leaving that for the planned audio backend work that I'm going to work on soon.  The intent of this patch is to reduce the crashes/timeouts in mochitests we're suffering from by reducing the risk of hitting free-while-in-use and other thread-safety issues in this code.

Comment 2

[cajbir (:cajbir)]

Comment on attachment 468571 [details] [diff] [review]
patch v0


>+  // PR_TRUE if this stream has encounter an error.

s/encounter/encountered

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: patch v0.1

Fix typo.

Comment 4

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

http://hg.mozilla.org/mozilla-central/rev/59c9f5b95889