312692 - E4X: appendChild() does not copy the child

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Aiko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051016 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051016 Firefox/1.6a1

appendChild() seems to append the child itself instead of a deep copy of it,
which may lead to the same child being present more than once. See testcase for
an example.

Reproducible: Always

Steps to Reproduce:
1. Run testcase
Actual Results:  
Second
Second

Expected Results:  
First
Second

Comment 1

[Aiko]

Attachment: testcase

Comment 2

[Aiko]

Attachment: another testcase

This is fun: you can even make a XML object its own child. Old reactionary
toString doesn't like such incestuous behaviour one bit and crashes as soon as
it encounters it.

Comment 3

[Brendan Eich [:brendan]]

This is a bug in ECMA-357.  It doesn't prevent you from creating such cycles. 
It should, so that all implementations that conform do so prevent, preventing
DOS atacks and unpleasant divergence just because of user error (which might
happen only now and then, so not be caught during unit or surface testing).

/be

Comment 4

[Brendan Eich [:brendan]]

What does Rhino do, btw?

/be

Comment 5

[Brendan Eich [:brendan]]

mrbkap points out that the ECMA-357 spec says (in its "Overview", but *not* in
the corresponding "Semantics"):

The appendChild method appends a deep copy of the given child to the end of this
XML object’s properties and returns this XML object.

So perhaps that's the fix.  It would certainly avoid cycles, but it would break
many cases that want object reference, not value, semantics.  And it does not
match insertChildBefore and insertChildAfter, which per spec can also be used to
make cycles, and which have no "deep copy" language in their "Overviews".

Bletch.  I suppose we could add cycle detection.

/be

Comment 6

[Brendan Eich [:brendan]]

Cycle detection means insuring that the would-be child can't reach the putative
parent via a path in the DAG.  Yes, DAG: we allow

  parent = <p/>;
  parent.child = "first";
  child = parent.child[0];
  parent.insertChildBefore(null, child);

to create a DAG with two arcs from parent to child, such that

  parent.child[0] === parent.child[1]

Round-tripping through toXMLString and the XML constructor (or eval) results in
two distinct (!==) but equal (==) kids.

This is how I'd like to fix this bug, and I'll fix all the append/insert cases
at the lowest common level.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #4)
> What does Rhino do, btw?

Rhino copies in the appendChild case, hangs or crashes in insert*Child cases
that mrbkap tried.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 200263 [details] [diff] [review]
proposed fix

r=mrbkap

Comment 10

[Aiko]

To clarify: your position is that the 'Overview' text of
appendChild/prependChild is incorrect and that these methods should use the
child itself, not a copy of it?
So, if I really wanted a copy, I'd have to do it myself; for example, instead of
|parent.appendChild(child)| I'd write |parent.appendChild(child.copy())| ?
Sounds good to me.

Just wondering: is a second edition of ECMA-357 being worked on?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> To clarify: your position is that the 'Overview' text of
> appendChild/prependChild is incorrect and that these methods should use the
> child itself, not a copy of it?

Yes.

> So, if I really wanted a copy, I'd have to do it myself; for example, instead of
> |parent.appendChild(child)| I'd write |parent.appendChild(child.copy())| ?
> Sounds good to me.

Good.

> Just wondering: is a second edition of ECMA-357 being worked on?

Yes, and it's based on the ISO version of ECMA-357, ISO/IEC 22537, and it is
almost too late to fix this hole in the spec to do with cycles and "deep copy"
verbiage in the Overviews of two methods, but not in the Semantics.  But I am
pushing for it to be fixed.

Otherwise, yet more errata to keep posted at mozilla.org....

/be

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 200263 [details] [diff] [review]
proposed fix

sr=shaver.   We want this for the branch?

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 200263 [details] [diff] [review]
proposed fix

Yes, we want this -- just had an ECMA TG1 call today and we talked about this. 
It's happening for Edition 2 of ECMA-357.

/be

Comment 14

[Brendan Eich [:brendan]]

Fixed on branch and trunk.

/be

Comment 15

[Bob Clary [:bc] (inactive)]

RCS file: /cvsroot/mozilla/js/tests/e4x/XML/13.4.4.3-01.js,v
done
Checking in 13.4.4.3-01.js;
/cvsroot/mozilla/js/tests/e4x/XML/13.4.4.3-01.js,v  <--  13.4.4.3-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/e4x/XML/13.4.4.3-02.js,v
done
Checking in 13.4.4.3-02.js;
/cvsroot/mozilla/js/tests/e4x/XML/13.4.4.3-02.js,v  <--  13.4.4.3-02.js
initial revision: 1.1
done

Comment 16

[Aiko]

(In reply to comment #15)

Bob, taking Brendan's decision into account, these testcases (and my comment 0) are not correct. The result of the first testcase should be
  <ul><li>Second</li><li>Second</li></ul>
and the second testcase should produce a "Cyclic XML value" error.

And it's XML.prettyPrinting, not XML.prettyPrint.

Comment 17

[Bob Clary [:bc] (inactive)]

(In reply to comment #16)
Checking in 13.4.4.3-01.js;
/cvsroot/mozilla/js/tests/e4x/XML/13.4.4.3-01.js,v  <--  13.4.4.3-01.js
new revision: 1.2; previous revision: 1.1
done
Checking in 13.4.4.3-02.js;
/cvsroot/mozilla/js/tests/e4x/XML/13.4.4.3-02.js,v  <--  13.4.4.3-02.js
new revision: 1.2; previous revision: 1.1
done

thanks for catching my mistakes.

Comment 18

[Bob Clary [:bc] (inactive)]

verified firefox 1.5 rc2 winxp/linux

Comment 19

[David Caldwell]

It looks like according to ECMA-357 the parent() method of an XML object that has been added to multiple parents should return the last parent to which it was added ... does this sound right?

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> It looks like according to ECMA-357 the parent() method of an XML object that
> has been added to multiple parents should return the last parent to which it
> was added ... does this sound right?

Yes. Grepping for '>parent = [^N]' in js/src/jsxml.c shows the places this happens, and cites chapter and verse.

/be

Comment 21

[Brendan Eich [:brendan]]

Jeff, did the second edition of ECMA-357 (or whatever went to ISO) actually specify that appendChild does not copy?

/be