Bug 1072780: patch 1 - clean up CurrentDriver() use off-MSG-thread; fix InCallback() r=roc
authorRandell Jesup <rjesup@jesup.org>
Sun, 28 Sep 2014 12:07:24 -0400
changeset 223034 649d337d331c197ec71fdc32a97f902d178449ba
parent 223033 d71444b752910f2987993a0a0d9fe1465ccf682d
child 223035 32eacf8f2efeeb298e7779f29fee9702ca1af7fa
push id7107
push userraliiev@mozilla.com
push dateMon, 13 Oct 2014 17:43:31 +0000
treeherdermozilla-aurora@b4b34e0acc75 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs1072780
milestone35.0a1
Bug 1072780: patch 1 - clean up CurrentDriver() use off-MSG-thread; fix InCallback() r=roc
content/media/GraphDriver.cpp
content/media/MediaStreamGraph.cpp
content/media/MediaStreamGraphImpl.h
--- a/content/media/GraphDriver.cpp
+++ b/content/media/GraphDriver.cpp
@@ -762,17 +762,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
@@ -1646,42 +1646,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];
@@ -2253,17 +2255,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);
@@ -2273,17 +2275,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()) {
@@ -2335,17 +2337,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
@@ -2459,38 +2461,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
@@ -409,16 +409,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() {
     return mDriver;
   }
 
   /**
    * Effectively set the new driver, while we are switching.
    * It is only safe to call this at the very end of an iteration, when there
    * has been a SwitchAtNextIteration call during the iteration. The driver
@@ -427,16 +430,26 @@ public:
   void SetCurrentDriver(GraphDriver* aDriver) {
     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.
    */