bug 1406830 store the AsyncCubeTask SharedThreadPool reference on the AudioCallbackDriver r?padenot draft
authorKarl Tomlinson <karlt+@karlt.net>
Tue, 26 Sep 2017 17:28:17 +1300
changeset 676616 787070b71ee1aad3f668fa0064262a8efef987de
parent 676615 8fdb05445cba470ab46474d0b4ac9e1a96ac3d04
child 734982 3e86dd8b40eb70271b742e49a6fc093fdc120ff2
push id83546
push userktomlinson@mozilla.com
push dateMon, 09 Oct 2017 04:58:54 +0000
reviewerspadenot
bugs1406830
milestone58.0a1
bug 1406830 store the AsyncCubeTask SharedThreadPool reference on the AudioCallbackDriver r?padenot The first AsyncCubebTask dispatch from AudioCallbackDriver::Start() may either be from MediaStreamGraphImpl::RunInStableState() on the main thread or ThreadedDriver::RunThread() on a threaded driver thread. These could potentially occur concurrently when there are multiple MediaStreamGraphs. This change removes the race around setting sThreadPool. SharedThreadPool::Get() would have returned the same pointer, and so that race was probably mostly benign apart from the potential to add an extra reference and so hang on shutdown in SharedThreadPool::SpinUntilEmpty(). Storing the reference to the SharedThreadPool on the object using it is the typical way to use SharedThreadPool. It lets the thread pool be released when not in use, and lets SharedThreadPool deal with multi-thread access and shutdown. MozReview-Commit-ID: 8WutVsAMfJo
dom/media/GraphDriver.cpp
dom/media/GraphDriver.h
dom/media/MediaStreamGraph.cpp
--- a/dom/media/GraphDriver.cpp
+++ b/dom/media/GraphDriver.cpp
@@ -21,18 +21,16 @@
 extern mozilla::LazyLogModule gMediaStreamGraphLog;
 #ifdef LOG
 #undef LOG
 #endif // LOG
 #define LOG(type, msg) MOZ_LOG(gMediaStreamGraphLog, type, msg)
 
 namespace mozilla {
 
-StaticRefPtr<nsIThreadPool> AsyncCubebTask::sThreadPool;
-
 GraphDriver::GraphDriver(MediaStreamGraphImpl* aGraphImpl)
   : mIterationStart(0),
     mIterationEnd(0),
     mGraphImpl(aGraphImpl),
     mWaitState(WAITSTATE_RUNNING),
     mCurrentTimeStamp(TimeStamp::Now()),
     mPreviousDriver(nullptr),
     mNextDriver(nullptr),
@@ -466,46 +464,30 @@ AsyncCubebTask::AsyncCubebTask(AudioCall
   NS_WARNING_ASSERTION(mDriver->mAudioStream || aOperation == INIT,
                        "No audio stream!");
 }
 
 AsyncCubebTask::~AsyncCubebTask()
 {
 }
 
-/* static */
-nsresult
-AsyncCubebTask::EnsureThread()
+SharedThreadPool*
+AudioCallbackDriver::GetInitShutdownThread()
 {
-  if (!sThreadPool) {
-    nsCOMPtr<nsIThreadPool> threadPool =
+  if (!mInitShutdownThread) {
+    mInitShutdownThread =
       SharedThreadPool::Get(NS_LITERAL_CSTRING("CubebOperation"), 1);
-    sThreadPool = threadPool;
-    // Need to null this out before xpcom-shutdown-threads Observers run
-    // since we don't know the order that the shutdown-threads observers
-    // will run.  ClearOnShutdown guarantees it runs first.
-    if (!NS_IsMainThread()) {
-      nsCOMPtr<nsIRunnable> runnable =
-        NS_NewRunnableFunction("AsyncCubebTask::EnsureThread", []() -> void {
-          ClearOnShutdown(&sThreadPool, ShutdownPhase::ShutdownThreads);
-        });
-      AbstractThread::MainThread()->Dispatch(runnable.forget());
-    } else {
-      ClearOnShutdown(&sThreadPool, ShutdownPhase::ShutdownThreads);
-    }
 
     const uint32_t kIdleThreadTimeoutMs = 2000;
 
-    nsresult rv = sThreadPool->SetIdleThreadTimeout(PR_MillisecondsToInterval(kIdleThreadTimeoutMs));
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
+    mInitShutdownThread->
+      SetIdleThreadTimeout(PR_MillisecondsToInterval(kIdleThreadTimeoutMs));
   }
 
-  return NS_OK;
+  return mInitShutdownThread;
 }
 
 NS_IMETHODIMP
 AsyncCubebTask::Run()
 {
   MOZ_ASSERT(mDriver);
 
   switch(mOperation) {
--- a/dom/media/GraphDriver.h
+++ b/dom/media/GraphDriver.h
@@ -468,16 +468,21 @@ public:
    * mStarted for details. */
   bool IsStarted();
 
   /* Tell the driver whether this process is using a microphone or not. This is
    * thread safe. */
   void SetMicrophoneActive(bool aActive);
 
   void CompleteAudioContextOperations(AsyncCubebOperation aOperation);
+
+  /* Fetch, or create a shared thread pool with up to one thread for
+   * AsyncCubebTask. */
+  SharedThreadPool* GetInitShutdownThread();
+
 private:
   /**
    * On certain MacBookPro, the microphone is located near the left speaker.
    * We need to pan the sound output to the right speaker if we are using the
    * mic and the built-in speaker, or we will have terrible echo.  */
   void PanOutputIfNeeded(bool aMicrophoneActive);
   /**
    * This is called when the output device used by the cubeb stream changes. */
@@ -533,19 +538,19 @@ private:
 
   struct AutoInCallback
   {
     explicit AutoInCallback(AudioCallbackDriver* aDriver);
     ~AutoInCallback();
     AudioCallbackDriver* mDriver;
   };
 
-  /* Thread for off-main-thread initialization and
-   * shutdown of the audio stream. */
-  nsCOMPtr<nsIThread> mInitShutdownThread;
+  /* Shared thread pool with up to one thread for off-main-thread
+   * initialization and shutdown of the audio stream via AsyncCubebTask. */
+  RefPtr<SharedThreadPool> mInitShutdownThread;
   /* This must be accessed with the graph monitor held. */
   AutoTArray<StreamAndPromiseForOperation, 1> mPromisesForOperation;
   /* Used to queue us to add the mixer callback on first run. */
   bool mAddedMixer;
 
   /* This is atomic and is set by the audio callback thread. It can be read by
    * any thread safely. */
   Atomic<bool> mInCallback;
@@ -561,31 +566,29 @@ private:
 class AsyncCubebTask : public Runnable
 {
 public:
 
   AsyncCubebTask(AudioCallbackDriver* aDriver, AsyncCubebOperation aOperation);
 
   nsresult Dispatch(uint32_t aFlags = NS_DISPATCH_NORMAL)
   {
-    nsresult rv = EnsureThread();
-    if (!NS_FAILED(rv)) {
-      rv = sThreadPool->Dispatch(this, aFlags);
+    SharedThreadPool* threadPool = mDriver->GetInitShutdownThread();
+    if (!threadPool) {
+      return NS_ERROR_FAILURE;
     }
-    return rv;
+    return threadPool->Dispatch(this, aFlags);
   }
 
 protected:
   virtual ~AsyncCubebTask();
 
 private:
-  static nsresult EnsureThread();
+  NS_IMETHOD Run() override final;
 
-  NS_IMETHOD Run() override final;
-  static StaticRefPtr<nsIThreadPool> sThreadPool;
   RefPtr<AudioCallbackDriver> mDriver;
   AsyncCubebOperation mOperation;
   RefPtr<MediaStreamGraphImpl> mShutdownGrip;
 };
 
 } // namespace mozilla
 
 #endif // GRAPHDRIVER_H_
--- a/dom/media/MediaStreamGraph.cpp
+++ b/dom/media/MediaStreamGraph.cpp
@@ -1558,16 +1558,25 @@ public:
     if (mGraph->mDriver->AsAudioCallbackDriver()) {
       MOZ_ASSERT(!mGraph->mDriver->AsAudioCallbackDriver()->InCallback());
     }
 #endif
 
     mGraph->mDriver->Shutdown(); // This will wait until it's shutdown since
                                  // we'll start tearing down the graph after this
 
+    // Release the driver now so that an AudioCallbackDriver will release its
+    // SharedThreadPool reference.  Each SharedThreadPool reference must be
+    // released before SharedThreadPool::SpinUntilEmpty() runs on
+    // xpcom-shutdown-threads.  Don't wait for GC/CC to release references to
+    // objects owning streams, or for expiration of mGraph->mShutdownTimer,
+    // which won't otherwise release its reference on the graph until
+    // nsTimerImpl::Shutdown(), which runs after xpcom-shutdown-threads.
+    mGraph->mDriver = nullptr;
+
     // Safe to access these without the monitor since the graph isn't running.
     // We may be one of several graphs. Drop ticket to eventually unblock shutdown.
     if (mGraph->mShutdownTimer && !mGraph->mForceShutdownTicket) {
       MOZ_ASSERT(false,
         "AudioCallbackDriver took too long to shut down and we let shutdown"
         " continue - freezing and leaking");
 
       // The timer fired, so we may be deeper in shutdown now.  Block any further