STEPS TO REPRODUCE: 1. Try the testcase in the URL field. 2. Note that there is a JavaScript exception. Shutdown pointed out in bug 320172 comment 13 that this is incorrect behavior, per ECMA. We're failing because we emit a JSOP_NAME bytecode for the decompiler, and the lookup fails.

Checking in regress-320887.js; /cvsroot/mozilla/js/tests/js1_6/Array/regress-320887.js,v <-- regress-320887.js initial revision: 1.1

note to self: this crashes the 1.8 branch with top 'o stack: Variables(JSContext * 0x03a4a818, JSTokenStream * 0x03df3378, JSTreeContext * 0x0012e81c) line 2164 + 3 bytes Fixed by Bug 320172

Obscure enough to not block on.

A fix for bug 442333 hides this bug, we need a better testcase.

bug 446026 on tracemonkey unhides this bug.

Here's a test case. Object.defineProperty(this, "x", {get: function () { FAIL; }}); eval("var x;"); // throws FAIL Clearly against ES5. Waldo says we emit the erroneous JSOP_GET* opcode here for the decompiler's benefit. That being the case this will be a bit of a pain to fix.

This is ugly, somebody should have a look at this.

The decompiler has gone the way of the dodo, so maybe this is easier to fix now. Anyone want to try?

Here's a quick attempt at a fix. The removal of the statement decompiler did not remove unnecessary source notes, in this case for decompiling var x, y=42, z; and the like. Jeff, could you please take this bug and further simplify the code if possible, and do whatever else is needed for the given testcase (jorendorff's). /be

Wish I could, but I'm swamped with other stuff at the moment. :-)

Evilpies, how about you? Free patch if you call from work or your parents house :-P. /be

I started looking at the code and your patch. But variable names like pn3 are annoying to work with.

(In reply to Tom Schuster [:evilpie] from comment #14) > I started looking at the code and your patch. But variable names like pn3 > are annoying to work with. (a) That was already there, I just moved the declaration. (b) Our job is to make the engine better in minimum viable patches that don't get stalled by stop-energy complaints about variable names. Either fix it in a patch revision, or at least mention it _en passant_ while adding greater value than your comment does. :-( (c) It's *this bug's patch*, not "your patch" or "my patch". How about another look? /be

From my point of view the patch is okay, but I already asked Jason about this bug yesterday, and he agreed to review this.

Comment on attachment 714290 [details] [diff] [review] must satisfy the full PNX_POPVAR contract (all combos) Review of attachment 714290 [details] [diff] [review]: ----------------------------------------------------------------- I just noticed that I forgot to submit this review. ;) ::: js/src/frontend/BytecodeEmitter.cpp @@ +3055,5 @@ > { > JS_ASSERT(pn->isArity(PN_LIST)); > JS_ASSERT(isLet == (emitOption == PushInitialValues)); > > + ParseNode *pn3 = NULL; Initializer maybe? @@ +3232,5 @@ > if (pn->pn_xflags & PNX_POPVAR) { > + if (pn3 && Emit1(cx, bce, JSOP_POP) < 0) > + return false; > + } else { > + if (!pn3 && Emit1(cx, bce, JSOP_UNDEFINED) < 0) This or the whole if {} block need a comment about what it's doing. This !pn3 check seems to encode that the last variable had an initialize, I am wondering if we could do this a little bit cleaner and move pn3 inside the loop.

Comment on attachment 714290 [details] [diff] [review] must satisfy the full PNX_POPVAR contract (all combos) Jason said he would review this patch.

Comment on attachment 714290 [details] [diff] [review] must satisfy the full PNX_POPVAR contract (all combos) Review of attachment 714290 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review flag. ::: js/src/frontend/BytecodeEmitter.cpp @@ +3055,5 @@ > { > JS_ASSERT(pn->isArity(PN_LIST)); > JS_ASSERT(isLet == (emitOption == PushInitialValues)); > > + ParseNode *pn3 = NULL; I agree with Tom about calling this lastInit or something. Your call. @@ -3233,5 @@ > if (!next) > break; > - off = tmp; > - noteIndex = NewSrcNote2(cx, bce, SRC_PCDELTA, 0); > - if (noteIndex < 0 || Emit1(cx, bce, JSOP_POP) < 0) Thanks for removing this. Filed follow-up bug 853178 to remove other unused PCDELTA notes. @@ +3229,1 @@ > return false; All this has gotten messy over the past few patches that touched it; would you mind recasting lines 3207-3229 as: if (pn3 && emitOption == InitializeVars) { JS_ASSERT_IF(pn2->isDefn(), pn3 == pn2->pn_expr); if (!pn2->pn_cookie.isFree()) { if (!EmitVarOp(cx, pn2, op, bce)) return false; } else { if (!EmitIndexOp(cx, op, atomIndex, bce)) return false; } #if JS_HAS_DESTRUCTURING emit_note_pop: #endif if (next) { if (Emit1(cx, bce, JSOP_POP) < 0) return false; } } This eliminates all the break or continue statements, but you would also have to add a condition in the loop head: >- for (ParseNode *pn2 = pn->pn_head; ; pn2 = next) { >+ for (ParseNode *pn2 = pn->pn_head; pn2; pn2 = next) { I can do this in a follow-up bug if you just want to get this landed. @@ +3229,5 @@ > return false; > } > > if (pn->pn_xflags & PNX_POPVAR) { > + if (pn3 && Emit1(cx, bce, JSOP_POP) < 0) I think something must be not quite right here, because the condition here does not match what happens in the loop. And a lot of tests do fail. For example, this test case asserts: for (let c in Array) {} dis(); with the following bytecode: main: 00000: undefined 00001: undefined 00002: getgname "Array" 00007: iter 1 00009: enterlet1 depth 1 {c: 0} 00014: goto 25 (+11) 00019: loophead 00020: iternext 00021: setlocal 1 00024: pop 00025: loopentry 00026: moreiter 00027: ifne 19 (-8) 00032: leaveforletin 00033: enditer 00034: popn 1 ...

Duping forward to the bug that finally fixes this.