Bug 1164463 - Clean up MediaManager shutdown to be reliable and avoid holding locks while Joining a thread. r=jib

dom/media/MediaManager.cpp

@@ -124,6 +124,8 @@ using dom::OwningBooleanOrMediaTrackConstraints;
 using dom::SupportedAudioConstraints;
 using dom::SupportedVideoConstraints;
 
+static Atomic<bool> sInShutdown;
+
 static bool
 HostInDomain(const nsCString &aHost, const nsCString &aPattern)
 {
@@ -946,7 +948,7 @@ public:
     nsRefPtr<nsDOMUserMediaStream> trackunion =
       nsDOMUserMediaStream::CreateTrackUnionStream(window, mListener,
                                                    mAudioSource, mVideoSource);
-    if (!trackunion) {
+    if (!trackunion || sInShutdown) {
       nsCOMPtr<nsIDOMGetUserMediaErrorCallback> onFailure = mOnFailure.forget();
       LOG(("Returning error for getUserMedia() - no stream"));
 
@@ -954,7 +956,8 @@ public:
       if (window) {
         nsRefPtr<MediaStreamError> error = new MediaStreamError(window,
             NS_LITERAL_STRING("InternalError"),
-            NS_LITERAL_STRING("No stream."));
+            sInShutdown ? NS_LITERAL_STRING("In shutdown") :
+                          NS_LITERAL_STRING("No stream."));
         onFailure->OnError(error);
       }
       return NS_OK;
@@ -1009,7 +1012,7 @@ public:
     // because that can take a while.
     // Pass ownership of trackunion to the MediaOperationTask
     // to ensure it's kept alive until the MediaOperationTask runs (at least).
-    MediaManager::GetMessageLoop()->PostTask(FROM_HERE,
+    MediaManager::PostTask(FROM_HERE,
       new MediaOperationTask(MEDIA_START, mListener, trackunion,
                              tracksAvailableCallback,
                              mAudioSource, mVideoSource, false, mWindowID,
@@ -1612,7 +1615,7 @@ MediaManager::Get() {
 
     nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
     if (obs) {
-      obs->AddObserver(sSingleton, "xpcom-shutdown", false);
+      obs->AddObserver(sSingleton, "xpcom-will-shutdown", false);
       obs->AddObserver(sSingleton, "getUserMedia:response:allow", false);
       obs->AddObserver(sSingleton, "getUserMedia:response:deny", false);
       obs->AddObserver(sSingleton, "getUserMedia:revoke", false);
@@ -1644,12 +1647,17 @@ MediaManager::GetInstance()
 }
 
 /* static */
-MessageLoop*
-MediaManager::GetMessageLoop()
+void
+MediaManager::PostTask(const tracked_objects::Location& from_here, Task* task)
 {
+  if (sInShutdown) {
+    // Can't safely delete task here since it may have items with specific
+    // thread-release requirements.
+    return;
+  }
   NS_ASSERTION(Get(), "MediaManager singleton?");
   NS_ASSERTION(Get()->mMediaThread, "No thread yet");
-  return Get()->mMediaThread->message_loop();
+  Get()->mMediaThread->message_loop()->PostTask(from_here, task);
 }
 
 /* static */ nsresult
@@ -1783,6 +1791,9 @@ MediaManager::GetUserMedia(
     task = new GetUserMediaTask(c, onSuccess.forget(),
       onFailure.forget(), windowID, listener, mPrefs);
   }
+  if (sInShutdown) {
+    return task->Denied(NS_LITERAL_STRING("In shutdown"));
+  }
 
   nsIURI* docURI = aWindow->GetDocumentURI();
 
@@ -1889,7 +1900,7 @@ MediaManager::GetUserMedia(
   // XXX No full support for picture in Desktop yet (needs proper UI)
   if (privileged ||
       (fake && !Preferences::GetBool("media.navigator.permission.fake"))) {
-    MediaManager::GetMessageLoop()->PostTask(FROM_HERE, task.forget());
+    MediaManager::PostTask(FROM_HERE, task.forget());
   } else {
     bool isHTTPS = false;
     if (docURI) {
@@ -1974,6 +1985,7 @@ MediaManager::GetUserMediaDevices(nsPIDOMWindow* aWindow,
 
   NS_ENSURE_TRUE(aOnFailure, NS_ERROR_NULL_POINTER);
   NS_ENSURE_TRUE(aOnSuccess, NS_ERROR_NULL_POINTER);
+  NS_ENSURE_TRUE(!sInShutdown, NS_ERROR_FAILURE);
 
   nsCOMPtr<nsIGetUserMediaDevicesSuccessCallback> onSuccess(aOnSuccess);
   nsCOMPtr<nsIDOMGetUserMediaErrorCallback> onFailure(aOnFailure);
@@ -1994,7 +2006,7 @@ MediaManager::GetUserMediaDevices(nsPIDOMWindow* aWindow,
     nsCOMPtr<nsILoadContext> loadContext = doc->GetLoadContext();
     inPrivateBrowsing = loadContext && loadContext->UsePrivateBrowsing();
   }
-  MediaManager::GetMessageLoop()->PostTask(FROM_HERE,
+  MediaManager::PostTask(FROM_HERE,
     new GetUserMediaDevicesTask(
       aConstraints, onSuccess.forget(), onFailure.forget(),
       (aInnerWindowID ? aInnerWindowID : aWindow->WindowID()),
@@ -2027,6 +2039,7 @@ MediaManager::GetBackend(uint64_t aWindowId)
   // This IS called off main-thread.
   MutexAutoLock lock(mMutex);
   if (!mBackend) {
+    MOZ_RELEASE_ASSERT(!sInShutdown);  // we should never create a new backend in shutdown
 #if defined(MOZ_WEBRTC)
     mBackend = new MediaEngineWebRTC(mPrefs);
 #else
@@ -2202,8 +2215,10 @@ MediaManager::Observe(nsISupports* aSubject, const char* aTopic,
       LOG(("%s: %dx%d @%dfps (min %d)", __FUNCTION__,
            mPrefs.mWidth, mPrefs.mHeight, mPrefs.mFPS, mPrefs.mMinFPS));
     }
-  } else if (!strcmp(aTopic, "xpcom-shutdown")) {
-    obs->RemoveObserver(this, "xpcom-shutdown");
+  } else if (!strcmp(aTopic, "xpcom-will-shutdown")) {
+    sInShutdown = true;
+
+    obs->RemoveObserver(this, "xpcom-will-shutdown");
     obs->RemoveObserver(this, "getUserMedia:response:allow");
     obs->RemoveObserver(this, "getUserMedia:response:deny");
     obs->RemoveObserver(this, "getUserMedia:revoke");
@@ -2216,22 +2231,43 @@ MediaManager::Observe(nsISupports* aSubject, const char* aTopic,
       prefs->RemoveObserver("media.navigator.video.default_minfps", this);
     }
 
+    // Close off any remaining active windows.
+    GetActiveWindows()->Clear();
+    mActiveCallbacks.Clear();
+    mCallIds.Clear();
+    {
+      MutexAutoLock lock(mMutex);
+      if (mBackend) {
+        mBackend->Shutdown(); // ok to invoke multiple times
+      }
+    }
+
     // Because mMediaThread is not an nsThread, we must dispatch to it so it can
     // clean up BackgroundChild. Continue stopping thread once this is done.
 
     class ShutdownTask : public Task
     {
     public:
-      explicit ShutdownTask(nsRunnable* aReply) : mReply(aReply) {}
+      ShutdownTask(TemporaryRef<MediaEngine> aBackend,
+                   nsRunnable* aReply)
+        : mReply(aReply)
+        , mBackend(aBackend) {}
     private:
       virtual void
       Run()
       {
+        LOG(("MediaManager Thread Shutdown"));
         MOZ_ASSERT(MediaManager::IsInMediaThread());
         mozilla::ipc::BackgroundChild::CloseForCurrentThread();
-        NS_DispatchToMainThread(mReply);
+        // must explicitly do this before dispatching the reply, since the reply may kill us with Stop()
+        mBackend = nullptr; // last reference, will invoke Shutdown() again
+
+        if (NS_FAILED(NS_DispatchToMainThread(mReply))) {
+          LOG(("Will leak thread: DispatchToMainthread of reply runnable failed in MediaManager shutdown"));
+        }
       }
       nsRefPtr<nsRunnable> mReply;
+      RefPtr<MediaEngine> mBackend;
     };
 
     // Post ShutdownTask to execute on mMediaThread and pass in a lambda
@@ -2241,20 +2277,25 @@ MediaManager::Observe(nsISupports* aSubject, const char* aTopic,
     // This is safe since this is guaranteed to be here since sSingleton isn't
     // cleared until the lambda function clears it.
 
-    MediaManager::GetMessageLoop()->PostTask(FROM_HERE, new ShutdownTask(
-        media::NewRunnableFrom([this]() mutable {
-      // Close off any remaining active windows.
+    // note that this == sSingleton
+    nsRefPtr<MediaManager> that(sSingleton);
+    // Release the backend (and call Shutdown()) from within the MediaManager thread
+    RefPtr<MediaEngine> temp;
+    {
       MutexAutoLock lock(mMutex);
-      GetActiveWindows()->Clear();
-      mActiveCallbacks.Clear();
-      mCallIds.Clear();
-      LOG(("Releasing MediaManager singleton and thread"));
-      // Note: won't be released immediately as the Observer has a ref to us
-      sSingleton = nullptr;
+      temp = mBackend.forget();
+    }
+    // Don't use MediaManager::PostTask() because we're sInShutdown=true here!
+    mMediaThread->message_loop()->PostTask(FROM_HERE, new ShutdownTask(
+        temp.forget(),
+        media::NewRunnableFrom([this, that]() mutable {
+      LOG(("MediaManager shutdown lambda running, releasing MediaManager singleton and thread"));
       if (mMediaThread) {
         mMediaThread->Stop();
       }
-      mBackend = nullptr;
+      // we hold a ref to 'that' which is the same as sSingleton
+      sSingleton = nullptr;
+
       return NS_OK;
     })));
     return NS_OK;
@@ -2299,8 +2340,11 @@ MediaManager::Observe(nsISupports* aSubject, const char* aTopic,
       }
     }
 
+    if (sInShutdown) {
+      return task->Denied(NS_LITERAL_STRING("In shutdown"));
+    }
     // Reuse the same thread to save memory.
-    MediaManager::GetMessageLoop()->PostTask(FROM_HERE, task.forget());
+    MediaManager::PostTask(FROM_HERE, task.forget());
     return NS_OK;
 
   } else if (!strcmp(aTopic, "getUserMedia:response:deny")) {
@@ -2498,8 +2542,7 @@ MediaManager::SanitizeDeviceIds(int64_t aSinceWhen)
   NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
   LOG(("%s: sinceWhen = %llu", __FUNCTION__, aSinceWhen));
 
-  MediaManager::GetMessageLoop()->PostTask(FROM_HERE,
-    new SanitizeDeviceIdsTask(aSinceWhen));
+  MediaManager::PostTask(FROM_HERE, new SanitizeDeviceIdsTask(aSinceWhen));
   return NS_OK;
 }
 
@@ -2616,12 +2659,10 @@ GetUserMediaCallbackMediaStreamListener::AudioConfig(bool aEchoOn,
 {
   if (mAudioSource) {
 #ifdef MOZ_WEBRTC
-    mMediaThread->message_loop()->PostTask(FROM_HERE,
+    MediaManager::PostTask(FROM_HERE,
       NewRunnableMethod(mAudioSource.get(), &MediaEngineSource::Config,
                         aEchoOn, aEcho, aAgcOn, aAGC, aNoiseOn,
                         aNoise, aPlayoutDelay));
-#else
-    unused << mMediaThread;
 #endif
   }
 }
@@ -2634,7 +2675,7 @@ GetUserMediaCallbackMediaStreamListener::Invalidate()
   // thread.
   // Pass a ref to us (which is threadsafe) so it can query us for the
   // source stream info.
-  MediaManager::GetMessageLoop()->PostTask(FROM_HERE,
+  MediaManager::PostTask(FROM_HERE,
     new MediaOperationTask(MEDIA_STOP,
                            this, nullptr, nullptr,
                            mAudioSource, mVideoSource,
@@ -2652,7 +2693,7 @@ GetUserMediaCallbackMediaStreamListener::StopScreenWindowSharing()
        mVideoSource->GetMediaSource() == dom::MediaSourceEnum::Application ||
        mVideoSource->GetMediaSource() == dom::MediaSourceEnum::Window)) {
     // Stop the whole stream if there's no audio; just the video track if we have both
-    MediaManager::GetMessageLoop()->PostTask(FROM_HERE,
+    MediaManager::PostTask(FROM_HERE,
       new MediaOperationTask(mAudioSource ? MEDIA_STOP_TRACK : MEDIA_STOP,
                              this, nullptr, nullptr,
                              nullptr, mVideoSource,
@@ -2670,7 +2711,7 @@ GetUserMediaCallbackMediaStreamListener::StopTrack(TrackID aID, bool aIsAudio)
   {
     // XXX to support multiple tracks of a type in a stream, this should key off
     // the TrackID and not just the type
-    MediaManager::GetMessageLoop()->PostTask(FROM_HERE,
+    MediaManager::PostTask(FROM_HERE,
       new MediaOperationTask(MEDIA_STOP_TRACK,
                              this, nullptr, nullptr,
                              aIsAudio  ? mAudioSource : nullptr,
@@ -2696,7 +2737,7 @@ void
 GetUserMediaCallbackMediaStreamListener::NotifyDirectListeners(MediaStreamGraph* aGraph,
                                                                bool aHasListeners)
 {
-  MediaManager::GetMessageLoop()->PostTask(FROM_HERE,
+  MediaManager::PostTask(FROM_HERE,
     new MediaOperationTask(MEDIA_DIRECT_LISTENERS,
                            this, nullptr, nullptr,
                            mAudioSource, mVideoSource,

dom/media/MediaManager.h

@@ -71,6 +71,7 @@ public:
 
   ~GetUserMediaCallbackMediaStreamListener()
   {
+    unused << mMediaThread;
     // It's OK to release mStream on any thread; they have thread-safe
     // refcounts.
   }
@@ -514,7 +515,7 @@ public:
   // from MediaManager thread.
   static MediaManager* Get();
   static MediaManager* GetIfExists();
-  static MessageLoop* GetMessageLoop();
+  static void PostTask(const tracked_objects::Location& from_here, Task* task);
 #ifdef DEBUG
   static bool IsInMediaThread();
 #endif

dom/media/webrtc/MediaEngine.h

@@ -64,6 +64,8 @@ public:
   virtual void EnumerateAudioDevices(dom::MediaSourceEnum,
                                      nsTArray<nsRefPtr<MediaEngineAudioSource> >*) = 0;
 
+  virtual void Shutdown() = 0;
+
 protected:
   virtual ~MediaEngine() {}
 };
@@ -81,6 +83,8 @@ public:
 
   virtual ~MediaEngineSource() {}
 
+  virtual void Shutdown() = 0;
+
   /* Populate the human readable name of this device in the nsAString */
   virtual void GetName(nsAString&) = 0;
 
@@ -245,6 +249,7 @@ public:
   /* This call reserves but does not start the device. */
   virtual nsresult Allocate(const dom::MediaTrackConstraints &aConstraints,
                             const MediaEnginePrefs &aPrefs) = 0;
+
 protected:
   explicit MediaEngineAudioSource(MediaEngineState aState)
     : MediaEngineSource(aState) {}

dom/media/webrtc/MediaEngineCameraVideoSource.h

@@ -61,6 +61,8 @@ public:
   uint32_t GetBestFitnessDistance(
       const nsTArray<const dom::MediaTrackConstraintSet*>& aConstraintSets) override;
 
+  virtual void Shutdown() override {};
+
 protected:
   struct CapabilityCandidate {
     explicit CapabilityCandidate(uint8_t index, uint32_t distance = 0)

dom/media/webrtc/MediaEngineDefault.h

@@ -36,6 +36,8 @@ class MediaEngineDefaultVideoSource : public nsITimerCallback,
 public:
   MediaEngineDefaultVideoSource();
 
+  virtual void Shutdown() override {};
+
   virtual void GetName(nsAString&) override;
   virtual void GetUUID(nsAString&) override;
 
@@ -104,6 +106,8 @@ class MediaEngineDefaultAudioSource : public nsITimerCallback,
 public:
   MediaEngineDefaultAudioSource();
 
+  virtual void Shutdown() override {};
+
   virtual void GetName(nsAString&) override;
   virtual void GetUUID(nsAString&) override;
 
@@ -158,15 +162,23 @@ public:
   {}
 
   virtual void EnumerateVideoDevices(dom::MediaSourceEnum,
-                                     nsTArray<nsRefPtr<MediaEngineVideoSource> >*);
+                                     nsTArray<nsRefPtr<MediaEngineVideoSource> >*) override;
   virtual void EnumerateAudioDevices(dom::MediaSourceEnum,
-                                     nsTArray<nsRefPtr<MediaEngineAudioSource> >*);
+                                     nsTArray<nsRefPtr<MediaEngineAudioSource> >*) override;
+  virtual void Shutdown() {
+    MutexAutoLock lock(mMutex);
+
+    mVSources.Clear();
+    mASources.Clear();
+  };
 
 protected:
   bool mHasFakeTracks;
 
 private:
-  ~MediaEngineDefault() {}
+  ~MediaEngineDefault() {
+    Shutdown();
+  }
 
   Mutex mMutex;
   // protected with mMutex:

dom/media/webrtc/MediaEngineTabVideoSource.h

@@ -18,6 +18,7 @@ class MediaEngineTabVideoSource : public MediaEngineVideoSource, nsIDOMEventList
     NS_DECL_NSITIMERCALLBACK
     MediaEngineTabVideoSource();
 
+    virtual void Shutdown() override {};
     virtual void GetName(nsAString_internal&) override;
     virtual void GetUUID(nsAString_internal&) override;
     virtual nsresult Allocate(const dom::MediaTrackConstraints &,

dom/media/webrtc/MediaEngineWebRTC.cpp

@@ -274,10 +274,9 @@ MediaEngineWebRTC::EnumerateVideoDevices(dom::MediaSourceEnum aMediaSource,
     }
   }
 
-  if (mHasTabVideoSource || dom::MediaSourceEnum::Browser == aMediaSource)
+  if (mHasTabVideoSource || dom::MediaSourceEnum::Browser == aMediaSource) {
     aVSources->AppendElement(new MediaEngineTabVideoSource());
-
-  return;
+  }
 #endif
 }
 
@@ -372,15 +371,43 @@ MediaEngineWebRTC::EnumerateAudioDevices(dom::MediaSourceEnum aMediaSource,
   }
 }
 
+static PLDHashOperator
+ClearVideoSource (const nsAString&, // unused
+                  MediaEngineVideoSource* aData,
+                  void *userArg)
+{
+  if (aData) {
+    aData->Shutdown();
+  }
+  return PL_DHASH_NEXT;
+}
+
+static PLDHashOperator
+ClearAudioSource (const nsAString&, // unused
+                  MediaEngineWebRTCAudioSource* aData,
+                  void *userArg)
+{
+  if (aData) {
+    aData->Shutdown();
+  }
+  return PL_DHASH_NEXT;
+}
+
 void
 MediaEngineWebRTC::Shutdown()
 {
   // This is likely paranoia
   MutexAutoLock lock(mMutex);
 
-  // Clear callbacks before we go away since the engines may outlive us
+  LOG(("%s", __FUNCTION__));
+  // Shutdown all the sources, since we may have dangling references to the
+  // sources in nsDOMUserMediaStreams waiting for GC/CC
+  mVideoSources.EnumerateRead(ClearVideoSource, nullptr);
+  mAudioSources.EnumerateRead(ClearAudioSource, nullptr);
   mVideoSources.Clear();
   mAudioSources.Clear();
+
+  // Clear callbacks before we go away since the engines may outlive us
   if (mVideoEngine) {
     mVideoEngine->SetTraceCallback(nullptr);
     webrtc::VideoEngine::Delete(mVideoEngine);

dom/media/webrtc/MediaEngineWebRTC.h

@@ -109,19 +109,20 @@ public:
 
   void Refresh(int aIndex);
 
+  virtual void Shutdown() override;
+
 protected:
   ~MediaEngineWebRTCVideoSource() { Shutdown(); }
 
 private:
   // Initialize the needed Video engine interfaces.
   void Init();
-  void Shutdown();
 
   // Engine variables.
   webrtc::VideoEngine* mVideoEngine; // Weak reference, don't free.
-  webrtc::ViEBase* mViEBase;
-  webrtc::ViECapture* mViECapture;
-  webrtc::ViERender* mViERender;
+  ScopedCustomReleasePtr<webrtc::ViEBase> mViEBase;
+  ScopedCustomReleasePtr<webrtc::ViECapture> mViECapture;
+  ScopedCustomReleasePtr<webrtc::ViERender> mViERender;
 
   int mMinFps; // Min rate we want to accept
   dom::MediaSourceEnum mMediaSource; // source of media (camera | application | screen)
@@ -195,12 +196,13 @@ public:
 
   NS_DECL_THREADSAFE_ISUPPORTS
 
+  virtual void Shutdown() override;
+
 protected:
   ~MediaEngineWebRTCAudioSource() { Shutdown(); }
 
 private:
   void Init();
-  void Shutdown();
 
   webrtc::VoiceEngine* mVoiceEngine;
   ScopedCustomReleasePtr<webrtc::VoEBase> mVoEBase;
@@ -239,12 +241,12 @@ public:
 
   // Clients should ensure to clean-up sources video/audio sources
   // before invoking Shutdown on this class.
-  void Shutdown();
+  void Shutdown() override;
 
   virtual void EnumerateVideoDevices(dom::MediaSourceEnum,
-                                    nsTArray<nsRefPtr<MediaEngineVideoSource> >*);
+                                     nsTArray<nsRefPtr<MediaEngineVideoSource>>*) override;
   virtual void EnumerateAudioDevices(dom::MediaSourceEnum,
-                                    nsTArray<nsRefPtr<MediaEngineAudioSource> >*);
+                                     nsTArray<nsRefPtr<MediaEngineAudioSource>>*) override;
 private:
   ~MediaEngineWebRTC() {
     Shutdown();

dom/media/webrtc/MediaEngineWebRTCAudio.cpp

@@ -478,11 +478,12 @@ MediaEngineWebRTCAudioSource::Shutdown()
 {
   if (!mInitDone) {
     // duplicate these here in case we failed during Init()
-    if (mChannel != -1) {
+    if (mChannel != -1 && mVoENetwork) {
       mVoENetwork->DeRegisterExternalTransport(mChannel);
     }
 
     delete mNullTransport;
+    mNullTransport = nullptr;
     return;
   }
 
@@ -514,6 +515,7 @@ MediaEngineWebRTCAudioSource::Shutdown()
   }
 
   delete mNullTransport;
+  mNullTransport = nullptr;
 
   mVoEProcessing = nullptr;
   mVoENetwork = nullptr;

dom/media/webrtc/MediaEngineWebRTCVideo.cpp

@@ -266,7 +266,7 @@ MediaEngineWebRTCVideoSource::Deallocate()
     // another thread anywhere else, b) ViEInputManager::DestroyCaptureDevice() grabs
     // an exclusive object lock and deletes it in a critical section, so all in all
     // this should be safe threadwise.
-    NS_DispatchToMainThread(WrapRunnable(mViECapture,
+    NS_DispatchToMainThread(WrapRunnable(mViECapture.get(),
                                          &webrtc::ViECapture::ReleaseCaptureDevice,
                                          mCaptureIndex),
                             NS_DISPATCH_SYNC);
@@ -422,9 +422,10 @@ MediaEngineWebRTCVideoSource::Shutdown()
   if (mState == kAllocated || mState == kStopped) {
     Deallocate();
   }
-  mViECapture->Release();
-  mViERender->Release();
-  mViEBase->Release();
+  mViECapture = nullptr;
+  mViERender = nullptr;
+  mViEBase = nullptr;
+
   mState = kReleased;
   mInitDone = false;
 }