639648 - crash due to stack exhaustion in BindToTree with setUserData

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nils]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.14) Gecko/20110221 Ubuntu/10.10 (maverick) Firefox/3.6.14
Build Identifier: 

following javascript in a HTML document will crash the browser:

tmp = document.createElement('iframe');
document.body.appendChild(tmp);
o52=document.createElement('h1');
o62=tmp.contentDocument.createElement('script');
o136=o52.setUserData('key',o62,function (o,k,d,s,ds) { o52.appendChild(d); });
o62.appendChild(o52);


Linux GDB stack backtrace:
(gdb) bt 10
#0  nsStyledElement::BindToTree (this=0x7fffd1463900, aDocument=0x0, 
    aParent=0x7fffd1494e40, aBindingParent=0x0, aCompileEventHandlers=1)
    at nsStyledElement.cpp:157
#1  0x00007ffff6c5287b in nsMappedAttributeElement::BindToTree (
    this=0x7fffd1463900, aDocument=0x0, aParent=0x7fffd1494e40, 
    aBindingParent=0x0, aCompileEventHandlers=1)
    at nsMappedAttributeElement.cpp:51
#2  0x00007ffff6ca4907 in nsGenericHTMLElement::BindToTree (
    this=0x7fffd1463900, aDocument=0x0, aParent=0x7fffd1494e40, 
    aBindingParent=0x0, aCompileEventHandlers=1)
    at nsGenericHTMLElement.cpp:869
#3  0x00007ffff6c4abb6 in nsGenericElement::BindToTree (this=0x7fffd1494e40, 
    aDocument=0x0, aParent=<value optimized out>, aBindingParent=0x0, 
    aCompileEventHandlers=<value optimized out>) at nsGenericElement.cpp:2657
#4  0x00007ffff6c66266 in nsStyledElement::BindToTree (this=0x7fffd1463900, 
    aDocument=0x0, aParent=0x7fffd1494e40, aBindingParent=0x0, 
    aCompileEventHandlers=1) at nsStyledElement.cpp:157
#5  0x00007ffff6c5287b in nsMappedAttributeElement::BindToTree (
    this=0x7fffd1463900, aDocument=0x0, aParent=0x7fffd1494e40, 
    aBindingParent=0x0, aCompileEventHandlers=1)
    at nsMappedAttributeElement.cpp:51
#6  0x00007ffff6ca4907 in nsGenericHTMLElement::BindToTree (
    this=0x7fffd1463900, aDocument=0x0, aParent=0x7fffd1494e40, 

Tested on FF4 beta and FF3 on Windows and Linux


Reproducible: Always

Comment 1

[Alex Vincent [:WeirdAl]]

Reading through the W3C specs, I translate the above fragment as follows:

The script element (belonging to an iframe document) is getting an h1 element (from the parent document) appended to it.  The script, in attempting to append the h1, might be adopting the parent node or importing it.

In the user data callback, d is o62, the script element.  There, the h1 element is told to append the script element which is the h1 element's parent (or should be, at some point).

What makes this surprising to me is that the first append operation (h1 as child of script) didn't throw WRONG_DOCUMENT_ERR.  They're supposedly created by different documents.

Nils:  out of curiousity, what is the "o" argument for your user data handler?

Comment 2

[Nils]

Alex: "o" is 5 = NODE_ADOPTED.

Interestingly the user data handler is only called once.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> They're supposedly created by different documents.

That part of the spec got changed.  Inserting into a different document auto-adopts.

Peter, want to take a look?

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: reporter's testcase

Comment 5

[Alex Vincent [:WeirdAl]]

Stepping through in the debugger, script thinks h1 is its child, and h1 thinks script is its child.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The fundamental problem is that the check done IsAllowedAsChild can be invalidated by userdata handlers running off of adoptNode.

This affects both nsINode::ReplaceOrInsertBefore and nsINode::doInsertChildAt (even though the latter usually doesn't do an IsAllowedAsChild check.

Jonas, this should be doable with a mutation guard, but what should the expected mutation count for the adopt be?  Just 0, so if the node was in the DOM somewhere we'd still recompute stuff?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I guess a mutation guard expecting 0 mutations is the best we can safely do.

For trunk, bug 650493 should fix this.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #7)
> For trunk, bug 650493 should fix this.

but we'll need a spot fix for aurora and 1.9.2

Comment 9

[Daniel Veditz [:dveditz]]

In reply to comment 6:
> The fundamental problem is that the check done IsAllowedAsChild can
> be invalidated by userdata handlers running off of adoptNode.

Can we write a testcase that demonstrates this in a scarier-looking way than a stack exhaustion crash?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm not sure.  insPos also becomes invalid, but we may protect against that well enough....  As in, get misbehavior but possibly not crashes.

Comment 11

[Asa Dotzler [:asa]]

Jonas, can we get an eta on a spot fix? Aurora is rapidly approaching the train station.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Comment 13

[Sheila Mooney]

Jonas, can you give us a risk assessment for the patch and understanding of side effects and test cases for side effects?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The risk is very low. We run the exact same code in many other locations in the same function to deal with exactly the same situation. All the patch does is add the same code in a situation that we had previously missed.

The main downside is that we will be slightly slower in some cases. However these cases are fairly rare (auto-adopting a node which is in another document, not just created another document) or really really rare (pages using userdata handlers or mutation listeners which make other DOM modifications). And the perf regression is small even when it happens. So it's not something that I think we need to worry about.

Comment 15

[Johnathan Nightingale [:johnath]]

Comment on attachment 531810 [details] [diff] [review]
Patch to fix

Can we get this in today?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

http://hg.mozilla.org/releases/mozilla-aurora/rev/0302a2047cac

Done!

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 531810 [details] [diff] [review]
Patch to fix

Approved for 1.9.2.18, a=dveditz for release-drivers

Comment 18

[Asa Dotzler [:asa]]

Can we get a verification, please, that this actually fixes the crash for Aurora/Beta?

Comment 19

[Asa Dotzler [:asa]]

OK. The "reporter's testcase" does not crash for me in latest Aurora build.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

The reporter's testcase is verified to crash pre-fix and to not crash post-fix on the same machine using the nightly Aurora build (Mozilla/5.0 (Windows NT 5.1; rv:5.0a2) Gecko/20110517 Firefox/5.0a2).

Comment 21

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

The patch doesn't apply cleanly to 1.9.2

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in on 1.9.2 branch.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d12c5c6f9c02

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.18pre) Gecko/20110606 Namoroka/3.6.18pre (.NET CLR 3.5.30729) using attached testcase. Testcase crashes 1.9.1.17 cleanly.