bug 1072780: patch 4 - Use atomics for EnsureNextIteration to close races around CurrentDriver r=roc

content/media/GraphDriver.cpp

@@ -53,7 +53,6 @@ GraphDriver::GraphDriver(MediaStreamGraphImpl* aGraphImpl)
     mNextStateComputedTime(0),
     mGraphImpl(aGraphImpl),
     mWaitState(WAITSTATE_RUNNING),
-    mNeedAnotherIteration(false),
     mCurrentTimeStamp(TimeStamp::Now()),
     mPreviousDriver(nullptr),
     mNextDriver(nullptr)
@@ -95,6 +94,7 @@ void GraphDriver::EnsureImmediateWakeUpLocked()
 {
   mGraphImpl->GetMonitor().AssertCurrentThreadOwns();
   mWaitState = WAITSTATE_WAKING_UP;
+  mGraphImpl->mGraphDriverAsleep = false; // atomic
   mGraphImpl->GetMonitor().Notify();
 }
 
@@ -113,22 +113,7 @@ void GraphDriver::UpdateStateComputedTime(GraphTime aStateComputedTime)
 
 void GraphDriver::EnsureNextIteration()
 {
-  MonitorAutoLock lock(mGraphImpl->GetMonitor());
-  EnsureNextIterationLocked();
-}
-
-void GraphDriver::EnsureNextIterationLocked()
-{
-  mGraphImpl->GetMonitor().AssertCurrentThreadOwns();
-
-  if (IsWaitingIndefinitly()) {
-    WakeUp();
-  }
-
-  if (mNeedAnotherIteration) {
-    return;
-  }
-  mNeedAnotherIteration = true;
+  mGraphImpl->EnsureNextIteration();
 }
 
 class MediaStreamGraphShutdownThreadRunnable : public nsRunnable {
@@ -361,7 +346,7 @@ SystemClockDriver::WaitForNextIteration()
 
   PRIntervalTime timeout = PR_INTERVAL_NO_TIMEOUT;
   TimeStamp now = TimeStamp::Now();
-  if (mNeedAnotherIteration) {
+  if (mGraphImpl->mNeedAnotherIteration) {
     int64_t timeoutMS = MEDIA_GRAPH_TARGET_PERIOD_MS -
       int64_t((now - mCurrentTimeStamp).ToMilliseconds());
     // Make sure timeoutMS doesn't overflow 32 bits by waking up at
@@ -369,8 +354,12 @@ SystemClockDriver::WaitForNextIteration()
     timeoutMS = std::max<int64_t>(0, std::min<int64_t>(timeoutMS, 60*1000));
     timeout = PR_MillisecondsToInterval(uint32_t(timeoutMS));
     STREAM_LOG(PR_LOG_DEBUG+1, ("Waiting for next iteration; at %f, timeout=%f", (now - mInitialTimeStamp).ToSeconds(), timeoutMS/1000.0));
+    if (mWaitState == WAITSTATE_WAITING_INDEFINITELY) {
+      mGraphImpl->mGraphDriverAsleep = false; // atomic
+    }
     mWaitState = WAITSTATE_WAITING_FOR_NEXT_ITERATION;
   } else {
+    mGraphImpl->mGraphDriverAsleep = true; // atomic
     mWaitState = WAITSTATE_WAITING_INDEFINITELY;
   }
   if (timeout > 0) {
@@ -380,8 +369,11 @@ SystemClockDriver::WaitForNextIteration()
           (TimeStamp::Now() - now).ToSeconds()));
   }
 
+  if (mWaitState == WAITSTATE_WAITING_INDEFINITELY) {
+    mGraphImpl->mGraphDriverAsleep = false; // atomic
+  }
   mWaitState = WAITSTATE_RUNNING;
-  mNeedAnotherIteration = false;
+  mGraphImpl->mNeedAnotherIteration = false;
 }
 
 void
@@ -389,6 +381,7 @@ SystemClockDriver::WakeUp()
 {
   mGraphImpl->GetMonitor().AssertCurrentThreadOwns();
   mWaitState = WAITSTATE_WAKING_UP;
+  mGraphImpl->mGraphDriverAsleep = false; // atomic
   mGraphImpl->GetMonitor().Notify();
 }
 
@@ -510,12 +503,14 @@ AsyncCubebTask::Run()
         LIFECYCLE_LOG("AsyncCubebOperation::SLEEP\n");
         MonitorAutoLock mon(mDriver->mGraphImpl->GetMonitor());
         // We might just have been awoken
-        if (mDriver->mNeedAnotherIteration) {
+        if (mDriver->mGraphImpl->mNeedAnotherIteration) {
           mDriver->mPauseRequested = false;
           mDriver->mWaitState = AudioCallbackDriver::WAITSTATE_RUNNING;
+          mDriver->mGraphImpl->mGraphDriverAsleep = false	; // atomic
           break;
         }
         mDriver->Stop();
+        mDriver->mGraphImpl->mGraphDriverAsleep = true; // atomic
         mDriver->mWaitState = AudioCallbackDriver::WAITSTATE_WAITING_INDEFINITELY;
         mDriver->mPauseRequested = false;
         mDriver->mGraphImpl->GetMonitor().Wait(PR_INTERVAL_NO_TIMEOUT);
@@ -718,7 +713,7 @@ void AudioCallbackDriver::WaitForNextIteration()
   // We can't block on the monitor in the audio callback, so we kick off a new
   // thread that will pause the audio stream, and restart it when unblocked.
   // We don't want to sleep when we haven't started the driver yet.
-  if (!mNeedAnotherIteration && mAudioStream && mGraphImpl->Running()) {
+  if (!mGraphImpl->mNeedAnotherIteration && mAudioStream && mGraphImpl->Running()) {
     STREAM_LOG(PR_LOG_DEBUG+1, ("AudioCallbackDriver going to sleep"));
     mPauseRequested = true;
     nsRefPtr<AsyncCubebTask> sleepEvent =

content/media/GraphDriver.h

@@ -228,8 +228,6 @@ protected:
   };
   WaitState mWaitState;
 
-  // True if the graph needs another iteration after the current iteration.
-  bool mNeedAnotherIteration;
   TimeStamp mCurrentTimeStamp;
   // This is non-null only when this driver has recently switched from an other
   // driver, and has not cleaned it up yet (for example because the audio stream

content/media/MediaStreamGraph.cpp

@@ -95,7 +95,7 @@ MediaStreamGraphImpl::FinishStream(MediaStream* aStream)
   // Force at least one more iteration of the control loop, since we rely
   // on UpdateCurrentTimeForStreams to notify our listeners once the stream end
   // has been reached.
-  CurrentDriver()->EnsureNextIteration();
+  EnsureNextIteration();
 
   SetStreamOrderDirty();
 }
@@ -778,7 +778,7 @@ MediaStreamGraphImpl::RecomputeBlocking(GraphTime aEndBlockingDecisions)
 
   if (blockingDecisionsWillChange) {
     // Make sure we wake up to notify listeners about these changes.
-    CurrentDriver()->EnsureNextIteration();
+    EnsureNextIteration();
   }
 }
 
@@ -1288,7 +1288,7 @@ MediaStreamGraphImpl::UpdateGraph(GraphTime aEndBlockingDecision)
   // computed in next loop.
   if (ensureNextIteration ||
       aEndBlockingDecision == CurrentDriver()->StateComputedTime()) {
-    CurrentDriver()->EnsureNextIteration();
+    EnsureNextIteration();
   }
 
   // Figure out which streams are blocked and when.
@@ -1382,7 +1382,7 @@ MediaStreamGraphImpl::Process(GraphTime aFrom, GraphTime aTo)
   }
 
   if (!allBlockedForever) {
-    CurrentDriver()->EnsureNextIteration();
+    EnsureNextIteration();
   }
 }
 
@@ -1468,7 +1468,7 @@ MediaStreamGraphImpl::ForceShutDown()
   {
     MonitorAutoLock lock(mMonitor);
     mForceShutDown = true;
-    CurrentDriver()->EnsureNextIterationLocked();
+    EnsureNextIterationLocked();
   }
 }
 
@@ -1644,7 +1644,7 @@ MediaStreamGraphImpl::RunInStableState(bool aSourceIsMSG)
         block->mMessages.SwapElements(mCurrentTaskMessageQueue);
         block->mGraphUpdateIndex = mNextGraphUpdateIndex;
         ++mNextGraphUpdateIndex;
-        CurrentDriver()->EnsureNextIterationLocked();
+        EnsureNextIterationLocked();
       }
 
       // If the MediaStreamGraph has more messages going to it, try to revive
@@ -2712,6 +2712,8 @@ MediaStreamGraphImpl::MediaStreamGraphImpl(bool aRealtime,
                                            dom::AudioChannel aChannel)
   : mProcessingGraphUpdateIndex(0)
   , mPortCount(0)
+  , mNeedAnotherIteration(false)
+  , mGraphDriverAsleep(false)
   , mMonitor("MediaStreamGraphImpl")
   , mLifecycleState(LIFECYCLE_THREAD_NOT_STARTED)
   , mEndTime(GRAPH_TIME_MAX)

content/media/MediaStreamGraphImpl.h

@@ -455,14 +455,19 @@ public:
     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();
+    mNeedAnotherIteration = true; // atomic
+    if (mGraphDriverAsleep) { // atomic
+      MonitorAutoLock mon(mMonitor);
+      CurrentDriver()->WakeUp(); // Might not be the same driver; might have woken already
+    }
+  }
+
+  void EnsureNextIterationLocked() {
+    mNeedAnotherIteration = true; // atomic
+    if (mGraphDriverAsleep) { // atomic
+      CurrentDriver()->WakeUp(); // Might not be the same driver; might have woken already
+    }
   }
 
   // Data members
@@ -504,6 +509,11 @@ public:
    */
   int32_t mPortCount;
 
+  // True if the graph needs another iteration after the current iteration.
+  Atomic<bool> mNeedAnotherIteration;
+  // GraphDriver may need a WakeUp() if something changes
+  Atomic<bool> mGraphDriverAsleep;
+
   // mMonitor guards the data below.
   // MediaStreamGraph normally does its work without holding mMonitor, so it is
   // not safe to just grab mMonitor from some thread and start monkeying with
@@ -512,7 +522,7 @@ public:
   Monitor mMonitor;
 
   // Data guarded by mMonitor (must always be accessed with mMonitor held,
-  // regardless of the value of mLifecycleState.
+  // regardless of the value of mLifecycleState).
 
   /**
    * State to copy to main thread
@@ -592,10 +602,6 @@ public:
    * at construction.
    */
   TrackRate mSampleRate;
-  /**
-   * True when another iteration of the control loop is required.
-   */
-  bool mNeedAnotherIteration;
   /**
    * True when we need to do a forced shutdown during application shutdown.
    */