[This is a spin-off of bug 373020 comment 3] SpiderMonkey explicitly deviates from ECMA-357 and uses a shared prototype object for XML and XMLList. This reflects ECMA TG1 thinking that XML and XMLList should be unified. Then to implement the E4X requirement that certain XML methods should not be available in XML lists, the code uses a complex schema that checks during the method lookup the xml class of the object and hides XML-only methods from the XML lists. This not only complicated the code but also leads for discrepancies like: js> var l = <><a/></>; js> uneval(l) <a/> js> l.setName('b') js> uneval(l) <b/> js> XMLList.prototype.function::setName(l, 'c'); js> uneval(l) <b/> I.e. while l.setName('b') applies setName to the first element of the list, an explicit call using function::setName simply do nothing as setName simply returns when it it gets XML list instance. Thus I suggest to replace this hide-during-lookup code by explicit checks in XML-only methods for xml list objects. Then the code should throw an exception when it gets list object that does not have exactly one element. This introduces another deviation from E4X specs as the new approach would throw an exception during method call, not during method lookup meaning that the arguments to the method would be evaluated. But the a simpler implementation with better error reporting should allow for this.

(In reply to comment #0) > js> var l = <><a/></>; > js> uneval(l) > <a/> > js> l.setName('b') > js> uneval(l) > <b/> > js> XMLList.prototype.function::setName(l, 'c'); Do you mean .call(l, 'c') here? > js> uneval(l) > <b/> Otherwise, you're calling setName on XMLList.prototype as |this|, passing l as the new name -- and l does not convert to a string usable as an XML name. /be

(In reply to comment #1) Yes, the example has a typo, here is the right session demonstrating the discrepancy: js> var l = <><a/></>; js> uneval(l) <a/> js> l.setName('b') js> uneval(l) <b/> js> XMLList.prototype.function::setName.call(l, 'c') js> uneval(l) <b/>

Here is untested implementation that implements the idea from comment 0 and also makes sure that xml.name(...) is equivalent to xml.function::name.call(name, ...) even when the name comes from String.prototype wor xml with simple content: js> var l = <><a>text</a></> js> l.charAt(0) t js> l.function::charAt.call(l, 0) t js> with (l) function::charAt(0) t

The patch with better naming and added NON_LIST_XML_METHOD_PROLOG in xml_insertChildBefore that the previous version missed.

The patch causes another incomparability since it no longer converts XML objects with simple content to Strings during the method call to ensure that x.someFunction() is equivalent to x.function::someFunction(). toString, toSource and valueOf in String.prototype are not generic so they can not be applied to XML. Before the patch after deleting toString from XML and Object <a>TEXT</a>.toString() still works as xml_getMethod converts xml to string. But after the patch: js> delete XML.prototype.function::toString true js> delete Object.prototype.toString true js> <a>TEXT</a>.toString() typein:3: TypeError: String.prototype.toString called on incompatible XML IMO the new behavior makes more sence since it restores the property that object.method() always use object as this during the call.

With the patch applied the bug 355257 starts to bite much harder as now not only x = <a><name/></a>; x.(name == "Foo"); print(x.function::name()); triggers the exception, but also x.name() for exactly the same reason: js> x = <a><name/></a>; <a> <name/> </a> js> x.(name == "Foo"); js> print(x.name()); typein:3: TypeError: x.name is not a function

Comment on attachment 258096 [details] [diff] [review] Implementation v2 Looks good to me. /be

The new version keeps an extra prototype chain lookup when searching for functions not to depend bug 355257.

(In reply to comment #8) > The new version keeps an extra prototype chain lookup when searching for > functions not to depend bug 355257. Plus it fixes the leak of native properties in xml_deleteProperty: since "0 in xml" creates a native property corresponding to 0 id, it is necessary to call js_DeleteProperty even for indexes.

Pinging for reviews.

Sorry, a bit busy with pre-spring break cleanup -- I think I should be able to review by Friday.

Comment on attachment 258241 [details] [diff] [review] Implementation v3 r=me pending jwalden's stamp of approval. /be

Comment on attachment 258241 [details] [diff] [review] Implementation v3 >Index: js.msg >+MSG_DEF(JSMSG_NON_LIST_XML_METHOD, 219, 2, JSEXN_TYPEERR, "{0} is applied to XML list with {1} elements") "Cannot call {0} method on an XML list with {1} elements"? >Index: jsxml.c >+ * in the interpretr, the only time we add a JSScopeProperty here is when an interpreter >+#define NON_LIST_XML_METHOD_PROLOG \ >+ JS_BEGIN_MACRO \ >+ xml = StartNonListXMLMethod(cx, &obj, argv); \ >+ if (!xml) \ >+ return JS_FALSE; \ >+ JS_END_MACRO Add a JS_ASSERT(xml->xml_class != JSXML_CLASS_LIST); at the end of the macro as a sanity check.

Patch to commit with the nits from the comment 13 addressed.

I committed the patch from comment 14 to trunk: Checking in js.msg; /cvsroot/mozilla/js/src/js.msg,v <-- js.msg new revision: 3.77; previous revision: 3.76 done Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.151; previous revision: 3.150 done

Nominating to branches as this patch not only fixes the bug 374163 but also restores the long held invariant that property (or method lookup) never alters "this" object. Once such deviation already lead to a GC hazard and removal of it makes code safer.

What kind of regression tests do we have for this code? This is a fairly large patch for the security branch.

I will leave it to Igor to comment on how well the current tests cover the changes. I will of course add the tests Igor developed in this bug now that it is fixed.

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

verified fixed linux, windows, mac* shell 20070406

Comment on attachment 259668 [details] [diff] [review] Implementation v3b spelling nits. >Index: jsxml.c >+ * >+ * FIXME This clashes with the function namespace implementation which also >+ * uses native properties. Effectively after xml_lookupProperty any property >+ * stored previously using assignments to xml.function::name will be removed. >+ * We partially workaround the problem in js_GetXMLFunction. There we take >+ * advatage of the fact that typically function:: is used to access the ^^^^^^^^ advantage >+ * functions is stored in XML.prototype so when js_GetProperty returns >+ * non-function property, it represents the result of GetProprty setter hiding ^^^^^^^^^^ GetProperty

The patch rewrites the comment with the goal of making the message clearer while fixing grammar and misspellings. Does it reads reasonable for a native speaker?

To minimize the risk of regressions in this 1.8 backport I have not included the trunk version changes to make sure that code like xmlWithSimpleContent.function::charAt works or that xml_deleteProperty removes properties with indexes. To reflect that I suggest to split the test case in e4x/Regress/regress-373082.js into to parts where the second should include test_5 and test_6 that tests for xmlWithSimpleContent.function::charAt.

(In reply to comment #23) > To reflect that I suggest to split the test case in > e4x/Regress/regress-373082.js into to parts where the second should include > test_5 and test_6 that tests for xmlWithSimpleContent.function::charAt. A better way to address it is to file a new bug about xmlWithSimpleContent.function::charAt and move the tests there.

I filed bug 376773 about xmlWithSimpleContent.function::charAt fixed by the trunk patch and sections 5 and 6 of e4x/Regress/regress-373082.jsshould be moved to the test case for the new bug.

This is the previous patch with fixed comment text before xml_lookupProperty.

Comment on attachment 260872 [details] [diff] [review] Better comment text. It matches or exceeds anything I would have written.

I committed to the trunk the xml_lookupProperty comments fix from comment 22: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.153; previous revision: 3.152 done

Comment on attachment 260886 [details] [diff] [review] Implementation for 1.8 branch v1b Looks good. /be

Comment on attachment 260886 [details] [diff] [review] Implementation for 1.8 branch v1b approved for 1.8.1.4, a=dveditz for release-drivers

This is a straightforward hand merge of 1.8 patch into 1.8.0 branch. The differences in the patches come from rename VALUE_IS_FUNCTION -> JSVAL_S_FUNCTION and the facts that js_GetXmlFunction does not present on 1.8.0 and js_GetClassPrototype has different signature there.

Comment on attachment 261513 [details] [diff] [review] Implementation for 1.8.0 branch Based on the diff of diffs, r=me. Thanks, /be

I committed the patch from comment 27 to MOZILLA_1_8_BRANCH: Checking in js.msg; /cvsroot/mozilla/js/src/js.msg,v <-- js.msg new revision: 3.43.8.15; previous revision: 3.43.8.14 done Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.66; previous revision: 3.50.2.65 done

Comment on attachment 261513 [details] [diff] [review] Implementation for 1.8.0 branch approved for 1.8.0.12, a=dveditz for release-drivers

I committed the patch from comment 33 to MOZILLA_1_8_0_BRANCH : Checking in js.msg; /cvsroot/mozilla/js/src/js.msg,v <-- js.msg new revision: 3.43.8.2.2.5; previous revision: 3.43.8.2.2.4 done Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.15.2.37; previous revision: 3.50.2.15.2.36 done

(In reply to comment #25) > I filed bug 376773 about xmlWithSimpleContent.function::charAt fixed by the > trunk patch and sections 5 and 6 of e4x/Regress/regress-373082.jsshould be > moved to the test case for the new bug. > /cvsroot/mozilla/js/tests/e4x/Regress/regress-373082.js,v <-- regress-373082.js new revision: 1.2; previous revision: 1.1 /cvsroot/mozilla/js/tests/e4x/XML/regress-376773.js,v <-- regress-376773.js new revision: 1.2; previous revision: 1.1

verified fixed windows, macppc, linux 1.8.0, 1.8.1 shell 20070417

To Roy Tam: Which compiler generated that error on 1.8.0? Also could you check that you have proper CVS tree as I do not see yet any problems with the way js_GetClassPrototype is used in js_GetXMLFunction from jsxml.c on 1.8.0 branch.

(In reply to bug 375270 comment #55) > (In reply to bug 375270 comment #54) > > (In reply to bug 375270 comment #53) > > > Igor: > > > Your changes to 1.8.0 branch ( > > > http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_0_BRANCH&branchtype=match&dir=&file=&filetype=match&who=igor%25mir2.org&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-12+11%3A31&maxdate=2007-04-16+11%3A31&cvsroot=%2Fcvsroot > > > ) > > > breaks the build, I got this error message: > > > > > > This is for bug 373082, not this bug. > > > > No I'm not talking about the warning messages. > the js_GetClassPrototype function in js_GetXMLFunction cause error when > compiling: > > c:/moz180\mozilla\js\src\jsxml.c(8094) : error C2198: 'js_GetClassPrototype' : > too few arguments for call through pointer-to-function > > /** jsxml.c Version 3.50.2.15.2.37 Source - line 8091 **/ > xml = (JSXML *) JS_GetPrivate(cx, obj); > if (HasSimpleContent(xml)) { > /* Search in String.prototype to implement 11.2.2.1 Step 3(f). */ > /* This -> */ ok = js_GetClassPrototype(cx, js_String_str, > &tvr.u.object); > if (!ok) > goto out; > JS_ASSERT(tvr.u.object); > ok = OBJ_GET_PROPERTY(cx, tvr.u.object, id, vp); > } > The code calls js_GetClassPrototype with 3 arguments: js_GetClassPrototype(cx, js_String_str, &tvr.u.object) On 1.8.0 the function is defined as: extern JSBool js_GetClassPrototype(JSContext *cx, const char *name, JSObject **protop); (see http://lxr.mozilla.org/mozilla1.8.0/source/js/src/jsobj.h#497) Yet the error is: 'js_GetClassPrototype' : too few arguments for call through pointer-to-function So it seems js_GetClassPrototype was somehow shadowed by some other definition. Can you grep your tree for js_GetClassPrototype to check that it matches http://lxr.mozilla.org/mozilla1.8.0/ident?i=js_GetClassPrototype ?

(In reply to comment #42) > The code calls js_GetClassPrototype with 3 arguments: > js_GetClassPrototype(cx, js_String_str, &tvr.u.object) > > On 1.8.0 the function is defined as: > extern JSBool js_GetClassPrototype(JSContext *cx, const char *name, JSObject > **protop); > > (see http://lxr.mozilla.org/mozilla1.8.0/source/js/src/jsobj.h#497) > > Yet the error is: > > 'js_GetClassPrototype' : too few arguments for call through pointer-to-function > > So it seems js_GetClassPrototype was somehow shadowed by some other definition. > Can you grep your tree for js_GetClassPrototype to check that it matches > http://lxr.mozilla.org/mozilla1.8.0/ident?i=js_GetClassPrototype > ? > I'm sorry that I forgot to mention that my work copy has Bug 321985 (patch 211045) patched. Hope this code changes works. - ok = js_GetClassPrototype(cx, js_String_str, &tvr.u.object); + classAtom = js_Atomize(cx, js_String_str, strlen(js_String_str), 0); + ok = js_GetClassPrototype(cx, NULL, classAtom, &tvr.u.object);

(In reply to comment #43) > I'm sorry that I forgot to mention that my work copy has Bug 321985 (patch > 211045) patched. > > Hope this code changes works. > - ok = js_GetClassPrototype(cx, js_String_str, &tvr.u.object); > + classAtom = js_Atomize(cx, js_String_str, strlen(js_String_str), 0); > + ok = js_GetClassPrototype(cx, NULL, classAtom, &tvr.u.object); In that case just pass cx->runtime->atomState.StringAtom as the atom argument.