355233 - Useless prototype chain loop in GetFunction from jsxml.c

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

The current code for GetFunction contains:

    do {
        /* XXXbe really want a separate scope for function::*. */
        if (!js_GetProperty(cx, obj, id, vp))
            return JS_FALSE;
        if (VALUE_IS_FUNCTION(cx, *vp)) {
            if (xml && OBJECT_IS_XML(cx, obj)) {
                fun = (JSFunction *) JS_GetPrivate(cx, JSVAL_TO_OBJECT(*vp));
                if (!FUN_INTERPRETED(fun) && fun->u.n.spare &&
                    (fun->u.n.spare & CLASS_TO_MASK(xml->xml_class)) == 0) {
                    /* XML method called on XMLList or vice versa. */
                    *vp = JSVAL_VOID;
                }
            }
            break;
        }
    } while ((obj = OBJ_GET_PROTO(cx, obj)) != NULL);
    return JS_TRUE;

But this prototype chain loop is unnecessary since js_GetProperty looks for prototype chain on its own.

Comment 1

[Igor Bukanov]

In fact there is a hidden bug in the code since it effectively assigns the meaning of JSFunction.u.n.spare. Here the function may come from Object.prototype where the meaning of spare is not constrained. 

To fix this it is not sufficient to check that the function comes from the xml object as script can alter XML.prototype. Thus the code should either check for list of functions or define the meaning of JSFunction.u.n.spare.

Comment 2

[Igor Bukanov]

I think the comment 1 reflect the fact that E4X does not define what should happen when XML-only method is applied to XML list. So this is a bug in specs since even without function:: extension one can get access to XML.prototype methods and call apply on them via:

function F() { }
F.prototype = new XML();
var obj = new F();
var removeNamespace = obj.removeNamespace;
removeNamespace.call(new XMLList(), uri);

Comment 3

[Brendan Eich [:brendan]]

I suspect (tell me if Rhino gets this right) that these methods should all be "bound methods", not reassignable or applicable to "the wrong type".

XMLList of length 1 is like XML, so maybe the apply case should work as if it were a direct call.  Otherwise TypeError.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> I suspect (tell me if Rhino gets this right) that these methods should all be
> "bound methods", not reassignable or applicable to "the wrong type".

In SM terms in Rhino XML and XMLList belong to different classes and XML-only methods insists that their parameter is XML.

> XMLList of length 1 is like XML, so maybe the apply case should work as if it
were a direct call.  Otherwise TypeError.

In fact this is what Rhino XMLList methods are doing, see as example
http://lxr.mozilla.org/mozilla/source/js/rhino/xmlimplsrc/org/mozilla/javascript/xmlimpl/XMLList.java#712

Note that function:: is not supported in Rhino, but one can still access the function of XML objects throw that prototype trick. 

Comment 5

[shutdown]

(In reply to comment #2)
> I think the comment 1 reflect the fact that E4X does not define what should
> happen when XML-only method is applied to XML list.

Standard ECMA-357 2nd Edition / December 2005
http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-357.pdf
I'm happy if there is a HTML version of the E4X specification...

13.4.4 Properties of the XML Prototype Object (Built-in Methods)
| None of the built-in functions defined on XML.prototype are generic.
| They throw a TypeError exception if the this value is not an XML object.
| Therefore, they cannot be transferred to other kinds of objects for use
| as a method.

13.5.4 Properties of the XMLList Prototype Object (Built-in Methods)
| None of the built-in functions defined on XMLList.prototype are generic.
| They throw a TypeError exception if the this value is not an XMLList object.
| Therefore, they cannot be transferred to other kinds of objects for use
| as a method.

"Treat XMLList of length 1 as XML" magic is done by CallMethod() described in
section 11.2.2.1 of the spec. In SM, implemented as JSXMLObjectOps->getMethod.

Comment 6

[Igor Bukanov]

(In reply to comment #5)
...
> 13.4.4 Properties of the XML Prototype Object (Built-in Methods)
> | None of the built-in functions defined on XML.prototype are generic.
> | They throw a TypeError exception if the this value is not an XML object.
> | Therefore, they cannot be transferred to other kinds of objects for use
> | as a method.
> 
> 13.5.4 Properties of the XMLList Prototype Object (Built-in Methods)
> | None of the built-in functions defined on XMLList.prototype are generic.
> | They throw a TypeError exception if the this value is not an XMLList object.
> | Therefore, they cannot be transferred to other kinds of objects for use
> | as a method.

OK, so both SM and Rhino got it wrong but in different ways. I file a separated bug about SM not throwing TypeError.

Comment 7

[Igor Bukanov]

This can be revisited after addressing the bug 355257.

Comment 8

[Nicholas Nethercote [inactive]]

e4x is going to be removed soon (bug 788293).