bug 1406831 don't tolerate just owning the monitor if AssertOnGraphThreadOrNotRunning() is not called on the correct thread r=pehrsons
authorKarl Tomlinson <karlt+@karlt.net>
Thu, 28 Sep 2017 15:30:48 +1300
changeset 428006 e3f39de40209900202f62dc52e68921156d9d0bb
parent 428005 f534d96564e0641b61e75411099afb60604d24f2
child 428007 0152a50835c91ddefcc6072044e316999556a866
push id97
push userfmarier@mozilla.com
push dateSat, 14 Oct 2017 01:12:59 +0000
reviewerspehrsons
bugs1406831
milestone58.0a1
bug 1406831 don't tolerate just owning the monitor if AssertOnGraphThreadOrNotRunning() is not called on the correct thread r=pehrsons Owning the monitor is not sufficient for consistent state if state can be accessed without the monitor. The requirements for SetCurrentDriver() are tighted to require both the monitor and correct thread, as CurrentDriver() can be called either with the monitor or on the graph thread. MozReview-Commit-ID: 90q7Pfa8jxn
dom/media/MediaStreamGraph.cpp
dom/media/MediaStreamGraph.h
dom/media/MediaStreamGraphImpl.h
--- a/dom/media/MediaStreamGraph.cpp
+++ b/dom/media/MediaStreamGraph.cpp
@@ -1110,31 +1110,27 @@ void
 MediaStreamGraph::NotifyOutputData(AudioDataValue* aBuffer, size_t aFrames,
                                    TrackRate aRate, uint32_t aChannels)
 {
   for (auto& listener : mAudioInputs) {
     listener->NotifyOutputData(this, aBuffer, aFrames, aRate, aChannels);
   }
 }
 
-void
-MediaStreamGraph::AssertOnGraphThreadOrNotRunning() const
+bool
+MediaStreamGraph::OnGraphThreadOrNotRunning() const
 {
   // 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
-#ifdef DEBUG
+  // or we're going to fail the assert anyway, so don't cross-check
+  // via CurrentDriver().
   MediaStreamGraphImpl const * graph =
     static_cast<MediaStreamGraphImpl const *>(this);
   // if all the safety checks fail, assert we own the monitor
-  if (!(graph->mDetectedNotRunning ?
-        NS_IsMainThread() : graph->mDriver->OnThread()))
-  {
-      graph->mMonitor.AssertCurrentThreadOwns();
-  }
-#endif
+  return graph->mDetectedNotRunning ?
+    NS_IsMainThread() : graph->mDriver->OnThread();
 }
 
 bool
 MediaStreamGraphImpl::ShouldUpdateMainThread()
 {
   if (mRealtime) {
     return true;
   }
--- a/dom/media/MediaStreamGraph.h
+++ b/dom/media/MediaStreamGraph.h
@@ -1362,29 +1362,36 @@ public:
 
   /**
    * Data going to the speakers from the GraphDriver's DataCallback
    * to notify any listeners (for echo cancellation).
    */
   void NotifyOutputData(AudioDataValue* aBuffer, size_t aFrames,
                         TrackRate aRate, uint32_t aChannels);
 
-  void AssertOnGraphThreadOrNotRunning() const;
+  void AssertOnGraphThreadOrNotRunning() const
+  {
+    MOZ_ASSERT(OnGraphThreadOrNotRunning());
+  }
 
 protected:
   explicit MediaStreamGraph(TrackRate aSampleRate)
     : mSampleRate(aSampleRate)
   {
     MOZ_COUNT_CTOR(MediaStreamGraph);
   }
   virtual ~MediaStreamGraph()
   {
     MOZ_COUNT_DTOR(MediaStreamGraph);
   }
 
+  // Intended only for assertions, either on graph thread, not running, or
+  // with monitor held.
+  bool OnGraphThreadOrNotRunning() const;
+
   // Media graph thread only
   nsTArray<nsCOMPtr<nsIRunnable> > mPendingUpdateRunnables;
 
   /**
    * Sample rate at which this graph runs. For real time graphs, this is
    * the rate of the audio mixer. For offline graphs, this is the rate specified
    * at construction.
    */
--- a/dom/media/MediaStreamGraphImpl.h
+++ b/dom/media/MediaStreamGraphImpl.h
@@ -451,17 +451,21 @@ public:
   void PausedIndefinitly();
   void ResumedFromPaused();
 
   /**
    * Not safe to call off the MediaStreamGraph thread unless monitor is held!
    */
   GraphDriver* CurrentDriver() const
   {
-    AssertOnGraphThreadOrNotRunning();
+#ifdef DEBUG
+    if (!OnGraphThreadOrNotRunning()) {
+      mMonitor.AssertCurrentThreadOwns();
+    }
+#endif
     return mDriver;
   }
 
   bool RemoveMixerCallback(MixerCallbackReceiver* aReceiver)
   {
     return mMixer.RemoveCallback(aReceiver);
   }
 
@@ -470,17 +474,20 @@ public:
    * 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
    * should return and pass the control to the new driver shortly after.
    * We can also switch from Revive() (on MainThread), in which case the
    * monitor is held
    */
   void SetCurrentDriver(GraphDriver* aDriver)
   {
+#ifdef DEBUG
+    mMonitor.AssertCurrentThreadOwns();
     AssertOnGraphThreadOrNotRunning();
+#endif
     mDriver = aDriver;
   }
 
   Monitor& GetMonitor()
   {
     return mMonitor;
   }