119044 - Mozilla crashes on XUL/DOM

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[arman]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.7) Gecko/20011221
BuildID:    2001122106

Mozilla crashes after loading attatched file and clicking on the
"HELLO" button.

------

<?xml version="1.0"?> <!-- -*- sgml -*- -->
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>

<window xmlns:html="http://www.w3.org/1999/xhtml"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

<script>
function bug(node)
{
    res = document.createElement(node.nodeName);
	
    if (node.childNodes &amp;&amp; node.childNodes.length) {
      n = bug(node.childNodes[0]);
      res.appendChild( n );
    } 
    return res;
}

function go()
{
  bug(document.getElementById("foo"));
}
</script>

<vbox>

<box id="foo">
  <button label="Hello There"/>
</box>

<button onclick="go()" label="HELLO"/>

</vbox>

</window>

Comment 1

[arman]

Attachment: XUL file which causes crash

Comment 2

[Olav Vitters]

Attachment: Stacktrace Linux current CVS

Adding stacktrace from todays CVS Linux. In this stacktrace, the line with
"nsXULElement::SetDocument" keeps repeating. Attaching only the "bt 50" and "bt
-50", the other 8596 lines are all the same (repeating
"nsXULElement::SetDocument").

Comment 3

[Olav Vitters]

Confirming

Comment 4

[Chase Tingley]

for easy clicking:

http://lxr.mozilla.org/seamonkey/source/content/xul/content/src/nsXULElement.cpp#2164

Comment 5

[Ben Tremblay]

2002011604 Win95
TB1781821H using local file test.xul

Comment 6

[Neil Deakin]

Attachment: don't allow appendChild to insert itself or an ancestor

this patch prevents the crash, such that the test case throws a script error
when called. Reporter, in the test case, I think you meant to declare the
variable 'res' with 'var' otherwise it will become a global and get overwritten
when the function is called recursively. This causes appendChild to try to
append itself.

Comment 7

[Brendan Eich [:brendan]]

=> bryner.

The patch should follow prevailing style by including a space before the {.

/be

Comment 8

[Brian Ryner (not reading)]

jst, does this patch look ok to you?

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 92026 [details] [diff] [review]
don't allow appendChild to insert itself or an ancestor

The patch looks correct as per the DOM (3 Core WD).  However from an
implentation standpoint, this check should probably happen before we QI to
nsIContent.

Comment 10

[Brian Ryner (not reading)]

Do we have a similar check for non-XUL content?  I don't see anything like this
in nsGenericElement::AppendChild().

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Bryner, yeah we do. 
http://lxr.mozilla.org/mozilla/source/content/base/src/nsGenericElement.cpp#2595

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 92026 [details] [diff] [review]
don't allow appendChild to insert itself or an ancestor

sr=jst

Comment 13

[Johnny Stenback (:jst)]

Oh, didn't see caillon's comment before I sr'ed, yes, this should happen ASAP in
the the method, my sr still stands if this new code is moved...

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 92026 [details] [diff] [review]
don't allow appendChild to insert itself or an ancestor

r=bryner (i'd suggest a space between the closing ')' and the '{' though)

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 92026 [details] [diff] [review]
don't allow appendChild to insert itself or an ancestor

Please make sure that this patch is not checked in as-is.  This check is
happening too late.  See my comment #9.

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 92026 [details] [diff] [review]
don't allow appendChild to insert itself or an ancestor

marking needs-work per caillon's comment.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: With the check moved earlier

This has corrected spacing, moves the check earlier so we avoid cycles when
this condition is met, and also adds a brief comment.

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 95667 [details] [diff] [review]
With the check moved earlier

Going to check this in with forwarded sr=jst and r=caillon (I think bryner's r=
still stands as well).

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Landed.