Closed Bug 315657 Opened 19 years ago Closed 12 years ago

E4X errata and consequent bug in our implementation

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX
mozilla1.9alpha1

People

(Reporter: brendan, Unassigned)

References

Details

(Keywords: helpwanted)

E4X underspecifies Namespace prefix presevation and matching, but even with what it does specify, the standard lacks important details between 10.2.1 ToXMLString, 13.3.5.4 [[GetNamespace]] and arguably other places. Worse, there is a straightforward erratum in 10.2.1 ToXMLString 17(b)(i), in addition to the [[GetNamspace]] and "a" instead of "an" typos: 10.2.1 ToXMLString Applied to the XML Type: 17(b)(i) Let ans be a copy of the result of calling [[GetNamspace]] on a.[[Name]] with argument AncestorNamespaces The argument to [[GetNamespace]] must be (AncestorNamespaces U namespaceDeclarations), as in Step 11, or attributes will not be scoped by the in-scope namespaces for their element. Having fixed this (already done in SpiderMonkey's implementation), the other erratum bites as follows. This is the main topic of this bug. 13.3.5.4 GetNamespace Step 3 says: 3. Find a Namespace ns in InScopeNamespaces, such that ns.uri == q.uri. If more than one such Namespace ns exists, the implementation may choose one of the matching Namespaces arbitrarily. NOTE: implementations that preserve prefixes in qualified names may additionally constrain ns, such that ns.prefix == q.[[Prefix]] The testcase, from Werner Sharp of Macromedia, is this: ns = new Namespace("moo", "http://moo/"); x1 = <moo:alpha xmlns:moo="http://moo/" xmlns:tar="http://tar/"> <moo:bravo attr="1">one</moo:bravo> </moo:alpha>; MYXML = new XML(x1), print (MYXML); MYXML.ns::bravo.@attr.setNamespace("zoo"), print (MYXML); SpiderMonkey asserts (serial <= n) (line 2524, jsxml.c), and the reason is that GetNamespace does not match Namespace(undefined, "zoo") against attr's name, which is QName("zoo", "attr"). The attr's name has undefined prefix (encoded as a null qn->prefix in SpiderMonkey's impl). 13.3.5.4 Step 3 says that a prefix-preserving implementation *may* (really, should be *must*) compare prefixes and match on equality, or some other set of conditions. I naively implemented GetNamespace to require ns.uri == q.uri && ns.prefix == q.prefix But this does not work even for trivial examples. Consider x = <t xmlns="http://foo.com"/> print(x.toXMLString()); According to 10.3.2.1, mapping the info-set for the given XML literal to E4X's data model, the default namespace attribute in t, which has "http://foo.com" as its uri, has an empty string as its prefix: 10.3.2.1 Step 1. If the [local name] property of a is "xmlns" a. Map ns.prefix to the empty string However, the QName for t itself has undefined (in ECMA-357, null in our impl) as its prefix. See 10.3.2.1 Step 6; see also http://www.w3.org/TR/xml-infoset/ 2.2.3, where "no value" is the specified prefix in the infoset data model. "No value" must map to undefined, not to the empty string or any other value, in JS. (It would be good to say this explicitly in ECMA-357 10.3.2.1.) So t.[[Name]].[[GetNamespace]](t.[[InScopeNamespaces]]) will not match undefined against the empty string via ==, and will therefore make a new namespace. This cruds up the output of ToXMLString. To fix this, I long ago extended the logic in SpiderMonkey's GetNamespace to be: ns.uri == q.uri && (ns.prefix == q.prefix || (ns.prefix ? ns.prefix : q.prefix) == "") But this still does not handle Werner's test case, because an attribute such as attr in that case, after the setNamespace("zoo"), will still have an undefined (null in our impl) prefix for its name. Notice how 10.2.1 Step 17 loops over the union of x.[[Attributes]] and the local namespaceDeclarations created in Step 16. This makes the bug bite only if the namespace declaration for "zoo" is processed before the attribute "attr", so it seems there is yet another erratum: declared namespaces should be processed before attributes, for deterministic [[GetNamespace]] results. Or possibly namespaces should be processed after attributes so that prefixes may be auto-generated in the copies of the declared namespaces after the attributes' namespaces have been matched by [[GetNamespace]] in 17(b)(i). This would fix the bug, I believe. But let's assume we process namespaceDeclarations before attributes, as the current impl does. ToXMLString, when it gets to 17(c)(ii)(1), will assign a new prefix to the copy of the namespace (undefined, "zoo") that it made at Step 10(a)(i). Then when processing "attr" at 17(b)(i), ToXMLString will call [[GetNamespace]] to match the attribute's name's uri and prefix against (see the second paragraph in this comment, above) (AncestorNamespaces U namespaceDeclarations). Since namespaces in namespaceDeclarations that lacked prefixes have had prefixes generated by this point, the prefix-less attribute name, QName("zoo", "attr"), will not match using the twice-revised GetNamespace condition. How to revise the condition further? Or is the problem that 17 loops over the union of the attributes and namespaceDeclarations in arbitrary order, generating prefixes which then break subsequent [[GetNamespace]] calls for attributes? /be
Assignee: general → brendan
QA Contact: brendan → general
Summary: E4X erratum and consequent bug in our implementation → E4X errata and consequent bug in our implementation
Keywords: helpwanted
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
Is any progress beeing made on this bug? I was bitten by it today (or something very similar, same assert failure), but I can not say I understand why - I do not understand be's explanation, I am afraid. In fact, I hardly understand XML namespaces at all. This is my testcase that fails - it may be equivalent to the one you already know about: xml = new XML( <collection/> ); var ns = new Namespace( "", "http://tar" ); xml.setNamespace( ns ); xml.record += <record>foo</record>; print( xml.toXMLString() + "\n" ); xml.record += <record>bar</record>; print( xml.toXMLString() + "\n" ); Assertion failure: serial <= n, at jsxml.c:2551 Trace/breakpoint trap I believe the lines have just changed a bit, as your testcase also asserts at 2551 in current CVS. Alternatively, any workaround on this bug? I have been trying to do things in a different order, but I seem to end up in the failed assert eventually anyway... Thanks in advance.
Jeff, would appreciate your opinion on this one when you have time. Is the bug as described? Also, if so, care to try a fix? :-) /be
Assignee: brendan → general
Blocks: e4x
E4X will be removed again from Spidermonkey (bug 788293)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.