Igor Bukanov Assignee
	Description • 18 years ago

[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.

	Brendan Eich [:brendan]
	Comment 1 • 18 years ago

(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

	Igor Bukanov Assignee
	Comment 2 • 18 years ago

(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/>

	Igor Bukanov Assignee
	Comment 3 • 18 years ago

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

	Igor Bukanov Assignee
	Comment 4 • 18 years ago

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

	Igor Bukanov Assignee
	Comment 5 • 18 years ago

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. 
 

	Igor Bukanov Assignee
	Comment 6 • 18 years ago

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

 

	Brendan Eich [:brendan]
	Comment 7 • 18 years ago

Comment on attachment 258096 [details] [diff] [review]
Implementation v2

Looks good to me.

/be

	Igor Bukanov Assignee
	Comment 8 • 18 years ago

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

	Igor Bukanov Assignee
	Comment 9 • 18 years ago

(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. 

	Igor Bukanov Assignee
	Comment 10 • 18 years ago

Pinging for reviews.

	Jeff Walden [:Waldo]
	Comment 11 • 18 years ago

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

	Brendan Eich [:brendan]
	Comment 12 • 18 years ago

Comment on attachment 258241 [details] [diff] [review]
Implementation v3

r=me pending jwalden's stamp of approval.

/be

	Jeff Walden [:Waldo]
	Comment 13 • 18 years ago

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.

	Igor Bukanov Assignee
	Comment 14 • 18 years ago

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

	Igor Bukanov Assignee
	Comment 15 • 18 years ago

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

	Igor Bukanov Assignee
	Comment 16 • 18 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 17 • 18 years ago

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

	Bob Clary [:bc] (inactive)
	Comment 18 • 18 years ago

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.

	Bob Clary [:bc] (inactive)
	Comment 19 • 18 years ago

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

	Bob Clary [:bc] (inactive)
	Comment 20 • 18 years ago

verified fixed linux, windows, mac* shell 20070406

	Bob Clary [:bc] (inactive)
	Comment 21 • 18 years ago

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

	Igor Bukanov Assignee
	Comment 22 • 18 years ago

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?

	Igor Bukanov Assignee
	Comment 23 • 18 years ago

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.

	Igor Bukanov Assignee
	Comment 24 • 18 years ago

(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.

	Igor Bukanov Assignee
	Comment 25 • 18 years ago

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.

	Igor Bukanov Assignee
	Comment 27 • 18 years ago

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

	Bob Clary [:bc] (inactive)
	Comment 29 • 18 years ago

Comment on attachment 260872 [details] [diff] [review]
Better comment text. 

It matches or exceeds anything I would have written.

	Igor Bukanov Assignee
	Comment 30 • 18 years ago

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

	Brendan Eich [:brendan]
	Comment 31 • 18 years ago

Comment on attachment 260886 [details] [diff] [review]
Implementation for 1.8 branch v1b

Looks good.

/be

	Daniel Veditz [:dveditz]
	Comment 32 • 18 years ago

Comment on attachment 260886 [details] [diff] [review]
Implementation for 1.8 branch v1b

approved for 1.8.1.4, a=dveditz for release-drivers

	Igor Bukanov Assignee
	Comment 33 • 18 years ago

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.

	Brendan Eich [:brendan]
	Comment 35 • 18 years ago

Comment on attachment 261513 [details] [diff] [review]
Implementation for 1.8.0 branch

Based on the diff of diffs, r=me. Thanks,

/be

	Igor Bukanov Assignee
	Comment 36 • 18 years ago

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

	Daniel Veditz [:dveditz]
	Comment 37 • 18 years ago

Comment on attachment 261513 [details] [diff] [review]
Implementation for 1.8.0 branch

approved for 1.8.0.12, a=dveditz for release-drivers

	Igor Bukanov Assignee
	Comment 38 • 18 years ago

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

	Bob Clary [:bc] (inactive)
	Comment 39 • 18 years ago

(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

	Bob Clary [:bc] (inactive)
	Comment 40 • 18 years ago

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

	Igor Bukanov Assignee
	Comment 41 • 18 years ago

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.

	Igor Bukanov Assignee
	Comment 42 • 18 years ago

(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
?

	Roy Tam
	Comment 43 • 18 years ago

(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);

	Igor Bukanov Assignee
	Comment 44 • 18 years ago

(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.