Bug 1540434 - Convert a bunch of rawptrs related to MediaManager to RefPtr. r=jib
authorAndreas Pehrson <apehrson@mozilla.com>
Tue, 16 Apr 2019 07:37:48 +0000
changeset 469633 904d371bd450
parent 469632 35aed2f899c3
child 469634 41227706dc29
push id35878
push userapavel@mozilla.com
push dateTue, 16 Apr 2019 15:43:40 +0000
treeherdermozilla-central@258af4e91151 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjib
bugs1540434
milestone68.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 1540434 - Convert a bunch of rawptrs related to MediaManager to RefPtr. r=jib When unplugging a camera in use in a video-only gUM capture, we see a crash in GetUserMediaWindowListener::Remove when trying to use `aListener` after its last reference was removed from an nsRefPtrHashTable at the beginning of the method. As `aListener` was a rawptr, neither this method itself, nor its caller, kept it alive. Making this method take a RefPtr instead of a rawptr solves this issue. This patch also converts other similar rawptrs for good measure. Differential Revision: https://phabricator.services.mozilla.com/D27520
dom/media/MediaManager.cpp
dom/media/MediaManager.h
--- a/dom/media/MediaManager.cpp
+++ b/dom/media/MediaManager.cpp
@@ -216,18 +216,18 @@ using dom::Promise;
 using dom::Sequence;
 using media::NewRunnableFrom;
 using media::NewTaskFrom;
 using media::Refcountable;
 
 static Atomic<bool> sHasShutdown;
 
 struct DeviceState {
-  DeviceState(const RefPtr<MediaDevice>& aDevice, bool aOffWhileDisabled)
-      : mOffWhileDisabled(aOffWhileDisabled), mDevice(aDevice) {
+  DeviceState(RefPtr<MediaDevice> aDevice, bool aOffWhileDisabled)
+      : mOffWhileDisabled(aOffWhileDisabled), mDevice(std::move(aDevice)) {
     MOZ_ASSERT(mDevice);
   }
 
   // true if we have stopped mDevice, this is a terminal state.
   // MainThread only.
   bool mStopped = false;
 
   // true if mDevice is currently enabled, i.e., turned on and capturing.
@@ -321,18 +321,19 @@ class SourceListener : public SupportsWe
   /**
    * Registers this source listener as belonging to the given window listener.
    */
   void Register(GetUserMediaWindowListener* aListener);
 
   /**
    * Marks this listener as active and adds itself as a listener to aStream.
    */
-  void Activate(SourceMediaStream* aStream, MediaDevice* aAudioDevice,
-                MediaDevice* aVideoDevice);
+  void Activate(RefPtr<SourceMediaStream> aStream,
+                RefPtr<MediaDevice> aAudioDevice,
+                RefPtr<MediaDevice> aVideoDevice);
 
   /**
    * Posts a task to initialize and start all associated devices.
    */
   RefPtr<SourceListenerPromise> InitializeAsync();
 
   /**
    * Stops all live tracks, finishes the associated MediaStream and cleans up
@@ -469,43 +470,46 @@ class GetUserMediaWindowListener {
       : mMediaThread(aThread),
         mWindowID(aWindowID),
         mPrincipalHandle(aPrincipalHandle),
         mChromeNotificationTaskPosted(false) {}
 
   /**
    * Registers an inactive gUM source listener for this WindowListener.
    */
-  void Register(SourceListener* aListener) {
+  void Register(RefPtr<SourceListener> aListener) {
     MOZ_ASSERT(NS_IsMainThread());
     MOZ_ASSERT(aListener);
     MOZ_ASSERT(!aListener->Activated());
     MOZ_ASSERT(!mInactiveListeners.Contains(aListener), "Already registered");
     MOZ_ASSERT(!mActiveListeners.Contains(aListener), "Already activated");
 
     aListener->Register(this);
-    mInactiveListeners.AppendElement(aListener);
+    mInactiveListeners.AppendElement(std::move(aListener));
   }
 
   /**
    * Activates an already registered and inactive gUM source listener for this
    * WindowListener.
    */
-  void Activate(SourceListener* aListener, SourceMediaStream* aStream,
-                MediaDevice* aAudioDevice, MediaDevice* aVideoDevice) {
+  void Activate(RefPtr<SourceListener> aListener,
+                RefPtr<SourceMediaStream> aStream,
+                RefPtr<MediaDevice> aAudioDevice,
+                RefPtr<MediaDevice> aVideoDevice) {
     MOZ_ASSERT(NS_IsMainThread());
     MOZ_ASSERT(aListener);
     MOZ_ASSERT(!aListener->Activated());
     MOZ_ASSERT(mInactiveListeners.Contains(aListener),
                "Must be registered to activate");
     MOZ_ASSERT(!mActiveListeners.Contains(aListener), "Already activated");
 
     mInactiveListeners.RemoveElement(aListener);
-    aListener->Activate(aStream, aAudioDevice, aVideoDevice);
-    mActiveListeners.AppendElement(do_AddRef(aListener));
+    aListener->Activate(std::move(aStream), std::move(aAudioDevice),
+                        std::move(aVideoDevice));
+    mActiveListeners.AppendElement(std::move(aListener));
   }
 
   /**
    * Removes all SourceListeners from this window listener.
    * Removes this window listener from the list of active windows, so callers
    * need to make sure to hold a strong reference.
    */
   void RemoveAll() {
@@ -545,32 +549,33 @@ class GetUserMediaWindowListener {
 
     MOZ_ASSERT(windowListener == this,
                "There should only be one window listener per window ID");
 
     LOG("GUMWindowListener %p removing windowID %" PRIu64, this, mWindowID);
     mgr->RemoveWindowID(mWindowID);
   }
 
-  bool Remove(SourceListener* aListener) {
+  bool Remove(const RefPtr<SourceListener>& aListener) {
     MOZ_ASSERT(NS_IsMainThread());
 
     if (!mInactiveListeners.RemoveElement(aListener) &&
         !mActiveListeners.RemoveElement(aListener)) {
       return false;
     }
 
     MOZ_ASSERT(!mInactiveListeners.Contains(aListener),
                "A SourceListener should only be once in one of "
                "mInactiveListeners and mActiveListeners");
     MOZ_ASSERT(!mActiveListeners.Contains(aListener),
                "A SourceListener should only be once in one of "
                "mInactiveListeners and mActiveListeners");
 
-    LOG("GUMWindowListener %p stopping SourceListener %p.", this, aListener);
+    LOG("GUMWindowListener %p stopping SourceListener %p.", this,
+        aListener.get());
     aListener->Stop();
 
     if (MediaDevice* removedDevice = aListener->GetVideoDevice()) {
       bool revokeVideoPermission = true;
       nsString removedRawId;
       nsString removedSourceType;
       removedDevice->GetRawId(removedRawId);
       removedDevice->GetMediaSource(removedSourceType);
@@ -962,54 +967,56 @@ static const MediaTrackConstraints& GetI
  * though that would complicate the constructors some.  Currently the
  * GetUserMedia spec does not allow for more than 2 streams to be obtained in
  * one call, to simplify handling of constraints.
  */
 class GetUserMediaStreamRunnable : public Runnable {
  public:
   GetUserMediaStreamRunnable(
       MozPromiseHolder<MediaManager::StreamPromise>&& aHolder,
-      uint64_t aWindowID, GetUserMediaWindowListener* aWindowListener,
-      SourceListener* aSourceListener, const ipc::PrincipalInfo& aPrincipalInfo,
-      const MediaStreamConstraints& aConstraints, MediaDevice* aAudioDevice,
-      MediaDevice* aVideoDevice, PeerIdentity* aPeerIdentity, bool aIsChrome)
+      uint64_t aWindowID, RefPtr<GetUserMediaWindowListener> aWindowListener,
+      RefPtr<SourceListener> aSourceListener,
+      const ipc::PrincipalInfo& aPrincipalInfo,
+      const MediaStreamConstraints& aConstraints,
+      RefPtr<MediaDevice> aAudioDevice, RefPtr<MediaDevice> aVideoDevice,
+      RefPtr<PeerIdentity> aPeerIdentity, bool aIsChrome)
       : Runnable("GetUserMediaStreamRunnable"),
         mHolder(std::move(aHolder)),
         mConstraints(aConstraints),
-        mAudioDevice(aAudioDevice),
-        mVideoDevice(aVideoDevice),
+        mAudioDevice(std::move(aAudioDevice)),
+        mVideoDevice(std::move(aVideoDevice)),
         mWindowID(aWindowID),
-        mWindowListener(aWindowListener),
-        mSourceListener(aSourceListener),
+        mWindowListener(std::move(aWindowListener)),
+        mSourceListener(std::move(aSourceListener)),
         mPrincipalInfo(aPrincipalInfo),
-        mPeerIdentity(aPeerIdentity),
+        mPeerIdentity(std::move(aPeerIdentity)),
         mManager(MediaManager::GetInstance()) {}
 
   ~GetUserMediaStreamRunnable() {
     mHolder.RejectIfExists(
         MakeRefPtr<MediaMgrError>(MediaMgrError::Name::AbortError), __func__);
   }
 
   class TracksCreatedListener : public MediaStreamTrackListener {
    public:
     TracksCreatedListener(
-        MediaManager* aManager,
+        RefPtr<MediaManager> aManager,
         MozPromiseHolder<MediaManager::StreamPromise>&& aHolder,
-        GetUserMediaWindowListener* aWindowListener, uint64_t aWindowID,
-        DOMMediaStream* aStream, MediaStreamTrack* aTrack,
+        RefPtr<GetUserMediaWindowListener> aWindowListener, uint64_t aWindowID,
+        RefPtr<DOMMediaStream> aStream, RefPtr<MediaStreamTrack> aTrack,
         RefPtr<GenericNonExclusivePromise>&& aFirstFramePromise)
-        : mWindowListener(aWindowListener),
+        : mWindowListener(std::move(aWindowListener)),
           mHolder(std::move(aHolder)),
-          mManager(aManager),
+          mManager(std::move(aManager)),
           mWindowID(aWindowID),
           mGraph(aTrack->GraphImpl()),
           mStream(new nsMainThreadPtrHolder<DOMMediaStream>(
-              "TracksCreatedListener::mStream", aStream)),
+              "TracksCreatedListener::mStream", aStream.forget())),
           mTrack(new nsMainThreadPtrHolder<MediaStreamTrack>(
-              "TracksCreatedListener::mTrack", aTrack)),
+              "TracksCreatedListener::mTrack", aTrack.forget())),
           mFirstFramePromise(aFirstFramePromise) {}
 
     ~TracksCreatedListener() {
       RejectIfExists(MakeRefPtr<MediaMgrError>(MediaMgrError::Name::AbortError),
                      __func__);
     }
 
     // TODO: The need for this should be removed by an upcoming refactor.
@@ -1137,23 +1144,23 @@ class GetUserMediaStreamRunnable : publi
       msg->RegisterCaptureStreamForWindow(
           mWindowID, domStream->GetInputStream()->AsProcessedStream());
       window->SetAudioCapture(true);
     } else {
       class LocalTrackSource : public MediaStreamTrackSource {
        public:
         LocalTrackSource(nsIPrincipal* aPrincipal, const nsString& aLabel,
                          const RefPtr<SourceListener>& aListener,
-                         const MediaSourceEnum aSource, const TrackID aTrackID,
-                         const PeerIdentity* aPeerIdentity)
+                         MediaSourceEnum aSource, TrackID aTrackID,
+                         RefPtr<PeerIdentity> aPeerIdentity)
             : MediaStreamTrackSource(aPrincipal, aLabel),
               mListener(aListener.get()),
               mSource(aSource),
               mTrackID(aTrackID),
-              mPeerIdentity(aPeerIdentity) {}
+              mPeerIdentity(std::move(aPeerIdentity)) {}
 
         MediaSourceEnum GetMediaSource() const override { return mSource; }
 
         const PeerIdentity* GetPeerIdentity() const override {
           return mPeerIdentity;
         }
 
         RefPtr<MediaStreamTrackSource::ApplyConstraintsPromise>
@@ -1455,28 +1462,28 @@ RefPtr<MediaManager::BadConstraintsPromi
  * Do not run this on the main thread. The success and error callbacks *MUST*
  * be dispatched on the main thread!
  */
 class GetUserMediaTask : public Runnable {
  public:
   GetUserMediaTask(const MediaStreamConstraints& aConstraints,
                    MozPromiseHolder<MediaManager::StreamPromise>&& aHolder,
                    uint64_t aWindowID,
-                   GetUserMediaWindowListener* aWindowListener,
-                   SourceListener* aSourceListener,
+                   RefPtr<GetUserMediaWindowListener> aWindowListener,
+                   RefPtr<SourceListener> aSourceListener,
                    const MediaEnginePrefs& aPrefs,
                    const ipc::PrincipalInfo& aPrincipalInfo, bool aIsChrome,
                    RefPtr<MediaManager::MediaDeviceSetRefCnt>&& aMediaDeviceSet,
                    bool aShouldFocusSource)
       : Runnable("GetUserMediaTask"),
         mConstraints(aConstraints),
         mHolder(std::move(aHolder)),
         mWindowID(aWindowID),
-        mWindowListener(aWindowListener),
-        mSourceListener(aSourceListener),
+        mWindowListener(std::move(aWindowListener)),
+        mSourceListener(std::move(aSourceListener)),
         mPrefs(aPrefs),
         mPrincipalInfo(aPrincipalInfo),
         mIsChrome(aIsChrome),
         mShouldFocusSource(aShouldFocusSource),
         mDeviceChosen(false),
         mMediaDeviceSet(aMediaDeviceSet),
         mManager(MediaManager::GetInstance()) {}
 
@@ -1602,24 +1609,24 @@ class GetUserMediaTask : public Runnable
 
   nsresult SetContraints(const MediaStreamConstraints& aConstraints) {
     mConstraints = aConstraints;
     return NS_OK;
   }
 
   const MediaStreamConstraints& GetConstraints() { return mConstraints; }
 
-  nsresult SetAudioDevice(MediaDevice* aAudioDevice) {
-    mAudioDevice = aAudioDevice;
+  nsresult SetAudioDevice(RefPtr<MediaDevice> aAudioDevice) {
+    mAudioDevice = std::move(aAudioDevice);
     mDeviceChosen = true;
     return NS_OK;
   }
 
-  nsresult SetVideoDevice(MediaDevice* aVideoDevice) {
-    mVideoDevice = aVideoDevice;
+  nsresult SetVideoDevice(RefPtr<MediaDevice> aVideoDevice) {
+    mVideoDevice = std::move(aVideoDevice);
     mDeviceChosen = true;
     return NS_OK;
   }
 
   uint64_t GetWindowID() { return mWindowID; }
 
  private:
   MediaStreamConstraints mConstraints;
@@ -3413,28 +3420,28 @@ void MediaManager::RemoveMediaDevicesCal
         DeviceChangeCallback::RemoveDeviceChangeCallbackLocked(observer);
         return;
       }
     }
   }
 }
 
 void MediaManager::AddWindowID(uint64_t aWindowId,
-                               GetUserMediaWindowListener* aListener) {
+                               RefPtr<GetUserMediaWindowListener> aListener) {
   MOZ_ASSERT(NS_IsMainThread());
   // Store the WindowID in a hash table and mark as active. The entry is removed
   // when this window is closed or navigated away from.
   // This is safe since we're on main-thread, and the windowlist can only
   // be invalidated from the main-thread (see OnNavigation)
   if (IsWindowStillActive(aWindowId)) {
     MOZ_ASSERT(false, "Window already added");
     return;
   }
 
-  GetActiveWindows()->Put(aWindowId, aListener);
+  GetActiveWindows()->Put(aWindowId, aListener.forget());
 }
 
 void MediaManager::RemoveWindowID(uint64_t aWindowId) {
   mActiveWindows.Remove(aWindowId);
 
   // get outer windowID
   auto* window = nsGlobalWindowInner::GetInnerWindowWithId(aWindowId);
   if (!window) {
@@ -3457,17 +3464,17 @@ void MediaManager::RemoveWindowID(uint64
 
   nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
   obs->NotifyObservers(nullptr, "recording-window-ended", data.get());
   LOG("Sent recording-window-ended for window %" PRIu64 " (outer %" PRIu64 ")",
       aWindowId, outerID);
 }
 
 bool MediaManager::IsWindowListenerStillActive(
-    GetUserMediaWindowListener* aListener) {
+    const RefPtr<GetUserMediaWindowListener>& aListener) {
   MOZ_DIAGNOSTIC_ASSERT(aListener);
   return aListener && aListener == GetWindowListener(aListener->WindowID());
 }
 
 void MediaManager::GetPref(nsIPrefBranch* aBranch, const char* aPref,
                            const char* aData, int32_t* aVal) {
   int32_t temp;
   if (aData == nullptr || strcmp(aPref, aData) == 0) {
@@ -3581,20 +3588,20 @@ void MediaManager::Shutdown() {
   // All that remains is shutting down the media thread.
   sHasShutdown = true;
 
   // 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 Runnable {
    public:
-    ShutdownTask(MediaManager* aManager, already_AddRefed<Runnable> aReply)
+    ShutdownTask(RefPtr<MediaManager> aManager, RefPtr<Runnable> aReply)
         : mozilla::Runnable("ShutdownTask"),
-          mManager(aManager),
-          mReply(aReply) {}
+          mManager(std::move(aManager)),
+          mReply(std::move(aReply)) {}
 
    private:
     NS_IMETHOD
     Run() override {
       LOG("MediaManager Thread Shutdown");
       MOZ_ASSERT(MediaManager::IsInMediaThread());
       // Must shutdown backend on MediaManager thread, since that's where we
       // started it from!
@@ -4060,44 +4067,45 @@ void SourceListener::Register(GetUserMed
   MOZ_ASSERT(aListener, "No listener");
   MOZ_ASSERT(!mWindowListener, "Already registered");
   MOZ_ASSERT(!Activated(), "Already activated");
 
   mPrincipalHandle = aListener->GetPrincipalHandle();
   mWindowListener = aListener;
 }
 
-void SourceListener::Activate(SourceMediaStream* aStream,
-                              MediaDevice* aAudioDevice,
-                              MediaDevice* aVideoDevice) {
+void SourceListener::Activate(RefPtr<SourceMediaStream> aStream,
+                              RefPtr<MediaDevice> aAudioDevice,
+                              RefPtr<MediaDevice> aVideoDevice) {
   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread");
 
-  LOG("SourceListener %p activating audio=%p video=%p", this, aAudioDevice,
-      aVideoDevice);
+  LOG("SourceListener %p activating audio=%p video=%p", this,
+      aAudioDevice.get(), aVideoDevice.get());
 
   MOZ_ASSERT(!mStopped, "Cannot activate stopped source listener");
   MOZ_ASSERT(!Activated(), "Already activated");
 
   mMainThreadCheck = GetCurrentVirtualThread();
-  mStream = aStream;
+  mStream = std::move(aStream);
   if (aAudioDevice) {
-    mAudioDeviceState = MakeUnique<DeviceState>(
-        aAudioDevice,
+    bool offWhileDisabled =
         aAudioDevice->GetMediaSource() == dom::MediaSourceEnum::Microphone &&
-            Preferences::GetBool(
-                "media.getusermedia.microphone.off_while_disabled.enabled",
-                true));
+        Preferences::GetBool(
+            "media.getusermedia.microphone.off_while_disabled.enabled", true);
+    mAudioDeviceState =
+        MakeUnique<DeviceState>(std::move(aAudioDevice), offWhileDisabled);
   }
 
   if (aVideoDevice) {
-    mVideoDeviceState = MakeUnique<DeviceState>(
-        aVideoDevice,
+    bool offWhileDisabled =
         aVideoDevice->GetMediaSource() == dom::MediaSourceEnum::Camera &&
-            Preferences::GetBool(
-                "media.getusermedia.camera.off_while_disabled.enabled", true));
+        Preferences::GetBool(
+            "media.getusermedia.camera.off_while_disabled.enabled", true);
+    mVideoDeviceState =
+        MakeUnique<DeviceState>(std::move(aVideoDevice), offWhileDisabled);
   }
 }
 
 RefPtr<SourceListener::SourceListenerPromise>
 SourceListener::InitializeAsync() {
   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread");
   MOZ_DIAGNOSTIC_ASSERT(!mStopped);
 
--- a/dom/media/MediaManager.h
+++ b/dom/media/MediaManager.h
@@ -178,26 +178,25 @@ class MediaManager final : public nsIMed
   WindowTable* GetActiveWindows() {
     MOZ_ASSERT(NS_IsMainThread());
     return &mActiveWindows;
   }
   GetUserMediaWindowListener* GetWindowListener(uint64_t aWindowId) {
     MOZ_ASSERT(NS_IsMainThread());
     return mActiveWindows.GetWeak(aWindowId);
   }
-  void AddWindowID(uint64_t aWindowId, GetUserMediaWindowListener* aListener);
+  void AddWindowID(uint64_t aWindowId,
+                   RefPtr<GetUserMediaWindowListener> aListener);
   void RemoveWindowID(uint64_t aWindowId);
   void SendPendingGUMRequest();
   bool IsWindowStillActive(uint64_t aWindowId) {
     return !!GetWindowListener(aWindowId);
   }
-  bool IsWindowListenerStillActive(GetUserMediaWindowListener* aListener);
-  // Note: also calls aListener->Remove(), even if inactive
-  void RemoveFromWindowList(uint64_t aWindowID,
-                            GetUserMediaWindowListener* aListener);
+  bool IsWindowListenerStillActive(
+      const RefPtr<GetUserMediaWindowListener>& aListener);
 
   typedef dom::NavigatorUserMediaSuccessCallback GetUserMediaSuccessCallback;
   typedef dom::NavigatorUserMediaErrorCallback GetUserMediaErrorCallback;
 
   MOZ_CAN_RUN_SCRIPT
   static void CallOnError(GetUserMediaErrorCallback& aCallback,
                           dom::MediaStreamError& aError);
   MOZ_CAN_RUN_SCRIPT