320887 - |var x| can throw a ReferenceError

Status: RESOLVED DUPLICATE of bug 1217099

(Core :: JavaScript Engine, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 1

[Bob Clary [:bc] (inactive)]

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

Comment 2

[Bob Clary [:bc] (inactive)]

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

Comment 3

[Brian Crowder]

Obscure enough to not block on.

Comment 4

[Brian Crowder]

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

Comment 5

[Bob Clary [:bc] (inactive)]

bug 446026 on tracemonkey unhides this bug.

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Tom S [:evilpie]]

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

Comment 8

[Jeff Walden [:Waldo]]

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

Comment 9

[Brendan Eich [:brendan]]

Attachment: quick fix attempt

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

Comment 10

[Jeff Walden [:Waldo]]

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

Comment 11

[Brendan Eich [:brendan]]

Attachment: A bit simplified, as hoped (more possible in this bug's scope?)

Comment 12

[Brendan Eich [:brendan]]

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

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: must satisfy the full PNX_POPVAR contract (all combos)

Comment 14

[Tom S [:evilpie]]

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

Comment 15

[Brendan Eich [:brendan]]

(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

Comment 16

[Tom S [:evilpie]]

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 17

[Tom S [:evilpie]]

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 18

[Tom S [:evilpie]]

Comment on attachment 714290 [details] [diff] [review]
must satisfy the full PNX_POPVAR contract (all combos)

Jason said he would review this patch.

Comment 19

[Jason Orendorff [:jorendorff]]

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
...

Comment 21

[Jason Orendorff [:jorendorff]]

Duping forward to the bug that finally fixes this.