Bug 827203: add locks against calling RemoveListener on a Destroy()ed MediaStream r=roc

dom/media/MediaManager.cpp

@@ -356,7 +356,7 @@ public:
     nsRefPtr<MediaInputPort> port = trackunion->GetStream()->AsProcessedStream()->
       AllocateInputPort(stream, MediaInputPort::FLAG_BLOCK_OUTPUT);
     trackunion->mSourceStream = stream;
-    trackunion->mPort = port;
+    trackunion->mPort = port.forget();
 
     nsPIDOMWindow *window = static_cast<nsPIDOMWindow*>
       (nsGlobalWindow::GetInnerWindowWithId(mWindowID));
@@ -368,8 +368,7 @@ public:
     // Activate our listener. We'll call Start() on the source when get a callback
     // that the MediaStream has started consuming. The listener is freed
     // when the page is invalidated (on navigation or close).
-    mListener->Activate(stream.forget(), port.forget(),
-                        mAudioSource, mVideoSource);
+    mListener->Activate(stream.forget(), mAudioSource, mVideoSource);
 
     // Dispatch to the media thread to ask it to start the sources,
     // because that can take a while
@@ -1069,7 +1068,7 @@ MediaManager::OnNavigation(uint64_t aWindowID)
   for (uint32_t i = 0; i < length; i++) {
     nsRefPtr<GetUserMediaCallbackMediaStreamListener> listener =
       listeners->ElementAt(i);
-    listener->Invalidate(true);
+    listener->Invalidate();
     listener->Remove();
   }
   listeners->Clear();
@@ -1232,9 +1231,11 @@ MediaManager::GetActiveMediaCaptureWindows(nsISupportsArray **aArray)
   return NS_OK;
 }
 
+// Can be invoked from EITHER MainThread or MSG thread
 void
-GetUserMediaCallbackMediaStreamListener::Invalidate(bool aNeedsFinish)
+GetUserMediaCallbackMediaStreamListener::Invalidate()
 {
+
   nsRefPtr<MediaOperationRunnable> runnable;
   // We can't take a chance on blocking here, so proxy this to another
   // thread.
@@ -1242,15 +1243,33 @@ GetUserMediaCallbackMediaStreamListener::Invalidate(bool aNeedsFinish)
   // source stream info.
   runnable = new MediaOperationRunnable(MEDIA_STOP,
                                         this, mAudioSource, mVideoSource,
-                                        aNeedsFinish);
+                                        mFinished);
   mMediaThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
 }
 
+// Called from the MediaStreamGraph thread
 void
 GetUserMediaCallbackMediaStreamListener::NotifyFinished(MediaStreamGraph* aGraph)
 {
-  Invalidate(false);
+  mFinished = true;
+  Invalidate();
   NS_DispatchToMainThread(new GetUserMediaListenerRemove(mWindowID, this));
 }
 
+// Called from the MediaStreamGraph thread
+// this can be in response to our own RemoveListener() (via ::Remove()), or
+// because the DOM GC'd the DOMLocalMediaStream/etc we're attached to.
+void
+GetUserMediaCallbackMediaStreamListener::NotifyRemoved(MediaStreamGraph* aGraph)
+{
+  {
+    MutexAutoLock lock(mLock); // protect access to mRemoved
+    MM_LOG(("Listener removed by DOM Destroy(), mFinished = %d", (int) mFinished));
+    mRemoved = true;
+  }
+  if (!mFinished) {
+    NotifyFinished(aGraph);
+  }
+}
+
 } // namespace mozilla

dom/media/MediaManager.h

@@ -73,21 +73,23 @@ public:
   GetUserMediaCallbackMediaStreamListener(nsIThread *aThread,
     uint64_t aWindowID)
     : mMediaThread(aThread)
-    , mWindowID(aWindowID) {}
+    , mWindowID(aWindowID)
+    , mFinished(false)
+    , mLock("mozilla::GUMCMSL")
+    , mRemoved(false) {}
 
   ~GetUserMediaCallbackMediaStreamListener()
   {
-    // It's OK to release mStream and mPort on any thread; they have thread-safe
+    // It's OK to release mStream on any thread; they have thread-safe
     // refcounts.
   }
 
   void Activate(already_AddRefed<SourceMediaStream> aStream,
-    already_AddRefed<MediaInputPort> aPort,
     MediaEngineSource* aAudioSource,
     MediaEngineSource* aVideoSource)
   {
+    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
     mStream = aStream; // also serves as IsActive();
-    mPort = aPort;
     mAudioSource = aAudioSource;
     mVideoSource = aVideoSource;
     mLastEndTimeAudio = 0;
@@ -107,20 +109,26 @@ public:
   }
 
   // implement in .cpp to avoid circular dependency with MediaOperationRunnable
-  void Invalidate(bool aNeedsFinish);
+  // Can be invoked from EITHER MainThread or MSG thread
+  void Invalidate();
 
   void
   Remove()
   {
     NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
+    // allow calling even if inactive (!mStream) for easier cleanup
     // Caller holds strong reference to us, so no death grip required
-    if (mStream) // allow even if inactive for easier cleanup
+    MutexAutoLock lock(mLock); // protect access to mRemoved
+    if (mStream && !mRemoved) {
+      MM_LOG(("Listener removed on purpose, mFinished = %d", (int) mFinished));
+      mRemoved = true; // RemoveListener is async, avoid races
       mStream->RemoveListener(this);
+    }
   }
 
   // Proxy NotifyPull() to sources
-  void
-  NotifyPull(MediaStreamGraph* aGraph, StreamTime aDesiredTime)
+  virtual void
+  NotifyPull(MediaStreamGraph* aGraph, StreamTime aDesiredTime) MOZ_OVERRIDE
   {
     // Currently audio sources ignore NotifyPull, but they could
     // watch it especially for fake audio.
@@ -132,18 +140,31 @@ public:
     }
   }
 
-  void
-  NotifyFinished(MediaStreamGraph* aGraph);
+  virtual void
+  NotifyFinished(MediaStreamGraph* aGraph) MOZ_OVERRIDE;
+
+  virtual void
+  NotifyRemoved(MediaStreamGraph* aGraph) MOZ_OVERRIDE;
 
 private:
+  // Set at construction
   nsCOMPtr<nsIThread> mMediaThread;
   uint64_t mWindowID;
-  nsRefPtr<MediaEngineSource> mAudioSource;
-  nsRefPtr<MediaEngineSource> mVideoSource;
-  nsRefPtr<SourceMediaStream> mStream;
-  nsRefPtr<MediaInputPort> mPort;
+
+  // Set at Activate on MainThread
+
+  // Accessed from MediaStreamGraph thread, MediaManager thread, and MainThread
+  // No locking needed as they're only addrefed except on the MediaManager thread
+  nsRefPtr<MediaEngineSource> mAudioSource; // threadsafe refcnt
+  nsRefPtr<MediaEngineSource> mVideoSource; // threadsafe refcnt
+  nsRefPtr<SourceMediaStream> mStream; // threadsafe refcnt
   TrackTicks mLastEndTimeAudio;
   TrackTicks mLastEndTimeVideo;
+  bool mFinished;
+
+  // Accessed from MainThread and MSG thread
+  Mutex mLock; // protects mRemoved access from MainThread
+  bool mRemoved;
 };
 
 typedef enum {