894104 - Crash using WebVTT getCueAsHTML from removed frame

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: testcase

With:
  user_pref("media.webvtt.enabled", true);

Assertion failure: value, at BindingUtils.h:659

> #0  mozilla::dom::WrapNewBindingObject<mozilla::dom::DocumentFragment>
> #1  0x0000000101cc4060 in mozilla::dom::WrapNewBindingObjectHelper<nsRefPtr<mozilla::dom::DocumentFragment>, true>::Wrap
> #2  0x0000000101cc3f9d in mozilla::dom::WrapNewBindingObject<nsRefPtr<mozilla::dom::DocumentFragment> >
> #3  0x000000010212b1f7 in mozilla::dom::TextTrackCueBinding::getCueAsHTML

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

This was caused by bug 882549.  That bug changed the implementation of getCueAsHTML to sometimes return null, but the IDL still claims it never returns null, and the binding generator generates code accordingly.  Either the IDL or the implementation is wrong.

Ralph, can you take this?  I doubt caitp will...

Comment 3

[Scoobidiver (away)]

On Windows: bp-777740b4-a2dc-447c-9231-cdf352130716.

Comment 4

[Ralph Giles (:rillian)]

Thanks for the analysis, bz.

Comment 5

[Ralph Giles (:rillian)]

See bug 895045 for a similar issue with Jetpack accessing closed tabs.

Comment 6

[Ralph Giles (:rillian)]

The testcase calls the TextTrackCue constructor on a detached iframe. We are supposed to 'discard the nested browsing context' (whatwg spec, iframe element) but we do so asynchronously. When the testcase calls TextTrackCue it happens to still be available, but really it shouldn't be. As such I think it makes the most sense for the constructor to throw if an owning document isn't available, rather than GetCueAsHTML() throwing, or any of the other options.

To see this, run the same sequence of operations manually in the console; frameWin.TextTrackCue will be undefined and the crash will not happen. Interestingly, calling the constructor from a setTimeout, even with a 30s delay, still crashes. It appears the timeout is blocking the teardown. See http://people.mozilla.org/~rgiles/2013/htmlcue-02.html for an example of this.

For comparison XMLHttpRequest can be created on the frameWin of a detached iframe. It loses reference to its origin, but can still fetch absolute urls. See http://people.mozilla.org/~rgiles/2013/htmlcue-04.html

Chrome also crashes on the testcase, although it is more robust to the htmlcue-0?.html variants. It posts a CORS violation because the iframe's 'data' protocol is different from the page's 'http' protocol, then throws "TypeError: Property 'TextTrackCue'/'XMLHttpRequest' is not a function". It seems Chrome manages to clean the frame's window in the other test variants.

Comment 7

[Ralph Giles (:rillian)]

Attachment: Part 1: retain a document reference in TextTracCue.

Comment 8

[Jesse Ruderman]

Attachment: testcase that calls the constructor before detaching the iframe

It also crashes if I call the constructor before detaching the iframe.  So while restricting the use of the constructor may be a good idea, it won't fix the crash bug.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The patch restricts the constructor _and_ takes a reference to the document in the constructor.  The latter part is what fixes the bug.

Comment 10

[Ralph Giles (:rillian)]

Or both really; the first testcase throws because there's no window when the constructor is called, the second succeeds because it still has a reference to the window. But the patch does handle both cases.

Jesse, what's the purpose of the statement

  frameWin.TextTrackCue;

in the testcase. Are you just checking if touching the property will crash?

Comment 11

[Ralph Giles (:rillian)]

Attachment: Part 1: Retain a document reference in TextTracCue.

Updated patch to fix commit message, comment.

Comment 12

[Ralph Giles (:rillian)]

Attachment: Part 2: add crashtest

Comment 13

[Ralph Giles (:rillian)]

Attachment: Part 3: Remove mGlobal.

Remove mGlobal and return mDocument instead from GetParentObject() as per irc discussion.

Comment 14

[Ralph Giles (:rillian)]

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

Comment 15

[Jesse Ruderman]

(In reply to Ralph Giles (:rillian) from comment #10)
> Jesse, what's the purpose of the statement
> 
>   frameWin.TextTrackCue;
> 
> in the testcase. Are you just checking if touching the property will crash?

If I remove it, two lines later I get "frameWin.TextTrackCue is not a function".  Maybe there's some kind of property lookup caching going on?  bz would know.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yes, interface objects are defined lazily on window objects.

Comment 17

[Ralph Giles (:rillian)]

Fascinating.

Patches are green on try.

Comment 18

[Ralph Giles (:rillian)]

Chromium crash filed as http://code.google.com/p/chromium/issues/detail?id=261653

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 778130 [details] [diff] [review]
Part 1: Retain a document reference in TextTracCue.

>+TextTrackCue::CreateCueOverlay()
>+{
>+  mDocument->CreateElem(NS_LITERAL_STRING("div"), nullptr,
>                        kNameSpaceID_XHTML,
>                        getter_AddRefs(mDisplayState));

Please fix the indent.  Likewise in TextTrackCue::ConvertInternalNodeToContent.

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 778131 [details] [diff] [review]
Part 2: add crashtest

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 778132 [details] [diff] [review]
Part 3: Remove mGlobal.

r=me

Comment 22

[Ralph Giles (:rillian)]

Attachment: Part 1: Retain a document reference in TextTrackCue.

Thanks for the review. Updated patch to fix indent. Carrying forward r=bz.

Comment 23

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed080cdb1b02
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5874eb61a9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb21d96a3274

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ed080cdb1b02
https://hg.mozilla.org/mozilla-central/rev/f5874eb61a9b
https://hg.mozilla.org/mozilla-central/rev/bb21d96a3274