MSE endOfStream() called within an 'updateend' event can fail with 'object no longer usable'

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

Assignee

Description

5 years ago
If endOfStream() is called on a MediaSource in an 'updateend' event, an error 'object no longer usable' can occur depending on timing.

I discovered this with code like:

  sb.addEventListener("updateend", function() {
    updateCount++;
    alert('updateend: ' + updateCount);
    if (updateCount == 1) {
      ms.endOfStream();
      v.play();
    }
  }

This failed on the 'endOfStream' call with an object no longer usable error. If I took out the alert it worked. The error can also be demonstrated using a setTimeout instead of using the alert to delay things:

  sb.addEventListener("updateend", function() {
    updateCount++;
    if (updateCount == 1) {
      setTimeout(function() {
        ms.endOfStream();
        v.play();
      }, 1000);
    }
  }

Testing in Chrome results in no error with both variants of the code.
Assignee

Updated

5 years ago
Blocks: MSE
Assignee

Comment 1

5 years ago
Posted patch Test case (obsolete) — Splinter Review
Adds a mochitest which fails with the error.
Assignee

Updated

5 years ago
Blocks: 1068483
Assignee

Updated

5 years ago
Assignee: nobody → cajbir.bugzilla
Assignee

Comment 2

5 years ago
The failure here is due to reaching SourceBuffer::Remove which raises an error if the MediaSource Ready State is not Open. At the time of failure it is Ended.
Assignee

Comment 3

5 years ago
SourceBuffer::Remove is called during the DurationChange handling as a result of MediaSource::EndOfStream.

The spec outlines what needs to be done in SourceBuffer::Remove when the MediaSource is Ended here: http://www.w3.org/TR/media-source/#widl-SourceBuffer-remove-void-double-start-unrestricted-double-end. This should be addressed in bug 1068483.

SourceBuffer::Remove ends up being called because of the duration change removing data outside of the new duration range. The spec for EndOfStream states (http://www.w3.org/TR/media-source/#end-of-stream-algorithm):

    Run the duration change algorithm with new duration set to the highest end time reported by the buffered attribute across all SourceBuffer objects in sourceBuffers.

The duration change algorithm (http://www.w3.org/TR/media-source/#duration-change-algorithm) is what specifies the call to Remove:

    If the new duration is less than old duration, then call remove(new duration, old duration) on all  objects in sourceBuffers.
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Assignee

Comment 4

5 years ago
Changing SourceBuffer::Remove so that it handles an ended media source per spec results in approximately 50 failures in the test_MediaSource test.

This is because at end of stream the highest buffered end time is 4.000 but the duration is 4.001. This causes SourceBuffer::Remove to reopen the stream which results in the tests for the ready state being "ended" to fail.

I'm investgating the cause for the duration discrepancy. In general though I'm unsure if test_MediaSource should be checking if the ready state is "ended" in the "souceended" event. Since the event is asynchronous it can be reopened before the event is processed. Something that bug 1068483 may have to deal with.
Assignee

Comment 5

5 years ago
Duration discrepancy is due to the final frame duration in WebMBufferedParser::CalculateBufferedForRange being 34000000 whereis on other frames it is 33000000. This value is computed by subtracting the timecode of the previous frame from the current one. It is added to the timecode of the current frame to compute the end time of that frame. This results in the end time of the last buffered range being 1000000 higher.
Assignee

Comment 6

5 years ago
Bug 1065207 is relevant here.
Assignee

Comment 7

5 years ago
Fix here, as discussed in team standups, is to store the duration as a fuzzy value which includes the possible error in the time based on the container type. This is similar to the way the time is compared in the TrackBuffer.

A solution to the problem of not knowing the end frame duration time is still to be investigated in bug 1065207.

Test is included which fails before patch, succeeds after.
Attachment #8486911 - Attachment is obsolete: true
Attachment #8509827 - Flags: review?(karlt)
Thank you for the helpful details of what is happening here and the spec
references.  The patch changes SourceBuffer::Remove() to comply with the spec.
My main comments on the patch would be that perhaps
GetHighestBufferedEndTime() should return something with the highest ended +
fuzz, and TimestampFuzz depends on the associated WebMBufferedParser, which
changes when new init segments are processed, suggesting that fuzz should be
more closely tracked with timestamps.  However I'm wondering whether we can
avoid this complication.

When error is not set and duration has not been reached, the relevant parts of
the end of stream algorithm are essentially.

* Change the readyState attribute value to "ended", and dispatch sourceended.

* Change the readyState attribute value to "open", and dispatch sourceopen (duration change algorithm)

* Notify the media element that it now has all of the media data.

This doesn't make sense to me because the media element shouldn't be notified
that it now has all the media data if the MediaSource is "open".
I wonder whether there is a spec bug here.

Blink doesn't run the duration change algorithm here.
See the log of
http://people.mozilla.org/~karlt/mse/mediasource_demo.html?file=mediasource_test.webm&chunks=5&eos_chunk=3&eos_delay=-1&delay_chunk=1000&start=-1
readyState remains at ended until appendBuffer is called again.
Two possible ways of making the end of stream algorithm more sane might be:

1. Like Blink, don't run the entire duration change algorithm.

2. Change the duration change algorithm to only call remove on Sourcebuffers
   when the new duration is less than the highest end time reported by the
   buffered attribute across all SourceBuffer objects in sourceBuffers.
   i.e. only when there is something to remove.

Option 1 might be better because it avoids some use of the fuzzy highest end time.
The spec bug is fixed in https://dvcs.w3.org/hg/html-media/rev/b3f086e62b53
by using a new range removal algorithm from duration-changed instead of doing all the steps that would be performed when calling remove().

https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#duration-change-algorithm

Still I don't think the range removal algorithm does anything when the duration change algorithm is invoked from endOfStream().
(In reply to Karl Tomlinson (:karlt) from comment #10)
> Still I don't think the range removal algorithm does anything when the
> duration change algorithm is invoked from endOfStream().

Well, it does dispatch apparently pointless updatestart update and updateend events.
Comment on attachment 8509827 [details] [diff] [review]
Store duration in MediaSource as a time+possible error

With the spec clearer now, I think we want to go with a different approach
here, one that doesn't ever re-open on end of stream.  That might either be
skipping much of the duration change algorithm, like Blink, or changing the duration change algorithm so as to run the range removal algorithm instead of calling remove(), as in the editors draft.

That will make the fuzz changes here unnecessary IIUC.  If we do need some
fuzz, for other reasons, then I think that can be dealt with separately and
will probably be based on a larger fuzz like EOS_FUZZ_US.

>+  v.preload = "auto";

What does this do?

>+  v.addEventListener("ended", function () {
>+    v.parentNode.removeChild(v);

Is it necessary to remove the element from the document?
Attachment #8509827 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #12)
> >+  v.preload = "auto";
> 
> What does this do?
> 
> >+  v.addEventListener("ended", function () {
> >+    v.parentNode.removeChild(v);
> 
> Is it necessary to remove the element from the document?

Both of these are unnecessary.  runWithMSE handles this already.
Assignee

Comment 14

5 years ago
Addresses review comments by implementing the revised spec range removal step and calling that from the duration change code. This results in extra update{start,end} events so test are modified to allow for this.

The actual coded frame removal algorithm isn't implemented. There was a TODO for this and I've raised bug 1096089 for it so it doesn't get lost.
Attachment #8509827 - Attachment is obsolete: true
Attachment #8519695 - Flags: review?(karlt)
Comment on attachment 8519695 [details] [diff] [review]
Fix with review comments addressed

> Bug 1065215 - MSE endOfStream() called within an 'updateend' event can fail with 'object no longer usable' - r=karl

Please update the comment to say how the behavior of the code is changed.
There are two changes here: reopening in remove and not running all of remove in the duration change algorithm.

>+  // Runs the range removal steps from the MSE specification on each SourceBUffer.

Bu
Attachment #8519695 - Flags: review?(karlt) → review+
Comment on attachment 8519695 [details] [diff] [review]
Fix with review comments addressed

>   if (mUpdating || mMediaSource->ReadyState() != MediaSourceReadyState::Open) {
>     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>     return;
>   }
>+  if (mMediaSource->ReadyState() == MediaSourceReadyState::Ended) {
>+    mMediaSource->SetReadyState(MediaSourceReadyState::Open);
>+  }

Actually this block is never reached because of the test above.

If you'd like to fix this in this patch then please remove the Open test.
Otherwise please leave out the Ended block.
Assignee

Comment 18

5 years ago
Address additional review comments. I removed the open and suggested. r+ carried forward.
Attachment #8519695 - Attachment is obsolete: true
Attachment #8519702 - Flags: review+
Assignee

Comment 19

5 years ago
Try build for updated patch: https://tbpl.mozilla.org/?tree=Try&rev=a19b34f11390
Assignee

Comment 20

5 years ago
Posted patch EME fix (obsolete) — Splinter Review
Fix for EME test failure. I have no idea if this is the right fix, but the test passes. Hopeing an EME expert can confirm if it is. This bug results in update{start,end} firing more than once and this fix attempts to allow for that.
Attachment #8519847 - Flags: review?(cpearce)
Assignee

Comment 22

5 years ago
Posted patch EME fixSplinter Review
Updated patch to use media source readyState to check if ended and add a comment explaining why.
Attachment #8519847 - Attachment is obsolete: true
Attachment #8519847 - Flags: review?(cpearce)
Attachment #8520217 - Flags: review?(cpearce)
Attachment #8520217 - Flags: review?(cpearce) → review+
Assignee

Comment 23

5 years ago
Try build for updated patch: https://tbpl.mozilla.org/?tree=Try&rev=05facb6502ba
Assignee

Comment 25

5 years ago
Backed out for:

mediasource-remove.html | Test remove transitioning readyState from 'ended' to 'open'. - expected FAIL

https://hg.mozilla.org/integration/mozilla-inbound/rev/b538f9579103
https://hg.mozilla.org/integration/mozilla-inbound/rev/81d9177f109c
Assignee

Comment 26

5 years ago
There is a web platform test for SourceBuffer.remove causing a transition from MediaSource.readyState ended=>open. This used to fail. With the patches in this bug it now passes. Change the metadata for the test so that it expects the pass.
Attachment #8521132 - Flags: review?(karlt)
Assignee

Comment 27

5 years ago
try build with web platform tests enabled: https://tbpl.mozilla.org/?tree=Try&rev=35f234214d58
Depends on: 1097500
Attachment #8521132 - Flags: review?(karlt) → review+
Assignee

Comment 28

5 years ago
Rebased on top of recent MSE landings. Try build: https://tbpl.mozilla.org/?tree=Try&rev=3197ed8b8d26
Attachment #8519702 - Attachment is obsolete: true
Attachment #8521692 - Flags: review+
Assignee

Comment 30

5 years ago
Backed out for test failure on Android:

https://hg.mozilla.org/integration/mozilla-inbound/rev/23d179a16cd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d008dbf3e9f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/722fca966383

INFO TEST-UNEXPECTED-FAIL | /tests/dom/media/mediasource/test/test_EndOfStream.html | Test timed out. - expected PASS
WARNING - TEST-UNEXPECTED-FAIL | /tests/dom/media/mediasource/test/test_WaitingOnMissingData.html | application timed out after 330 seconds with no output
WARNING - PROCESS-CRASH | /tests/dom/media/mediasource/test/test_WaitingOnMissingData.html | application crashed [@ libc.so + 0xcff0]
Assignee

Comment 31

5 years ago
Discussed the Android timeout with team and decision made to disable it on that platform. Spun bug 1101187 to investigate/fix/enable. Carry r+ forward.
Attachment #8521692 - Attachment is obsolete: true
Attachment #8524876 - Flags: review+
Assignee

Comment 34

5 years ago
Backed out for failure on Mulet:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccae5be1b381
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3335fb1f0b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9f48366319

36 INFO TEST-UNEXPECTED-FAIL | /tests/dom/media/mediasource/test/test_EndOfStream.html | Test timed out. - expected PASS

This was on the try run in comment 32 but I missed it due to being below the fold. I'll disable on Mulet, comment in bug 1101187 and reland.
https://hg.mozilla.org/mozilla-central/rev/dc01d68c1c7a
https://hg.mozilla.org/mozilla-central/rev/2be3cdd2ed7c
https://hg.mozilla.org/mozilla-central/rev/2beca8e05628
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.