Closed Bug 514570 (strictThis) Opened 15 years ago Closed 14 years ago

ES5 strict mode: this not generally coerced to an object

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: jimb, Assigned: jimb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(7 files, 15 obsolete files)

5.08 KB, patch
brendan
: review+
jorendorff
: review+
Details | Diff | Splinter Review
21.15 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
25.59 KB, patch
Details | Diff | Splinter Review
3.42 KB, patch
brendan
: review+
Details | Diff | Splinter Review
2.65 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
708 bytes, patch
dvander
: review+
Details | Diff | Splinter Review
63.74 KB, patch
Details | Diff | Splinter Review
From ES5 Annex C:

If this is evaluated within strict mode code, then the this value is not coerced to an object. A this value of null or undefined is not converted to the global object and primitive values are not converted to wrapper objects. The this value passed via a function call (including calls made using Function.prototype.apply and Function.prototype.call) do not coerce the passed this value to an object (10.4.3, 11.1.1, 15.3.4.3, 15.3.4.4).

It seems like bug 412571 covers part of this, but not every case.
Not boxing this is a semi-invasive change since it touches both interpreter and tracer, and of the remaining ES5 work it's probably the most fragile change -- very much needed sooner rather than later for extra time to hammer on it.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
blocking2.0: --- → ?
OS: Linux → All
Priority: -- → P1
Hardware: x86 → All
Depends on: blazinglyfastthis
(Also, to clarify comment 0, ES5's semantics say *the function* is what's responsible for potentially boxing.  The strictness of the calling code is not relevant to whether |this| is, or is not, boxed when a function is called with a primitive |this|.)
I think I'm going to make fairly significant changes to the meaning of JSStackFrame::thisv in bug 556277.

  Before: if fp->flags & JSFRAME_COMPUTED_THIS,
          then fp->thisv is boxed ES3 'this'
          else fp->thisv is a primitive this, or some kind of scope object
               which js_ComputeThis can use to calculate the ES3 'this'

  After: if fp->thisv is a primitive,
         then it's a primitive this, or NULL to indicate that 'this' is
              the global;
         else fp->thisv is the boxed ES3 'this'

It shouldn't pose any extra difficulties here, but it would be a waste to do a bunch of hacking on this bug and then try to rebase across my change.

I'll try to get it done today.
blocking2.0: ? → beta1+
blocking2.0: beta1+ → beta2+
So, about the effect that this bug has on the eagerly-computed-|this| patch in 556277 and its JaegerMonkey friend... I think jorendorff said he had given this some thought already, but I want to make sure we both came up with the same result.

Since we defer computation of the global object, and given that PrimitiveToObject is a fully reversible operation, we should be able to laboriously de-objectify primitives in strict mode functions, keeping the non-strict path fast/eager. We'll have to use JSVAL_MAGIC to do JSVAL_NULL's current job of flagging global value resolution, since JSVAL_NULL is a valid thisValue as of ES5's |Function.prototype.apply(null, ...)| change referenced above. (15.3.4.3)

ES3 given |this| as:
- undefined: resolve to global
- null: resolve to global
- magic: resolve to global
- other: guaranteed to be an object by the eager-|this| invariant, provide as-is as |this|

ES5 given |this| as:
- undefined: provide as-is as |this|
- null: provide as-is as |this|
- magic: resolve to global
- other: guaranteed to be an object by the eager-|this| invariant, try to unbox if it's a primitive class, otherwise provide as-is as |this|

Our if-null-then-compute-global code in JaegerMonkey will simply become if-not-object-then-compute-global code for methods compiled as ES3.
I'm a bit out of context here but worried about

> - other: guaranteed to be an object by the eager-|this| invariant, try to unbox
if it's a primitive class, otherwise provide as-is as |this|

Does this amount to non-coercion for strict code in both the primitive and wrapper cases? IOW, a primitive value must be provided as is, and a wrapper must be provided as is.

Also, what's "magic"?

Why will there still be methods compiled as ES3? Will these share a heap with ES5 code?
Firstly, very sloppy terminology on my part, especially for an ES5 bug. :-)

I meant to distinguish strict mode versus non-strict-mode methods, not ES3 and ES5 code.

(In reply to comment #6)
> Does this amount to non-coercion for strict code in both the primitive and
> wrapper cases? IOW, a primitive value must be provided as is, and a wrapper
> must be provided as is.

Yeah, so I was thinking we'd distinguish between the given-as-wrapper and given-as-primitive-then-wrapped cases so you could unwrap in strict mode code if necessary. These acrobatics might help perf in JaegerMonkey non-strict code, but it's ugly, so I'm talking over some alternative ideas with jorendorff today.

> Also, what's "magic"?

A special jsval in the interpreter that indicates some special action is necessary, used to be JSVAL_HOLE.
Also, I cited an irrelevant part of the document, should have cited 10.4.3 (Entering Function Code).
blocking2.0: beta2+ → betaN+
blocking2.0: betaN+ → beta5+
Can we get an update here?
I haven't started on this; I'm planning to start after bug 514568 has a patch up for review.
I think this needs a beta, but doesn't need to be b5.
blocking2.0: beta5+ → beta6+
I have part of a patch for this -- cross your fingers for me to finish it today!  :-)
Blocks: 575535
Better one flag that both natives and strict-mode functions can both use than a multiplicity of them, I say.
Attachment #473939 - Flags: review?(brendan)
The spec says to push |undefined|.  This moves us a step closer to actually meaningfully doing that for strict mode stuff, and it's separable, so why not keep it separate?
Attachment #473941 - Flags: review?(brendan)
This also fixes bug 575535.

One minor interesting note is the under-specifiedness of String.prototype.replace with a callback, and what |this| is used for that callback.  Security concerns of the secure-JS sort suggest we really do want to pass |undefined| there and let the callee box if non-strict, even if the spec is...less than forthcoming about what should be used.
Attachment #473942 - Flags: review?(brendan)
This is really bug 575522, but it seems better to keep the queue of changes all here, for ease of application/pushing/etc.

JSFUN_PRIMITIVE_THIS isn't actually needed here, because .call/.apply don't futz with |this| in any way whatsoever, but it seems better to be explicit about this, at least to me, even if it doesn't matter.
Attachment #473944 - Flags: review?(brendan)
And now for patch 5 - Tests...except not really.  :-(

I ran out of time to write tests, beyond a really simple one or two, so you don't get any for the bulk of the strict-this work.  I can say that when the ES5 test suite lands (bug 496923) we pick up 19 more passes (give or take, depending on what in-progress patchwork is applied, and so on) with this patch, at least.  But really, we need some for tracing (most importantly, I think), for deliberate exercising of method-JIT changes for this, and just for general-purpose correctness.  I will beg forgiveness and draw upon my surplusage of tests in other bugs to escape responsibility for these particular tests -- I will be much obliged to whoever writes and lands them.

I should also note that at least #3 is going to be broken semi-soon when JSStackFrame::thisv goes away, so you want to coordinate that with Luke in some manner.  :-)
Oh, #3 introduces a warning in my gcc for |-1 * sizeof(Value)| -- too little time to spend any determining the kosher way to do that, someone else's help on that much appreciated.  :-)
And another note: strict mode introduces some optimization opportunities in much of how |this| works -- could introduce a new opcode that just retrieves argv[-1] directly, not even do any allows-primitive-this checks, etc.  I did none of it: get it right is what matters, make it fast can follow as time permits.  :-)
jimb will take this over the finish line.
Assignee: jwalden+bmo → jim
Whiteboard: [ETA: 9/22]
Refreshed for current tip.
Attachment #473939 - Attachment is obsolete: true
Attachment #477299 - Flags: review?(brendan)
Attachment #473939 - Flags: review?(brendan)
Rebased to tip.
Attachment #473941 - Attachment is obsolete: true
Attachment #477302 - Flags: review?(brendan)
Attachment #473941 - Flags: review?(brendan)
Missed a rejected patch hunk.
Attachment #477302 - Attachment is obsolete: true
Attachment #477306 - Flags: review?(brendan)
Attachment #477302 - Flags: review?(brendan)
Patch 3, the pièce de résistance, is now a piece of resistance: it conflicts pretty badly with the fix for bug 539144 (Make formal args a jit-time const offset from fp; rm argv/argc/thisv/script/callobj).

---Signed, the committee for the preservation of accents in patch names
Okay; I've spent today working through patch 3, but it's not done yet. ETA end of day tomorrow.

Both Luke's patch and Waldo's patch restructure the way the 'this' computation works, in different ways. I don't yet see the pattern well enough to reconcile them.
Perspective: out of a 22k patch with 40 hunks, 28 hunks fail to apply. Today I learned how stack frames, arguments, and locals interact; tomorrow morning I will take the time get a solid understanding of |this| handling in the current code, the common ancestor code, and Waldo's code.
Okay, this one is ready for review.
Attachment #477299 - Attachment is obsolete: true
Attachment #478007 - Flags: review?(jorendorff)
Attachment #477299 - Flags: review?(brendan)
Attachment #477306 - Flags: review?(brendan) → review?(jorendorff)
Comment on attachment 478007 [details] [diff] [review]
1 - Convert primitive-this-of-type function flags into a single primitive-this flag, to pave way for strict mode unadulterated-this passing.

I just noticed that proxies are not transparent enough for a wrapper to
act like a String object when passed to a String method. That's not
something worth worrying about in this bug, though, if ever.

In jsinterp.h, ReportIncompatibleMethod:
>+        JS_ASSERT(thisv.isUndefined() || thisv.isNull());

I don't think thisv can be undefined here.

In jsinterp.h, comment on declaration of js::GetPrimitiveThis:
>+ * and extract its private slot value to return via *vpp.  If |this| is a
>+ * primitive not of the desired type, report an error as though a |this| value
>+ * had been computed from it.

Everything after "report an error" seems like it's trying to be precise,
but it's not succeeding. Please either clarify it or drop it.

>@@ -832,10 +838,7 @@ ComputeThisFromVpInPlace(JSContext *cx, 
> JS_ALWAYS_INLINE bool
> PrimitiveThisTest(JSFunction *fun, const Value &v)
> {
>-    uint16 flags = fun->flags;
>-    return (v.isString() && !!(flags & JSFUN_THISP_STRING)) ||
>-           (v.isNumber() && !!(flags & JSFUN_THISP_NUMBER)) ||
>-           (v.isBoolean() && !!(flags & JSFUN_THISP_BOOLEAN));
>+    return v.isNull() || (!v.isObject() && fun->acceptsPrimitiveThis());
> }

I would have expected:

    JS_ASSERT(!v.isUndefined());
    return !v.isObjectOrNull() && fun->acceptsPrimitiveThis();

The bit about v.isNull() is a change in behavior for this function, right?

Looks good otherwise. r=me with those comments addressed.
Attachment #478007 - Flags: review?(jorendorff) → review+
It looks to me like traced code, pre-patch, assumes that primitive "this" values have already been wrapped before entering trace, as JSTracer::getThis either simply fetches the existing thisValue, or pushes the global object (i.e. it only handles 10.4.3 steps 2 and 4.), and tracing calls to methods on unwrapped primitives leaves trace.

(As an experiment, I tried placing an 'abort' on the '!isObject' case of ComputeThisFromArgv, and neither js/src/tests nor js/src/trace-tests ever hit it.)

But Waldo's patch 3 (the major one) changes the rules so that 'this' is passed unwrapped to functions as a matter of course. Looking at the changes he made to JSTracer::getThis, it seems that that function still does not call anything that would ensure that 'this' gets wrapped on trace. It wraps while recording, and then generates trace code that fetches 'this' from the stack frame, which will still be the unwrapped primitive when we run the trace.
(In reply to comment #29)
> and tracing calls to methods on
> unwrapped primitives leaves trace.

I should say, "recording a call to a method on an unwrapped primitive aborts recording."
Status: most of the patch is looking pretty good at this point, but I need to understand better how the tracer is supposed to work. Tomorrow morning I'll post a version of the patch sans tracer, just for discussion, and then try to understand the issues there.
I am out of time for today. Mostly staring at the code instead of reviewing it. I'll try again tomorrow.
With patches 1 and 2 applied, this code:

  "x".replace("x", Math.sin);

causes this:

Assertion failure: args.thisv().isObject() || args.thisv().isUndefined() || PrimitiveThisTest(fun, args.thisv()), at ../jsinterp.cpp:551

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x001727b5 in JS_Assert (s=0x297e68 "args.thisv().isObject() || args.thisv().isUndefined() || PrimitiveThisTest(fun, args.thisv())", file=0x297880 "../jsinterp.cpp", ln=551) at ../jsutil.cpp:80
80	    *((int *) NULL) = 0;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0  0x001727b5 in JS_Assert (s=0x297e68 "args.thisv().isObject() || args.thisv().isUndefined() || PrimitiveThisTest(fun, args.thisv())", file=0x297880 "../jsinterp.cpp", ln=551) at ../jsutil.cpp:80
#1  0x000b9155 in js::Invoke (cx=0x609f70, argsRef=@0xbfffe4e4, flags=0) at jsinterp.cpp:551
#2  0x00152014 in str_replace_flat_lambda (cx=0x609f70, argc=2, vp=0x10000c0, rdata=@0xbfffe4a0, fm=@0xbfffe4b0) at ../jsstr.cpp:2421
#3  0x00152cea in js::str_replace (cx=0x609f70, argc=2, vp=0x10000c0) at ../jsstr.cpp:2500
#4  0x000a6326 in js::Interpret (cx=0x609f70, entryFrame=0x1000088, inlineCallCount=0, interpFlags=0) at ../jsinterp.cpp:4503
#5  0x000b860f in js::RunScript (cx=0x609f70, script=0x610fd0, fun=0x0, scopeChain=@0x1402000) at jsinterp.cpp:510
#6  0x000b8bbb in js::Execute (cx=0x609f70, chain=0x1402000, script=0x610fd0, prev=0x0, flags=0, result=0xbffff650) at jsinterp.cpp:775
#7  0x00012943 in JS_EvaluateUCScriptForPrincipals (cx=0x609f70, obj=0x1402000, principals=0x0, chars=0x610e10, length=27, filename=0x288b02 "-e", lineno=1, rval=0xbffff650) at ../jsapi.cpp:4781
#8  0x000129eb in JS_EvaluateScriptForPrincipals (cx=0x609f70, obj=0x1402000, principals=0x0, bytes=0xbffff86f "\"x\".replace(\"x\", Math.sin);", nbytes=27, filename=0x288b02 "-e", lineno=1, rval=0xbffff650) at ../jsapi.cpp:4805
#9  0x00012a4f in JS_EvaluateScript (cx=0x609f70, obj=0x1402000, bytes=0xbffff86f "\"x\".replace(\"x\", Math.sin);", nbytes=27, filename=0x288b02 "-e", lineno=1, rval=0xbffff650) at ../jsapi.cpp:4814
#10 0x0000b20d in ProcessArgs (cx=0x609f70, obj=0x1402000, argv=0xbffff798, argc=2) at ../../shell/js.cpp:816
#11 0x0000b41b in shell (cx=0x609f70, argc=2, argv=0xbffff798, envp=0xbffff7a4) at ../../shell/js.cpp:5251
#12 0x0000b540 in main (argc=2, argv=0xbffff798, envp=0xbffff7a4) at ../../shell/js.cpp:5347
Patch #3 deletes that assertion.
Additional comment on patch #1: js_GetPrimitiveThis is still mentioned in a comment in jsstr.cpp.

But the comment is misleading anyway. It should say something like:

/*
 * String.prototype.{toString,toSource,valueOf} throw a TypeError if the
 * this-argument is not a string or a String object. So those methods use
 * js::GetPrimitiveThis which provides that behavior.
 *
 * By standard, the rest of the String methods must ToString the
 * this-argument rather than throw a TypeError. So those methods use
 * NORMALIZE_THIS instead.
 */
>@@ -4956,7 +4954,7 @@ BEGIN_CASE(JSOP_CALLARG)
>     METER_SLOT_OP(op, slot);
>     PUSH_COPY(argv[slot]);
>     if (op == JSOP_CALLARG)
>-        PUSH_NULL();
>+        PUSH_UNDEFINED();
> }
> END_CASE(JSOP_GETARG)

>@@ -15793,7 +15787,7 @@ TraceRecorder::record_JSOP_CALLARG()
> {
>     uintN slot = GET_ARGNO(cx->regs->pc);
>     stack(0, arg(slot));
>-    stack(1, INS_NULL());
>+    stack(1, INS_UNDEFINED());
>     return ARECORD_CONTINUE;
> }

>@@ -1183,7 +1183,7 @@ mjit::Compiler::generateMethod()
>           {
>             jsop_getarg(GET_SLOTNO(PC));
>             if (op == JSOP_CALLARG)
>-                frame.push(NullValue());
>+                frame.push(UndefinedValue());
>           }
>           END_CASE(JSOP_GETARG)

*sigh*
In methodjit/PunboxAssembler.h, class Assembler:
>+    Jump testUndefined(Assembler::Condition cond, RegisterID reg) {
>+        return branchPtr(cond, reg, ImmShiftedTag(JSVAL_SHIFTED_TAG_UNDEFINED));
>+    }
>+
>+    Jump testUndefined(Assembler::Condition cond, Address address) {
>+        loadValue(address, Registers::ValueReg);
>+        convertValueToType(Registers::ValueReg);
>+        return branchPtr(cond, Registers::ValueReg, ImmShiftedTag(JSVAL_SHIFTED_TAG_UNDEFINED));
>+    }

The convertValueToType call here looks unnecessary to me. It's just a
copy of what's in testNull though. We emit
  (a & TypeMask) == JSVAL_SHIFTED_TAG_UNDEFINED
instead of just
  a == JSVAL_VOID

Same thing is already in testNull. Is that really necessary?
Comment on attachment 477306 [details] [diff] [review]
2 - Push |undefined| rather than |null| when calling functions without a specified |this| value, per ES5.

In jsemit.cpp, js_EmitTree:
>          * Then (or in a call case that has no explicit reference-base object)
>-         * we emit JSOP_NULL as a placeholder local GC root to hold the |this|
>-         * parameter: in the operator new case, the newborn instance; in the
>-         * base-less call case, a cookie meaning "use the global object as the
>-         * |this| value" (or in ES5 strict mode, "use undefined", so we should
>-         * use JSOP_PUSH instead of JSOP_NULL -- see bug 514570).
>+         * we emit JSOP_PUSH as a placeholder local GC root to hold the |this|
>+         * parameter: in the operator new case, the newborn instance, and
>+         * otherwise |undefined| (which non-strict mode functions will box into
>+         * the global object).

The bit about a "placeholder local GC root" is obsolete. The real reason
we need a slot there is that the JS stack layout (the calling
convention, basically) requires a slot for thisv, even if you're
constructing.

In jsemit.cpp, js_EmitTree:
>                callop = true;          /* suppress JSOP_NULL after */

This comment should also have been changed to JSOP_PUSH.

In jsinterp.cpp, SLOW_PUSH_THISV:
>         if (!thisp->getParent() ||                                       \
>             (clasp = thisp->getClass()) == &js_CallClass ||              \
>             clasp == &js_BlockClass ||                                   \
>             clasp == &js_DeclEnvClass) {                                 \
>-            /* Normal case: thisp is global or an activation record. */  \
>-            /* Callee determines |this|. */                              \
>-            thisp = NULL;                                                \
>+            PUSH_UNDEFINED();                                            \
>         } else {                                                         \
>             thisp = thisp->thisObject(cx);                               \
>             if (!thisp)                                                  \
>                 goto error;                                              \
>+            PUSH_OBJECT(*thisp);                                         \
>         }                                                                \
>-        PUSH_OBJECT_OR_NULL(thisp);                                      \

At issue here is removing the comment.

This code is weird enough to warrant a comment IMHO, perhaps "Push the
ImplicitThisValue for the Environment Record associated with obj. See
ES5 sections 10.2.1.1.6 and 10.2.1.2.6 (ImplicitThisValue) and section
11.2.3 (Function Calls)."

In jsinterp.cpp, js::Interpret, CASE(JSOP_CALLUPVAR_DBG):
>     /* Minimize footprint with generic code instead of NATIVE_GET. */
>     obj2->dropProperty(cx, prop);
>     Value *vp = regs.sp;
>-    PUSH_NULL();
>+    PUSH_UNDEFINED();
>     if (!obj->getProperty(cx, id, vp))
>         goto error;

FWIW, I don't think it's necessary to change this one. It doesn't matter though.

In jsstr.cpp, FindReplaceLength:
>-        args.thisv().setNull();
>+        args.thisv().setUndefined();

str_replace_flat_lambda needs the same change.

In methodjit/FrameState.h, class FrameState:
>+     * Helper function. Tests if a slot's type is undefined. Condition should
>+     * be Equal or NotEqual.

Maybe this is a dumb thing to even bring up, but it seems like since the
implementation asserts, we should say "must", not "should". Same thing
for the other testFoo comments.

r=me with the one bug fixed; fix the comments if you want to.
Attachment #477306 - Flags: review?(jorendorff) → review+
Untested; posting for folks to look at.  Smaller than the original, because I took out a renaming.
Attachment #473942 - Attachment is obsolete: true
Attachment #473942 - Flags: review?(brendan)
Comment on attachment 478595 [details] [diff] [review]
3 - Don't box |this| for strict mode functions. (rebased to tip)

This has a minor flaw (with serious consequences) fixed.  Many fewer test suite failures.
Attachment #478595 - Attachment description: 3 - Don't box |this| for strict mode functions. → 3 - Don't box |this| for strict mode functions. (rebased to tip)
Attachment #478465 - Attachment is obsolete: true
This patch allows |this|-wrapping to occur on-demand in some cases; previously, all wrapping occurred before entering the function. However, the recent changes to the stack frame layout mean that frames for eval called within a function have a *copy* of the function's |this| value, rather than sharing it (perhaps it was thus before; I don't know). This means that lazily wrapping a primitive |this| from an eval'd script will fail to update the function's |this|.  For example:

function f(s) { eval(s); return this; };
print(uneval(f.call(true, "this.x = 10").x))

should print '10', but prints '(void 0)' under the current regime (with an assertion removed that detected this case).
Okay, that's done. Doing one more test run, hopefully #3 is ready for review now.
No js test suite regressions.
Attachment #478595 - Attachment is obsolete: true
Attachment #478820 - Flags: review?(jorendorff)
This does cause a trace-test regression: sunspider/check-3d-cube.js hits an assertion as the tracer tries to restore the VM stack from a side exit. It runs into a slot the type map says is a non-function object, but whose actual value in the tracing stack is zero. I believe this is the 'Init' function's scope chain. I'll continue to debug tomorrow.
The slot in question is the |this| value passed to CreateP on line 312. It's passed as UNDEFINED, and it seems that it's not being converted to an object on trace.
Whiteboard: [ETA: 9/22] → [ETA: needed][needs-review-brendan]
This one clears up the check-3d-cube.js trace-tests failure.
Attachment #478820 - Attachment is obsolete: true
Attachment #479163 - Flags: review?(jorendorff)
Attachment #478820 - Flags: review?(jorendorff)
Attachment #477306 - Attachment is obsolete: true
Whiteboard: [ETA: needed][needs-review-brendan] → [ETA: 9/28][needs-review-brendan]
Bah, just need to fix some uses of JS_GetFrameThis in jsd and xpconnect. But need to knock off for the day. Should take a few minutes, max.
Attachment #479480 - Flags: review?(brendan)
It used to be:
JSObject *JS_GetFrameThis(JSContext *, JSStackFrame *);

Now it is:
JSBool JS_GetFrameThis(JSContext *, JSStackFrame *, jsval *);

(In strict mode code, |this| values that are primitives don't get wrapped.)
Attachment #479482 - Flags: review?(jorendorff)
Comment on attachment 479480 [details] [diff] [review]
Adapt jsd to new JS_GetFrameThis arguments.

It used to be:
JSObject *JS_GetFrameThis(JSContext *, JSStackFrame *);

Now it is:
JSBool JS_GetFrameThis(JSContext *, JSStackFrame *, jsval *);

(In strict mode code, |this| values that are primitives don't get wrapped.)
(In reply to comment #37)
> The convertValueToType call here looks unnecessary to me. It's just a
> copy of what's in testNull though. We emit
>   (a & TypeMask) == JSVAL_SHIFTED_TAG_UNDEFINED
> instead of just
>   a == JSVAL_VOID
> 
> Same thing is already in testNull. Is that really necessary?

It's not, in PunboxAssembler.  I'll attach a patch to fix this and testNull.
Attachment #479551 - Flags: review? → review?(dvander)
Blocks: 600693
The testNull code in the patches was a braindead copy of the testUndefined code, so if the testNull stuff is sub-optimal it seems likely the extant testUndefined code is as well.
(In reply to comment #55)
> The testNull code in the patches was a braindead copy of the testUndefined
> code, so if the testNull stuff is sub-optimal it seems likely the extant
> testUndefined code is as well.

Indeed --- that was the motivation.  The new code you added is fixed similarly.
Comment on attachment 479551 [details] [diff] [review]
[committed] Simplify code generated for tests against 'null', in cases where we might as well test the whole value.

Nice catch!
Attachment #479551 - Flags: review?(dvander) → review+
I've run into a few mochitest failures; three down, two to go.
Okay, here's the pièce de résistance, all sorted out, with all tests passing under all jits, mochitest stuff cleared up, and a bright ray of sunshine resting on the upper right corner.

It is my fondest hope (and Jeff's) that this humble offering will be worthy of your attention and find favor in your esteemed judgement.
Attachment #479163 - Attachment is obsolete: true
Attachment #479703 - Flags: review?(jorendorff)
Attachment #479163 - Flags: review?(jorendorff)
Attachment #479551 - Attachment description: Simplify code generated for tests against 'null', in cases where we might as well test the whole value. → [committed] Simplify code generated for tests against 'null', in cases where we might as well test the whole value.
Comment on attachment 479480 [details] [diff] [review]
Adapt jsd to new JS_GetFrameThis arguments.

>@@ -342,11 +343,13 @@ jsd_GetThisForStackFrame(JSDContext* jsd
> 
>     if( jsd_IsValidFrameInThreadState(jsdc, jsdthreadstate, jsdframe) )
>     {
>+        JSBool success;
>+        jsval thisval;
>         JS_BeginRequest(jsdthreadstate->context);
>-        obj = JS_GetFrameThis(jsdthreadstate->context, jsdframe->fp);
>+        success = JS_GetFrameThis(jsdthreadstate->context, jsdframe->fp, &thisval);
>         JS_EndRequest(jsdthreadstate->context);
>-        if(obj)
>-            jsdval = JSD_NewValue(jsdc, OBJECT_TO_JSVAL(obj));
>+        if(success)
>+            jsdval = JSD_NewValue(jsdc, thisval);

Nit: use ok as canonical name, not success, if there's a mix of styles (I see both success and ok used, only sparsely, though).

>-    else
>-    {
>-        buf = JS_smprintf("%sleaving %s\n",
>-                _indentSpaces(--indent),
>-                funName);
>+    else {
>+        printf("%sleaving %s\n", _indentSpaces(--indent), funName);
>     }

Stick with prevailing brace style when in Rome (or dark Volsce infected forests to the north ;-).

/be
Attachment #479480 - Flags: review?(brendan) → review+
Depends on: 600943
Seems like nsHTMLPluginObjElementSH::Call isn't ready to cope with primitive |this| values. It assumes that argv[-1] is an object. Mochitest failure.
Basically, various things in the browser aren't expecting to get non-Object |this| values. Working through the list...
Comment on attachment 479482 [details] [diff] [review]
Adapt XPConnect to new JS_GetFrameThis arguments.

>+        if (!gotThisVal ||
>+            !showThisProps ||
>+            !JSVAL_IS_OBJECT(thisVal) ||
>+            !JS_GetPropertyDescArray(cx, JSVAL_TO_OBJECT(thisVal),
>+                                     &thisProps))

Instead of !JSVAL_IS_OBJECT(thisVal), use JSVAL_IS_PRIMITIVE(thisVal), because

    JSVAL_IS_OBJECT(JSVAL_NULL) ===> true      /!\

r=me with that.
Attachment #479482 - Flags: review?(jorendorff) → review+
Depends on: 601168
Filed blocking bug 601168 for nsHTMLPluginObjElementSH failure, with patch.
Filed blocking bug 600943 (Date.prototype.toJSON) with patch; already r=jwalden.
(In reply to comment #63)
> Instead of !JSVAL_IS_OBJECT(thisVal), use JSVAL_IS_PRIMITIVE(thisVal), because

Thanks. *sigh* I'll get better about that soon...
(In reply to comment #60)
> Nit: use ok as canonical name, not success, if there's a mix of styles (I see
> both success and ok used, only sparsely, though).

Done.

> Stick with prevailing brace style when in Rome (or dark Volsce infected forests
> to the north ;-).

D'oh --- unintentional change. Fixed.
(In reply to comment #38)
> The bit about a "placeholder local GC root" is obsolete. The real reason
> we need a slot there is that the JS stack layout (the calling
> convention, basically) requires a slot for thisv, even if you're
> constructing.

Updated comment:

+         * Then (or in a call case that has no explicit reference-base
+         * object) we emit JSOP_PUSH to produce the |this| slot required
+         * for calls (which non-strict mode functions will box into the
+         * global object).
> This comment should also have been changed to JSOP_PUSH.

Fixed.

> At issue here is removing the comment.
> 
> This code is weird enough to warrant a comment IMHO, perhaps "Push the
> ImplicitThisValue for the Environment Record associated with obj. See
> ES5 sections 10.2.1.1.6 and 10.2.1.2.6 (ImplicitThisValue) and section
> 11.2.3 (Function Calls)."

Used this as the comment.

> In jsinterp.cpp, js::Interpret, CASE(JSOP_CALLUPVAR_DBG):
> >     /* Minimize footprint with generic code instead of NATIVE_GET. */
> >     obj2->dropProperty(cx, prop);
> >     Value *vp = regs.sp;
> >-    PUSH_NULL();
> >+    PUSH_UNDEFINED();
> >     if (!obj->getProperty(cx, id, vp))
> >         goto error;
> 
> FWIW, I don't think it's necessary to change this one. It doesn't matter
> though.

Took out the change.

> In jsstr.cpp, FindReplaceLength:
> >-        args.thisv().setNull();
> >+        args.thisv().setUndefined();
> 
> str_replace_flat_lambda needs the same change.

Thank you!  Fixed.

> In methodjit/FrameState.h, class FrameState:
> >+     * Helper function. Tests if a slot's type is undefined. Condition should
> >+     * be Equal or NotEqual.
> 
> Maybe this is a dumb thing to even bring up, but it seems like since the
> implementation asserts, we should say "must", not "should". Same thing
> for the other testFoo comments.

Comments changed.
(In reply to comment #35)
> But the comment is misleading anyway. It should say something like:
> 
> /*
>  * String.prototype.{toString,toSource,valueOf} throw a TypeError if the
>  * this-argument is not a string or a String object. So those methods use
>  * js::GetPrimitiveThis which provides that behavior.
>  *
>  * By standard, the rest of the String methods must ToString the
>  * this-argument rather than throw a TypeError. So those methods use
>  * NORMALIZE_THIS instead.
>  */

Fixed.
(In reply to comment #28)
> Comment on attachment 478007 [details] [diff] [review]
> In jsinterp.h, ReportIncompatibleMethod:
> >+        JS_ASSERT(thisv.isUndefined() || thisv.isNull());
> 
> I don't think thisv can be undefined here.

Once the whole patch series is applied, it can:

js> String.prototype.toSource.call(undefined)    
typein:4: TypeError: String.prototype.toSource called on incompatible undefined
I'll improve the message.

> In jsinterp.h, comment on declaration of js::GetPrimitiveThis:
> >+ * and extract its private slot value to return via *vpp.  If |this| is a
> >+ * primitive not of the desired type, report an error as though a |this| value
> >+ * had been computed from it.
> 
> Everything after "report an error" seems like it's trying to be precise,
> but it's not succeeding. Please either clarify it or drop it.

Will do.

> > JS_ALWAYS_INLINE bool
> > PrimitiveThisTest(JSFunction *fun, const Value &v)
> > {
> >-    uint16 flags = fun->flags;
> >-    return (v.isString() && !!(flags & JSFUN_THISP_STRING)) ||
> >-           (v.isNumber() && !!(flags & JSFUN_THISP_NUMBER)) ||
> >-           (v.isBoolean() && !!(flags & JSFUN_THISP_BOOLEAN));
> >+    return v.isNull() || (!v.isObject() && fun->acceptsPrimitiveThis());
> > }
> 
> I would have expected:
> 
>     JS_ASSERT(!v.isUndefined());
>     return !v.isObjectOrNull() && fun->acceptsPrimitiveThis();
>
> The bit about v.isNull() is a change in behavior for this function, right?
Yeah; I've changed this to:

  return !v.isPrimitive() || fun->acceptsPrimitiveThis();

which seems like a pretty direct reading of its function.
The latest iteration.
Attachment #479703 - Attachment is obsolete: true
Attachment #480292 - Flags: review?(jorendorff)
Attachment #479703 - Flags: review?(jorendorff)
Comment on attachment 473944 [details] [diff] [review]
4 - Make Object.prototype.toString handle null/undefined this per ES5+errata

Hi, Brendan. This is ready for your review.
Alias: strictThis
Comment on attachment 479703 [details] [diff] [review]
3 - Don't box |this| for strict mode functions.

While reviewing this, I noticed that:

    // this passes
    var threw = 0;
    try { Function('"use strict"; with (x) {y;}'); } catch (exc) { threw++; }
    assertEq(threw, 1);

    // but this fails!
    var f = Function('"use strict"; return this;');
    assertEq(f(), void 0);

f->inStrictMode() is true, but f->acceptsPrimitiveThis() is false. This is
because f does not go through Parser::functionDef, where the
JSFUN_PRIMITIVE_THIS flag is set.

I think this could be fixed by getting rid of JSFUN_PRIMITIVE_THIS and
acceptsPrimitiveThis() entirely and using inStrictMode() instead. Native
functions do not have to advertise whether or not they accept a primitive this.
They can simply call GetPrimitiveThis (or directly examine vp[1]) if they do;
and use JS_THIS_OBJECT if they do not.

----

In JS_THIS_OBJECT, the common case where vp[1] is already an object may be
inlined. Maybe it's not worth it. We do call JS_THIS_OBJECT in every quickstub
method though.

Variants of JS_InstanceOf and js::InstanceOf that take a jsval/Value would save
code in a few places (as in iterator_next, where trying to coerce thisv to an
object is futile). Also possibly not worth it.

In jsinterp.cpp, CASE(JSOP_UNBRANDTHIS):
>+    Value &thisv = regs.fp->thisValue();
>+    if (thisv.isObject()) {
>+        JSObject *obj = &thisv.toObject();
>+        if (obj->isNative() && !obj->unbrand(cx))
>+            goto error;
>+    }

I'd like to have a test case in the suite that takes the !thisv.isObject()
branch here.

> BEGIN_CASE(JSOP_GETTHISPROP)
>-    obj = regs.fp->computeThisObject(cx);
>-    if (!obj)
>+    if (!regs.fp->computeThis(cx))
>         goto error;
>     i = 0;
>+    vp = &regs.fp->thisValue();
>     PUSH_NULL();
>-    goto do_getprop_with_obj;
>+    goto do_getprop_with_lval;

Setting vp to point to the stack frame's this slot is no good, because we have:

    do_getprop_with_lval:
      VALUE_TO_OBJECT(cx, vp, obj);

which writes back to *vp. Here's a test detecting the bug:

    var f = String.prototype.m = function () {
        "use strict";
        assertEq(this, "s");             // pass
        return [this.m, this];           // GETTHISPROP overwrites "this"
    };
    var a = "s".m();
    assertEq(a[0], f);                   // pass
    assertEq(a[1], "s");                 // FAIL

I think the fix is to use PUSH_COPY, like GETARGPROP and GETLOCALPROP do.

In CASE(JSOP_CALLPROP):
>     /* Wrap primitive lval in object clothing if necessary. */
>     if (lval.isPrimitive()) {
>         /* FIXME: https://bugzilla.mozilla.org/show_bug.cgi?id=412571 */
>         JSObject *funobj;
>         if (!IsFunctionObject(rval, &funobj) ||
>             !PrimitiveThisTest(funobj->getFunctionPrivate(), lval)) {
>             if (!js_PrimitiveToObject(cx, &regs.sp[-1]))
>                 goto error;
>         }
>     }

You can delete all that.

While you're in there, the goto end_callprop; looks pretty silly, since you
could say the same thing with an else-block.

In jsinterp.h, struct JSStackFrame:
>+    ptrdiff_t offsetThisValue() const {
>+        return (char *) &thisValue() - (char *) this;
>+    }

This doesn't seem to be used anywhere.

In js/src/jsinterpinlines.h, JSStackFrame::computeThis:
>+inline bool
>+JSStackFrame::computeThis(JSContext *cx)
> {
>     js::Value &thisv = thisValue();
>-    if (JS_LIKELY(!thisv.isPrimitive()))
>-        return &thisv.toObject();
>+    if (JS_LIKELY(thisv.isObject()))
>+        return true;

This JS_LIKELY might be bogus. Certainly it's easy to construct code where the
condition is false:

    function global() { return this; }    // Note: not strict-mode code
    for (i=0; i<100; i++) {
        global();
        eval(""); // or anything else that causes us not to trace the loop
    }

Please consider checking to see if this JS_LIKELY measurably improves perf
and ripping it out if not.

In jsobj.cpp, obj_eval:
>+    /*
>+     * Direct calls to eval are supposed to see the caller's |this|. If we
>+     * haven't wrapped that yet, do so now.
>+     */
>+    caller->computeThis(cx);

This needs to check the return value, right?

In jstracer.cpp, TraceRecorder::getThis:
>+    if (fp->fun()->inStrictMode() || !thisv.isPrimitive()) {

Style nit: thisv.isObject() rather than !thisv.isPrimitive(), please.

>     /*
>      * Compute 'this' now. The result is globalObj->thisObject(),
>      * which is trace-constant. getThisObject writes back to fp->argv[-1],
>      * so do the same on trace.
>      */

Please change fp->argv[-1] to fp->thisValue() in this comment.

However this comment and the surrounding code still assume that fp->thisValue()
is either an object or undefined. It can be a string, as here:

    var HOTLOOP = this.tracemonkey ? tracemonkey.HOTLOOP : 8;
    var a;
    function f(n) {
	for (var i = 0; i < HOTLOOP; i++)
	    if (i == HOTLOOP - 2)
		a = this;
    }
    f.call("s", 1);
    f.call("s", 1);  // Assertion failure: thisObj == globalObj
    assertEq(typeof a, "object");
    assertEq("" + a, "s");

It's fine with me if we don't trace such cases.

>+    if (!fp->computeThis(cx))
>+        RETURN_ERROR("computeThis failed");
>+    /* thisv is a reference, so it'll see the newly computed |this|. */

Blank line before the comment, please. Or just delete it and call thisValue()
again. I wouldn't complain. (As it stands, this method does so anyway in the
end!)

In jsvalue.h, BOX_NON_DOUBLE_JSVAL:
>+    JS_ASSERT_IF(type == JSVAL_TYPE_STRING ||
>+                 type == JSVAL_TYPE_OBJECT ||
>+                 type == JSVAL_TYPE_NONFUNOBJ ||
>+                 type == JSVAL_TYPE_FUNOBJ,
>+                 payload != 0);

OK. While you're at it, STRING_TO_JSVAL_IMPL should check for this easy mistake
too. And so on. Both OBJECT_TO_JSVAL_IMPLs already assert (as does
js::Value::setObject, redundantly; remove that one, please?).

In methodjit/Compiler.cpp, mjit::Compiler::jsop_this():
>         frame.push(thisvAddr);
>-        Jump null = frame.testUndefined(Assembler::Equal, frame.peek(-1));
>-        stubcc.linkExit(null, Uses(1));
>+        Jump notObj = frame.testObject(Assembler::NotEqual, frame.peek(-1));
>+        stubcc.linkExit(notObj, Uses(1));
>         stubcc.leave();
>         stubcc.call(stubs::This);
>         stubcc.rejoin(Changes(1));

The test, branch, and call to stubs::This are unnecessary if
  * we're in strict-mode code
  * we're in a global script
  * we're in an eval script.

Right? If so, wrap the five lines starting with notObj in an if-block.

This patch seems to be missing a lot of tests.
Attachment #480292 - Flags: review?(jorendorff)
Comment on attachment 473944 [details] [diff] [review]
4 - Make Object.prototype.toString handle null/undefined this per ES5+errata

Stealing review.
Attachment #473944 - Flags: review+
Is that theft enough to remove the review request from Brendan? IOW, is this puppy ready to land?
Comment on attachment 473944 [details] [diff] [review]
4 - Make Object.prototype.toString handle null/undefined this per ES5+errata

Great! This fixes bug 575522.

/be
Attachment #473944 - Flags: review?(brendan) → review+
(In reply to comment #75)
> Is that theft enough to remove the review request from Brendan? IOW, is this
> puppy ready to land?

One review is enough, and jorendorff can r+ for me any day.

/be
(In reply to comment #73)
> While reviewing this, I noticed that:
> 
>     // this passes
>     var threw = 0;
>     try { Function('"use strict"; with (x) {y;}'); } catch (exc) { threw++; }
>     assertEq(threw, 1);
> 
>     // but this fails!
>     var f = Function('"use strict"; return this;');
>     assertEq(f(), void 0);
> 
> f->inStrictMode() is true, but f->acceptsPrimitiveThis() is false. This is
> because f does not go through Parser::functionDef, where the
> JSFUN_PRIMITIVE_THIS flag is set.
> 
> I think this could be fixed by getting rid of JSFUN_PRIMITIVE_THIS and
> acceptsPrimitiveThis() entirely and using inStrictMode() instead. Native
> functions do not have to advertise whether or not they accept a primitive this.
> They can simply call GetPrimitiveThis (or directly examine vp[1]) if they do;
> and use JS_THIS_OBJECT if they do not.

Filed as follow-up bug 610684.
The follow-up is bug 601684.
Whiteboard: [ETA: 9/28][needs-review-brendan] → [ETA: 9/28]
(In reply to comment #73)
> In JS_THIS_OBJECT, the common case where vp[1] is already an object may be
> inlined. Maybe it's not worth it. We do call JS_THIS_OBJECT in every quickstub
> method though.

I'm not sure what you're suggesting here. Are you suggesting that JS_THIS be simplified to always simply call JS_ComputeThis, instead of checking if vp[1] is a primitive first?

> Variants of JS_InstanceOf and js::InstanceOf that take a jsval/Value would save
> code in a few places (as in iterator_next, where trying to coerce thisv to an
> object is futile). Also possibly not worth it.

As in, avoid all the jsval->JSObject *->jsval conversions?  I also think InstanceOf is a horrible name for a function that throws an error if its argument is not the sort of thing we were hoping for, rather than one that simply returns yay or nay. An error-checking function that returns the JSObject * or NULL seems like it would be comfortable. Filed as follow-up bug 601709.

> 
> In jsinterp.cpp, CASE(JSOP_UNBRANDTHIS):
> >+    Value &thisv = regs.fp->thisValue();
> >+    if (thisv.isObject()) {
> >+        JSObject *obj = &thisv.toObject();
> >+        if (obj->isNative() && !obj->unbrand(cx))
> >+            goto error;
> >+    }
> 
> I'd like to have a test case in the suite that takes the !thisv.isObject()
> branch here.

Done. Found and fixed a JM abort; thanks!

> 
> > BEGIN_CASE(JSOP_GETTHISPROP)
> >-    obj = regs.fp->computeThisObject(cx);
> >-    if (!obj)
> >+    if (!regs.fp->computeThis(cx))
> >         goto error;
> >     i = 0;
> >+    vp = &regs.fp->thisValue();
> >     PUSH_NULL();
> >-    goto do_getprop_with_obj;
> >+    goto do_getprop_with_lval;
> 
> Setting vp to point to the stack frame's this slot is no good, because we have:
> 
>     do_getprop_with_lval:
>       VALUE_TO_OBJECT(cx, vp, obj);
> 
> which writes back to *vp. Here's a test detecting the bug:
> 
>     var f = String.prototype.m = function () {
>         "use strict";
>         assertEq(this, "s");             // pass
>         return [this.m, this];           // GETTHISPROP overwrites "this"
>     };
>     var a = "s".m();
>     assertEq(a[0], f);                   // pass
>     assertEq(a[1], "s");                 // FAIL
> 
> I think the fix is to use PUSH_COPY, like GETARGPROP and GETLOCALPROP do.

Done.

> In CASE(JSOP_CALLPROP):
> >     /* Wrap primitive lval in object clothing if necessary. */
> >     if (lval.isPrimitive()) {
> >         /* FIXME: https://bugzilla.mozilla.org/show_bug.cgi?id=412571 */
> >         JSObject *funobj;
> >         if (!IsFunctionObject(rval, &funobj) ||
> >             !PrimitiveThisTest(funobj->getFunctionPrivate(), lval)) {
> >             if (!js_PrimitiveToObject(cx, &regs.sp[-1]))
> >                 goto error;
> >         }
> >     }
> 
> You can delete all that.

Done. I found another assertion that needed to be removed, and some more assumptions that |this| is an object.

> While you're in there, the goto end_callprop; looks pretty silly, since you
> could say the same thing with an else-block.

Yes, I had that in my notes; done.

> In jsinterp.h, struct JSStackFrame:
> >+    ptrdiff_t offsetThisValue() const {
> >+        return (char *) &thisValue() - (char *) this;
> >+    }
> 
> This doesn't seem to be used anywhere.

This was used by the original TraceRecorder::getThis, which got simplified. Thanks.

> In js/src/jsinterpinlines.h, JSStackFrame::computeThis:
> >+inline bool
> >+JSStackFrame::computeThis(JSContext *cx)
> > {
> >     js::Value &thisv = thisValue();
> >-    if (JS_LIKELY(!thisv.isPrimitive()))
> >-        return &thisv.toObject();
> >+    if (JS_LIKELY(thisv.isObject()))
> >+        return true;
> 
> This JS_LIKELY might be bogus. Certainly it's easy to construct code where the
> condition is false:
> 
>     function global() { return this; }    // Note: not strict-mode code
>     for (i=0; i<100; i++) {
>         global();
>         eval(""); // or anything else that causes us not to trace the loop
>     }
> 
> Please consider checking to see if this JS_LIKELY measurably improves perf
> and ripping it out if not.

I'll measure it.

> In jsobj.cpp, obj_eval:
> >+    /*
> >+     * Direct calls to eval are supposed to see the caller's |this|. If we
> >+     * haven't wrapped that yet, do so now.
> >+     */
> >+    caller->computeThis(cx);
> 
> This needs to check the return value, right?

Of course --- thanks.

> In jstracer.cpp, TraceRecorder::getThis:
> >+    if (fp->fun()->inStrictMode() || !thisv.isPrimitive()) {
> 
> Style nit: thisv.isObject() rather than !thisv.isPrimitive(), please.

Fixed.

> >     /*
> >      * Compute 'this' now. The result is globalObj->thisObject(),
> >      * which is trace-constant. getThisObject writes back to fp->argv[-1],
> >      * so do the same on trace.
> >      */
> 
> Please change fp->argv[-1] to fp->thisValue() in this comment.

Done.

> However this comment and the surrounding code still assume that fp->thisValue()
> is either an object or undefined. It can be a string, as here:
> 
>     var HOTLOOP = this.tracemonkey ? tracemonkey.HOTLOOP : 8;
>     var a;
>     function f(n) {
>     for (var i = 0; i < HOTLOOP; i++)
>         if (i == HOTLOOP - 2)
>         a = this;
>     }
>     f.call("s", 1);
>     f.call("s", 1);  // Assertion failure: thisObj == globalObj
>     assertEq(typeof a, "object");
>     assertEq("" + a, "s");
> 
> It's fine with me if we don't trace such cases.

I've added this as a trace test, and I have a patch for it (we stop recording if the computeThis call is going to wrap a primitive).

> 
> >+    if (!fp->computeThis(cx))
> >+        RETURN_ERROR("computeThis failed");
> >+    /* thisv is a reference, so it'll see the newly computed |this|. */
> 
> Blank line before the comment, please. Or just delete it and call thisValue()
> again. I wouldn't complain. (As it stands, this method does so anyway in the
> end!)

Fixed. (I used thisv throughout.)

I'll get to the rest tomorrow morning. Attaching new patch.
Latest rev. Almost all points in comment 73 addressed; will get to the rest tomorrow morning.
Attachment #480292 - Attachment is obsolete: true
Jason found a small script that hits an assertion while recording the trace, but runs correctly if the statement |options().split(',')| is inserted. So there's something inappropriate going on.
Sorted out. Evaluating |this| off-trace can cause lazily initialized classes to be initialized, which reshapes the global and resets loop counts, meaning that we didn't record the iterations we were expecting. Calling options().split(',') above the loop causes the String class to be initialized earlier, so we record the path we had expected. Test revised to be more robust.
Blocks: 602002
(In reply to comment #73)
> In jsvalue.h, BOX_NON_DOUBLE_JSVAL:
> >+    JS_ASSERT_IF(type == JSVAL_TYPE_STRING ||
> >+                 type == JSVAL_TYPE_OBJECT ||
> >+                 type == JSVAL_TYPE_NONFUNOBJ ||
> >+                 type == JSVAL_TYPE_FUNOBJ,
> >+                 payload != 0);
> 
> OK. While you're at it, STRING_TO_JSVAL_IMPL should check for this easy mistake
> too. And so on. Both OBJECT_TO_JSVAL_IMPLs already assert (as does
> js::Value::setObject, redundantly; remove that one, please?).

I've made sure both BOX_NON_DOUBLE_JSVAL and STRING_TO_JSVAL_IMPL versions have the assert, and removed the assert from setObject. It didn't seem appropriate to check the 'private' and 'magic' cases. Beyond INTERNED_STRING_TO_JSID (fixed), I didn't see any "and so on"; did you have something specific in mind?

> In methodjit/Compiler.cpp, mjit::Compiler::jsop_this():
> >         frame.push(thisvAddr);
> >-        Jump null = frame.testUndefined(Assembler::Equal, frame.peek(-1));
> >-        stubcc.linkExit(null, Uses(1));
> >+        Jump notObj = frame.testObject(Assembler::NotEqual, frame.peek(-1));
> >+        stubcc.linkExit(notObj, Uses(1));
> >         stubcc.leave();
> >         stubcc.call(stubs::This);
> >         stubcc.rejoin(Changes(1));
> 
> The test, branch, and call to stubs::This are unnecessary if
>   * we're in strict-mode code
>   * we're in a global script
>   * we're in an eval script.
> 
> Right? If so, wrap the five lines starting with notObj in an if-block.

I made an equivalent change (after thinking hard and talking with folks on IRC about what's going on here).

> This patch seems to be missing a lot of tests.

You're telling me.
Revised per all jorendorff's comments.
Attachment #481069 - Flags: review?
Attachment #481069 - Flags: review? → review?(jorendorff)
(In reply to comment #84)
> > This patch seems to be missing a lot of tests.
> 
> You're telling me.

This is not meant to be a flip response; I want to see this all covered, and I'll work on a patch to do that immediately.
Please note that we have now created a branch for beta 7 work. In addition to landing your fix on mozilla-central default, please also land it on mozilla-central GECKO20b7pre_20101006_RELBRANCH

(note: when landing on mozilla-central default, you will be given priority in the landing queue at https://wiki.mozilla.org/LandingQueue )
This bug should not have gone two days without an update. What's up?
(In reply to comment #80)
> (In reply to comment #73)
> > In JS_THIS_OBJECT, the common case where vp[1] is already an object may be
> > inlined. Maybe it's not worth it. We do call JS_THIS_OBJECT in every quickstub
> > method though.
> 
> I'm not sure what you're suggesting here. Are you suggesting that JS_THIS be
> simplified to always simply call JS_ComputeThis, instead of checking if vp[1]
> is a primitive first?

I'm suggesting the opposite: right now, with or without these patches, JS_THIS always simply calls JS_ComputeThis.  I'm suggesting that it check first!

> > While you're in there, the goto end_callprop; looks pretty silly, since you
> > could say the same thing with an else-block.
> 
> Yes, I had that in my notes; done.

I still see the label in the patch that's up for review now.

> > Please consider checking to see if this JS_LIKELY measurably improves perf
> > and ripping it out if not.
> 
> I'll measure it.

This is still listed as a todo item in the commit-message text at the top of the current patch. Not a priority, though.
In jsinterp.cpp, CASE(JSOP_CALLPROP):
> > >     /* Wrap primitive lval in object clothing if necessary. */
> > >     if (lval.isPrimitive()) {
> > >         /* FIXME: https://bugzilla.mozilla.org/show_bug.cgi?id=412571 */
> > >         JSObject *funobj;
> > >         if (!IsFunctionObject(rval, &funobj) ||
> > >             !PrimitiveThisTest(funobj->getFunctionPrivate(), lval)) {
> > >             if (!js_PrimitiveToObject(cx, &regs.sp[-1]))
> > >                 goto error;
> > >         }
> > >     }
> > 
> > You can delete all that.
> 
> Done. I found another assertion that needed to be removed, and some more
> assumptions that |this| is an object.

I hate to do this to you, but it looks like stubs::CallProp still has a copy of
all this code, and there's a bunch of code in mjit::Compiler::jsop_callprop_str
that seems to be doing the same thing (...but isn't, not quite).

The difference in behavior can be observed, barely:

  var s = "grape";
  function f() { "use strict"; return this; }
  var p = Proxy.createFunction(f, f);
  String.prototype.p = p;
  assertEq(s.p(), "grape");  // fails with -m, passes without -m

In jsobj.cpp, obj_eval:
>+        /*
>+         * Direct calls to eval are supposed to see the caller's |this|. If we
>+         * haven't wrapped that yet, do so now.
>+         */
>+        caller->computeThis(cx);

The context conflicts with a patch I landed today. But in a good way.
Now this can go in the new "if (directCall)" block.

Still needs to check the return value, though!

In jstracer.cpp, TraceRecorder::getThis():
>+    if (fp->fun()->inStrictMode() || thisv.isObject()) {
>         /*
>          * fp->argv[-1] has already been computed. Since the

Sorry I missed this earlier: could you change that fp->argv[-1] to
fp->thisValue() too?

In jsvalue.h:
> BOX_NON_DOUBLE_JSVAL(JSValueType type, uint64 *slot)
> {
>     jsval_layout l;
>     JS_ASSERT(type > JSVAL_TYPE_DOUBLE && type <= JSVAL_UPPER_INCL_TYPE_OF_BOXABLE_SET);
>+    JS_ASSERT_IF(type == JSVAL_TYPE_STRING ||
>+                 type == JSVAL_TYPE_OBJECT ||
>+                 type == JSVAL_TYPE_NONFUNOBJ ||
>+                 type == JSVAL_TYPE_FUNOBJ,
>+                 payload != 0);
>     l.s.tag = JSVAL_TYPE_TO_TAG(type & 0xF);
>     l.s.payload.u32 = *(uint32 *)slot;

This one doesn't compile: payload isn't defined.

(The same assertion compiles fine in the other definition of
BOX_NON_DOUBLE_JSVAL.)

In tests/ecma_5/strict/unbrand-non-object.js:
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
[...]
>+function foo() {
>+  "use strict";
>+  this.insert = function(){ bar(); };
[...]
>+try {
>+    foo.call(undefined);
>+    exception = null;

Indentation.

>+assertEq(true, TypeError.prototype.isPrototypeOf(exception));

assertEq(exception instanceof TypeError, true);

(The error message assertEq generates on a failure is "got <arg0>, expected
<arg1>", fwiw. Like Mochitest's is() function.)

r=me with comment 89 and this addressed and some tests.

I don't mean to be a jerk about the tests but I think landing something as wide-ranging and tricky as this, this close to a release, without tests would be a mistake.
Attachment #481069 - Flags: review?(jorendorff) → review+
(In reply to comment #89)
> > > Please consider checking to see if this JS_LIKELY measurably improves perf
> > > and ripping it out if not.
> > 
> > I'll measure it.
> 
> This is still listed as a todo item in the commit-message text at the top of
> the current patch. Not a priority, though.

This was my mistake forgetting that implicit global-this calls pushed null (undefined, after a couple patches here), and that given that case, vp[1] could very easily not be an object.  Had I thought about this not-uncommon case I definitely wouldn't have added the likely-check here -- I say remove it and get on with life, no reason to bother testing for a not-no-brainer condition.

If jimb is busy or sufficiently uninterested, I don't mind writing the necessary tests for this (although I do have my own slate of bugs -- as does everyone else here, of course).  I enjoy writing tests, especially nitpicky ones, and had I not run out of time a month ago I'd definitely have written this bug's fair share.

On the other hand, the handoff here is long since complete, so it would be more than reasonable to just finish it off as is currently planned.  Your call.
(In reply to comment #90)
> >+        /*
> >+         * Direct calls to eval are supposed to see the caller's |this|. If we
> >+         * haven't wrapped that yet, do so now.
> >+         */
> >+        caller->computeThis(cx);
> 
> The context conflicts with a patch I landed today. But in a good way.
> Now this can go in the new "if (directCall)" block.
> 
> Still needs to check the return value, though!

... I'm sorry. This was an older version of the patch. I'll address your other comments and re-post.
(In reply to comment #91)
> If jimb is busy or sufficiently uninterested, I don't mind writing the
> necessary tests for this (although I do have my own slate of bugs -- as does
> everyone else here, of course).  I enjoy writing tests, especially nitpicky
> ones, and had I not run out of time a month ago I'd definitely have written
> this bug's fair share.

I'm writing the tests right now.
(In reply to comment #90)
> In jsinterp.cpp, CASE(JSOP_CALLPROP):
> > > >     /* Wrap primitive lval in object clothing if necessary. */
> > > >     if (lval.isPrimitive()) {
> > > >         /* FIXME: https://bugzilla.mozilla.org/show_bug.cgi?id=412571 */
> > > >         JSObject *funobj;
> > > >         if (!IsFunctionObject(rval, &funobj) ||
> > > >             !PrimitiveThisTest(funobj->getFunctionPrivate(), lval)) {
> > > >             if (!js_PrimitiveToObject(cx, &regs.sp[-1]))
> > > >                 goto error;
> > > >         }
> > > >     }
> > > 
> > > You can delete all that.
> > 
> > Done. I found another assertion that needed to be removed, and some more
> > assumptions that |this| is an object.
> 
> I hate to do this to you, but it looks like stubs::CallProp still has a copy of
> all this code, and there's a bunch of code in mjit::Compiler::jsop_callprop_str
> that seems to be doing the same thing (...but isn't, not quite).
> 
> The difference in behavior can be observed, barely:
> 
>   var s = "grape";
>   function f() { "use strict"; return this; }
>   var p = Proxy.createFunction(f, f);
>   String.prototype.p = p;
>   assertEq(s.p(), "grape");  // fails with -m, passes without -m

Okay, I've taken these out too (and deleted stubs::WrapPrimitiveThis), without apparent ill effect.

> In jsobj.cpp, obj_eval:
> >+        /*
> >+         * Direct calls to eval are supposed to see the caller's |this|. If we
> >+         * haven't wrapped that yet, do so now.
> >+         */
> >+        caller->computeThis(cx);
> 
> The context conflicts with a patch I landed today. But in a good way.
> Now this can go in the new "if (directCall)" block.
> 
> Still needs to check the return value, though!

This had all been taken care of in the revised patch.

> In jstracer.cpp, TraceRecorder::getThis():
> >+    if (fp->fun()->inStrictMode() || thisv.isObject()) {
> >         /*
> >          * fp->argv[-1] has already been computed. Since the
> 
> Sorry I missed this earlier: could you change that fp->argv[-1] to
> fp->thisValue() too?

Fixed.

> In jsvalue.h:
> > BOX_NON_DOUBLE_JSVAL(JSValueType type, uint64 *slot)
> > {
> >     jsval_layout l;
> >     JS_ASSERT(type > JSVAL_TYPE_DOUBLE && type <= JSVAL_UPPER_INCL_TYPE_OF_BOXABLE_SET);
> >+    JS_ASSERT_IF(type == JSVAL_TYPE_STRING ||
> >+                 type == JSVAL_TYPE_OBJECT ||
> >+                 type == JSVAL_TYPE_NONFUNOBJ ||
> >+                 type == JSVAL_TYPE_FUNOBJ,
> >+                 payload != 0);
> >     l.s.tag = JSVAL_TYPE_TO_TAG(type & 0xF);
> >     l.s.payload.u32 = *(uint32 *)slot;
> 
> This one doesn't compile: payload isn't defined.

Had already been fixed.

> In tests/ecma_5/strict/unbrand-non-object.js:
> >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> [...]
> >+function foo() {
> >+  "use strict";
> >+  this.insert = function(){ bar(); };
> [...]
> >+try {
> >+    foo.call(undefined);
> >+    exception = null;
> 
> Indentation.

Fixed.

> >+assertEq(true, TypeError.prototype.isPrototypeOf(exception));
> 
> assertEq(exception instanceof TypeError, true);
> 
> (The error message assertEq generates on a failure is "got <arg0>, expected
> <arg1>", fwiw. Like Mochitest's is() function.)

Fixed, here and in the other new tests.

> I don't mean to be a jerk about the tests but I think landing something as
> wide-ranging and tricky as this, this close to a release, without tests would
> be a mistake.

Deliberate ignorance isn't a great approach to getting things done quickly.
Okay, I've verified (using gcov) that our tests cover the code affected by the patch --- except for the JM testUndefined functions, which are no longer used, but seem reasonable to leave in place.

I found another bit of primitive wrapping code in JM (in the PIC), and took it out with no ill effects.

All the tests you've suggested are now part of the test suite.

Running tests now.
(In reply to comment #95)
> All the tests you've suggested are now part of the test suite.

... along with others I've added.
The latest rev seems to regress js1_8/extensions/regress-394709.js when run under JaegerMonkey. Checking to see if this is a true regression, or just conservative GC noise.
That one seems to intermittently fail on tip.
It's conservative GC noise. Test patched as follows:

--- a/js/src/tests/js1_8/extensions/regress-394709.js
+++ b/js/src/tests/js1_8/extensions/regress-394709.js
@@ -65,10 +65,15 @@ function test()
 
   runtest();
   gc();
-  var counter = countHeap();
+  var count1 = countHeap();
   runtest();
   gc();
-  if (counter != countHeap())
+  var count2 = countHeap();
+  runtest();
+  gc();
+  var count3 = countHeap();
+  /* Try to be tolerant of conservative GC noise: we want a steady leak. */
+  if (count1 < count2 && count2 < count3)
     throw "A leaky watch point is detected";
 
   function runtest () {
Revised per comments, new test cases added.
Attachment #481982 - Flags: review?(jorendorff)
Regression in browser:
js1_5/Regress/regress-475645-02.js
Attachment #481982 - Flags: review?(jorendorff)
Attachment #480846 - Attachment is obsolete: true
Attachment #481069 - Attachment is obsolete: true
Easy fix, looks like; will add corresponding shell test.
Found a bunch more bugs:

// We incorrectly wrap primitives when calling subscript expressions:
var strictString = 'strict';
Boolean.prototype.strict = strict;
assertEq(true[strictString](), true); // fails

// We incorrectly wrap primitives passed to primitive property getters
// and setters:
var strict = 'strict';
function strictThis() { 'use strict'; return this; }
var savedThis;
function strictSaveThis(x) { 'use strict'; savedThis = this; }
Object.defineProperty(Boolean.prototype, 'strict', { get: strictThis, set: strictSaveThis });
assertEq(true.strict,  true);
assertEq(true[strict], true);
assertEq((true.strict  = 1, savedThis), true);
assertEq((true[strict] = 1, savedThis), true);
It's just ridiculous how many distinct pieces of code do this one promotion.
Perhaps those "bunch more bugs" should be handled in a followup?  (Even a b7+ bug if we want strict-this support to be beta-atomic -- I could go either way on the argument, depending on how well we clarified any such limitation in b7 release info and such.)  This bug's quite long in the tooth, and it presents ongoing compatibility concerns with other work.  (I keep mentioning potential conflicts to people when they say what sort of changes they're working on.)  Getting what's done in place (or even just the many partway patches here that have been r+'d), and cleaning up previously-unrecognized edges in followups, means this the great majority of this bug's changes no longer pose conflicts with other work.  It also makes reviews easier: smaller bites to review on each iteration of work.  This all seems like a very good thing to me.
Fixed the first batch.
(In reply to comment #105)
> Perhaps those "bunch more bugs" should be handled in a followup?

Let's see where we are on Monday.
Strict primitive property getters look extremely hard to fix: 

function strictThis() { 'use strict'; return this; }
Object.defineProperty(Boolean.prototype, 'strict', { get: strictThis });
assertEq(true.strict,  true);

The primitive gets wrapped in the JSOP_GETPROP case, but the getters don't get invoked until several calls in from there (youngest first):

ExternalInvoke
js::ExternalGetOrSet
js::Shape::get
js_NativeGetInline
js_GetPropertyHelperWithShapeInline
js_GetPropertyHelperInline
js_GetPropertyHelper
js::Interpret

None of these take a 'this' value distinct from the object whose method is being invoked, as the custom [[Get]] in ES5 8.7.1 requires. A clean way to propagate the correct 'this' isn't apparent to me.
Filed as follow-up bug 603201.
More tests, more primitive-wrapping code removed.
Attachment #481982 - Attachment is obsolete: true
Looks *great*.
Our current thinking is that this should land on M-C (and thus beta7) after the compartments patch lands. Compartments has already landed on TM, and I've adapted this patch queue to run there; no regressions.

I understand jst has no cycles to spare at the moment to review bug 601168, but that does block this (i.e. this patch introduces regressions without that patch).
Comment on attachment 482147 [details] [diff] [review]
3 - Don't box |this| for strict mode functions.

>+JS_PUBLIC_API(JSBool)
>+JS_GetFrameThis(JSContext *cx, JSStackFrame *fp, jsval *thisv)
> {
>     if (fp->isDummyFrame())
>+        return false;

Pre-existing bug, but shouldn't this not silently fail, rather succeed with undefined in *thisv?

>@@ -4197,51 +4196,38 @@ BEGIN_CASE(JSOP_CALLPROP)

Shouldn't this op use ValuePropertyBearer now?

/be
(In reply to comment #113)
> >@@ -4197,51 +4196,38 @@ BEGIN_CASE(JSOP_CALLPROP)
> 
> Shouldn't this op use ValuePropertyBearer now?

Yes, I believe it could.

(Will look at dummy frames tomorrow.)
Depends on: 603201
Rebasing, testing interim patches for bug 603201, I found that this patch queue left at least one "next opcode in a genexp sequence is JSOP_NULL" local assert in jsopcode.cpp. See first hunk in the rebase-hack attachment in bug 603201.

Probably need to check all JSOP_NULL occurrences in that file, make any that are matching a default |this| push be JSOP_PUSH.

/be
http://hg.mozilla.org/tracemonkey/rev/8e418df1af74
Whiteboard: [ETA: 9/28] → [fixed-in-tracemonkey]
(In reply to comment #115)
> Rebasing, testing interim patches for bug 603201, I found that this patch queue
> left at least one "next opcode in a genexp sequence is JSOP_NULL" local assert
> in jsopcode.cpp. See first hunk in the rebase-hack attachment in bug 603201.
> 
> Probably need to check all JSOP_NULL occurrences in that file, make any that
> are matching a default |this| push be JSOP_PUSH.

Resolved on IRC: mq confusion; all okay.
Depends on: 604323
http://hg.mozilla.org/mozilla-central/rev/56b07ce2d65c
seems to contain the fix. Is it accidental? Or is it possible to mark as FIXED?
http://hg.mozilla.org/mozilla-central/rev/66710af05aa1
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 605295
Depends on: 604971
Depends on: 611276
You need to log in before you can comment on or make changes to this bug.