Bug 1538727 - Move TrackListener and track-PrincipalChangeObserver removal to Stop(). r=bryce
authorAndreas Pehrson <apehrson@mozilla.com>
Tue, 30 Apr 2019 18:39:21 +0000
changeset 530822 eabc25a9ff765345ca75887af521696951da6694
parent 530821 1fdcbbc79e87fbad2984a6e1f047e71d5cd6c4f8
child 530823 b081558961139b81c626b6d37055d4c60204a013
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbryce
bugs1538727
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 1538727 - Move TrackListener and track-PrincipalChangeObserver removal to Stop(). r=bryce These are added in start, and we get into unexpected state if they notify us after Stop() (when MediaEncoder shuts down and internally removes the tracks it is encoding) but before Shutdown() when we remove the listeners. This is not symmetrical. The proper thing to do is to remove these listeners in Stop() as well. Depends on D29080 Differential Revision: https://phabricator.services.mozilla.com/D29081
dom/media/MediaRecorder.cpp
--- a/dom/media/MediaRecorder.cpp
+++ b/dom/media/MediaRecorder.cpp
@@ -567,16 +567,29 @@ class MediaRecorder::Session : public Pr
   void Stop() {
     LOG(LogLevel::Debug, ("Session.Stop %p", this));
     MOZ_ASSERT(NS_IsMainThread());
 
     if (mEncoder) {
       mEncoder->Stop();
     }
 
+    // Remove main thread state added in Start().
+    if (mMediaStream) {
+      mMediaStream->UnregisterTrackListener(this);
+      mMediaStream = nullptr;
+    }
+
+    {
+      auto tracks(std::move(mMediaStreamTracks));
+      for (RefPtr<MediaStreamTrack>& track : tracks) {
+        track->RemovePrincipalChangeObserver(this);
+      }
+    }
+
     if (mRunningState.isOk() &&
         mRunningState.unwrap() == RunningState::Idling) {
       LOG(LogLevel::Debug, ("Session.Stop Explicit end task %p", this));
       // End the Session directly if there is no ExtractRunnable.
       DoSessionEndTask(NS_OK);
     } else if (mRunningState.isOk() &&
                (mRunningState.unwrap() == RunningState::Starting ||
                 mRunningState.unwrap() == RunningState::Running)) {
@@ -1123,17 +1136,17 @@ class MediaRecorder::Session : public Pr
             return ShutdownPromise::CreateAndResolve(true, __func__);
           },
           []() {
             MOZ_ASSERT_UNREACHABLE("Unexpected reject");
             return ShutdownPromise::CreateAndReject(false, __func__);
           });
     }
 
-    // Remove main thread state.
+    // Remove main thread state. This could be needed if Stop() wasn't called.
     if (mMediaStream) {
       mMediaStream->UnregisterTrackListener(this);
       mMediaStream = nullptr;
     }
 
     {
       auto tracks(std::move(mMediaStreamTracks));
       for (RefPtr<MediaStreamTrack>& track : tracks) {