645468 - js_TryMethod is usually the wrong answer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

There's nothing like it in the spec.  Worse, the places we use it only rarely (?, haven't checked them all) want the semantics it implements.  We should fix all the users to implement whatever semantics they're supposed to implement and probably remove js_TryMethod completely.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

js_TryValueOf should really also go away, rather than be kind of fixed up in a way that makes it accord somewhat with some parts of the spec.  But there's a limit to how much should be done in a bug, afield from its original purpose.  That seems something to fix as part of bug 646129, which I think should be fixed by repurposing JSClass::convert to be exactly [[DefaultValue]], rather than...whatever it is these days.

https://developer.mozilla.org/en/JSClass.convert suggests we once had JSObjectOps::defaultValue, and thus we...probably...didn't have this bug at one time, but it's all kind of a mess of partly-overlapping functionality.  Time to simplify -- but elsewhere, in a followup.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 522831 [details] [diff] [review]
Patch

Nice work ECMA-man!  Also nice job with your past/future pursuits to make our use of convert/default/ToObject/etc match the spec.  This type of code frustrated me greatly a year ago.

One nit:

>+     * Get the property with the given id, then call it as a function with the
>+     * given arguments, providing this object as |this|.  If the property isn't
>+     * callable a TypeError will be thrown.  On success the value returned by
>+     * the call is stored in *vp.

I think the style is single space after period.  Certainly for the other Brendan-comments in jsobj.h.

> JSBool
> js_TryValueOf(JSContext *cx, JSObject *obj, JSType type, Value *rval)
> {
>     Value fval;
>+    jsid id = ATOM_TO_JSID(cx->runtime->atomState.valueOfAtom);
>+    if (!js_GetMethod(cx, obj, id, JSGET_NO_METHOD_BARRIER, &fval))
>         return false;
>+    if (js_IsCallable(fval)) {
>+        Value v;
>+        Value argv[] = { StringValue(cx->runtime->atomState.typeAtoms[type]) };
>+        if (!ExternalInvoke(cx, ObjectValue(*obj), fval, JS_ARRAY_LENGTH(argv), argv, &v))
>+            return false;
>+        if (v.isPrimitive()) {
>+            *rval = v;
>+            return true;
>+        }
>+    }
>+    return true;
> }

It seems a bit weird to only set *rval on the v.isPrimitive() path.  Tracing this back to the primary caller, DefaultValue, I can see why this is ok; the previous (object) value remains and then throws later down.  The previous version of js_TryValueOf also sometimes left rval unset, but for a slightly different set of cases.  The whole area is kooky.  Maybe you will sweep all this up in your followup bug; I hope so.

Comment 4

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/0906d9490eaf

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/0906d9490eaf

Comment 7

[Brendan Eich [:brendan]]

Great to see js_TryMethod go -- thanks. It predated ECMA-262 and went back to the first JS impl ("Mocha"). The spec cleaned up things, and modeling the code on it where possible is a win.

Cc'ing reviewers is good. Dependency bugs need fixing in same release cycle as this one.

How'd that stack overflow bug 650574 come in, though? The patch did not remove any CHECK_STACK macro uses.

/be

Comment 8

[Jeff Walden [:Waldo]]

The stack overflow occurs because js_TryMethod had a JS_CHECK_RECURSION at the start but the semi-inlinings here do not, I believe.  That should be fairly easy to fix by adding a check to js::DefaultValue, I think -- I'll probably get to it on Monday.