290056 - E4X: Spidermonkey shell crashes when serializing an XML object where the name of an attribute was changed with setName

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Martin Honnen]

Trying to explore the impact of the setName method I have written the following
test case:

var link = <link type="simple" />;
var xlinkNamespace = new Namespace('xlink', 'http://www.w3.org/1999/xlink');
link.addNamespace(xlinkNamespace);
print('In scope namespace: ' + link.inScopeNamespaces());
print('XML markup: ' + link.toXMLString());
[e-mail](new QName(xlinkNamespace.uri, 'type'));
print('name(): ' + link.@*::*[0].name());
print(link.toXMLString());

When run in the Spidermonkey shell the output is:

In scope namespace: ,http://www.w3.org/1999/xlink
XML markup: <link xmlns:xlink="http://www.w3.org/1999/xlink" type="simple"/>
name(): http://www.w3.org/1999/xlink::type
Assertion failure: serial <= n, at jsxml.c:2498

so it seems to crash when trying to execute toXMLString after the qualified name
of the attribute has been changed.

Rhino runs the test case without crashing with the following output:

In scope namespace: http://www.w3.org/1999/xlink,
XML markup: <link type="simple" xmlns:xlink="http://www.w3.org/1999/xlink"/>
name(): http://www.w3.org/1999/xlink::type
<link xlink:type="simple" xmlns:xlink="http://www.w3.org/1999/xlink"/>

Comment 1

[Martin Honnen]

Attachment: test case that crashes Spidermonkey shell

Comment 2

[Brendan Eich [:brendan]]

Namespace prefix generation, yum.

Martin: isn't Rhino showing a different bug?  Why is it prefixing type with
xlink: ?  The type attribute's name is in no namespace.

/be

Comment 3

[Martin Honnen]

(In reply to comment #2)
>
> Martin: isn't Rhino showing a different bug?  Why is it prefixing type with
> xlink: ?  The type attribute's name is in no namespace.

Not sure what you refer to, the XML markup Rhino outputs before setName is called is
  <link type="simple" xmlns:xlink="http://www.w3.org/1999/xlink"/>
after setName is called it is
  <link xlink:type="simple" xmlns:xlink="http://www.w3.org/1999/xlink"/>
which seems correct to me as setName has been called with
  new QName(xlinkNamespace.uri, 'type')
as the argument.

Comment 4

[Brendan Eich [:brendan]]

Never mind, I misread the test.  Fixing now, thanks for the report (again! keep
'em coming ;-).

/be

Comment 5

[Brendan Eich [:brendan]]

I claim ECMA-357 has a spec bug in 10.2.1 "ToXMLString Applied to the XML Type":

17. For each an in attrAndNamespaces
 a. Let s be the result of concatenating s and the space <SP> character
 b. If Type(an) is XML and an.[[Class]] == "attribute"
  i. Let ans be a copy of the result of calling [[GetNamspace]] on a.[[Name]]
     with argument AncestorNamespaces

The last line should read

     with argument AncestorNamespaces U namespaceDeclarations

as namespaceDeclarations (the xmlns= and xmlns:prefix= attributes in this
element) are in-scope for this element's attributes.

I also claim 13.4.4.35 XML.prototype.setName and 13.4.4.36
XML.prototype.setNamespace are incomplete.  They say absolutely nothing about
updating [[InScopeNamespaces]].  Other parts of the spec manually update this
set, but these methods assume that it magically keeps itself up-to-date with
respect to x.[[Name]].

/be

Comment 6

[Brendan Eich [:brendan]]

I just wrote:

> as namespaceDeclarations (the xmlns= and xmlns:prefix= attributes in this
> element) are in-scope for this element's attributes.

but of course, a default namespace does not apply to unprefixed attribute names.
 Still, for this testcase, the xlink prefix is in scope and GetNamespace should
match it against the attribute's [[Name]], once 13.4.4.35 is fixed to give that
QName the right uri and prefix.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix

These changes follow directly from the errata identified in comment 5.	I also
fixed the missing step 4 in 13.4.4.35, and the bungled step 1 in 13.4.4.36 (PIs
should be excluded by XML.prototype.setNamespace).

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

The previous patch failed on the following testcase:

var link = <link type="simple" />;
var xlinkNamespace = new Namespace('xlink', 'http://www.w3.org/1999/xlink');
link.addNamespace(xlinkNamespace);
print('In scope namespace: ' + link.inScopeNamespaces());
print('XML markup: ' + link.toXMLString());
[e-mail](new QName(xlinkNamespace, 'type'));
print('name(): ' + link.@*::*[0].name());
print(link.toXMLString());

Note how xlinkNamespace, not xlinkNamespace.uri, is passed to new QName(). 
This Namespace first argument causes new QName to propagate the 'xlink' prefix
from the first arg to the new name, botching the JS_ASSERT(!nameqn->prefix) in
the patch I'm obsoleting.

Another crucial testcase, this one for XML.prototype.setNamespace:

var link = <link type="simple" />;
var xlinkNamespace = new Namespace('xlink', 'http://www.w3.org/1999/xlink');
print('XML markup: ' + link.toXMLString());
[e-mail](xlinkNamespace);
print('name(): ' + link.@*::*[0].name());
print(link.toXMLString());

Did I say it sucks how much code was necessary in setName?  The spec has a big
hole there.

/be

Comment 9

[Brendan Eich [:brendan]]

*** Bug 277774 has been marked as a duplicate of this bug. ***

Comment 10

[Brendan Eich [:brendan]]

Fixed.

Martin, Bob: several ECMA TG1 meetings later, it's clear that
e4x/Regress/regress-278112.js is a bogus test, prompted by my marking bug 277774
INVALID and causing a Rhino bug to be filed.  I'm reopening 277774 and dup'ing
it against this bug, and marking the Rhino bug invalid.  We need a test that
wants setNamespace (and setName) to affect namespaceDeclarations, etc.

The original E4X design intention, TG1 members who were there say, was for the
[[InScopeNamespaces]] internal property to be kept up to date auto-magically.
The spec actually maintains it in some places, so it really should elsewhere, in
particular in 13.4.4.35 and 13.4.4.36.  Then, so long as it's mapped from the
[in-scope namespaces] info-set property, and so long as it is kept more or less
consistent with what prefixes are actually *used*, everything works well enough.

You cannot distinguish between a declared namespace and one that was added via a
mutating method such as setName, but that's considered a matter of indifference.

I'll have to rip out the declared flag in JSXMLNamespace, and rework logic, and
add some unused-namespace GC, to match this intention.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

for testing the crash only:
RCS file: /cvsroot/mozilla/js/tests/e4x/Regress/regress-290056.js,v
done
Checking in regress-290056.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-290056.js,v  <--  regress-290056.js
initial revision: 1.1

re comment 10. i'll start now.

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed.