Bug 1172394 - Make dom::MediaTrack lifetime spec compliant. r=bryce
☠☠ backed out by 7272d77d4e80 ☠ ☠
authorAndreas Pehrson <apehrson@mozilla.com>
Wed, 13 Nov 2019 08:48:16 +0000
changeset 501785 89ad0b553b0f657612e3082a9c37f36221bb38c6
parent 501784 744fb77a58333b632dbf6820c77c7d8e97674b2c
child 501786 a796541fe5ef045738f1c10d95830b71fc5c024d
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbryce
bugs1172394
milestone72.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 1172394 - Make dom::MediaTrack lifetime spec compliant. r=bryce This makes us forget tracks at the right times. The spec also says no removetrack events should be fired because of this, yet it seems to be something other user agents do: https://wpt.fyi/results/media-source/mediasource-avtracks.html This is of low importance however, since MediaTracks are prefed off by default. Differential Revision: https://phabricator.services.mozilla.com/D52038
dom/html/HTMLMediaElement.cpp
dom/html/HTMLMediaElement.h
dom/media/MediaDecoder.cpp
dom/media/MediaDecoderOwner.h
dom/media/test/test_mediatrack_consuming_mediaresource.html
dom/media/test/test_mediatrack_replay_from_end.html
--- a/dom/html/HTMLMediaElement.cpp
+++ b/dom/html/HTMLMediaElement.cpp
@@ -1912,27 +1912,16 @@ void HTMLMediaElement::AbortExistingLoad
   bool fireTimeUpdate = false;
 
   // We need to remove FirstFrameListener before VideoTracks get emptied.
   if (mFirstFrameListener) {
     mSelectedVideoStreamTrack->RemoveVideoOutput(mFirstFrameListener);
     mFirstFrameListener = nullptr;
   }
 
-  // When aborting the existing loads, empty the objects in audio track list and
-  // video track list, no events (in particular, no removetrack events) are
-  // fired as part of this. Ending MediaTrack sends track ended notifications,
-  // so we empty the track lists prior.
-  if (AudioTracks()) {
-    AudioTracks()->EmptyTracks();
-  }
-  if (VideoTracks()) {
-    VideoTracks()->EmptyTracks();
-  }
-
   if (mDecoder) {
     fireTimeUpdate = mDecoder->GetCurrentTime() != 0.0;
     ShutdownDecoder();
   }
   if (mSrcStream) {
     EndSrcMediaStreamPlayback();
   }
 
@@ -1974,16 +1963,17 @@ void HTMLMediaElement::AbortExistingLoad
                  "How did someone setup a new stream/decoder already?");
     // ChangeNetworkState() will call UpdateAudioChannelPlayingState()
     // indirectly which depends on mPaused. So we need to update mPaused first.
     if (!mPaused) {
       mPaused = true;
       RejectPromises(TakePendingPlayPromises(), NS_ERROR_DOM_MEDIA_ABORT_ERR);
     }
     ChangeNetworkState(NETWORK_EMPTY);
+    RemoveMediaTracks();
     ChangeReadyState(HAVE_NOTHING);
 
     // TODO: Apply the rules for text track cue rendering Bug 865407
     if (mTextTrackManager) {
       mTextTrackManager->GetTextTracks()->SetCuesInactive();
     }
 
     if (fireTimeUpdate) {
@@ -2021,16 +2011,17 @@ void HTMLMediaElement::AbortExistingLoad
 }
 
 void HTMLMediaElement::NoSupportedMediaSourceError(
     const nsACString& aErrorDetails) {
   if (mDecoder) {
     ShutdownDecoder();
   }
   mErrorSink->SetError(MEDIA_ERR_SRC_NOT_SUPPORTED, aErrorDetails);
+  RemoveMediaTracks();
   ChangeDelayLoadStatus(false);
   UpdateAudioChannelPlayingState();
   RejectPromises(TakePendingPlayPromises(),
                  NS_ERROR_DOM_MEDIA_NOT_SUPPORTED_ERR);
 }
 
 typedef void (HTMLMediaElement::*SyncSectionFn)();
 
@@ -2466,16 +2457,18 @@ void HTMLMediaElement::DealWithFailedEle
 void HTMLMediaElement::LoadFromSourceChildren() {
   NS_ASSERTION(mDelayingLoadEvent,
                "Should delay load event (if in document) during load");
   NS_ASSERTION(mIsLoadingFromSourceChildren,
                "Must remember we're loading from source children");
 
   AddMutationObserverUnlessExists(this);
 
+  RemoveMediaTracks();
+
   while (true) {
     Element* child = GetNextSource();
     if (!child) {
       // Exhausted candidates, wait for more candidates to be appended to
       // the media element.
       mLoadWaitStatus = WAITING_FOR_SOURCE;
       ChangeNetworkState(NETWORK_NO_SOURCE);
       ChangeDelayLoadStatus(false);
@@ -5114,16 +5107,20 @@ void HTMLMediaElement::ProcessMediaFragm
     mFragmentStart = parser.GetStartTime();
   }
 }
 
 void HTMLMediaElement::MetadataLoaded(const MediaInfo* aInfo,
                                       UniquePtr<const MetadataTags> aTags) {
   MOZ_ASSERT(NS_IsMainThread());
 
+  if (mDecoder) {
+    ConstructMediaTracks(aInfo);
+  }
+
   SetMediaInfo(*aInfo);
 
   mIsEncrypted =
       aInfo->IsEncrypted() || mPendingEncryptedInitData.IsEncrypted();
   mTags = std::move(aTags);
   mLoadedDataFired = false;
   ChangeReadyState(HAVE_METADATA);
 
@@ -5229,22 +5226,16 @@ void HTMLMediaElement::DecodeError(const
   nsAutoString src;
   GetCurrentSrc(src);
   AutoTArray<nsString, 1> params = {src};
   ReportLoadError("MediaLoadDecodeError", params);
 
   DecoderDoctorDiagnostics diagnostics;
   diagnostics.StoreDecodeError(OwnerDoc(), aError, src, __func__);
 
-  if (AudioTracks()) {
-    AudioTracks()->EmptyTracks();
-  }
-  if (VideoTracks()) {
-    VideoTracks()->EmptyTracks();
-  }
   if (mIsLoadingFromSourceChildren) {
     mErrorSink->ResetError();
     if (mSourceLoadCandidate) {
       DispatchAsyncSourceError(mSourceLoadCandidate);
       QueueLoadFromSourceTask();
     } else {
       NS_WARNING("Should know the source we were loading from!");
     }
@@ -7236,22 +7227,20 @@ bool HTMLMediaElement::IsAudible() const
   if (mMuted || (std::fabs(Volume()) <= 1e-7)) {
     return false;
   }
 
   return mIsAudioTrackAudible;
 }
 
 void HTMLMediaElement::ConstructMediaTracks(const MediaInfo* aInfo) {
-  if (mMediaTracksConstructed || !aInfo) {
+  if (!aInfo) {
     return;
   }
 
-  mMediaTracksConstructed = true;
-
   AudioTrackList* audioList = AudioTracks();
   if (audioList && aInfo->HasAudio()) {
     const TrackInfo& info = aInfo->mAudio;
     RefPtr<AudioTrack> track = MediaTrackList::CreateAudioTrack(
         audioList->GetOwnerGlobal(), info.mId, info.mKind, info.mLabel,
         info.mLanguage, info.mEnabled);
 
     audioList->AddTrack(track);
@@ -7268,22 +7257,19 @@ void HTMLMediaElement::ConstructMediaTra
     track->SetEnabledInternal(info.mEnabled, MediaTrack::FIRE_NO_EVENTS);
   }
 }
 
 void HTMLMediaElement::RemoveMediaTracks() {
   if (mAudioTrackList) {
     mAudioTrackList->RemoveTracks();
   }
-
   if (mVideoTrackList) {
     mVideoTrackList->RemoveTracks();
   }
-
-  mMediaTracksConstructed = false;
 }
 
 class MediaElementGMPCrashHelper : public GMPCrashHelper {
  public:
   explicit MediaElementGMPCrashHelper(HTMLMediaElement* aElement)
       : mElement(aElement) {
     MOZ_ASSERT(NS_IsMainThread());  // WeakPtr isn't thread safe.
   }
--- a/dom/html/HTMLMediaElement.h
+++ b/dom/html/HTMLMediaElement.h
@@ -688,20 +688,16 @@ class HTMLMediaElement : public nsGeneri
     CREATE_PATTERN,
     CREATE_IMAGEBITMAP,
     CAPTURE_STREAM,
   };
   void MarkAsContentSource(CallerAPI aAPI);
 
   Document* GetDocument() const override;
 
-  void ConstructMediaTracks(const MediaInfo* aInfo) override;
-
-  void RemoveMediaTracks() override;
-
   already_AddRefed<GMPCrashHelper> CreateGMPCrashHelper() override;
 
   nsISerialEventTarget* MainThreadEventTarget() {
     return mMainThreadEventTarget;
   }
 
   // Set the sink id (of the output device) that the audio will play. If aSinkId
   // is empty the default device will be set.
@@ -1244,16 +1240,28 @@ class HTMLMediaElement : public nsGeneri
   // and queues a task to resolve them also to dispatch a "playing" event.
   void NotifyAboutPlaying();
 
   already_AddRefed<Promise> CreateDOMPromise(ErrorResult& aRv) const;
 
   // Pass information for deciding the video decode mode to decoder.
   void NotifyDecoderActivityChanges() const;
 
+  // Constructs an AudioTrack in mAudioTrackList if aInfo reports that audio is
+  // available, and a VideoTrack in mVideoTrackList if aInfo reports that video
+  // is available.
+  void ConstructMediaTracks(const MediaInfo* aInfo);
+
+  // Removes all MediaTracks from mAudioTrackList and mVideoTrackList and fires
+  // "removetrack" on the lists accordingly.
+  // Note that by spec, this should not fire "removetrack". However, it appears
+  // other user agents do, per
+  // https://wpt.fyi/results/media-source/mediasource-avtracks.html.
+  void RemoveMediaTracks();
+
   // Mark the decoder owned by the element as tainted so that the
   // suspend-video-decoder is disabled.
   void MarkAsTainted();
 
   virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName,
                                 const nsAttrValue* aValue,
                                 const nsAttrValue* aOldValue,
                                 nsIPrincipal* aMaybeScriptedPrincipal,
@@ -1802,20 +1810,16 @@ class HTMLMediaElement : public nsGeneri
   // True if media element has been marked as 'tainted' and can't
   // participate in video decoder suspending.
   bool mHasSuspendTaint = false;
 
   // True if media element has been forced into being considered 'hidden'.
   // For use by mochitests. Enabling pref "media.test.video-suspend"
   bool mForcedHidden = false;
 
-  // True if audio tracks and video tracks are constructed and added into the
-  // track list, false if all tracks are removed from the track list.
-  bool mMediaTracksConstructed = false;
-
   Visibility mVisibilityState = Visibility::Untracked;
 
   UniquePtr<ErrorSink> mErrorSink;
 
   // This wrapper will handle all audio channel related stuffs, eg. the
   // operations of tab audio indicator, Fennec's media control. Note:
   // mAudioChannelWrapper might be null after GC happened.
   RefPtr<AudioChannelAgentCallback> mAudioChannelWrapper;
--- a/dom/media/MediaDecoder.cpp
+++ b/dom/media/MediaDecoder.cpp
@@ -375,19 +375,16 @@ void MediaDecoder::Shutdown() {
     nsCOMPtr<nsIRunnable> r =
         NS_NewRunnableFunction("MediaDecoder::Shutdown", [self]() {
           self->mVideoFrameContainer = nullptr;
           MediaShutdownManager::Instance().Unregister(self);
         });
     mAbstractMainThread->Dispatch(r.forget());
   }
 
-  // Ask the owner to remove its audio/video tracks.
-  GetOwner()->RemoveMediaTracks();
-
   ChangeState(PLAY_STATE_SHUTDOWN);
   mVideoDecodingOberver->UnregisterEvent();
   mVideoDecodingOberver = nullptr;
   mOwner = nullptr;
 }
 
 void MediaDecoder::NotifyXPCOMShutdown() {
   MOZ_ASSERT(NS_IsMainThread());
@@ -659,17 +656,16 @@ void MediaDecoder::MetadataLoaded(
   LOG("MetadataLoaded, channels=%u rate=%u hasAudio=%d hasVideo=%d",
       aInfo->mAudio.mChannels, aInfo->mAudio.mRate, aInfo->HasAudio(),
       aInfo->HasVideo());
 
   mMediaSeekable = aInfo->mMediaSeekable;
   mMediaSeekableOnlyInBufferedRanges =
       aInfo->mMediaSeekableOnlyInBufferedRanges;
   mInfo = aInfo.release();
-  GetOwner()->ConstructMediaTracks(mInfo);
   mDecoderStateMachine->EnsureOutputStreamManagerHasTracks(*mInfo);
 
   // Make sure the element and the frame (if any) are told about
   // our new size.
   if (aEventVisibility != MediaDecoderEventVisibility::Suppressed) {
     mFiredMetadataLoaded = true;
     GetOwner()->MetadataLoaded(mInfo, std::move(aTags));
   }
@@ -851,22 +847,16 @@ void MediaDecoder::ChangeState(PlayState
   if (mNextState == aState) {
     mNextState = PLAY_STATE_PAUSED;
   }
 
   if (mPlayState != aState) {
     DDLOG(DDLogCategory::Property, "play_state", ToPlayStateStr(aState));
   }
   mPlayState = aState;
-
-  if (mPlayState == PLAY_STATE_PLAYING) {
-    GetOwner()->ConstructMediaTracks(mInfo);
-  } else if (IsEnded()) {
-    GetOwner()->RemoveMediaTracks();
-  }
 }
 
 bool MediaDecoder::IsLoopingBack(double aPrevPos, double aCurPos) const {
   // If current position is early than previous position and we didn't do seek,
   // that means we looped back to the start position.
   return mLooping && !mSeekRequest.Exists() && aCurPos < aPrevPos;
 }
 
--- a/dom/media/MediaDecoderOwner.h
+++ b/dom/media/MediaDecoderOwner.h
@@ -134,24 +134,16 @@ class MediaDecoderOwner {
   virtual void NotifyXPCOMShutdown() = 0;
 
   // Dispatches a "encrypted" event to the HTMLMediaElement, with the
   // provided init data. Actual dispatch may be delayed until HAVE_METADATA.
   // Main thread only.
   virtual void DispatchEncrypted(const nsTArray<uint8_t>& aInitData,
                                  const nsAString& aInitDataType) = 0;
 
-  // Called by the media decoder to create audio/video tracks and add to its
-  // owner's track list.
-  virtual void ConstructMediaTracks(const MediaInfo* aInfo) = 0;
-
-  // Called by the media decoder to removes all audio/video tracks from its
-  // owner's track list.
-  virtual void RemoveMediaTracks() = 0;
-
   // Notified by the decoder that a decryption key is required before emitting
   // further output.
   virtual void NotifyWaitingForKey() {}
 
   /*
    * Methods that are used only in Gecko go here. We provide defaul
    * implementations so they can compile in Servo without modification.
    */
--- a/dom/media/test/test_mediatrack_consuming_mediaresource.html
+++ b/dom/media/test/test_mediatrack_consuming_mediaresource.html
@@ -5,29 +5,29 @@
   <script src="/tests/SimpleTest/SimpleTest.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
   <script type="text/javascript" src="manifest.js"></script>
 </head>
 <body>
 <pre id="test">
 <script class="testbody" type="text/javascript">
 
-var manager = new MediaTestManager;
+const manager = new MediaTestManager;
 
 function startTest(test, token) {
-  var elemType = getMajorMimeType(test.type);
-  var element = document.createElement(elemType);
+  const elemType = getMajorMimeType(test.type);
+  const element = document.createElement(elemType);
 
-  var audioOnchange = 0;
-  var audioOnaddtrack = 0;
-  var audioOnremovetrack = 0;
-  var videoOnchange = 0;
-  var videoOnaddtrack = 0;
-  var videoOnremovetrack = 0;
-  var isPlaying = false;
+  let audioOnchange = 0;
+  let audioOnaddtrack = 0;
+  let audioOnremovetrack = 0;
+  let videoOnchange = 0;
+  let videoOnaddtrack = 0;
+  let videoOnremovetrack = 0;
+  let isPlaying = false;
 
   isnot(element.audioTracks, undefined,
         'HTMLMediaElement::AudioTracks() property should be available.');
   isnot(element.videoTracks, undefined,
         'HTMLMediaElement::VideoTracks() property should be available.');
 
   element.audioTracks.onaddtrack = function(e) {
     audioOnaddtrack++;
@@ -48,36 +48,53 @@ function startTest(test, token) {
   element.videoTracks.onremovetrack = function(e) {
     videoOnremovetrack++;
   }
 
   element.videoTracks.onchange = function(e) {
     videoOnchange++;
   }
 
-  function checkTrackRemoved() {
+  function checkTrackNotRemoved() {
+    is(audioOnremovetrack, 0, 'Should have no calls of onremovetrack on audioTracks.');
+    is(videoOnremovetrack, 0, 'Should have no calls of onremovetrack on videoTracks.');
     if (isPlaying) {
-      if (test.hasAudio) {
-        is(audioOnremovetrack, 1, 'Calls of onremovetrack on audioTracks should be 1.');
-        is(element.audioTracks.length, 0, 'The length of audioTracks should be 0.');
-      }
-      if (test.hasVideo) {
-        is(videoOnremovetrack, 1, 'Calls of onremovetrack on videoTracks should be 1.');
-        is(element.videoTracks.length, 0, 'The length of videoTracks should be 0.');
-      }
+      is(element.audioTracks.length, test.hasAudio ? 1 : 0,
+        'Expected length of audioTracks.');
+      is(element.videoTracks.length, test.hasVideo ? 1 : 0,
+        'Expected length of videoTracks.');
+    }
+  }
+
+  function checkTrackRemoved() {
+    is(element.audioTracks.length, 0, 'The length of audioTracks should be 0.');
+    is(element.videoTracks.length, 0, 'The length of videoTracks should be 0.');
+    if (isPlaying) {
+      is(audioOnremovetrack, test.hasAudio ? 1 : 0,
+        'Expected calls of onremovetrack on audioTracks.');
+      is(videoOnremovetrack, test.hasVideo ? 1 : 0,
+        'Expected calls of onremovetrack on videoTracks.');
     }
   }
 
   function onended() {
     ok(true, 'Event ended is expected to be fired on element.');
-    checkTrackRemoved();
+    checkTrackNotRemoved();
     element.onended = null;
     element.onplaying = null;
     element.onpause = null;
-    manager.finished(element.token);
+    element.src = "";
+    is(element.audioTracks.length, 0, 'audioTracks have been forgotten');
+    is(element.videoTracks.length, 0, 'videoTracks have been forgotten');
+    is(audioOnremovetrack, 0, 'No audio removetrack events yet');
+    is(videoOnremovetrack, 0, 'No video removetrack events yet');
+    setTimeout(() => {
+      checkTrackRemoved();
+      manager.finished(element.token);
+    }, 100);
   }
 
   function checkTrackAdded() {
     isPlaying = true;
     if (test.hasAudio) {
       is(audioOnaddtrack, 1, 'Calls of onaddtrack on audioTracks should be 1.');
       is(element.audioTracks.length, 1, 'The length of audioTracks should be 1.');
       ok(element.audioTracks[0].enabled, 'Audio track should be enabled as default.');
--- a/dom/media/test/test_mediatrack_replay_from_end.html
+++ b/dom/media/test/test_mediatrack_replay_from_end.html
@@ -5,38 +5,39 @@
   <script src="/tests/SimpleTest/SimpleTest.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
   <script type="text/javascript" src="manifest.js"></script>
 </head>
 <body>
 <pre id="test">
 <script class="testbody" type="text/javascript">
 
-var manager = new MediaTestManager;
+const manager = new MediaTestManager;
 
 function startTest(test, token) {
   // Scenario to test:
   // 1. Audio tracks and video tracks should be added to the track list when
-  //    playing, and all tracks should be removed from the list after we seek
-  //    to the end.
-  // 2. All tracks should be added back to the list if we replay from the end,
-  //    and all tracks should be removed from the list after we seek to the end.
-  // 3. After seek to the middle from end of playback, all tracks should be
-  //    added back to the list if we play from here, and all tracks should be
-  //    removed from the list after we seek to the end.
+  //    metadata has loaded, and all tracks should remain even after we seek to
+  //    the end.
+  // 2. No tracks should be added back to the list if we replay from the end,
+  //    and no tracks should be removed from the list after we seek to the end.
+  // 3. After seek to the middle from end of playback, all tracks should remain
+  //    in the list if we play from here, and no tracks should be removed from
+  //    the list after we seek to the end.
+  // 4. Unsetting the media element's src attribute should remove all tracks.
 
-  var elemType = getMajorMimeType(test.type);
-  var element = document.createElement(elemType);
+  const elemType = getMajorMimeType(test.type);
+  const element = document.createElement(elemType);
 
-  var audioOnaddtrack = 0;
-  var audioOnremovetrack = 0;
-  var videoOnaddtrack = 0;
-  var videoOnremovetrack = 0;
-  var isPlaying = false;
-  var steps = 0;
+  let audioOnaddtrack = 0;
+  let audioOnremovetrack = 0;
+  let videoOnaddtrack = 0;
+  let videoOnremovetrack = 0;
+  let isPlaying = false;
+  let steps = 0;
 
   element.audioTracks.onaddtrack = function(e) {
     audioOnaddtrack++;
   }
 
   element.audioTracks.onremovetrack = function(e) {
     audioOnremovetrack++;
   }
@@ -44,26 +45,33 @@ function startTest(test, token) {
   element.videoTracks.onaddtrack = function(e) {
     videoOnaddtrack++;
   }
 
   element.videoTracks.onremovetrack = function(e) {
     videoOnremovetrack++;
   }
 
-  function testTrackEventCalls(expectedCalls) {
+  function testExpectedAddtrack(expectedCalls) {
     if (test.hasAudio) {
       is(audioOnaddtrack, expectedCalls,
          'Calls of onaddtrack on audioTracks should be '+expectedCalls+' times.');
+    }
+    if (test.hasVideo) {
+      is(videoOnaddtrack, expectedCalls,
+         'Calls of onaddtrack on videoTracks should be '+expectedCalls+' times.');
+    }
+  }
+
+  function testExpectedRemovetrack(expectedCalls) {
+    if (test.hasAudio) {
       is(audioOnremovetrack, expectedCalls,
          'Calls of onremovetrack on audioTracks should be '+expectedCalls+' times.');
     }
     if (test.hasVideo) {
-      is(videoOnaddtrack, expectedCalls,
-         'Calls of onaddtrack on videoTracks should be '+expectedCalls+' times.');
       is(videoOnremovetrack, expectedCalls,
          'Calls of onremovetrack on videoTracks should be '+expectedCalls+' times.');
     }
   }
 
   function finishTesting() {
     element.onpause = null;
     element.onseeked = null;
@@ -71,31 +79,39 @@ function startTest(test, token) {
     element.onended = null;
     manager.finished(element.token);
   }
 
   function onended() {
     if (isPlaying) {
       switch(steps) {
         case 1:
-          testTrackEventCalls(1);
+          testExpectedAddtrack(1);
+          testExpectedRemovetrack(0);
           element.onplaying = onplaying;
           element.play();
           steps++;
           break;
         case 2:
-          testTrackEventCalls(2);
+          testExpectedAddtrack(1);
+          testExpectedRemovetrack(0);
           element.currentTime = element.duration * 0.5;
           element.onplaying = onplaying;
           element.play();
           steps++;
           break;
         case 3:
-          testTrackEventCalls(3);
-          finishTesting();
+          testExpectedAddtrack(1);
+          testExpectedRemovetrack(0);
+          element.src = "";
+          setTimeout(() => {
+            testExpectedAddtrack(1);
+            testExpectedRemovetrack(1);
+            finishTesting();
+          }, 0);
           break;
       }
     } else {
       ok(true, 'Finish the test anyway if ended is fired before other events.');
       finishTesting();
     }
   }