373082 - Simpler sharing of XML and XMLList functions

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

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

Comment 1

[Brendan Eich [:brendan]]

(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

Comment 2

[Igor Bukanov]

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

Comment 3

[Igor Bukanov]

Attachment: Implementation v1

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

Comment 4

[Igor Bukanov]

Attachment: Implementation v2

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

Comment 5

[Igor Bukanov]

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. 
 

Comment 6

[Igor Bukanov]

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 7

[Brendan Eich [:brendan]]

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

Looks good to me.

/be

Comment 8

[Igor Bukanov]

Attachment: Implementation v3

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

Comment 9

[Igor Bukanov]

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

Comment 10

[Igor Bukanov]

Pinging for reviews.

Comment 11

[Jeff Walden [:Waldo]]

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

Comment 12

[Brendan Eich [:brendan]]

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

r=me pending jwalden's stamp of approval.

/be

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Igor Bukanov]

Attachment: Implementation v3b

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

Comment 15

[Igor Bukanov]

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

Comment 16

[Igor Bukanov]

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.

Comment 17

[Daniel Veditz [:dveditz]]

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

Comment 18

[Bob Clary [:bc] (inactive)]

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.

Comment 19

[Bob Clary [:bc] (inactive)]

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

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed linux, windows, mac* shell 20070406

Comment 21

[Bob Clary [:bc] (inactive)]

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

Comment 22

[Igor Bukanov]

Attachment: Better comment text.

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?

Comment 23

[Igor Bukanov]

Attachment: Implementation for 1.8 branch

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.

Comment 24

[Igor Bukanov]

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

Comment 25

[Igor Bukanov]

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.

Comment 26

[Igor Bukanov]

Attachment: diff between trunk and 1.8.1 versions

Comment 27

[Igor Bukanov]

Attachment: Implementation for 1.8 branch v1b

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

Comment 28

[Igor Bukanov]

Attachment: diff between trunk and 1.8.1 versions

Comment 29

[Bob Clary [:bc] (inactive)]

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

It matches or exceeds anything I would have written.

Comment 30

[Igor Bukanov]

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 31

[Brendan Eich [:brendan]]

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

Looks good.

/be

Comment 32

[Daniel Veditz [:dveditz]]

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

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 33

[Igor Bukanov]

Attachment: Implementation for 1.8.0 branch

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 34

[Igor Bukanov]

Attachment: diff between 1.8 and 1.8.0 patches

Comment 35

[Brendan Eich [:brendan]]

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

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

/be

Comment 36

[Igor Bukanov]

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 37

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0.12, a=dveditz for release-drivers

Comment 38

[Igor Bukanov]

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

Comment 39

[Bob Clary [:bc] (inactive)]

(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

Comment 40

[Bob Clary [:bc] (inactive)]

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

Comment 41

[Igor Bukanov]

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.

Comment 42

[Igor Bukanov]

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

Comment 43

[Roy Tam]

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

Comment 44

[Igor Bukanov]

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