871747 - [webvtt] should HTMLTrackElement load resources outside a document?

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

HTMLTrackElement::BindToTree() only tries to load a resource if aDocument is non-null. This is cargo-culted from HTMLMediaElement::BindToTree() where the same conditional was added in bug 503989.

Per irc discussion, bz's reading of the spec is that the load should fire whether the element has a parent document or not, and so the conditional on aDocument should be removed.

Before we do that, I want to understand if the spec is correct here.

Comment 1

[Ralph Giles (:rillian)]

Roc, do you remember why you added HTMLMediaElement;:BindToTree() check in bug 503989?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't remember. If we don't leak with that check taken out, I think you should take it out.

Comment 3

[Ralph Giles (:rillian)]

Attachment: remove aDocument check from HTMLMediaElement::BindToTree

Test patch removing the check from HTMLMediaElement.

Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=9804b644c799 to check for leaks.

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 749309 [details] [diff] [review]
remove aDocument check from HTMLMediaElement::BindToTree

My patch is silly, and crashes on content/canvas/test/crossorigin/test_video_crossorigin.html dereferencing a null aDocument.

https://tbpl.mozilla.org/?tree=Try&rev=ccf39027a215

Comment 5

[Benjamin Chen [:bechen]]

https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLTrackElement.cpp#266

The TrackElement call MediaElement::RunInStableState is not correct. Because the MediaElement::AbortExistingLoads will cancel the runnable of stable state by adding |mCurrentLoadID|. 
So the TrackElement::LoadResource should not use MediaElement::RunInStableState regardless inside/outside document.

ex:
v is MediaElement,t is TrackElement.
//
v.appendChild(t); // trigger HTMLTrackElement::BindToTree, dispatch a task to stable state.
v.src = xxx; // trigger the AbortExistingLoads, the t won't load any resource because the task be cancelled.
//

Comment 6

[Benjamin Chen [:bechen]]

Attachment: Bug 871747 - Load the TrackElement outside the document.

Review commit: https://reviewboard.mozilla.org/r/60794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60794/

Comment 7

[Benjamin Chen [:bechen]]

Attachment: Bug 871747 - Enable html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter.html, TextTrack/cues.html, TextTrackList/length.html, track/track-element/cloneNode.html, query-encoding/utf-8.html, utf-16be.html, utf-16le.html, windows-1

* * *
[mq]: bug1281406length.html

Review commit: https://reviewboard.mozilla.org/r/60796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60796/

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8765395 [details]
Bug 871747 - Load the TrackElement outside the document.

https://reviewboard.mozilla.org/r/60794/#review57724

Thanks for fixing this.

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8765396 [details]
Bug 871747 - Enable html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter.html, TextTrack/cues.html, TextTrackList/length.html, track/track-element/cloneNode.html, query-encoding/utf-8.html, utf-16be.html, utf-16le.html, windows-1

https://reviewboard.mozilla.org/r/60796/#review57726

Comment 10

[Benjamin Chen [:bechen]]

Comment on attachment 8765395 [details]
Bug 871747 - Load the TrackElement outside the document.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60794/diff/1-2/

Comment 11

[Benjamin Chen [:bechen]]

Comment on attachment 8765396 [details]
Bug 871747 - Enable html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter.html, TextTrack/cues.html, TextTrackList/length.html, track/track-element/cloneNode.html, query-encoding/utf-8.html, utf-16be.html, utf-16le.html, windows-1

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60796/diff/1-2/

Comment 12

[Benjamin Chen [:bechen]]

Comment on attachment 8765395 [details]
Bug 871747 - Load the TrackElement outside the document.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60794/diff/2-3/

Comment 13

[Benjamin Chen [:bechen]]

Comment on attachment 8765396 [details]
Bug 871747 - Enable html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter.html, TextTrack/cues.html, TextTrackList/length.html, track/track-element/cloneNode.html, query-encoding/utf-8.html, utf-16be.html, utf-16le.html, windows-1

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60796/diff/2-3/

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8765396 [details]
Bug 871747 - Enable html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter.html, TextTrack/cues.html, TextTrackList/length.html, track/track-element/cloneNode.html, query-encoding/utf-8.html, utf-16be.html, utf-16le.html, windows-1

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60796/diff/3-4/

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8765396 [details]
Bug 871747 - Enable html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter.html, TextTrack/cues.html, TextTrackList/length.html, track/track-element/cloneNode.html, query-encoding/utf-8.html, utf-16be.html, utf-16le.html, windows-1

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60796/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad45d87d0b34
Load the TrackElement outside the document. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e63a1be0b9
Enable html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter.html, TextTrack/cues.html, TextTrackList/length.html, track/track-element/cloneNode.html, query-encoding/utf-8.html, utf-16be.html, utf-16le.html, windows-1251.html, windows-1252.html. r=rillian

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad45d87d0b34
https://hg.mozilla.org/mozilla-central/rev/78e63a1be0b9