Bug 1072780 - Patch 1: Clean up CurrentDriver() use off-MSG-thread; fix InCallback(). r=roc, a=lmandel
authorRandell Jesup <rjesup@jesup.org>
Sun, 28 Sep 2014 12:07:24 -0400
changeset 218141 6fc7dace7ec946e2e5c2a727a55feb7a70421a39
parent 218140 c89cf06d1f3feb0e94c43d97715761c0956927a2
child 218142 e3397237012d16c364a5a403df91273299dce19f
push id7041
push userryanvm@gmail.com
push dateTue, 07 Oct 2014 02:24:58 +0000
treeherdermozilla-aurora@d3baf1d41b83 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc, lmandel
bugs1072780
milestone34.0a2
Bug 1072780 - Patch 1: Clean up CurrentDriver() use off-MSG-thread; fix InCallback(). r=roc, a=lmandel
content/media/GraphDriver.cpp
content/media/MediaStreamGraph.cpp
content/media/MediaStreamGraphImpl.h
--- a/content/media/GraphDriver.cpp
+++ b/content/media/GraphDriver.cpp
@@ -765,17 +765,22 @@ AudioCallbackDriver::DataCallback(AudioD
 {
   bool stillProcessing;
 
   if (mPauseRequested) {
     PodZero(aBuffer, aFrames * mGraphImpl->AudioChannelCount());
     return aFrames;
   }
 
-  DebugOnly<AutoInCallback> aic(AutoInCallback(this));
+#ifdef DEBUG
+  // DebugOnly<> doesn't work here... it forces an initialization that will cause
+  // mInCallback to be set back to false before we exit the statement.  Do it by
+  // hand instead.
+  AutoInCallback aic(this);
+#endif
 
   if (mStateComputedTime == 0) {
     MonitorAutoLock mon(mGraphImpl->GetMonitor());
     // Because this function is called during cubeb_stream_init (to prefill the
     // audio buffers), it can be that we don't have a message here (because this
     // driver is the first one for this graph), and the graph would exit. Simply
     // return here until we have messages.
     if (!mGraphImpl->MessagesQueued()) {
--- a/content/media/MediaStreamGraph.cpp
+++ b/content/media/MediaStreamGraph.cpp
@@ -1653,42 +1653,44 @@ MediaStreamGraphImpl::RunInStableState(b
       // processing.
       if (mLifecycleState == LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP &&
           mRealtime && !mForceShutDown) {
         mLifecycleState = LIFECYCLE_RUNNING;
         // Revive the MediaStreamGraph since we have more messages going to it.
         // Note that we need to put messages into its queue before reviving it,
         // or it might exit immediately.
         {
-          MonitorAutoUnlock unlock(mMonitor);
           LIFECYCLE_LOG("Reviving a graph (%p) ! %s\n",
               this, CurrentDriver()->AsAudioCallbackDriver() ? "AudioDriver" :
                                                                "SystemDriver");
-          CurrentDriver()->Revive();
+          nsRefPtr<GraphDriver> driver = CurrentDriver();
+          MonitorAutoUnlock unlock(mMonitor);
+          driver->Revive();
         }
       }
     }
 
     // Don't start the thread for a non-realtime graph until it has been
     // explicitly started by StartNonRealtimeProcessing.
     if (mLifecycleState == LIFECYCLE_THREAD_NOT_STARTED &&
         (mRealtime || mNonRealtimeProcessing)) {
       mLifecycleState = LIFECYCLE_RUNNING;
       // Start the thread now. We couldn't start it earlier because
       // the graph might exit immediately on finding it has no streams. The
       // first message for a new graph must create a stream.
       {
         // We should exit the monitor for now, because starting a stream might
         // take locks, and we don't want to deadlock.
-        MonitorAutoUnlock unlock(mMonitor);
         LIFECYCLE_LOG("Starting a graph (%p) ! %s\n",
                       this,
                       CurrentDriver()->AsAudioCallbackDriver() ? "AudioDriver" :
                                                                  "SystemDriver");
-        CurrentDriver()->Start();
+        nsRefPtr<GraphDriver> driver = CurrentDriver();
+        MonitorAutoUnlock unlock(mMonitor);
+        driver->Start();
       }
     }
 
     if ((mForceShutDown || !mRealtime) &&
         mLifecycleState == LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP) {
       // Defer calls to RunDuringShutdown() to happen while mMonitor is not held.
       for (uint32_t i = 0; i < mBackMessageQueue.Length(); ++i) {
         MessageBlock& mb = mBackMessageQueue[i];
@@ -2261,17 +2263,17 @@ SourceMediaStream::DestroyImpl()
 }
 
 void
 SourceMediaStream::SetPullEnabled(bool aEnabled)
 {
   MutexAutoLock lock(mMutex);
   mPullEnabled = aEnabled;
   if (mPullEnabled && GraphImpl()) {
-    GraphImpl()->CurrentDriver()->EnsureNextIteration();
+    GraphImpl()->EnsureNextIteration();
   }
 }
 
 void
 SourceMediaStream::AddTrack(TrackID aID, TrackRate aRate, TrackTicks aStart,
                             MediaSegment* aSegment)
 {
   MutexAutoLock lock(mMutex);
@@ -2281,17 +2283,17 @@ SourceMediaStream::AddTrack(TrackID aID,
   // We resample all audio input tracks to the sample rate of the audio mixer.
   data->mOutputRate = aSegment->GetType() == MediaSegment::AUDIO ?
                       GraphImpl()->AudioSampleRate() : aRate;
   data->mStart = aStart;
   data->mCommands = TRACK_CREATE;
   data->mData = aSegment;
   data->mHaveEnough = false;
   if (auto graph = GraphImpl()) {
-    graph->CurrentDriver()->EnsureNextIteration();
+    graph->EnsureNextIteration();
   }
 }
 
 void
 SourceMediaStream::ResampleAudioToGraphSampleRate(TrackData* aTrackData, MediaSegment* aSegment)
 {
   if (aSegment->GetType() != MediaSegment::AUDIO ||
       aTrackData->mInputRate == GraphImpl()->AudioSampleRate()) {
@@ -2343,17 +2345,17 @@ SourceMediaStream::AppendToTrack(TrackID
       ApplyTrackDisabling(aID, aSegment, aRawSegment);
 
       ResampleAudioToGraphSampleRate(track, aSegment);
 
       // Must notify first, since AppendFrom() will empty out aSegment
       NotifyDirectConsumers(track, aRawSegment ? aRawSegment : aSegment);
       track->mData->AppendFrom(aSegment); // note: aSegment is now dead
       appended = true;
-      GraphImpl()->CurrentDriver()->EnsureNextIteration();
+      GraphImpl()->EnsureNextIteration();
     } else {
       aSegment->Clear();
     }
   }
   return appended;
 }
 
 void
@@ -2467,38 +2469,38 @@ SourceMediaStream::EndTrack(TrackID aID)
   // ::EndAllTrackAndFinished() can end these before the sources call this
   if (!mFinished) {
     TrackData *track = FindDataForTrack(aID);
     if (track) {
       track->mCommands |= TRACK_END;
     }
   }
   if (auto graph = GraphImpl()) {
-    graph->CurrentDriver()->EnsureNextIteration();
+    graph->EnsureNextIteration();
   }
 }
 
 void
 SourceMediaStream::AdvanceKnownTracksTime(StreamTime aKnownTime)
 {
   MutexAutoLock lock(mMutex);
   MOZ_ASSERT(aKnownTime >= mUpdateKnownTracksTime);
   mUpdateKnownTracksTime = aKnownTime;
   if (auto graph = GraphImpl()) {
-    graph->CurrentDriver()->EnsureNextIteration();
+    graph->EnsureNextIteration();
   }
 }
 
 void
 SourceMediaStream::FinishWithLockHeld()
 {
   mMutex.AssertCurrentThreadOwns();
   mUpdateFinished = true;
   if (auto graph = GraphImpl()) {
-    graph->CurrentDriver()->EnsureNextIteration();
+    graph->EnsureNextIteration();
   }
 }
 
 void
 SourceMediaStream::EndAllTrackAndFinish()
 {
   MutexAutoLock lock(mMutex);
   for (uint32_t i = 0; i < mUpdateTracks.Length(); ++i) {
--- a/content/media/MediaStreamGraphImpl.h
+++ b/content/media/MediaStreamGraphImpl.h
@@ -415,16 +415,19 @@ public:
 
   /**
    * Signal to the graph that the thread has paused indefinitly,
    * or resumed.
    */
   void PausedIndefinitly();
   void ResumedFromPaused();
 
+  /**
+   * Not safe to call off the MediaStreamGraph thread unless monitor is held!
+   */
   GraphDriver* CurrentDriver() {
 #ifdef DEBUG
     // #ifdef since we're not wrapping it all in MOZ_ASSERT()
     if (!mDriver->OnThread()) {
       mMonitor.AssertCurrentThreadOwns();
     }
 #endif
     return mDriver;
@@ -440,16 +443,26 @@ public:
     MOZ_ASSERT(mDriver->OnThread());
     mDriver = aDriver;
   }
 
   Monitor& GetMonitor() {
     return mMonitor;
   }
 
+  /**
+   * Must implement here to avoid dangerous data races around CurrentDriver() -
+   * we don't want stuff off MSG thread using "graph->CurrentDriver()->EnsureNextIteration()"
+   * because CurrentDriver may change (and it's a TSAN data race)
+   */
+  void EnsureNextIteration() {
+    MonitorAutoLock mon(mMonitor);
+    CurrentDriver()->EnsureNextIterationLocked();
+  }
+
   // Data members
   //
   /**
    * Graphs own owning references to their driver, until shutdown. When a driver
    * switch occur, previous driver is either deleted, or it's ownership is
    * passed to a event that will take care of the asynchronous cleanup, as
    * audio stream can take some time to shut down.
    */