MediaSource incorrectly detached from media element when an error occurs

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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"
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 7

3 years ago
mozreview-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-
(Assignee)

Comment 8

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

3 years ago
mozreview-review-reply
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 12

3 years ago
mozreview-review
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+

Comment 13

3 years ago
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)

Comment 15

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

3 years ago
mozreview-review
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

Comment 25

3 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

3 years ago
grmbl grmbl
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

3 years ago
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

Comment 33

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d392ffb70683
https://hg.mozilla.org/mozilla-central/rev/d51a0f8c1c76
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1333576

Updated

2 years ago
Depends on: 1352995
You need to log in before you can comment on or make changes to this bug.