576596 - removal of JSObjectOps::(call|construct|hasInstance)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

JSObjectOps::(call|construct|hasInstance) duplicate the corresponding members of JSClass and their implementations in js_ObjectOps forward the calls into the corresponding class members. But this indirection does not add any new functionality while adding extra complexity to the code. So I suggest to remove it and always rely on JSClass for callability tests.

Comment 1

[Jason Orendorff [:jorendorff]]

Note that ES5 primitive this poses a minor problem here.

Currently the engine calls ToObject on the this-argument before calling JSClass::call. But a wrapper of an ES5 strict function should not do this.

  String.prototype.me = evalcx("(function () { 'use strict'; return this; })");
  assertEq("s".me(), "s");  // must return a primitive string

Once the engine actually supports ES5 primitive this, wrappers will need some kind of call hook that doesn't auto-ToObject in order to pass this test. Maybe a JSCLASS_ flag would suffice, along the lines of JSFUN_THISP_PRIMITIVE and JSFUN_FAST_NATIVE.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> Note that ES5 primitive this poses a minor problem here.
> 
> Currently the engine calls ToObject on the this-argument before calling
> JSClass::call.

This is true both for JSClass::call and JSObjectOps::call.

> Once the engine actually supports ES5 primitive this, wrappers will need some
> kind of call hook that doesn't auto-ToObject in order to pass this test. Maybe
> a JSCLASS_ flag would suffice, along the lines of JSFUN_THISP_PRIMITIVE and
> JSFUN_FAST_NATIVE.

JSObjectOps::call already deviates from JSClass::call as the former has started to use the fast call signature. So eliminating JSObjectOps::call entitles adding a flag that is not exposed via public API. But given the above perhaps we should just add it to jsapi.h so supporting primitive this for non-function callable could be added later transparently.

Comment 3

[Igor Bukanov]

Attachment: v1

In the patch to deal with fast/slow differences in the call operation between JSObjectOps and JSClass I added non-public CLASS_CALL_IS_FAST flag. JSCallOp is very new and its semantic could change.

In xpconnect the only necessary changes were to remove call/constructor clearance from XPC_WN_NoCall_JSOps since in xpconnect the code sets JSClass::(call|construct), not JSObjectOps, for callable objects, see http://hg.mozilla.org/tracemonkey/file/9749ef55a16b/js/src/xpconnect/src/xpcwrappednativejsops.cpp#l1730

Comment 4

[Igor Bukanov]

Attachment: v2

Here is a patch update for fat value changes

Comment 5

[Jason Orendorff [:jorendorff]]

> JS_REQUIRES_STACK bool
> InvokeConstructor(JSContext *cx, const InvokeArgsGuard &args)
> {
>+    JS_ASSERT(!js_FunctionClass.construct);
>     Value *vp = args.getvp();
> 
>+    if (vp->isPrimitive()) {
>+        /* Use js_ValueToFunction to report an error. */
>+        JS_ALWAYS_TRUE(!js_ValueToFunction(cx, vp, JSV2F_CONSTRUCT));
>+        return false;
>     }
>+    JSObject *obj2 = &vp->toObject();
>+

Looking at this I discovered a pre-existing bug. Section 11.2.1 of ES5 requires that we throw a TypeError without triggering the getter in the test below:

var a = 0;
try {
    new {get prototype() { a = 1; }};
    throw "should not get here";
} catch (exc) {
    assertEq(exc instanceof TypeError, true);
}
assertEq(a, 0);

Separate bug. But if you'd like to fix it while you're in this code, I wouldn't
object.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 457964 [details] [diff] [review]
v2

With these changes, I think js_Construct can be removed from jsobj.h/cpp and moved to jsinterp.cpp where it can be static (file-scope) function.

r=me with that.

Comment 7

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/defafc105ee0 - pushed with js_Call, js_Construct and js_HasInstance moved to jsinterp.cpp so the compiler has more possibilities to optimize.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> http://hg.mozilla.org/tracemonkey/rev/defafc105ee0 - pushed with js_Call,
> js_Construct and js_HasInstance moved to jsinterp.cpp so the compiler has more
> possibilities to optimize.

Style note for later:

We've been removing js_ prefixes when touching affected code, e.g. js::Interpret. Feel free to do this. There's a mopping up bug for later to get all js_ prefixes but if you're already in there, incremental action helps.

/be

Comment 9

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/fe1bd5e611e8 - follow up to rename js_HasInstance into HasInstance

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/defafc105ee0