Closed Bug 1383122 Opened 7 years ago Closed 7 years ago

[MSE] Receiving unexpected error event.

Categories

(Core :: Audio/Video: Playback, enhancement)

54 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

The following mochitest:

(from https://hg.mozilla.org/try/file/64659fa295f4/dom/media/mediasource/test/test_PlayEventsAutoPlaying.html)

runWithMSE(function(ms, el) {
  el.controls = true;
  el.autoplay = true;
  var eventCounts = { play: 0, playing: 0};
  function ForbiddenEvents(e) {
    var v = e.target;
    ok(v.readyState >= v.HAVE_FUTURE_DATA, "Must not have received event too early");
    is(eventCounts[e.type], 0, "event should have only be fired once");
    eventCounts[e.type]++;
  }
  once(ms, 'sourceopen').then(function() {
    // Log events for debugging.
    var events = ["suspend", "play", "canplay", "canplaythrough", "loadstart", "loadedmetadata",
                  "loadeddata", "playing", "ended", "error", "stalled", "emptied", "abort",
                  "waiting", "pause", "durationchange", "seeking", "seeked"];
    function logEvent(e) {
      info("got " + e.type + " event");
    }
    events.forEach(function(e) {
      el.addEventListener(e, logEvent);
    });
    el.addEventListener("play", ForbiddenEvents);
    el.addEventListener("playing", ForbiddenEvents);

    ok(true, "Receive a sourceopen event");
    var videosb = ms.addSourceBuffer("video/mp4");
    el.addEventListener("error", function(e) {
      ok(false, "should not fire '" + e + "' event");
    });
    is(el.readyState, el.HAVE_NOTHING, "readyState is HAVE_NOTHING");
    var promises = [];
    promises.push(fetchAndLoad(videosb, 'bipbop/bipbop_video', ['init'], '.mp4'));
    promises.push(once(el, 'loadedmetadata'));
    Promise.all(promises)
    .then(function() {
       ok(true, "got loadedmetadata event");
       var promises = [];
       promises.push(once(el, 'loadeddata'));
       promises.push(once(el, 'canplay'));
       promises.push(once(el, 'play'));
       promises.push(once(el, 'playing'));
       promises.push(once(el, 'ended'));
       // We're only adding 1.6s worth of data, not enough for readyState to change to HAVE_ENOUGH_DATA
       // So we end the media source so that all the playable data is available.
       promises.push(fetchAndLoad(videosb, 'bipbop/bipbop_video', range(1, 3), '.m4s')
                     .then(() => ms.endOfStream()));
       return Promise.all(promises);
    })
    .then(function() {
      ok(true, "got all required event");
      SimpleTest.finish();
    })
  });
});


Receives an error event after SimpleTest.finish() is called.

The error received is "404 Media Not Found" and is dispatched during shutdown.
Comment on attachment 8888860 [details]
Bug 1383122 - P1. Amend Mochitest to ensure error doesn't occur.

https://reviewboard.mozilla.org/r/159872/#review165556
Attachment #8888860 - Flags: review?(jwwang) → review+
Comment on attachment 8888862 [details]
Bug 1383122 - P3. Don't set src attribute to null.

https://reviewboard.mozilla.org/r/159876/#review165560

::: dom/media/test/eme.js:385
(Diff revision 1)
>  function CleanUpMedia(v) {
>    v.setMediaKeys(null);
>    v.remove();
>    v.onerror = null;
> -  v.src = null;
> +  v.removeAttribute("src");
> +  v.load();

What is this |v.load()| for?
Comment on attachment 8888861 [details]
Bug 1383122 - P2. Amend mochitest to ensure null is properly converted to "null" string.

https://reviewboard.mozilla.org/r/159874/#review165604

::: dom/media/test/test_source_null.html:28
(Diff revision 1)
>  var v = document.getElementById('v');
>  v.src = null; // crashes on NULL access if not handled
>  
>  ok(true, "setting video.src to null didn't crash!");
> +var srcPath = v.src.split('/');
> +ok(srcPath.length >= 3, "src should be within dom/media/test");

This test is obscure. We can test:

let src1 = String(document.location);
let src2 = v.src;
let lastSlash = src1.lastIndexOf('/');
ok(src1.substr(0, lastSlash) == src2.substr(0, lastSlash), "should have the same path.");
Attachment #8888861 - Flags: review?(jwwang) → review+
Comment on attachment 8888862 [details]
Bug 1383122 - P3. Don't set src attribute to null.

https://reviewboard.mozilla.org/r/159876/#review165608

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/media/test/manifest.js#1567
It would be nice to also fix removeNodeAndSource().

::: dom/media/mediasource/test/mediasource.js:26
(Diff revision 1)
>  
>      document.body.appendChild(el);
>      SimpleTest.registerCleanupFunction(function () {
>        el.remove();
>        // Don't trigger load algorithm to prevent 'error' events.
>        el.preload = "none";

setting 'preload' is no longer necessary.

::: dom/media/test/eme.js:383
(Diff revision 1)
>   * Clean up the |v| element.
>   */
>  function CleanUpMedia(v) {
>    v.setMediaKeys(null);
>    v.remove();
>    v.onerror = null;

This isn't necessary anymore since we won't trigger 'error' event.

::: dom/media/test/file_access_controls.html:125
(Diff revision 1)
>      }
>    }
>  
>    if (gVideo) {
>      gVideo.remove();
>      gVideo.preload = "none"; // Don't trigger load algorithm.

Remove this line.
Attachment #8888862 - Flags: review?(jwwang) → review+
Comment on attachment 8888862 [details]
Bug 1383122 - P3. Don't set src attribute to null.

https://reviewboard.mozilla.org/r/159876/#review165614

::: dom/media/test/eme.js:385
(Diff revision 1)
>  function CleanUpMedia(v) {
>    v.setMediaKeys(null);
>    v.remove();
>    v.onerror = null;
> -  v.src = null;
> +  v.removeAttribute("src");
> +  v.load();

i quoted the relevant spec in the commit message.

the key is that removing the attribute itself doesnt cause the load algorithm to run as per spec.
its only during load that as part of the algorithm, the previous media will be removed.
Comment on attachment 8888862 [details]
Bug 1383122 - P3. Don't set src attribute to null.

https://reviewboard.mozilla.org/r/159876/#review165608

> setting 'preload' is no longer necessary.

that's out of scope though
Comment on attachment 8888862 [details]
Bug 1383122 - P3. Don't set src attribute to null.

https://reviewboard.mozilla.org/r/159876/#review165608

I'll add this in a P4 patch...
Comment on attachment 8889329 [details]
Bug 1383122 - P4. Don't set src attribute to empty string.

https://reviewboard.mozilla.org/r/160402/#review166008
Attachment #8889329 - Flags: review?(jwwang) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c019d2c3663c
P1. Amend Mochitest to ensure error doesn't occur. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5a812783ca55
P2. Amend mochitest to ensure null is properly converted to "null" string. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/43c119144a32
P3. Don't set src attribute to null. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/54a7da0c5524
P4. Don't set src attribute to empty string. r=jwwang
Assignee: nobody → jyavenard
You need to log in before you can comment on or make changes to this bug.