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)
Core
JavaScript Engine
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.
Comment 1•14 years ago
|
||
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
Updated•14 years ago
|
Depends on: blazinglyfastthis
Comment 2•14 years ago
|
||
(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|.)
Comment 3•14 years ago
|
||
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.
Updated•14 years ago
|
blocking2.0: ? → beta1+
Updated•14 years ago
|
blocking2.0: beta1+ → beta2+
Comment 5•14 years ago
|
||
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.
Comment 6•14 years ago
|
||
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?
Comment 7•14 years ago
|
||
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.
Comment 8•14 years ago
|
||
Also, I cited an irrelevant part of the document, should have cited 10.4.3 (Entering Function Code).
Updated•14 years ago
|
blocking2.0: beta2+ → betaN+
Updated•14 years ago
|
blocking2.0: betaN+ → beta5+
Comment 9•14 years ago
|
||
Can we get an update here?
Assignee | ||
Comment 10•14 years ago
|
||
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+
Comment 12•14 years ago
|
||
I have part of a patch for this -- cross your fingers for me to finish it today! :-)
Comment 13•14 years ago
|
||
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)
Comment 14•14 years ago
|
||
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)
Comment 15•14 years ago
|
||
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)
Comment 16•14 years ago
|
||
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)
Comment 17•14 years ago
|
||
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. :-)
Comment 18•14 years ago
|
||
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. :-)
Comment 19•14 years ago
|
||
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. :-)
Updated•14 years ago
|
Whiteboard: [ETA: 9/22]
Assignee | ||
Comment 21•14 years ago
|
||
Refreshed for current tip.
Attachment #473939 -
Attachment is obsolete: true
Attachment #477299 -
Flags: review?(brendan)
Attachment #473939 -
Flags: review?(brendan)
Assignee | ||
Comment 22•14 years ago
|
||
Rebased to tip.
Attachment #473941 -
Attachment is obsolete: true
Attachment #477302 -
Flags: review?(brendan)
Attachment #473941 -
Flags: review?(brendan)
Assignee | ||
Comment 23•14 years ago
|
||
Missed a rejected patch hunk.
Attachment #477302 -
Attachment is obsolete: true
Attachment #477306 -
Flags: review?(brendan)
Attachment #477302 -
Flags: review?(brendan)
Assignee | ||
Comment 24•14 years ago
|
||
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
Assignee | ||
Comment 25•14 years ago
|
||
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.
Assignee | ||
Comment 26•14 years ago
|
||
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.
Assignee | ||
Comment 27•14 years ago
|
||
Okay, this one is ready for review.
Attachment #477299 -
Attachment is obsolete: true
Attachment #478007 -
Flags: review?(jorendorff)
Attachment #477299 -
Flags: review?(brendan)
Assignee | ||
Updated•14 years ago
|
Attachment #477306 -
Flags: review?(brendan) → review?(jorendorff)
Comment 28•14 years ago
|
||
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+
Assignee | ||
Comment 29•14 years ago
|
||
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.
Assignee | ||
Comment 30•14 years ago
|
||
(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."
Assignee | ||
Comment 31•14 years ago
|
||
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.
Comment 32•14 years ago
|
||
I am out of time for today. Mostly staring at the code instead of reviewing it. I'll try again tomorrow.
Comment 33•14 years ago
|
||
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
Assignee | ||
Comment 34•14 years ago
|
||
Patch #3 deletes that assertion.
Comment 35•14 years ago
|
||
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. */
Comment 36•14 years ago
|
||
>@@ -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*
Comment 37•14 years ago
|
||
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 38•14 years ago
|
||
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+
Assignee | ||
Comment 39•14 years ago
|
||
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)
Assignee | ||
Comment 40•14 years ago
|
||
Assignee | ||
Comment 41•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #478465 -
Attachment is obsolete: true
Assignee | ||
Comment 42•14 years ago
|
||
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).
Assignee | ||
Comment 43•14 years ago
|
||
Okay, that's done. Doing one more test run, hopefully #3 is ready for review now.
Assignee | ||
Comment 44•14 years ago
|
||
No js test suite regressions.
Attachment #478595 -
Attachment is obsolete: true
Attachment #478820 -
Flags: review?(jorendorff)
Assignee | ||
Comment 45•14 years ago
|
||
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.
Assignee | ||
Comment 46•14 years ago
|
||
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.
Updated•14 years ago
|
Whiteboard: [ETA: 9/22] → [ETA: needed][needs-review-brendan]
Assignee | ||
Comment 47•14 years ago
|
||
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)
Assignee | ||
Comment 48•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #477306 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [ETA: needed][needs-review-brendan] → [ETA: 9/28][needs-review-brendan]
Assignee | ||
Comment 49•14 years ago
|
||
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.
Assignee | ||
Comment 50•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #479480 -
Flags: review?(brendan)
Assignee | ||
Comment 51•14 years ago
|
||
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)
Assignee | ||
Comment 52•14 years ago
|
||
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.)
Assignee | ||
Comment 53•14 years ago
|
||
(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.
Assignee | ||
Comment 54•14 years ago
|
||
Attachment #479551 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #479551 -
Flags: review? → review?(dvander)
Comment 55•14 years ago
|
||
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.
Assignee | ||
Comment 56•14 years ago
|
||
(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+
Assignee | ||
Comment 58•14 years ago
|
||
I've run into a few mochitest failures; three down, two to go.
Assignee | ||
Comment 59•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
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 60•14 years ago
|
||
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+
Assignee | ||
Comment 61•14 years ago
|
||
Seems like nsHTMLPluginObjElementSH::Call isn't ready to cope with primitive |this| values. It assumes that argv[-1] is an object. Mochitest failure.
Assignee | ||
Comment 62•14 years ago
|
||
Basically, various things in the browser aren't expecting to get non-Object |this| values. Working through the list...
Comment 63•14 years ago
|
||
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+
Assignee | ||
Comment 64•14 years ago
|
||
Filed blocking bug 601168 for nsHTMLPluginObjElementSH failure, with patch.
Assignee | ||
Comment 65•14 years ago
|
||
Filed blocking bug 600943 (Date.prototype.toJSON) with patch; already r=jwalden.
Assignee | ||
Comment 66•14 years ago
|
||
(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...
Assignee | ||
Comment 67•14 years ago
|
||
(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.
Assignee | ||
Comment 68•14 years ago
|
||
(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.
Assignee | ||
Comment 69•14 years ago
|
||
(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.
Assignee | ||
Comment 70•14 years ago
|
||
(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.
Assignee | ||
Comment 71•14 years ago
|
||
The latest iteration.
Attachment #479703 -
Attachment is obsolete: true
Attachment #480292 -
Flags: review?(jorendorff)
Attachment #479703 -
Flags: review?(jorendorff)
Assignee | ||
Comment 72•14 years ago
|
||
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.
Updated•14 years ago
|
Alias: strictThis
Comment 73•14 years ago
|
||
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 = ®s.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, ®s.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.
Updated•14 years ago
|
Attachment #480292 -
Flags: review?(jorendorff)
Comment 74•14 years ago
|
||
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+
Comment 75•14 years ago
|
||
Is that theft enough to remove the review request from Brendan? IOW, is this puppy ready to land?
Comment 76•14 years ago
|
||
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+
Comment 77•14 years ago
|
||
(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
Assignee | ||
Comment 78•14 years ago
|
||
(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.
Comment 79•14 years ago
|
||
The follow-up is bug 601684.
Updated•14 years ago
|
Whiteboard: [ETA: 9/28][needs-review-brendan] → [ETA: 9/28]
Assignee | ||
Comment 80•14 years ago
|
||
(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 = ®s.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, ®s.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.
Assignee | ||
Comment 81•14 years ago
|
||
Latest rev. Almost all points in comment 73 addressed; will get to the rest tomorrow morning.
Attachment #480292 -
Attachment is obsolete: true
Assignee | ||
Comment 82•14 years ago
|
||
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.
Assignee | ||
Comment 83•14 years ago
|
||
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.
Assignee | ||
Comment 84•14 years ago
|
||
(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.
Assignee | ||
Comment 85•14 years ago
|
||
Revised per all jorendorff's comments.
Attachment #481069 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #481069 -
Flags: review? → review?(jorendorff)
Assignee | ||
Comment 86•14 years ago
|
||
(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.
Comment 87•14 years ago
|
||
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 )
Comment 88•14 years ago
|
||
This bug should not have gone two days without an update. What's up?
Comment 89•14 years ago
|
||
(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.
Comment 90•14 years ago
|
||
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, ®s.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.
Updated•14 years ago
|
Attachment #481069 -
Flags: review?(jorendorff) → review+
Comment 91•14 years ago
|
||
(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.
Assignee | ||
Comment 92•14 years ago
|
||
(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.
Assignee | ||
Comment 93•14 years ago
|
||
(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.
Assignee | ||
Comment 94•14 years ago
|
||
(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, ®s.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.
Assignee | ||
Comment 95•14 years ago
|
||
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.
Assignee | ||
Comment 96•14 years ago
|
||
(In reply to comment #95) > All the tests you've suggested are now part of the test suite. ... along with others I've added.
Assignee | ||
Comment 97•14 years ago
|
||
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.
Assignee | ||
Comment 99•14 years ago
|
||
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 () {
Assignee | ||
Comment 100•14 years ago
|
||
Revised per comments, new test cases added.
Attachment #481982 -
Flags: review?(jorendorff)
Assignee | ||
Comment 101•14 years ago
|
||
Regression in browser: js1_5/Regress/regress-475645-02.js
Assignee | ||
Updated•14 years ago
|
Attachment #481982 -
Flags: review?(jorendorff)
Assignee | ||
Updated•14 years ago
|
Attachment #480846 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #481069 -
Attachment is obsolete: true
Assignee | ||
Comment 102•14 years ago
|
||
Easy fix, looks like; will add corresponding shell test.
Assignee | ||
Comment 103•14 years ago
|
||
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);
Assignee | ||
Comment 104•14 years ago
|
||
It's just ridiculous how many distinct pieces of code do this one promotion.
Comment 105•14 years ago
|
||
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.
Assignee | ||
Comment 106•14 years ago
|
||
Fixed the first batch.
Assignee | ||
Comment 107•14 years ago
|
||
(In reply to comment #105) > Perhaps those "bunch more bugs" should be handled in a followup? Let's see where we are on Monday.
Assignee | ||
Comment 108•14 years ago
|
||
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.
Assignee | ||
Comment 109•14 years ago
|
||
Filed as follow-up bug 603201.
Assignee | ||
Comment 110•14 years ago
|
||
More tests, more primitive-wrapping code removed.
Attachment #481982 -
Attachment is obsolete: true
Comment 111•14 years ago
|
||
Looks *great*.
Assignee | ||
Comment 112•14 years ago
|
||
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 113•14 years ago
|
||
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
Assignee | ||
Comment 114•14 years ago
|
||
(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.)
Comment 115•14 years ago
|
||
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
Assignee | ||
Comment 116•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/8e418df1af74
Whiteboard: [ETA: 9/28] → [fixed-in-tracemonkey]
Assignee | ||
Comment 117•14 years ago
|
||
(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.
Comment 118•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/56b07ce2d65c seems to contain the fix. Is it accidental? Or is it possible to mark as FIXED?
Comment 119•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/66710af05aa1
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•