Bug 827203 - Add locks against calling RemoveListener on a Destroy()ed MediaStream. r=roc, a=akeybl
authorRandell Jesup <rjesup@jesup.org>
Mon, 07 Jan 2013 21:44:43 -0500
changeset 123573 c947094aa28b5f50d00835562042fe6a918c4f72
parent 123572 64b4d4cb1f06caf1822b34b385b5fcdea1419082
child 123574 7289797550a42310dc582ac72c36d64c36ab7dfe
push id3166
push userryanvm@gmail.com
push dateMon, 14 Jan 2013 02:30:50 +0000
treeherdermozilla-aurora@a3ed3feed8f4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc, akeybl
bugs827203
milestone20.0a2
Bug 827203 - Add locks against calling RemoveListener on a Destroy()ed MediaStream. r=roc, a=akeybl
dom/media/MediaManager.cpp
dom/media/MediaManager.h
--- a/dom/media/MediaManager.cpp
+++ b/dom/media/MediaManager.cpp
@@ -351,30 +351,29 @@ public:
     MediaStreamGraph* gm = MediaStreamGraph::GetInstance();
     nsRefPtr<SourceMediaStream> stream = gm->CreateSourceStream(nullptr);
 
     // connect the source stream to the track union stream to avoid us blocking
     trackunion->GetStream()->AsProcessedStream()->SetAutofinish(true);
     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));
     if (window && window->GetExtantDoc()) {
       trackunion->CombineWithPrincipal(window->GetExtantDoc()->NodePrincipal());
     }
 
     // The listener was added at the begining in an inactive state.
     // 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
     nsIThread *mediaThread = MediaManager::GetThread();
     nsRefPtr<MediaOperationRunnable> runnable(
       new MediaOperationRunnable(MEDIA_START, mListener,
                                  mAudioSource, mVideoSource, false));
     mediaThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
@@ -1064,17 +1063,17 @@ MediaManager::OnNavigation(uint64_t aWin
   if (!listeners) {
     return;
   }
 
   uint32_t length = listeners->Length();
   for (uint32_t i = 0; i < length; i++) {
     nsRefPtr<GetUserMediaCallbackMediaStreamListener> listener =
       listeners->ElementAt(i);
-    listener->Invalidate(true);
+    listener->Invalidate();
     listener->Remove();
   }
   listeners->Clear();
 
   RemoveWindowID(aWindowID);
   // listeners has been deleted
 }
 
@@ -1227,30 +1226,50 @@ MediaManager::GetActiveMediaCaptureWindo
     return rv;
 
   mActiveWindows.EnumerateRead(WindowsHashToArrayFunc, array);
 
   *aArray = array;
   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.
   // Pass a ref to us (which is threadsafe) so it can query us for the
   // 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
--- a/dom/media/MediaManager.h
+++ b/dom/media/MediaManager.h
@@ -68,31 +68,33 @@ class GetUserMediaNotificationEvent: pub
  */
 class GetUserMediaCallbackMediaStreamListener : public MediaStreamListener
 {
 public:
   // Create in an inactive state
   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;
     mLastEndTimeVideo = 0;
 
     mStream->AddListener(this);
   }
 
@@ -102,53 +104,72 @@ public:
   }
   SourceMediaStream *GetSourceStream()
   {
     MOZ_ASSERT(mStream);
     return mStream->AsSourceStream();
   }
 
   // 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.
     if (mAudioSource) {
       mAudioSource->NotifyPull(aGraph, mStream, kAudioTrack, aDesiredTime, mLastEndTimeAudio);
     }
     if (mVideoSource) {
       mVideoSource->NotifyPull(aGraph, mStream, kVideoTrack, aDesiredTime, mLastEndTimeVideo);
     }
   }
 
-  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 {
   MEDIA_START,
   MEDIA_STOP
 } MediaOperation;
 
 // Generic class for running long media operations like Start off the main