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

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

v2
20.76 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
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.
(Assignee)

Comment 2

8 years ago
(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.
(Assignee)

Comment 3

8 years ago
Created attachment 455882 [details] [diff] [review]
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
Attachment #455882 - Flags: review?(jorendorff)
(Assignee)

Comment 4

8 years ago
Created attachment 457964 [details] [diff] [review]
v2

Here is a patch update for fat value changes
Attachment #455882 - Attachment is obsolete: true
Attachment #457964 - Flags: review?(jorendorff)
Attachment #455882 - Flags: review?(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 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.
Attachment #457964 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

8 years ago
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.
Whiteboard: fixed-in-tracemonkey
(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
(Assignee)

Comment 9

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

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/defafc105ee0
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.