bug 1406831 don't tolerate just owning the monitor if AssertOnGraphThreadOrNotRunning() is not called on the correct thread r?pehrsons draft
authorKarl Tomlinson <karlt+@karlt.net>
Thu, 28 Sep 2017 15:30:48 +1300
changeset 676612 f2e3eda785cb52413686a1cb16b30c3f01784716
parent 676611 0ebde4ac3da80fea901f379605200c05c31a7ab0
child 676613 cbc3e798bd8d3f5a898e06f8d6666bf96629e4c7
push id83545
push userktomlinson@mozilla.com
push dateMon, 09 Oct 2017 04:51:14 +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;
   }