Closed Bug 882549 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: posidron, Assigned: caitlin.potter)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files, 8 obsolete files)

73 bytes, text/html
Details
3.35 KB, text/plain
Details
5.91 KB, patch
caitlin.potter
: review+
Details | Diff | Splinter Review
Attached file callstack
Crash Signature: [@ nsCOMPtr<nsINodeInfo>::operator->() | nsINode::OwnerDoc()]
Severity: normal → critical
Assignee: nobody → caitlin.potter
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.
Attachment #762159 - Flags: review?(giles)
Attachment #762159 - Flags: review?(cdiehl)
There's also some code in ConvertLeafNodeToContent that will probably need to change with this.

(line 283) nsNodeInfoManager* nimgr = mTrackElement->NodeInfo()->NodeInfoManager();
Style fixup.
Attachment #762159 - Attachment is obsolete: true
Attachment #762159 - Flags: review?(giles)
Attachment #762159 - Flags: review?(cdiehl)
Attachment #762172 - Flags: review?(giles)
Attachment #762172 - Flags: review?(cdiehl)
Good point Rick, good point... I'll look into that.
Addressed concern raised by Rick - Using window document to get NodeInfoManager instead of HTMLTrackElement which may or may not exist
Attachment #762172 - Attachment is obsolete: true
Attachment #762172 - Flags: review?(giles)
Attachment #762172 - Flags: review?(cdiehl)
Attachment #762202 - Flags: review?(giles)
Attachment #762202 - Flags: review?(cdiehl)
Same as V3, but now with testcase used as crashtest. Take your pick which one you like better!
Attachment #762295 - Flags: review?(giles)
Attachment #762295 - Flags: review?(cdiehl)
Cleaned up crashtest html slightly
Attachment #762295 - Attachment is obsolete: true
Attachment #762295 - Flags: review?(giles)
Attachment #762295 - Flags: review?(cdiehl)
Attachment #762600 - Flags: review?(giles)
Attachment #762600 - Flags: review?(cdiehl)
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.
Attachment #762202 - Attachment is obsolete: true
Attachment #762202 - Flags: review?(giles)
Attachment #762202 - Flags: review?(cdiehl)
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 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.
Attachment #762600 - Flags: review?(giles) → review+
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++
Updated commit message to credit bz and Ms2ger for information provided.
Attachment #762600 - Attachment is obsolete: true
Attachment #762600 - Flags: review?(cdiehl)
Attachment #764362 - Flags: review+
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.
(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.
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'.
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.
(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.
(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.
>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.
>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.
(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.
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.
(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.
GetDoc(), as the "Get" prefix implies, can return null. Please null-check all places where you call it.
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).
Attachment #764362 - Attachment is obsolete: true
Attachment #764887 - Flags: review?(Ms2ger)
Attachment #764887 - Flags: review?(Ms2ger)
Don't use smart pointer for reference to nsIDocument -- "raw po
Attachment #764887 - Attachment is obsolete: true
Stupid browser. *backspace "raw po and type in "<smaug> nsIDocument* should be enough here"*
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.
Attachment #764887 - Attachment is obsolete: false
Attachment #764887 - Flags: review+
Finish removing smartpointer-ness from 'document' references (document= window->GetDoc() instead of document(window->GetDoc()))
Attachment #764887 - Attachment is obsolete: true
Attachment #764985 - Attachment is obsolete: true
Attachment #764991 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/14967188583e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 894104
You need to log in before you can comment on or make changes to this bug.