Bug 1072775: Additional assertions for MediaStreamGraph/GraphDriver r=roc

content/media/GraphDriver.cpp

@@ -260,6 +260,7 @@ ThreadedDriver::Stop()
 
   if (mThread) {
     mThread->Shutdown();
+    mThread = nullptr;
   }
 }
 
@@ -400,19 +401,25 @@ public:
   NS_IMETHOD Run()
   {
     MOZ_ASSERT(NS_IsMainThread());
+    MOZ_ASSERT(mThread);
+
     mThread->Shutdown();
+    mThread = nullptr;
     return NS_OK;
   }
 private:
-  nsRefPtr<nsIThread> mThread;
+  nsCOMPtr<nsIThread> mThread;
 };
 
 OfflineClockDriver::~OfflineClockDriver()
 {
   // transfer the ownership of mThread to the event
+  // XXX should use .forget()/etc
+  if (mThread) {
     nsCOMPtr<nsIRunnable> event = new MediaStreamGraphShutdownThreadRunnable2(mThread);
     mThread = nullptr;
     NS_DispatchToMainThread(event);
+  }
 }
 
 void
@@ -741,19 +748,16 @@ AudioCallbackDriver::DeviceChangedCallback_s(void* aUser)
 }
 
 bool AudioCallbackDriver::InCallback() {
-  MonitorAutoLock mon(mGraphImpl->GetMonitor());
   return mInCallback;
 }
 
 AudioCallbackDriver::AutoInCallback::AutoInCallback(AudioCallbackDriver* aDriver)
   : mDriver(aDriver)
 {
-  MonitorAutoLock mon(mDriver->mGraphImpl->GetMonitor());
   mDriver->mInCallback = true;
 }
 
 AudioCallbackDriver::AutoInCallback::~AutoInCallback() {
-  MonitorAutoLock mon(mDriver->mGraphImpl->GetMonitor());
   mDriver->mInCallback = false;
 }
 

content/media/GraphDriver.h

@@ -11,6 +11,7 @@
 #include "AudioBufferUtils.h"
 #include "AudioMixer.h"
 #include "AudioSegment.h"
+#include "mozilla/Atomics.h"
 
 struct cubeb_stream;
 
@@ -198,6 +199,8 @@ public:
     return mGraphImpl;
   }
 
+  virtual bool OnThread() = 0;
+
 protected:
   // Time of the start of this graph iteration.
   GraphTime mIterationStart;
@@ -262,6 +265,9 @@ public:
   uint32_t IterationDuration() {
     return MEDIA_GRAPH_TARGET_PERIOD_MS;
   }
+
+  virtual bool OnThread() MOZ_OVERRIDE { return !mThread || NS_GetCurrentThread() == mThread; }
+
 protected:
   nsCOMPtr<nsIThread> mThread;
 };
@@ -384,6 +390,8 @@ public:
    */
   bool InCallback();
 
+  virtual bool OnThread() MOZ_OVERRIDE { return !mStarted || InCallback(); }
+
   /* Whether the underlying cubeb stream has been started. See comment for
    * mStarted for details. */
   bool IsStarted();
@@ -449,8 +457,7 @@ private:
    * shutdown of the audio stream. */
   nsCOMPtr<nsIThread> mInitShutdownThread;
   dom::AudioChannel mAudioChannel;
-  /* This can only be accessed with the graph's monitor held. */
-  bool mInCallback;
+  Atomic<bool> mInCallback;
   /* A thread has been created to be able to pause and restart the audio thread,
    * but has not done so yet. This indicates that the callback should return
    * early */

content/media/MediaStreamGraph.cpp

@@ -1486,11 +1486,18 @@ public:
 
     LIFECYCLE_LOG("Shutting down graph %p", mGraph.get());
 
-    if (mGraph->CurrentDriver()->AsAudioCallbackDriver()) {
-      MOZ_ASSERT(!mGraph->CurrentDriver()->AsAudioCallbackDriver()->InCallback());
+    // We've asserted the graph isn't running.  Use mDriver instead of CurrentDriver
+    // to avoid thread-safety checks
+#if 0 // AudioCallbackDrivers are released asynchronously anyways
+    // XXX a better test would be have setting mDetectedNotRunning make sure
+    // any current callback has finished and block future ones -- or just
+    // handle it all in Shutdown()!
+    if (mGraph->mDriver->AsAudioCallbackDriver()) {
+      MOZ_ASSERT(!mGraph->mDriver->AsAudioCallbackDriver()->InCallback());
     }
+#endif
 
-    mGraph->CurrentDriver()->Shutdown();
+    mGraph->mDriver->Shutdown();
 
     // mGraph's thread is not running so it's OK to do whatever here
     if (mGraph->IsEmpty()) {
@@ -1720,6 +1727,7 @@ MediaStreamGraphImpl::RunInStableState(bool aSourceIsMSG)
 #endif
 }
 
+
 static NS_DEFINE_CID(kAppShellCID, NS_APPSHELL_CID);
 
 void

content/media/MediaStreamGraphImpl.h

@@ -150,6 +150,12 @@ public:
   void Init();
   // The following methods run on the graph thread (or possibly the main thread if
   // mLifecycleState > LIFECYCLE_RUNNING)
+  void AssertOnGraphThreadOrNotRunning() {
+    // either we're on the right thread (and calling CurrentDriver() is safe),
+    // or we're going to assert anyways, so don't cross-check CurrentDriver
+    MOZ_ASSERT(mDriver->OnThread() ||
+               (mLifecycleState > LIFECYCLE_RUNNING && NS_IsMainThread()));
+  }
   /*
    * This does the actual iteration: Message processing, MediaStream ordering,
    * blocking computation and processing.
@@ -418,6 +424,12 @@ public:
    * 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;
   }
 
@@ -428,6 +440,7 @@ public:
    * should return and pass the control to the new driver shortly after.
    */
   void SetCurrentDriver(GraphDriver* aDriver) {
+    MOZ_ASSERT(mDriver->OnThread());
     mDriver = aDriver;
   }