366117 - Update XMLToXMLString for ECMA-354 ed. 2

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

The numbering of the steps for converting an element to a string in jsxml.c are based on ECMA-354 edition 1, which isn't the version made available on the ECMA site.  (I don't even know where one would find ed. 1 now -- I feel lucky to have a copy of it.)  This makes comparing code to spec difficult, so we should update the code to reflect the 2nd ed. numbering.  I don't of any bugs the 2nd ed. wording fixes (tho I assume it fixes some), but at the least we should update the code for the readability win.

Several things:

I don't like uri_match (mismatched argument types and thus importance of argument ordering), but I'm not sure there's a better way to get the namespace for an attribute from ancdecls, unfortunately.  (Maybe there's a cleaner way to do this?  I doubt there is -- this is XML namespaces, after all.)

I had to split up the spec's step 11 so that step 13 works correctly, for fairly obvious reasons noted in the patch.  We can add that to the list of errata...

I'm pretty sure 16(b)(i) should use (AncestorNamespaces U namespaceDeclarations) for the argument to [[GetNamespace]].  However, note that because our [[GetNamespace]] does prefix matching, we can't actually use [[GetNamespace]] here or we'd generate a new prefix -- again -- for attributes in namespaces with undefined prefixes.  For the full details, see the long comment in step 16 in the patch.  I'm pretty certain with the changes I've made, there's no need for prefix generation in 16(b).  More errata...

In step 11 in code, is |prefix| rooted in the cx->newborn array so it can't be GC'd in js_NewXMLNamespace (or in a local root stack)?  I think it should be, but I'm not sure.

The change to step 11 (element portion) within |if (IS_EMPTY(prefix))| is necessary so that ancdecls doesn't contain multiple namespaces with the same prefix, which is a problem for our use of XMLArrayFindMember later on.  The first test in the testcase in the patch is code which would trigger a bug if this change weren't present.

The change to PutProperty is to deal with bug 357471, which I folded in here -- I don't think it's entirely necessary for the rest of the patch.

Last spec bug with ToXMLString: in 10.2.2, the spec is missing something along the lines of "If AncestorNamespaces was not given, let AncestorNamespaces = {}".

Comment 1

[Brendan Eich [:brendan]]

Sorry, still trying to get the time and energy to review. Maybe tonight.

/be

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 250683 [details] [diff] [review]
Patch

I timed out as a reviewer; David, could you take a look? This bug and erratum deserves Rhino attention too. Thanks,

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 250683 [details] [diff] [review]
Patch

Perhaps Igor will do it; I have failed.

/be

Comment 4

[Igor Bukanov]

 (In reply to comment #0)
> I don't of any bugs the 2nd
> ed. wording fixes (tho I assume it fixes some), but at the least we should
> update the code for the readability win.

So the patch does not change the list of failures against the test suite? Lately I have found a simple way to test for it via running 
  ./jsDriver.pl -e smdebug -L slow-n.tests 
before and after a patch and diffing *-failures.txt files.

> 
> Several things:
> 
> I don't like uri_match (mismatched argument types and thus importance of
> argument ordering), but I'm not sure there's a better way to get the namespace
> for an attribute from ancdecls, unfortunately.  (Maybe there's a cleaner way to
> do this?  I doubt there is -- this is XML namespaces, after all.)

I would define a helper function aka FindNamespaceByUri that would just use straight loop for compiler-time typesafty.

...
> I'm pretty sure 16(b)(i) should use (AncestorNamespaces U
> namespaceDeclarations) for the argument to [[GetNamespace]].  However, note
> that because our [[GetNamespace]] does prefix matching, we can't actually use
> [[GetNamespace]] here or we'd generate a new prefix -- again -- for attributes
> in namespaces with undefined prefixes.  For the full details, see the long
> comment in step 16 in the patch.  I'm pretty certain with the changes I've
> made, there's no need for prefix generation in 16(b).  More errata...

There is a similar issue in xml_setName. It would be nice to share the code and comments.

> 
> In step 11 in code, is |prefix| rooted in the cx->newborn array so it can't be
> GC'd in js_NewXMLNamespace (or in a local root stack)?  I think it should be,
> but I'm not sure.

It is in a local root stack.

> The change to PutProperty is to deal with bug 357471, which I folded in here 

I made bug 357471 to depend on this one to record the patch dependancy.

Comment 5

[Igor Bukanov]

Comment on attachment 250683 [details] [diff] [review]
Patch

>         if (IS_EMPTY(prefix)) {
>+            /* Remove the original namespace (with undefined >             /*
>-             * In the spec, ancdecls has no name, but is always written out as
>-             * (AncestorNamespaces U namespaceDeclarations).  Since we compute
>-             * that union in ancdecls, any time we append a namespace strong
>-             * ref to decls, we must also append a weak ref to ancdecls.  Order
>-             * matters here: code at label out: releases strong refs in decls.
>+             * One would think that an appropriate member would always be
>+             * found, but in fact this is not the case.  The reason is that
>+             * the spec uses [[GetNamespace]] in 16(b)(i), while we search
>+             * through (AncestorNamespaces U namespaceDeclarations) instead.
>+             * The spec's method, however, introduces a problem.
>+             *
>+             * The problem (and erratum) is that using attr->name passes both
>+             * the namespace URI and the prefix.  Since attr->name contains
>+             * contains the old prefix (possibly undefined, or NULL in our

double contains here

Comment 6

[Igor Bukanov]

Comment on attachment 250683 [details] [diff] [review]
Patch

>         if (IS_EMPTY(prefix)) {
>+            /* Remove the original namespace (with undefined >             /*
>-             * In the spec, ancdecls has no name, but is always written out as
>-             * (AncestorNamespaces U namespaceDeclarations).  Since we compute
>-             * that union in ancdecls, any time we append a namespace strong
>-             * ref to decls, we must also append a weak ref to ancdecls.  Order
>-             * matters here: code at label out: releases strong refs in decls.
>+             * One would think that an appropriate member would always be
>+             * found, but in fact this is not the case.  The reason is that
>+             * the spec uses [[GetNamespace]] in 16(b)(i), while we search
>+             * through (AncestorNamespaces U namespaceDeclarations) instead.
>+             * The spec's method, however, introduces a problem.
>+             *
>+             * The problem (and erratum) is that using attr->name passes both
>+             * the namespace URI and the prefix.  Since attr->name contains
>+             * contains the old prefix (possibly undefined, or NULL in our

double contains here

Comment 7

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Comment 8

[David Mandelin [:dmandelin]]

(In reply to Nochum Sossonko [:Natch] from comment #7)
> These bugs are all part of a search I made for js bugs that are getting lost
> in transit:
> 
> http://tinyurl.com/jsDeadEndBugs
> 
> They all have a review+'ed, non-obsoleted patch and are not marked
> fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300
> days. Some of these got lost simply because the assignee/patch provider
> never requested a checkin, or just because they were forgotten about.