Bug 813399 - Remove workarounds for sync main thread deadlocks caused by audio remoting. r=cpearce

content/media/AudioStream.h

@@ -49,14 +49,10 @@ public:
   // Initialize the audio stream. aNumChannels is the number of audio
   // channels (1 for mono, 2 for stereo, etc) and aRate is the sample rate
   // (22050Hz, 44100Hz, etc).
-  // Unsafe to call with a monitor held due to synchronous event execution
-  // on the main thread, which may attempt to acquire any held monitor.
   virtual nsresult Init(int32_t aNumChannels, int32_t aRate,
                         const dom::AudioChannelType aAudioStreamType) = 0;
 
   // Closes the stream. All future use of the stream is an error.
-  // Unsafe to call with a monitor held due to synchronous event execution
-  // on the main thread, which may attempt to acquire any held monitor.
   virtual void Shutdown() = 0;
 
   // Write audio data to the audio hardware.  aBuf is an array of AudioDataValues
@@ -73,8 +69,6 @@ public:
   virtual void SetVolume(double aVolume) = 0;
 
   // Block until buffered audio data has been consumed.
-  // Unsafe to call with a monitor held due to synchronous event execution
-  // on the main thread, which may attempt to acquire any held monitor.
   virtual void Drain() = 0;
 
   // Pause audio playback
@@ -96,8 +90,6 @@ public:
 
   // Returns the minimum number of audio frames which must be written before
   // you can be sure that something will be played.
-  // Unsafe to call with a monitor held due to synchronous event execution
-  // on the main thread, which may attempt to acquire any held monitor.
   virtual int32_t GetMinWriteSize() = 0;
 
   int GetRate() { return mRate; }

content/media/MediaDecoderStateMachine.cpp

@@ -975,34 +975,10 @@ void MediaDecoderStateMachine::AudioLoop()
     channels = mInfo.mAudioChannels;
     rate = mInfo.mAudioRate;
     NS_ASSERTION(audioStartTime != -1, "Should have audio start time by now");
-  }
 
-  // It is unsafe to call some methods of AudioStream with the decoder
-  // monitor held, as on Android those methods do a synchronous dispatch to
-  // the main thread. If the audio thread holds the decoder monitor while
-  // it does a synchronous dispatch to the main thread, we can get deadlocks
-  // if the main thread tries to acquire the decoder monitor before the
-  // dispatched event has finished (or even started!) running. Methods which
-  // are unsafe to call with the decoder monitor held are documented as such
-  // in AudioStream.h.
-  nsRefPtr<AudioStream> audioStream = AudioStream::AllocateStream();
-  // In order to access decoder with the monitor held but avoid the dead lock
-  // issue explaned above, to hold monitor here only for getting audio channel type.
-  AudioChannelType audioChannelType;
-  {
-    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
-    audioChannelType = mDecoder->GetAudioChannelType();
-  }
-  audioStream->Init(channels, rate, audioChannelType);
+    mAudioStream = AudioStream::AllocateStream();
+    mAudioStream->Init(channels, rate, mDecoder->GetAudioChannelType());
 
-  {
-    // We must hold the monitor while setting mAudioStream or whenever we query
-    // the playback position off the audio thread. This ensures the audio stream
-    // is always alive when we use it off the audio thread. Note that querying
-    // the playback position does not do a synchronous dispatch to the main
-    // thread, so it's safe to call with the decoder monitor held.
-    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
-    mAudioStream = audioStream;
     volume = mVolume;
     mAudioStream->SetVolume(volume);
   }
@@ -1041,9 +1017,6 @@ void MediaDecoderStateMachine::AudioLoop()
       setVolume = volume != mVolume;
       volume = mVolume;
 
-      // Note audio stream IsPaused() does not do synchronous dispatch to the
-      // main thread on Android, so can be called safely with the decoder
-      // monitor held.
       if (IsPlaying() && mAudioStream->IsPaused()) {
         mAudioStream->Resume();
       }
@@ -1153,9 +1126,10 @@ void MediaDecoderStateMachine::AudioLoop()
   }
   LOG(PR_LOG_DEBUG, ("%p Reached audio stream end.", mDecoder.get()));
   {
-    // Must hold lock while anulling the audio stream to prevent
+    // Must hold lock while shutting down and anulling the audio stream to prevent
     // state machine thread trying to use it while we're destroying it.
     ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
+    mAudioStream->Shutdown();
     mAudioStream = nullptr;
     mEventManager.Clear();
     if (!mAudioCaptured) {
@@ -1166,11 +1140,6 @@ void MediaDecoderStateMachine::AudioLoop()
     }
   }
 
-  // Must not hold the decoder monitor while we shutdown the audio stream, as
-  // it makes a synchronous dispatch on Android.
-  audioStream->Shutdown();
-  audioStream = nullptr;
-
   LOG(PR_LOG_DEBUG, ("%p Audio stream finished playing, audio thread exit", mDecoder.get()));
 }
 
@@ -2182,19 +2151,16 @@ int64_t
 MediaDecoderStateMachine::GetAudioClock()
 {
   NS_ASSERTION(OnStateMachineThread(), "Should be on state machine thread.");
-  mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
-  if (!HasAudio() || mAudioCaptured)
-    return -1;
   // We must hold the decoder monitor while using the audio stream off the
   // audio thread to ensure that it doesn't get destroyed on the audio thread
   // while we're using it.
+  mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
+  if (!HasAudio() || mAudioCaptured)
+    return -1;
   if (!mAudioStream) {
     // Audio thread hasn't played any data yet.
     return mAudioStartTime;
   }
-  // Note that querying the playback position does not do a synchronous
-  // dispatch to the main thread on Android, so it's safe to call with
-  // the decoder monitor held here.
   int64_t t = mAudioStream->GetPosition();
   return (t == -1) ? -1 : t + mAudioStartTime;
 }