Bug 802538: Make sure gUM MediaStreams are destroyed (and fix other leaks) r=derf

dom/media/MediaManager.cpp

@@ -668,13 +668,13 @@ public:
      * 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));
       }
@@ -966,6 +966,7 @@ MediaManager::OnNavigation(uint64_t aWindowID)
     nsRefPtr<GetUserMediaCallbackMediaStreamListener> listener =
       listeners->ElementAt(i);
     listener->Invalidate();
+    listener->Remove();
   }
   listeners->Clear();
 

dom/media/MediaManager.h

@@ -72,12 +72,11 @@ typedef enum {
   MEDIA_STOP
 } MediaOperation;
 
-// Generic class for running long media operations off the main thread, and
+class GetUserMediaCallbackMediaStreamListener;
-// then (because nsDOMMediaStreams aren't threadsafe), re-sends itseld to
+
-// MainThread to release mStream.  This is part of the reason we use an
+// Generic class for running long media operations like Start off the main
-// operation type - we can change it to repost the runnable to MainThread
+// thread, and then (because nsDOMMediaStreams aren't threadsafe),
-// to do operations with the nsDOMMediaStreams, while we can't assign or
+// ProxyReleases mStream since it's cycle collected.
-// copy a nsRefPtr to a nsDOMMediaStream
 class MediaOperationRunnable : public nsRunnable
 {
 public:
@@ -108,7 +107,9 @@ public:
     // 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);
     }
   }
 
@@ -170,6 +171,10 @@ public:
           NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
         }
         break;
+
+      default:
+        MOZ_ASSERT(false,"invalid MediaManager operation");
+        break;
     }
     return NS_OK;
   }
@@ -199,6 +204,18 @@ public:
     , 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()
   {
@@ -207,8 +224,13 @@ public:
     // 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.
+    // which is valid as long as the mStream pointer here is.
-    runnable = new MediaOperationRunnable(MEDIA_STOP, 
+    // 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);
@@ -216,6 +238,14 @@ public:
     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)
@@ -234,6 +264,7 @@ public:
   NotifyFinished(MediaStreamGraph* aGraph)
   {
     Invalidate();
+    // XXX right now this calls Finish, which isn't ideal but doesn't hurt
   }
 
 private: