412571 - JSStackFrame.thisp should be jsval thisv; no need to wrap primitive this passed to scripted method

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brendan Eich [:brendan]]

The great work a few years ago by Andreas <[e-mail]> to avoid auto-wrapping primitive strings, numbers, and booleans with String, Number, and Boolean clothes when calling native methods that opt into such primitive |this| values should be extended to avoid auto-wrapping when calling scripted methods.

JSStackFrame.thisp should become a jsval thisv (it's still needed as a root, in case argv[-1] is mutated, or argv is null as at top-level).

/be

Comment 1

[Brendan Eich [:brendan]]

Not a big deal unless you add scripted methods to standard primitive wrapper prototypes (String.prototype, etc.). Will get after 1.9/fx3.

/be

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This'd make some TM stuff easier too, since we'd prefer to be tracking a jsval rather than the JSFRAME_COMPUTED_THIS/thisp stuff.

Comment 3

[Brendan Eich [:brendan]]

We must not force eager |this| computation, however. Is changing thisp to thisv really going to help tracing? Either way you have a "0-tagged" pointer.

/be

Comment 4

[Andreas Gal :gal]

Stealing with permission.

Comment 5

[Jim Blandy :jimb]

ECMAScript 5 (draft) requires that primitive values not be wrapped.  SpiderMonkey at present says:

js> String.prototype.f = function () { print(uneval(this)); }
function () {
    print(uneval(this));
}
js> "hi!".f()
(new String("hi!"))
js> 

But ECMAScript 5 requires that output to be simply: "hi!"

Andreas tells me he won't be able to get to this soon, so I've put it back in the pool.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> ECMAScript 5 (draft) requires that primitive values not be wrapped. 

Only in strict mode, so the test is something like:

String.prototype.f = function () { "use strict"; return typeof this; }
"hi".f() // "string"

/be

Comment 7

[Luke Wagner [:luke]]

Needed for bug 460904.

Comment 8

[Luke Wagner [:luke]]

I'm going to summarize the situation as I see it, and please let me know if I'm missing something:

We eagerly box non-primitive jsvals in JSOP_CALL{PROP|NAME|...}, and lazily do the rest of this-computation (scope-chain walking + security checks) using the JSFRAME_COMPUTED_THIS flag + JSContext::thisp.

Now, bug 515496 is going to make this-computation (other than primitive-boxing) BLAZINGLY FAST such that we don't need lazy this-computation to avoid that cost.  Next, w.r.t primitive boxing, as far as I understand, this can only pop up when one does:

  {String, Number, Boolean}.prototype.foo = userFunction;
  "abcd".foo();
  (4).foo();
  true.foo();

or

  foo.apply(1)

I'm guessing this isn't common and that the major reason for the whole lazy this-computation was avoiding the security checks that Blake is about to remove.  Is that correct?

Assuming "yes", once bug 515496 lands, it seems that, contrary to the goal of this bug, we would actually want to make *all* this-computation eager so that we could remove JSFRAME_COMPUTED_THIS and all the associated lazy computation logic in the interpreter and tracer.  I think JSStackFrame::thisp could go too.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> We eagerly box non-primitive jsvals in JSOP_CALL{PROP|NAME|...},

"box" is overloaded (so is "wrap"), and if the object is non-primitive I'm confused (if primitive then I get what you mean).

Let's say instead that apart from native methods whose flags say they are prepared for a primitive |this|, we convert nominal |this| parameter values to object type eagerly in the JOF_CALLOP opcodes.

But we avoid further security checks required when accessing |this| in the function body, to avoid paying up front for what may not happen ever (no |this| usage in the body) or on a hot control flow path.

> and lazily do
> the rest of this-computation (scope-chain walking + security checks) using the
> JSFRAME_COMPUTED_THIS flag + JSContext::thisp.

Right.

> I'm guessing this isn't common and that the major reason for the whole lazy
> this-computation was avoiding the security checks that Blake is about to
> remove.  Is that correct?

Yes, the security check is costly for functions, especially for those that never use |this| at all or on the hot control flow path, wherefore the lazy thisp computation.

The security checks are separate from the issue of primitive |this| conversion. The latter is expensive in its own right and worth avoiding at least for the built-in prototype methods. This bug asked for it to be avoided for all methods whether native or user-defined, but doing so for user-defined functions is incompatible with ES1-3. It is required by ES5 strict mode, an opt-in mode, of course.

> Assuming "yes", once bug 515496 lands, it seems that, contrary to the goal of
> this bug, we would actually want to make *all* this-computation eager so that
> we could remove JSFRAME_COMPUTED_THIS and all the associated lazy computation
> logic in the interpreter and tracer.  I think JSStackFrame::thisp could go too.

We must not convert |this| to object for the native functions currently specified to accept primitive-this-type. We should not make ES5 strict mode harder to implement than it already is in patching this bug. So were you proposing to convert to object any primitive |this| in the JSOP_CALL{PROP,...} opcode bodies?

If mrbkap does his thing over in bug 515496 then you can remove (or mrbkap should already have removed) JSFRAME_COMPUTED_THIS etc. But this is separate from the question of callee/this-computing ops converting primitive |this| to object type.

/be

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Let's say instead that apart from native methods whose flags say they are
> prepared for a primitive |this|, we convert nominal |this| parameter values to
> object type eagerly in the JOF_CALLOP opcodes.

The "apart from native methods" bit applies only to JSOP_CALLPROP of course. We don't bother optimizing

  var x = 'slice';
  return "abcd"[x](1,4);

There is no JSOP_NAME variant JOF_CALLOP-flagged opcode, since the only way to call slice without s. or "abcd". on its left is via a with statement, which will convert the with-head expression value to object type if primitive.

/be

Comment 11

[Luke Wagner [:luke]]

> (In reply to comment #8)
> > We eagerly box non-primitive jsvals in JSOP_CALL{PROP|NAME|...},
> 
> "box" is overloaded (so is "wrap"), and if the object is non-primitive I'm
> confused (if primitive then I get what you mean).

You're right, I meant primitive.

> We must not convert |this| to object for the native functions currently
> specified to accept primitive-this-type.

Oops, I forgot to say that, but, yes, the JSFUN_THISP_FLAGS-related logic should definitely stay.

> We should not make ES5 strict mode
> harder to implement than it already is in patching this bug. So were you
> proposing to convert to object any primitive |this| in the JSOP_CALL{PROP,...}
> opcode bodies?

I guess all I'm really saying is that I don't see this bug (allow primitive 'this' to interpreted functions) as a big win (due to its rare occurrence), especially when JS_COMPUTED_THIS and 'thisp' seem like they can be otherwise cut out with bug 515496.

Comment 12

[Luke Wagner [:luke]]

(In reply to comment #11)
Well, at the very least, as we discussed, thisp -> thisv (without changing when boxing happens in JSOP_CALLX) is both more honest about thisp contents (in lieu of the jsval-to-JSObject* casts) and will allow bug 460904 to slide a primitive 'this' by the JS_ASSERT(JSVAL_IS_OBJECT(vp[1])) in JSOP_CALL.

Comment 13

[Brendan Eich [:brendan]]

And to capture the crucial reason for JSStackFrame.this[pv]: we need a GC root that natives can't override to protect their |obj| parameter against premature GC if they trigger a GC after overwriting their own argv[-1].

/be

Comment 14

[Brendan Eich [:brendan]]

Also, global frames need this[pv] because they have null argv. We could split frame subtypes out via some C++ thin struct inheritance but that would split code too (no virtuals, so if statements -> runtime costs).

An attempt to find uses:

$ egrep '(fp->|frame\.)thisp' *.cpp
jsdbgapi.cpp:        return fp->thisp;
jsdbgapi.cpp:    if (!fp->thisp && fp->argv)
jsdbgapi.cpp:        fp->thisp = js_ComputeThis(cx, JS_TRUE, fp->argv);
jsdbgapi.cpp:    return fp->thisp;
jsgc.cpp:    JS_ASSERT(JSVAL_IS_OBJECT((jsval)fp->thisp) ||
jsgc.cpp:    JS_CALL_VALUE_TRACER(trc, (jsval)fp->thisp, "this");
jsinterp.cpp:    frame.thisp = (JSObject *)vp[1];
jsinterp.cpp:        ok = native(cx, frame.thisp, argc, frame.argv, &frame.rval);
jsinterp.cpp:        frame.thisp = down->thisp;
jsinterp.cpp:        frame.thisp = chain;
jsinterp.cpp:        frame.thisp = frame.thisp->thisObject(cx);
jsinterp.cpp:        if (!frame.thisp) {
jsiter.cpp:    gen->frame.thisp = fp->thisp;
jsobj.cpp:        MaybeDumpObject("this", fp->thisp);
jsops.cpp:                fp->rval = OBJECT_TO_JSVAL(fp->thisp);
jsops.cpp:                        fp->rval = OBJECT_TO_JSVAL(fp->thisp);
jsops.cpp:                    newifp->frame.thisp = (JSObject *)vp[1];
jstracer.cpp:                fp->thisp = JSVAL_TO_OBJECT(fp->argv[-1]);
jstracer.cpp:    newifp->frame.thisp = NULL; // will be updated in FlushNativeStackFrame
jstracer.cpp:    // fp->thisp is really a jsval, so reinterpret_cast here, not JSVAL_TO_OBJECT.
jstracer.cpp:    fp->thisp = (JSObject *) state.nativeVp[1];
jstracer.cpp:        JS_ASSERT(OBJECT_TO_JSVAL(fp->thisp) == fp->argv[-1]);
jstracer.cpp:        JS_ASSERT(OBJECT_TO_JSVAL(fp->thisp) == fp->argv[-1]);
jstracer.cpp:     * It's safe to just use cx->fp->thisp here because getThis() returns
jstracer.cpp:    CHECK_STATUS(getProp(cx->fp->thisp, this_ins));

Not a lot going on here. As Luke noted when we talked, interpreted uses must COMPUTE_THIS due to laziness. It's the natives that need an inviolable GC root in addition to argv[-1]. But global code needs that root too, and lacks argv.

/be

Comment 15

[Luke Wagner [:luke]]

Attachment: v.1

This patch changes |JSObject *thisp| to |jsval thisv|.  The changes are mostly simple.  An important part of the patch is the removal of the assertion in JSOP_CALL that only allows |this| to be an object; while JSOP_CALLX will still cause this to be true for normal interpreted calls, bug 460904 may allow a primitive |this| to sneak into self-hosted calls.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 399906 [details] [diff] [review]
v.1

>@@ -1121,40 +1121,40 @@ JS_GetFrameCallObject(JSContext *cx, JSS
...
>-    if (!fp->thisp && fp->argv)
>-        fp->thisp = js_ComputeThis(cx, JS_TRUE, fp->argv);
>+    if (JSVAL_IS_NULL(fp->thisv) && fp->argv)
>+        fp->thisv = OBJECT_TO_JSVAL(js_ComputeThis(cx, JS_TRUE, fp->argv));

Pre-existing bug, but could you null-check js_ComputeThis's return value in a temporary and propagate error if null? Thanks.

/be

Comment 17

[Luke Wagner [:luke]]

(In reply to comment #16)
> (From update of attachment 399906 [details] [diff] [review])
> >@@ -1121,40 +1121,40 @@ JS_GetFrameCallObject(JSContext *cx, JSS
> ...
> >-    if (!fp->thisp && fp->argv)
> >-        fp->thisp = js_ComputeThis(cx, JS_TRUE, fp->argv);
> >+    if (JSVAL_IS_NULL(fp->thisv) && fp->argv)
> >+        fp->thisv = OBJECT_TO_JSVAL(js_ComputeThis(cx, JS_TRUE, fp->argv));
> 
> Pre-existing bug, but could you null-check js_ComputeThis's return value in a
> temporary and propagate error if null? Thanks.

Now that I look at this in the context of the whole function, it seems that a null value is returned if js_ComputeThis returns null and returning early would leak a dormant frame chain entry.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 399906 [details] [diff] [review] [details])
> > >@@ -1121,40 +1121,40 @@ JS_GetFrameCallObject(JSContext *cx, JSS
> > ...
> > >-    if (!fp->thisp && fp->argv)
> > >-        fp->thisp = js_ComputeThis(cx, JS_TRUE, fp->argv);
> > >+    if (JSVAL_IS_NULL(fp->thisv) && fp->argv)
> > >+        fp->thisv = OBJECT_TO_JSVAL(js_ComputeThis(cx, JS_TRUE, fp->argv));
> > 
> > Pre-existing bug, but could you null-check js_ComputeThis's return value in a
> > temporary and propagate error if null? Thanks.
> 
> Now that I look at this in the context of the whole function, it seems that a
> null value is returned if js_ComputeThis returns null and returning early would
> leak a dormant frame chain entry.

Ok, so don't leak, but do propagate an error return rather than hiding the pending exception or already-reported OOM.

/be

Comment 19

[Luke Wagner [:luke]]

(In reply to comment #18)
> Ok, so don't leak, but do propagate an error return rather than hiding the
> pending exception or already-reported OOM.

I guess my pointing out the leak was irrelevant; what I meant to say was that a null return value was already being propagated.

Comment 20

[Brendan Eich [:brendan]]

Indeed -- to quote from SNL first season, "never mind!" ;-)

/be

Comment 21

[Luke Wagner [:luke]]

What's all I hear about this leeks in Firefox?  Why I thought vegetables were an important part of a well-balanced diet!

http://hg.mozilla.org/tracemonkey/rev/7eff6f4aee73

Comment 22

[Jeff Walden [:Waldo]]

No leeks in SpiderMonkey, but we do have lots of "unions".

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/7eff6f4aee73