350442 - toXMLString omits namespace definition

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Lubos Ures]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: JavaScript-C 1.7 pre-release 1 2006-04-04

After adding an attribute to tag the atribute's namespace prefix is retained correctly but the namespace definition is lost; toXMLString thus produces invalid XML markup. The XML object itself seems to be OK.


Reproducible: Always

Steps to Reproduce:
1. Run example from additional information under jsshell.


Actual Results:  
<tag1 xmlns="http://ns1" ns2:a1="a1 from ns2"/>

Expected Results:  
<tag1 xmlns="http://ns1" xmlns:ns2="http://ns2" ns2:a1="a1 from ns2"/>

Example:
var t1 = <tag1 xmlns="http://ns1"/>
var n2 = new Namespace("http://ns2")
t1.@n2::a1="a1 from ns2"
print(t1.toXMLString())

Comment 1

[Brendan Eich [:brendan]]

Jeff, you game to fix another E4X toXMLString bug?

/be

Comment 2

[Jeff Walden [:Waldo]]

(In reply to comment #1)
> Jeff, you game to fix another E4X toXMLString bug?

Sure, I'll take a stab at it.  Not sure exactly when I'll get to it, tho -- at the moment I have other, more pressing matters to process.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Move some code around

Comment 4

[Jeff Walden [:Waldo]]

In the language in the spec, after outputting "<tag1" we iterate over namespaces and attributes, outputting each as needed.  For each attribute hit, we look up its namespace to get its prefix.  If the prefix is undefined, we generate a unique prefix, create a Namespace corresponding to the attribute's namespaceURI with .prefix equal to the one generated, and *add that Namespace to the set of ancestor namespaces and namespace declarations* so that we iterate over the Namespace while generating the string for the current element (and thus add " xmlns:ns='nameSpaceURI'" to the string).

In the current implementation, however, we output strings for namespaces before outputting strings for attributes.  Consequently, if we have to generate a prefix for an attribute, we create the Namespace for it and add it to the set of ancestor declarations and namespace declarations, *but we never iterate over it to output the "xmlns:ns='namespaceURI'" string* and thus produce the behavior described here.  The namespace is still added to the correct set, however, so all other behavior would still be the same.

The fix is simple: since generating attribute strings can require new namespaces to be created and declared on the element, do that before generating namespace strings.

I tested the patch against all js/tests/e4x tests; it works with no failures not present in a build created from the current tree.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 235859 [details] [diff] [review]
Move some code around

To quote Basil Fawlty: BRILLIANT!

You are close to becoming E4X sub-module owner :->.

/be

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 235859 [details] [diff] [review]
Move some code around

Drivers sayz: Please land on the trunk before requesting 1.8.1approval, and also let us know if this is a JS1.7 regression or fixing an existing bug? Thanks!

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 235859 [details] [diff] [review]
Move some code around

Landed on trunk.

This fixes an already-existing bug which wouldn't have been a JS1.7 regression, for the record.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 235859 [details] [diff] [review]
Move some code around

a=beltzner on behalf of 181drivers

Comment 9

[Jeff Walden [:Waldo]]

Fixed on branch.

Comment 10

[Bob Clary [:bc] (inactive)]

Checking in regress-350442.js;
/cvsroot/mozilla/js/tests/e4x/Namespace/regress-350442.js,v  <--  regress-350442.js
initial revision: 1.1

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.8, 1.9 20060906 windows/mac*/linux