Closed
Bug 366117
Opened 18 years ago
Closed 12 years ago
Update XMLToXMLString for ECMA-354 ed. 2
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
WONTFIX
mozilla1.9alpha1
People
(Reporter: Waldo, Unassigned)
References
Details
Attachments
(1 file)
22.08 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
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 = {}".
Attachment #250683 -
Flags: review?(brendan)
Comment 1•18 years ago
|
||
Sorry, still trying to get the time and energy to review. Maybe tonight. /be
Comment 2•18 years ago
|
||
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
Attachment #250683 -
Flags: review?(dcaldwell)
Comment 3•18 years ago
|
||
Comment on attachment 250683 [details] [diff] [review] Patch Perhaps Igor will do it; I have failed. /be
Attachment #250683 -
Flags: review?(brendan) → review?(igor)
Comment 4•18 years ago
|
||
(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.
Blocks: 357471
Comment 5•18 years ago
|
||
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
Attachment #250683 -
Flags: review?(igor) → review+
Comment 6•18 years ago
|
||
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•14 years ago
|
||
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.
Reporter | ||
Updated•13 years ago
|
Assignee: jwalden+bmo → general
Updated•12 years ago
|
Attachment #250683 -
Flags: review?(dcaldwell)
Comment 8•12 years ago
|
||
(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.
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.
Description
•