Closed Bug 1302656 Opened 8 years ago Closed 8 years ago

MediaSource incorrectly detached from media element when an error occurs

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

Attachments

(2 files)

Bug 1302656: P1. Don't detach mediasource when error occurs.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1302656: P2. Update expected wpt results.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details 
This cause failure of the test "Signaling 'decode' error via endOfStream() after initialization segment has been appended and the HTMLMediaElement has reached HAVE_METADATA."

When a decode error occurs (either via an invalid playback or through explicit call to endOfStream("decode), the mediasource object is detached from the media element.

Such operation is never documented in neither the MSE nor the HTML5 media element.
the only time the mediasource is to be detached from the media element is described in: 
https://w3c.github.io/media-source/index.html#mediasource-detach

and is " where the media element is going to transition to NETWORK_EMPTY"
I've opened bug https://github.com/w3c/media-source/issues/162 and https://github.com/w3c/web-platform-tests/issues/3726

While we do have a bug regardless, in that we always detach the mediasource from the media element whenever an error occurs.

We still fail the WPT test as it assumes that the mediasource should be detached when readyState  is HAVE_NOTHING ; which to me is incorrect. it should be detached regardless
This part of the spec related to the events fired, has about the "emptied" event:
"A media element whose networkState was previously not in the NETWORK_EMPTY state has just switched to that state (either because of a fatal error during load that's about to be reported, or because the load() method was invoked while the resource selection algorithm was already running)."

which seems to indicate that a fatal error during load (which would cause error to be set to MEDIA_ERR_SRC_NOT_SUPPORTED and networkState to become NETWORK_NO_SOURCE) should in fact becomes NETWORK_EMPTY.

In any case, we can assume that the mediasource should only be detached if an error occurs during the media element load algorithm
Assignee: nobody → jyavenard
Comment on attachment 8806606 [details]
Bug 1302656: P2. Update expected wpt results.

https://reviewboard.mozilla.org/r/90004/#review89528
Attachment #8806606 - Flags: review?(gsquelart) → review+
Comment on attachment 8806605 [details]
Bug 1302656: P1. Don't detach mediasource when error occurs.

https://reviewboard.mozilla.org/r/90002/#review89534

::: dom/media/MediaDecoder.cpp
(Diff revision 1)
>  
>  bool
>  MediaDecoder::OwnerHasError() const
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  MOZ_ASSERT(!IsShutdown());

We can't remove the assertion because mOwner is valid until Shutdown() is called which also means OwnerHasError() should only be called before Shutdown(). Why do you want to remove the assertion?
Attachment #8806605 - Flags: review?(jwwang) → review-
Comment on attachment 8806605 [details]
Bug 1302656: P1. Don't detach mediasource when error occurs.

https://reviewboard.mozilla.org/r/90002/#review89534

> We can't remove the assertion because mOwner is valid until Shutdown() is called which also means OwnerHasError() should only be called before Shutdown(). Why do you want to remove the assertion?

OwnerHasError is only ever called from the SourceBuffer code.
Currently, OwnerHasError is in effect never called because of https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/SourceBuffer.cpp#492
IsAttached() would return false.

Currently, as the MediaSourceDecoder is always destroyed whenever there's an error, IsAttached() happens to always return false.
If we were to enter the code OwnerHasError, then MediaDecoder::IsShutdown would always return false.

I can re-add the assertion there, it doesn't matter. As it it it can't be triggered.

I don't believe this warrant a r- however, re-adding an assert is easy. Unless you think the entire change is invalid.
Comment on attachment 8806605 [details]
Bug 1302656: P1. Don't detach mediasource when error occurs.

https://reviewboard.mozilla.org/r/90002/#review89534

> OwnerHasError is only ever called from the SourceBuffer code.
> Currently, OwnerHasError is in effect never called because of https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/SourceBuffer.cpp#492
> IsAttached() would return false.
> 
> Currently, as the MediaSourceDecoder is always destroyed whenever there's an error, IsAttached() happens to always return false.
> If we were to enter the code OwnerHasError, then MediaDecoder::IsShutdown would always return false.
> 
> I can re-add the assertion there, it doesn't matter. As it it it can't be triggered.
> 
> I don't believe this warrant a r- however, re-adding an assert is easy. Unless you think the entire change is invalid.

The assertion is important becuase we used to have some UAF bugs where mOwner is accessed after Shutdown().
Comment on attachment 8806605 [details]
Bug 1302656: P1. Don't detach mediasource when error occurs.

https://reviewboard.mozilla.org/r/90002/#review89550
Attachment #8806605 - Flags: review?(jwwang) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff16d055e178
P1. Don't detach mediasource when error occurs. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/22dcdb840e8a
P2. Update expected results. r=gerald
backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6005435&repo=autoland
Flags: needinfo?(jyavenard)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d607b2d92597
Backed out changeset 22dcdb840e8a 
https://hg.mozilla.org/integration/autoland/rev/9a01ef30c0dc
Backed out changeset ff16d055e178 for w1 failures
Comment on attachment 8806605 [details]
Bug 1302656: P1. Don't detach mediasource when error occurs.

https://reviewboard.mozilla.org/r/90000/#review90394

Finally green on try
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/416dbe42e4a3
P1. Don't detach mediasource when error occurs. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/e8ce7f515092
P2. Update expected wpt results. r=gerald
grmbl grmbl
Flags: needinfo?(jyavenard)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d392ffb70683
P1. Don't detach mediasource when error occurs. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/d51a0f8c1c76
P2. Update expected wpt results. r=gerald
https://hg.mozilla.org/mozilla-central/rev/d392ffb70683
https://hg.mozilla.org/mozilla-central/rev/d51a0f8c1c76
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1333576
Depends on: 1352995
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: