Bug 1298698: Block race between EnsureNextIteration and WaitForNextIteration r=karlt a=ritu
authorRandell Jesup <rjesup@jesup.org>
Mon, 29 Aug 2016 10:41:01 -0400
changeset 350083 d35ac86c5271f6ad22bf1f76b1bc8adb0398e7bd
parent 350082 3cb50e5df49728e19c6f5bcb4c5b3fe34a9db53c
child 350084 a4ae1930e7b24b026385064a78bb3b5891416468
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskarlt, ritu
bugs1298698
milestone50.0a2
Bug 1298698: Block race between EnsureNextIteration and WaitForNextIteration r=karlt a=ritu
dom/media/GraphDriver.cpp
dom/media/MediaStreamGraphImpl.h
--- a/dom/media/GraphDriver.cpp
+++ b/dom/media/GraphDriver.cpp
@@ -393,49 +393,65 @@ OfflineClockDriver::GetCurrentTimeStamp(
 
 void
 SystemClockDriver::WaitForNextIteration()
 {
   mGraphImpl->GetMonitor().AssertCurrentThreadOwns();
 
   PRIntervalTime timeout = PR_INTERVAL_NO_TIMEOUT;
   TimeStamp now = TimeStamp::Now();
-  if (mGraphImpl->mNeedAnotherIteration) {
+
+  // This lets us avoid hitting the Atomic twice when we know we won't sleep
+  bool another = mGraphImpl->mNeedAnotherIteration; // atomic
+  if (!another) {
+    mGraphImpl->mGraphDriverAsleep = true; // atomic
+    mWaitState = WAITSTATE_WAITING_INDEFINITELY;
+  }
+  // NOTE: mNeedAnotherIteration while also atomic may have changed before
+  // we could set mGraphDriverAsleep, so we must re-test it.
+  // (EnsureNextIteration sets mNeedAnotherIteration, then tests
+  // mGraphDriverAsleep
+  if (another || mGraphImpl->mNeedAnotherIteration) { // atomic
     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
     // least once a minute, if we need to wake up at all
     timeoutMS = std::max<int64_t>(0, std::min<int64_t>(timeoutMS, 60*1000));
     timeout = PR_MillisecondsToInterval(uint32_t(timeoutMS));
-    STREAM_LOG(LogLevel::Verbose, ("Waiting for next iteration; at %f, timeout=%f", (now - mInitialTimeStamp).ToSeconds(), timeoutMS/1000.0));
+    STREAM_LOG(LogLevel::Verbose,
+               ("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) {
     mGraphImpl->GetMonitor().Wait(timeout);
     STREAM_LOG(LogLevel::Verbose, ("Resuming after timeout; at %f, elapsed=%f",
           (TimeStamp::Now() - mInitialTimeStamp).ToSeconds(),
           (TimeStamp::Now() - now).ToSeconds()));
   }
 
   if (mWaitState == WAITSTATE_WAITING_INDEFINITELY) {
     mGraphImpl->mGraphDriverAsleep = false; // atomic
   }
+  // Note: this can race against the EnsureNextIteration setting
+  // WAITSTATE_RUNNING and setting mGraphDriverAsleep to false, so you can
+  // have an iteration with WAITSTATE_WAKING_UP instead of RUNNING.
   mWaitState = WAITSTATE_RUNNING;
-  mGraphImpl->mNeedAnotherIteration = false;
+  mGraphImpl->mNeedAnotherIteration = false; // atomic
 }
 
 void SystemClockDriver::WakeUp()
 {
   mGraphImpl->GetMonitor().AssertCurrentThreadOwns();
+  // Note: this can race against the thread setting WAITSTATE_RUNNING and
+  // setting mGraphDriverAsleep to false, so you can have an iteration
+  // with WAITSTATE_WAKING_UP instead of RUNNING.
   mWaitState = WAITSTATE_WAKING_UP;
   mGraphImpl->mGraphDriverAsleep = false; // atomic
   mGraphImpl->GetMonitor().Notify();
 }
 
 OfflineClockDriver::OfflineClockDriver(MediaStreamGraphImpl* aGraphImpl, GraphTime aSlice)
   : ThreadedDriver(aGraphImpl),
     mSlice(aSlice)
--- a/dom/media/MediaStreamGraphImpl.h
+++ b/dom/media/MediaStreamGraphImpl.h
@@ -505,16 +505,18 @@ public:
   Monitor& GetMonitor()
   {
     return mMonitor;
   }
 
   void EnsureNextIteration()
   {
     mNeedAnotherIteration = true; // atomic
+    // Note: GraphDriver must ensure that there's no race on setting
+    // mNeedAnotherIteration and mGraphDriverAsleep -- see WaitForNextIteration()
     if (mGraphDriverAsleep) { // atomic
       MonitorAutoLock mon(mMonitor);
       CurrentDriver()->WakeUp(); // Might not be the same driver; might have woken already
     }
   }
 
   void EnsureNextIterationLocked()
   {