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

--HG--
extra : moz-landing-system : lando

dom/html/HTMLMediaElement.cpp

@@ -1917,17 +1917,6 @@ void HTMLMediaElement::AbortExistingLoads() {
     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();
@@ -1979,6 +1968,7 @@ void HTMLMediaElement::AbortExistingLoads() {
       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
@@ -2026,6 +2016,7 @@ void HTMLMediaElement::NoSupportedMediaSourceError(
     ShutdownDecoder();
   }
   mErrorSink->SetError(MEDIA_ERR_SRC_NOT_SUPPORTED, aErrorDetails);
+  RemoveMediaTracks();
   ChangeDelayLoadStatus(false);
   UpdateAudioChannelPlayingState();
   RejectPromises(TakePendingPlayPromises(),
@@ -2471,6 +2462,8 @@ void HTMLMediaElement::LoadFromSourceChildren() {
 
   AddMutationObserverUnlessExists(this);
 
+  RemoveMediaTracks();
+
   while (true) {
     Element* child = GetNextSource();
     if (!child) {
@@ -5119,6 +5112,10 @@ void HTMLMediaElement::MetadataLoaded(const MediaInfo* aInfo,
                                       UniquePtr<const MetadataTags> aTags) {
   MOZ_ASSERT(NS_IsMainThread());
 
+  if (mDecoder) {
+    ConstructMediaTracks(aInfo);
+  }
+
   SetMediaInfo(*aInfo);
 
   mIsEncrypted =
@@ -5234,12 +5231,6 @@ void HTMLMediaElement::DecodeError(const MediaResult& aError) {
   DecoderDoctorDiagnostics diagnostics;
   diagnostics.StoreDecodeError(OwnerDoc(), aError, src, __func__);
 
-  if (AudioTracks()) {
-    AudioTracks()->EmptyTracks();
-  }
-  if (VideoTracks()) {
-    VideoTracks()->EmptyTracks();
-  }
   if (mIsLoadingFromSourceChildren) {
     mErrorSink->ResetError();
     if (mSourceLoadCandidate) {
@@ -7241,12 +7232,10 @@ bool HTMLMediaElement::IsAudible() const {
 }
 
 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;
@@ -7273,12 +7262,9 @@ void HTMLMediaElement::RemoveMediaTracks() {
   if (mAudioTrackList) {
     mAudioTrackList->RemoveTracks();
   }
-
   if (mVideoTrackList) {
     mVideoTrackList->RemoveTracks();
   }
-
-  mMediaTracksConstructed = false;
 }
 
 class MediaElementGMPCrashHelper : public GMPCrashHelper {

dom/html/HTMLMediaElement.h

@@ -693,10 +693,6 @@ class HTMLMediaElement : public nsGenericHTMLElement,
 
   Document* GetDocument() const override;
 
-  void ConstructMediaTracks(const MediaInfo* aInfo) override;
-
-  void RemoveMediaTracks() override;
-
   already_AddRefed<GMPCrashHelper> CreateGMPCrashHelper() override;
 
   nsISerialEventTarget* MainThreadEventTarget() {
@@ -1249,6 +1245,18 @@ class HTMLMediaElement : public nsGenericHTMLElement,
   // 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();
@@ -1807,10 +1815,6 @@ class HTMLMediaElement : public nsGenericHTMLElement,
   // 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;

dom/media/MediaDecoder.cpp

@@ -380,9 +380,6 @@ void MediaDecoder::Shutdown() {
     mAbstractMainThread->Dispatch(r.forget());
   }
 
-  // Ask the owner to remove its audio/video tracks.
-  GetOwner()->RemoveMediaTracks();
-
   ChangeState(PLAY_STATE_SHUTDOWN);
   mVideoDecodingOberver->UnregisterEvent();
   mVideoDecodingOberver = nullptr;
@@ -664,7 +661,6 @@ void MediaDecoder::MetadataLoaded(
   mMediaSeekableOnlyInBufferedRanges =
       aInfo->mMediaSeekableOnlyInBufferedRanges;
   mInfo = aInfo.release();
-  GetOwner()->ConstructMediaTracks(mInfo);
   mDecoderStateMachine->EnsureOutputStreamManagerHasTracks(*mInfo);
 
   // Make sure the element and the frame (if any) are told about
@@ -856,12 +852,6 @@ void MediaDecoder::ChangeState(PlayState 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 {

dom/media/MediaDecoderOwner.h

@@ -139,14 +139,6 @@ class MediaDecoderOwner {
   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() {}

dom/media/test/test_mediatrack_consuming_mediaresource.html

@@ -10,19 +10,19 @@
 <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);
+  const elemType = getMajorMimeType(test.type);
-  var element = document.createElement(elemType);
+  const element = document.createElement(elemType);
 
-  var audioOnchange = 0;
+  let audioOnchange = 0;
-  var audioOnaddtrack = 0;
+  let audioOnaddtrack = 0;
-  var audioOnremovetrack = 0;
+  let audioOnremovetrack = 0;
-  var videoOnchange = 0;
+  let videoOnchange = 0;
-  var videoOnaddtrack = 0;
+  let videoOnaddtrack = 0;
-  var videoOnremovetrack = 0;
+  let videoOnremovetrack = 0;
-  var isPlaying = false;
+  let isPlaying = false;
 
   isnot(element.audioTracks, undefined,
         'HTMLMediaElement::AudioTracks() property should be available.');
@@ -53,26 +53,43 @@ function startTest(test, token) {
     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(element.audioTracks.length, test.hasAudio ? 1 : 0,
-        is(audioOnremovetrack, 1, 'Calls of onremovetrack on audioTracks should be 1.');
+        'Expected length of audioTracks.');
-        is(element.audioTracks.length, 0, 'The length of audioTracks should be 0.');
+      is(element.videoTracks.length, test.hasVideo ? 1 : 0,
-      }
+        'Expected length of videoTracks.');
-      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.');
+
-      }
+  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() {

dom/media/test/test_mediatrack_replay_from_end.html

@@ -10,28 +10,29 @@
 <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
+  //    metadata has loaded, and all tracks should remain even after we seek to
-  //    to the end.
+  //    the end.
-  // 2. All tracks should be added back to the list if we replay from the end,
+  // 2. No 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.
+  //    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 be
+  // 3. After seek to the middle from end of playback, all tracks should remain
-  //    added back to the list if we play from here, and all tracks should be
+  //    in the list if we play from here, and no tracks should be removed from
-  //    removed from the list after we seek to the end.
+  //    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);
+  const elemType = getMajorMimeType(test.type);
-  var element = document.createElement(elemType);
+  const element = document.createElement(elemType);
 
-  var audioOnaddtrack = 0;
+  let audioOnaddtrack = 0;
-  var audioOnremovetrack = 0;
+  let audioOnremovetrack = 0;
-  var videoOnaddtrack = 0;
+  let videoOnaddtrack = 0;
-  var videoOnremovetrack = 0;
+  let videoOnremovetrack = 0;
-  var isPlaying = false;
+  let isPlaying = false;
-  var steps = 0;
+  let steps = 0;
 
   element.audioTracks.onaddtrack = function(e) {
     audioOnaddtrack++;
@@ -49,16 +50,23 @@ function startTest(test, token) {
     videoOnremovetrack++;
   }
 
-  function testTrackEventCalls(expectedCalls) {
+  function testExpectedAddtrack(expectedCalls) {
     if (test.hasAudio) {
       is(audioOnaddtrack, expectedCalls,
          'Calls of onaddtrack on audioTracks should be '+expectedCalls+' times.');
-      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.');
+    }
+  }
+
+  function testExpectedRemovetrack(expectedCalls) {
+    if (test.hasAudio) {
+      is(audioOnremovetrack, expectedCalls,
+         'Calls of onremovetrack on audioTracks should be '+expectedCalls+' times.');
+    }
+    if (test.hasVideo) {
       is(videoOnremovetrack, expectedCalls,
          'Calls of onremovetrack on videoTracks should be '+expectedCalls+' times.');
     }
@@ -76,21 +84,29 @@ function startTest(test, token) {
     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);
+          testExpectedAddtrack(1);
-          finishTesting();
+          testExpectedRemovetrack(0);
+          element.src = "";
+          setTimeout(() => {
+            testExpectedAddtrack(1);
+            testExpectedRemovetrack(1);
+            finishTesting();
+          }, 0);
           break;
       }
     } else {