Note: There are a few cases of duplicates in user autocompletion which are being worked on.

nsBuiltinDecoderStateMachine::GetAudioClock() does not hold audio monitor

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 468571 [details] [diff] [review]
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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attachment #468571 - Flags: review?(chris.double)

Comment 2

7 years ago
Comment on attachment 468571 [details] [diff] [review]
patch v0


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

s/encounter/encountered
Attachment #468571 - Flags: review?(chris.double) → review+
(Assignee)

Comment 3

7 years ago
Created attachment 468594 [details] [diff] [review]
patch v0.1

Fix typo.
Attachment #468571 - Attachment is obsolete: true
Attachment #468594 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #468594 - Flags: approval2.0?
Attachment #468594 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/59c9f5b95889
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.