Bug 802538: Make sure gUM MediaStreams are destroyed (and fix other leaks) r=derf
authorRandell Jesup <rjesup@jesup.org>
Fri, 28 Dec 2012 15:27:57 -0500
changeset 126280 0d78b96618078a34a2e75163e25aef6cced472ba
parent 126279 70dc4af0325bb8a0c7e8462ddf9051324e221b4a
child 126281 35a7f17ac707789e5d16f1730cc09886e9eb9645
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersderf
bugs802538
milestone20.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 802538: Make sure gUM MediaStreams are destroyed (and fix other leaks) r=derf
dom/media/MediaManager.cpp
dom/media/MediaManager.h
--- a/dom/media/MediaManager.cpp
+++ b/dom/media/MediaManager.cpp
@@ -663,23 +663,23 @@ public:
 
     /**
      * We only display available devices in the UI for now. We can easily
      * change this later, when we implement a more sophisticated UI that
      * lets the user revoke a device currently held by another tab (or
      * we decide to provide a stream from a device already allocated).
      */
     for (i = 0; i < videoCount; i++) {
-      nsRefPtr<MediaEngineVideoSource> vSource = videoSources[i];
+      MediaEngineVideoSource *vSource = videoSources[i];
       if (vSource->IsAvailable()) {
         devices->AppendElement(new MediaDevice(vSource));
       }
     }
     for (i = 0; i < audioCount; i++) {
-      nsRefPtr<MediaEngineAudioSource> aSource = audioSources[i];
+      MediaEngineAudioSource *aSource = audioSources[i];
       if (aSource->IsAvailable()) {
         devices->AppendElement(new MediaDevice(aSource));
       }
     }
 
     NS_DispatchToMainThread(new DeviceSuccessCallbackRunnable(
       mSuccess, mError, *devices
     ));
@@ -961,16 +961,17 @@ MediaManager::OnNavigation(uint64_t aWin
     return;
   }
 
   uint32_t length = listeners->Length();
   for (uint32_t i = 0; i < length; i++) {
     nsRefPtr<GetUserMediaCallbackMediaStreamListener> listener =
       listeners->ElementAt(i);
     listener->Invalidate();
+    listener->Remove();
   }
   listeners->Clear();
 
   GetActiveWindows()->Remove(aWindowID);
 }
 
 nsresult
 MediaManager::Observe(nsISupports* aSubject, const char* aTopic,
--- a/dom/media/MediaManager.h
+++ b/dom/media/MediaManager.h
@@ -67,22 +67,21 @@ class GetUserMediaNotificationEvent: pub
     GetUserMediaStatus mStatus;
 };
 
 typedef enum {
   MEDIA_START,
   MEDIA_STOP
 } MediaOperation;
 
-// Generic class for running long media operations off the main thread, and
-// then (because nsDOMMediaStreams aren't threadsafe), re-sends itseld to
-// MainThread to release mStream.  This is part of the reason we use an
-// operation type - we can change it to repost the runnable to MainThread
-// to do operations with the nsDOMMediaStreams, while we can't assign or
-// copy a nsRefPtr to a nsDOMMediaStream
+class GetUserMediaCallbackMediaStreamListener;
+
+// Generic class for running long media operations like Start off the main
+// thread, and then (because nsDOMMediaStreams aren't threadsafe),
+// ProxyReleases mStream since it's cycle collected.
 class MediaOperationRunnable : public nsRunnable
 {
 public:
   MediaOperationRunnable(MediaOperation aType,
     nsDOMMediaStream* aStream,
     MediaEngineSource* aAudioSource,
     MediaEngineSource* aVideoSource)
     : mType(aType)
@@ -103,17 +102,19 @@ public:
     {}
 
   ~MediaOperationRunnable()
   {
     // nsDOMMediaStreams are cycle-collected and thus main-thread-only for
     // refcounting and releasing
     if (mStream) {
       nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
-      NS_ProxyRelease(mainThread,mStream,false);
+      nsDOMMediaStream *stream;
+      mStream.forget(&stream);
+      NS_ProxyRelease(mainThread, stream, true);
     }
   }
 
   NS_IMETHOD
   Run()
   {
     // No locking between these is required as all the callbacks for the
     // same MediaStream will occur on the same thread.
@@ -165,16 +166,20 @@ public:
           mSourceStream->Finish();
 
           nsRefPtr<GetUserMediaNotificationEvent> event =
             new GetUserMediaNotificationEvent(GetUserMediaNotificationEvent::STOPPING);
 
           NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
         }
         break;
+
+      default:
+        MOZ_ASSERT(false,"invalid MediaManager operation");
+        break;
     }
     return NS_OK;
   }
 
 private:
   MediaOperation mType;
   nsRefPtr<MediaEngineSource> mAudioSource; // threadsafe
   nsRefPtr<MediaEngineSource> mVideoSource; // threadsafe
@@ -194,33 +199,58 @@ public:
     nsDOMMediaStream* aStream,
     MediaEngineSource* aAudioSource,
     MediaEngineSource* aVideoSource)
     : mMediaThread(aThread)
     , mAudioSource(aAudioSource)
     , mVideoSource(aVideoSource)
     , mStream(aStream) {}
 
+  ~GetUserMediaCallbackMediaStreamListener()
+  {
+    // In theory this could be released from the MediaStreamGraph thread (RemoveListener)
+    if (mStream) {
+      nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
+      nsDOMMediaStream *stream;
+      mStream.forget(&stream);
+      // Releases directly if on MainThread already
+      NS_ProxyRelease(mainThread, stream, false);
+    }
+  }
+
   void
   Invalidate()
   {
     nsRefPtr<MediaOperationRunnable> runnable;
 
     // We can't take a chance on blocking here, so proxy this to another
     // thread.
     // XXX FIX! I'm cheating and passing a raw pointer to the sourcestream
-    // which is valid as long as the mStream pointer here is.  Need a better solution.
-    runnable = new MediaOperationRunnable(MEDIA_STOP, 
+    // which is valid as long as the mStream pointer here is.
+    // Solutions (see bug 825235):
+    // a) if on MainThread, pass mStream and let it addref
+    //    (MediaOperation will need to ProxyRelease however)
+    // b) if on MediaStreamGraph thread, dispatch a runnable to MainThread
+    //    to call Invalidate() (with a strong ref to this listener)
+    runnable = new MediaOperationRunnable(MEDIA_STOP,
                                           mStream->GetStream()->AsSourceStream(),
                                           mAudioSource, mVideoSource);
     mMediaThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
 
     return;
   }
 
+  void
+  Remove()
+  {
+    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
+    // Caller holds strong reference to us, so no death grip required
+    mStream->GetStream()->RemoveListener(this);
+  }
+
   // Proxy NotifyPull() to sources
   void
   NotifyPull(MediaStreamGraph* aGraph, StreamTime aDesiredTime)
   {
     // Currently audio sources ignore NotifyPull, but they could
     // watch it especially for fake audio.
     if (mAudioSource) {
       mAudioSource->NotifyPull(aGraph, aDesiredTime);
@@ -229,16 +259,17 @@ public:
       mVideoSource->NotifyPull(aGraph, aDesiredTime);
     }
   }
 
   void
   NotifyFinished(MediaStreamGraph* aGraph)
   {
     Invalidate();
+    // XXX right now this calls Finish, which isn't ideal but doesn't hurt
   }
 
 private:
   nsCOMPtr<nsIThread> mMediaThread;
   nsRefPtr<MediaEngineSource> mAudioSource;
   nsRefPtr<MediaEngineSource> mVideoSource;
   nsRefPtr<nsDOMMediaStream> mStream;
 };