882549 - WebVTT crash [@mozilla::dom::TextTrackCue::GetCueAsHTML]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/ac7d9177c7ee

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V1

Initial patch to solve these problems, based on suggestions from :bz in #content.

Also tests for null-ness in TextTrackCue::ConvertNodeTreeToDOMTree, because without this, getCueAsHTML() was segfaulting due to not having an actual node tree.

Comment 3

[Rick Eyre (:reyre)]

There's also some code in ConvertLeafNodeToContent that will probably need to change with this.

(line 283) nsNodeInfoManager* nimgr = mTrackElement->NodeInfo()->NodeInfoManager();

Comment 4

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V2

Style fixup.

Comment 5

[Caitlin Potter (:caitp)]

Good point Rick, good point... I'll look into that.

Comment 6

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V3

Addressed concern raised by Rick - Using window document to get NodeInfoManager instead of HTMLTrackElement which may or may not exist

Comment 7

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V4

Same as V3, but now with testcase used as crashtest. Take your pick which one you like better!

Comment 8

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V5

Cleaned up crashtest html slightly

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 762202 [details] [diff] [review]
Don't rely on mElement to find nsIDocument in TextTrackCue. V3

I assume the v3 patch is superceeded by v5.

Comment 10

[Caitlin Potter (:caitp)]

I had left them uncancelled because there is a chance that the crashtest isn't wanted, or would be added later. Essentially V5 is the same as V3, but with the crashtest added.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 762600 [details] [diff] [review]
Don't rely on mElement to find nsIDocument in TextTrackCue. V5

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

Looks fine to me, but I really have no idea about mGlobal vs mElement->OwnerDoc. Did you get feedback from someone in #content on this? If so please document the motivation in the commit message.

Comment 12

[Caitlin Potter (:caitp)]

It was suggested by either bz or Ms2ger, I can't remember at this point :( and of course my IRC client doesn't have logging enabled. I will credit both of them for explaining how to access the DOM 'window' from C++

Comment 13

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V6

Updated commit message to credit bz and Ms2ger for information provided.

Comment 14

[Ralph Giles (:rillian)]

Thanks. I was hoping to find out why using mGlobal was better than mElement's ownerdoc, but a reference to authority is better than nothing.

Comment 15

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #14)
> Thanks. I was hoping to find out why using mGlobal was better than
> mElement's ownerdoc, but a reference to authority is better than nothing.

So the reasoning here is that we don't always have a track element when TextTrackCues are created, for example via JS, so when GetCueAsHTML() is called mTrackElement will be null and result in a seg fault.

In the case where you create a TextTrackCue in JS, like 'new TextTrackCue()', mGlobal would be the window so we would have access to the owning document via mGlobal in this situation.

It's my suspicion that we will always be able to get access to the owning document through mGlobal, at least that's what I got from bz.

I was thinking that we would need DOM review on this as well before it landed. Just to make sure that we aren't going to run into situations where mGlobal isn't what we intended and we seg fault or crash or something.

Comment 16

[Caitlin Potter (:caitp)]

Sorry, I'll explain that here... mElement is not guaranteed to be valid (it's only really valid for TextTrackCues created by WebVTTParser).

If they're created via the DOM api, there isn't necessarily a <track> element for them to reference, hence the access violation reported by :cdiehl.

mGlobal should always be available, and /should/ always refer to essentially the DOM 'window' element, so we can /always/ use that to find 'document', whereas we can't always trust mElement to get us 'document'.

Comment 17

[Caitlin Potter (:caitp)]

In fact, following this change, TextTrackCue probably shouldn't keep a reference to a <track> element at all, as there is no guarantee that it would be valid or not, and isn't really used for anything.

Comment 18

[Ralph Giles (:rillian)]

(In reply to Caitlin Potter (:caitp) from comment #17)
> In fact, following this change, TextTrackCue probably shouldn't keep a
> reference to a <track> element at all

Please file a follow-up bug then.

Comment 19

[Rick Eyre (:reyre)]

(In reply to Caitlin Potter (:caitp) from comment #17)
> In fact, following this change, TextTrackCue probably shouldn't keep a
> reference to a <track> element at all, as there is no guarantee that it
> would be valid or not, and isn't really used for anything.

Well we still need it to get access to the MediaElement so we can attach the cue div to the video div. We'd have to put in some other infrastructure for that if we want to remove it completely.

However, the code that actually handles attaching the computed DOM tree of nodes to the cue div and then to the video div is going to end up being moved to TextTrackList, most likely, in the processing model bug.

Comment 20

[Caitlin Potter (:caitp)]

>I was thinking that we would need DOM review on this as well before it landed. Just to make
>sure that we aren't going to run into situations where mGlobal isn't what we intended and we
>seg fault or crash or something.

From the information that we've got from bz and Ms2ger, it should always point to 'window', WITH THE EXCEPTION of js that is not running within a browser context (and does not have a 'window' object, for instance).

It is amazing that there is no real guarantee or anything, and if Gecko were less terrible it would certainly be easier to have such a guarantee -- But we should be -fairly- safe, based on what they told us, at least.

It could potentially cause problems in certain kinds of test programs, but I think this is fairly unlikely.

Comment 21

[Caitlin Potter (:caitp)]

>Well we still need it to get access to the MediaElement so we can attach the cue div to the
>video div. We'd have to put in some other infrastructure for that if we want to remove it
>completely.

No, TextTrackCue is not the one messing around in the MediaElement. That is the responsibility of TextTrack, I believe. It does not make sense for the TextTrack, or TextTrackList, to find the media element via its cues.

Comment 22

[Rick Eyre (:reyre)]

(In reply to Caitlin Potter (:caitp) from comment #21)
> >Well we still need it to get access to the MediaElement so we can attach the cue div to the
> >video div. We'd have to put in some other infrastructure for that if we want to remove it
> >completely.
> 
> No, TextTrackCue is not the one messing around in the MediaElement. That is
> the responsibility of TextTrack, I believe. It does not make sense for the
> TextTrack, or TextTrackList, to find the media element via its cues.

https://mxr.mozilla.org/mozilla-central/source/content/media/TextTrackCue.cpp#108

This was a temporary thing to get initial support in. Cargo culted over from the demo. The code in RenderCue() and code dealing with the cue overlay in TextTrackCue are going to be moving soon though.

Comment 23

[Caitlin Potter (:caitp)]

Certainly, if it IS in fact the object messing around in the MediaElement, this behaviour needs to change, because it's problematic for obvious reasons.

The only real glue between the TextTrack and the HTMLMediaElement that I can see in the spec, is from TextTracks which are created via HTMLMediaElement.addTextTrack(). To my knowledge, it's not possible to create a TextTrack in any other way. So this should probably be used as the glue between the media element and the track. You can sort that out in your other bug.

Comment 24

[Ralph Giles (:rillian)]

(In reply to Rick Eyre (:reyre) from comment #15)

> So the reasoning here is that we don't always have a track element when
> TextTrackCues are created, for example via JS, so when GetCueAsHTML() is
> called mTrackElement will be null and result in a seg fault.

That makes sense. I just didn't know if there were other reasons. This is the sort of 'why' it's very helpful to record in the commit message to clarify what you were thinking to anyone looking at this change later.

> I was thinking that we would need DOM review on this as well before it
> landed. Just to make sure that we aren't going to run into situations where
> mGlobal isn't what we intended and we seg fault or crash or something.

Review from someone in #content would be appropriate here. Please request someone.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

GetDoc(), as the "Get" prefix implies, can return null. Please null-check all places where you call it.

Comment 26

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V7

Now with lots of null-checking.

Followup bug will be opened to throw in Constructor when aGlobal is not an nsPIDOMWindow, and a mechanism will be added to notify (native) callers that an error occurred (these functions are not exposed to js).

Comment 27

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V8

Don't use smart pointer for reference to nsIDocument -- "raw po

Comment 28

[Caitlin Potter (:caitp)]

Stupid browser. *backspace "raw po and type in "<smaug> nsIDocument* should be enough here"*

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 764887 [details] [diff] [review]
Don't rely on mElement to find nsIDocument in TextTrackCue. V7

nsIDocument* is enough for accessing window->GetDoc() since you don't do anything which could kill the document object before/while using it.

Comment 30

[Caitlin Potter (:caitp)]

Attachment: Don't rely on mElement to find nsIDocument in TextTrackCue. V9

Finish removing smartpointer-ness from 'document' references (document= window->GetDoc() instead of document(window->GetDoc()))

Comment 31

[Ralph Giles (:rillian)]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14967188583e

Comment 33

[Caitlin Potter (:caitp)]

build/webapp-zip.js:40: ReferenceError: GAIA_DEV_PIXELS_PER_PX is not defined these busted builds look unrelated to this patch. I'm not sure about that stuff. The patch is from M-C and not M-I, fwiw

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14967188583e