680858 - E4X: setNamespace generates invalid XML - duplicate attributes

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Rasmus Jensen]

Changing the namespace may lead to duplicate attribute names, ie.:

    var xml = <root xmlns:ns="http://example.org/"><blah/></root>
    var ns = new Namespace('ns','http://example.org/');
    xml.blah.@foo = 'bar';
    [e-mail](ns);
    xml.blah.@foo = 'baz';
    [e-mail](ns);


yields the following invalid/unparseable xml:

    <root xmlns:ns="http://example.org/">
      <blah ns:foo="bar" ns:foo="baz"/>
    </root>

The bug is SpiderMonkey specific, ie. Rhino yields this xml:

    <root xmlns:ns="http://example.org/">
      <blah ns:foo="baz"/>
    </root>


In the E4X standard [[Attributes]] is a set
with element-identity on x.[[Name]] (9.1.1),
but [[Attributes]] is implemented as an XMLList,
and thus changing x.[[Name]] can lead to
duplicates in the set.

Comment 1

[Rasmus Jensen]

Attachment: avoid duplicate attributes

I have attached a patch that fixes setNamespace,
such that changing a name of an attribute 
replaces possible duplicate, 
- same as Rhino in the testcase.
It also includes unit-test.

Is the patch ok,
or what needs to be changed
to get it accepted?

Comment 2

[Rasmus Jensen]

Added keyword checkin-needed to the bug,
as it contains the patch that needs review 
and checkin, to fix it.

Comment 3

[Jeff Walden [:Waldo]]

A patch is only ready for checkin after it's been properly reviewed.

I've been trying to work through the rest of my review queue before getting to this; it's a bit of a pain, because people keep piling stuff on it.  Unfortunately, E4X is lower priority than most other work, so a patch involving it, all else equal, takes second seat to other work.  Hopefully I'll be able to get to the review this week — I'd be really surprised if I don't.

Comment 4

[Jeff Walden [:Waldo]]

Um, wow.  Clearly I have been doing way too much blog-post-editing in the last few hours if I typed out "—" rather than a literal "--".

Comment 5

[Rasmus Jensen]

(In reply to Jeff Walden (remove +bmo to email) from comment #3)
> [...]
> involving it, all else equal, takes second seat to other work.  Hopefully
> I'll be able to get to the review this week — I'd be really surprised
> if I don't.

Thanks :)

Comment 6

[melkor]

setNamespace has other issues as well. See http://bugzilla.mozilla.org/show_bug.cgi?id=634525

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 554814 [details] [diff] [review]
avoid duplicate attributes

Sorry for the delay...

Unsurprisingly, this is an E4X spec bug.  I say "unsurprisingly" because historically, a large fraction of semantic bugs reported against our E4X implementation turn out to be spec bugs, even design flaws in the spec.  It is for this reason that our E4X implementation has been in maintenance mode for years, and it is why we have mostly stopped tracking E4X errata.  This is also why we have recently disabled E4X in ES5 strict mode code, see bug 695577, and it is why future ECMAScript editions will not include E4X.  It was an interesting experiment at the time.  But I think now there is general agreement that it was not a success.

Because this is a spec bug, any behavior here after this patch will be non-standard.  For this reason I recommend you not rely on this behavior even after this patch lands.  Moreover, with ES5's native JSON support, there are better alternatives to XML for data serialization and deserialization.  You should seriously consider whether some alternative to E4X, such as JSON, might serve your needs equally well.

I've pushed your patch (with minor formatting tweaks and such, and a commit message and proper attribution) to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2833dc64d964

It should make its way into mozilla-central in a day or so, to be in Firefox 10.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 554814 [details] [diff] [review]
avoid duplicate attributes

>--- a/js/src/jsxml.cpp
>+++ b/js/src/jsxml.cpp

>+static JSBool qn_match(const void *xml, const void *qn) {
>+    return qname_identity(((JSXML *)xml)->name, (JSObject *)qn);

This effectively const_casts for no reason, no?

Comment 9

[Jeff Walden [:Waldo]]

That's the method signature XMLArrayFindMember requires.

Now, looking at the uses, maybe that could be changed in a cleanup.  But this isn't an area of code where we're doing cleanup, except when doing so benefits the broader codebase.

Comment 10

[Nicholas Nethercote [inactive]]

e4x is being removed (bug 788293).