Ralph Giles (:rillian) Reporter
	Description • 11 years ago

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.

	Ralph Giles (:rillian) Reporter
	Comment 1 • 11 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 11 years ago

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

	Ralph Giles (:rillian) Reporter
	Comment 3 • 11 years ago

Test patch removing the check from HTMLMediaElement.

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

	Ralph Giles (:rillian) Reporter
	Comment 4 • 11 years ago

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

	Benjamin Chen [:bechen] Assignee
	Comment 5 • 8 years ago

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.
//

	Benjamin Chen [:bechen] Assignee
	Comment 6 • 8 years ago

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

	Benjamin Chen [:bechen] Assignee
	Comment 7 • 8 years ago

* * *
[mq]: bug1281406length.html

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

	Ralph Giles (:rillian) Reporter
	Comment 8 • 8 years ago

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

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

Thanks for fixing this.

	Ralph Giles (:rillian) Reporter
	Comment 9 • 8 years ago

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

	Benjamin Chen [:bechen] Assignee
	Comment 10 • 8 years ago

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/

	Benjamin Chen [:bechen] Assignee
	Comment 11 • 8 years ago

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/

	Benjamin Chen [:bechen] Assignee
	Comment 12 • 8 years ago

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/

	Benjamin Chen [:bechen] Assignee
	Comment 13 • 8 years ago

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/

	Benjamin Chen [:bechen] Assignee
	Comment 14 • 8 years ago

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/

	Benjamin Chen [:bechen] Assignee
	Comment 15 • 8 years ago

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/

	Pulsebot
	Comment 16 • 8 years ago

Pushed by cbook@mozilla.com:
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

	Carsten Book [:Tomcat]
	Comment 17 • 8 years ago
bugherder

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