868611 - [webvtt] Implement HTMLTrackElement::UnbindFromTree

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

We hook bind-to-tree so we can trigger a load if we're parented to a media element, but we don't do anything when we're unbound. Follow-up from https://bugzilla.mozilla.org/show_bug.cgi?id=833385#c48

Comment 1

[Rick Eyre (:reyre)]

Attachment: v1: Implement HTMLTrackElement::UnbindFromTree

This is all I can see that we need to do as of now. We might, perhaps, need to change HTMLTrackElement::ReadyState?

I don't think we need to do anything with the WebVTTLoadListener as if we are being Unbound we're probably not going to receive any more data. Even if we do receive anymore data we can still access HTMLTrackElement's data from the LoadListener so we won't crash.

Thoughts Ralph?

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 766711 [details] [diff] [review]
v1: Implement HTMLTrackElement::UnbindFromTree

Review of attachment 766711 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

I agree we don't need to mess with the TextTrack state. When we hook up HTMLMediaElement events blocking on TextTrack loads we'll need to make sure RemoveTextTrack() takes care of clearing any blocks, rejigger the active tracks, etc. But I think this is fine for now.

Comment 3

[Rick Eyre (:reyre)]

Attachment: v1: Implement HTMLTrackElement::UnbindFromTree r=rillian

Thanks for the review Ralph.

Updating commit message to reflect Ralph's r+.

Carrying forward r=rillian

Try push: https://tbpl.mozilla.org/?tree=Try&rev=9727a70fd17b

Comment 4

[Rick Eyre (:reyre)]

Looks green.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abdcad6d6fb4

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes. Also, your commit message has a typo in it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5acfb878895f

https://tbpl.mozilla.org/php/getParsedLog.php?id=24998457&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24997130&tree=Mozilla-Inbound

Comment 7

[Rick Eyre (:reyre)]

Attachment: v2: Implement HTMLTrackElement::UnbindFromTree r=rillian

Sorry about that Ryan.

- Fixed typo.
- Added in null check for mTrack before calling RemoveElement so we don't crash.

Comment 8

[Rick Eyre (:reyre)]

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

Comment 9

[Rick Eyre (:reyre)]

Comment on attachment 772061 [details] [diff] [review]
v2: Implement HTMLTrackElement::UnbindFromTree r=rillian

Still not working.

Comment 10

[Rick Eyre (:reyre)]

Attachment: v3: Implement HTMLTrackElement::UnbindFromTree r=rillian

I've just added some guards to check before we deref mTextTracks and mMediaParent which might not always be there.

Carrying forward r=rillian

Comment 11

[Rick Eyre (:reyre)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=b602cf702431
And an ASAN: https://tbpl.mozilla.org/?tree=Try&rev=3bbf27a48554

Comment 12

[Rick Eyre (:reyre)]

Try looks green.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3088626d9e34

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3088626d9e34