js_TryMethod is usually the wrong answer

RESOLVED FIXED in mozilla5

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey, URL)

Attachments

(1 attachment)

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.
Created attachment 522831 [details] [diff] [review]
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.
Attachment #522831 - Flags: review?(luke)

Comment 2

7 years ago
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.
Attachment #522831 - Flags: review?(luke) → review+
Duplicate of this bug: 647831
http://hg.mozilla.org/tracemonkey/rev/0906d9490eaf
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.2
Duplicate of this bug: 190809
Depends on: 648739
Depends on: 649017
http://hg.mozilla.org/mozilla-central/rev/0906d9490eaf
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 650574
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
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.
Depends on: 653789
Depends on: 657585
Depends on: 660438
Depends on: 715387
Depends on: 717497

Updated

6 years ago
Depends on: 743301
You need to log in before you can comment on or make changes to this bug.