283349 - Assertion failure: ns->prefix, at jsxml.c:2298

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

The following script:

var x = <x>text</x>;
var ns = new Namespace("http://foo.com/bar");
x.addNamespace(ns);
print(x.toXMLString());

Generates assertion failure when run in js shell built from trunk on 2005-02-23:
js -x ~/s/x.js
Assertion failure: ns->prefix, at jsxml.c:2298
Aborted

Comment 1

[Brendan Eich [:brendan]]

Attachment: handle null ns->prefix in GetNamespace

Oops, got carried away there -- too many degrees of freedom in E4X on whether a
namespace can have an undefined (null in this impl) prefix, the empty string
(meaning default namespace), or a non-empty string.  GetNamespace was ignoring
the undefined (null) case that can arise programmatically via 'ns = new
Namespace; x.addNamespace(ns)'.

In general, qn->prefix can be null or the empty string, too.  For a namespace
ns, !ns->prefix means that no prefix was specified, while IS_EMPTY(ns->prefix)
means this is a default namespace.  Since qn->prefix is set only by calls to
js_NewXMLQName*(), and some of those pass NULL for prefix while others pass a
variable that came from ns->prefix (which could be the empty string), we have
to test like so:

(!ns->prefix || IS_EMPTY(ns->prefix)) && (!qn->prefix || IS_EMPTY(qn->prefix))

if either ns->prefix or qn->prefix is null, when comparing a namespace's prefix
to a qname's prefix.

Looking for review from igor and shaver, to maximize E4X brainprint.

/be

Comment 2

[Brendan Eich [:brendan]]

From ECMA-357 13.3.5.4 [[GetNamespace]]:

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]]

That NOTE is less than helpful.  As the "Erratum, very tricky, ..." comment just
barely visible in the patch explains, due to how xml-infoset is mapped into e4x,
q.[[Prefix]] for an unprefixed XML tag will be undefined, but we want the
default namespace (prefix == "") to match.

This patch makes the reverse-match work too: an ns with ns.prefix *undefined*
(null in the code) matches a q.[[Prefix]] == "".  You won't get an XML node with
[[Name]].[[Prefix]] == "" via the ECMA-357-specified mapping from xml-infoset,
but you could construct such a thing with setName.

Whee!  I'm still thinking dark thoughts about E4X distinguishing "no prefix"
from "default namespace prefix (empty string)", but not carrying the logic thru
as required.

/be

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 175348 [details] [diff] [review]
handle null ns->prefix in GetNamespace

sr=shaver with the IS_EMPTY(ns->prefix ? ns->prefix : qn->prefix)
simplification.

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix I just checked in

Comment 5

[Bob Clary [:bc] (inactive)]

js/tests/e4x/Regress/regress-283349.js checked in.

Comment 6

[Bob Clary [:bc] (inactive)]

verified fixed.