379483 - "Assertion failure: top < ss->printer->script->depth" with destructuring catch

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

Debug:

js> function () { try { } catch([e]) { [1]; } }
Assertion failure: top < ss->printer->script->depth, at jsopcode.c:871


Opt:

js> function () { try { } catch([e]) { [1]; } }
out of memory


This is a recent regression.  I'm guessing bug 375794 caused it.

Comment 1

[Brendan Eich [:brendan]]

Disassembly, with stack depth after the op on the right:

flags: LAMBDA INTERPRETED
main:
00000:  try                                             0
00001:  goto 34 (33)                                    0
00004:  setsp 0                                         0
00007:  enterblock depth 0 {e: 0}                       1
00010:  exception                                       2
00011:  dup                                             3
00012:  zero                                            4
00013:  getelem                                         3
00014:  setlocalpop 0                                   2
00017:  pop                                             1
00018:  callname "Array"                                3
00021:  newinit                                         2
00022:  zero                                            3
00023:  one                                             4
00024:  initelem                                        2
00025:  endinit                                         2
00026:  pop                                             1
00027:  leaveblock 1                                    0
00030:  goto 34 (4)                                     0
00033:  nop                                             0
00034:  stop

Source notes:
  0:     1 [   1] hidden  
  1:     7 [   6] catch   
  3:    11 [   4] decl     offset 2
  5:    27 [  16] xdelta  
  6:    27 [   0] catch    stack depth 1
  8:    30 [   3] hidden  
  9:    33 [   3] endbrace

The stack budget balances, the bytecode is well-formed. But by the time the decompiler reaches bytecode offset 23 (JSOP_ONE), the decompiler's model stack is full (4 deep).

Debugging the JSOP_ENTERBLOCK case in the decompiler, it's clear that the destructuring catch head stack management is wrong. It just recently changed to push /*EXCEPTION*/ (the exception_cookie) for the JSOP_EXCEPTION it handles, but the destructuring case never pops that cookie. Assume the stack has depth 2 at this point (the block containing e, and the exception_cookie), and contrast with the non-destructuring catch variable else clause:

#if JS_HAS_DESTRUCTURING
                    if (*pc == JSOP_DUP) {
                        pc = DecompileDestructuring(ss, pc, endpc);
                        if (!pc) {
                            ok = JS_FALSE;
                            goto enterblock_out;
                        }
                        LOCAL_ASSERT(*pc == JSOP_POP);
                        pc += JSOP_POP_LENGTH;
                        lval = PopStr(ss, JSOP_NOP);
                        js_puts(jp, lval);
                    } else {
#endif
                        LOCAL_ASSERT(*pc == JSOP_SETLOCALPOP);
                        i = GET_UINT16(pc);
                        pc += JSOP_SETLOCALPOP_LENGTH;
                        (void) PopOff(ss, JSOP_NOP);
                        atom = atomv[i - OBJ_BLOCK_DEPTH(cx, obj)];
                        str = ATOM_TO_STRING(atom);
                        if (!QuoteString(&jp->sprinter, str, 0)) {
                            ok = JS_FALSE;
                            goto enterblock_out;
                        }
#if JS_HAS_DESTRUCTURING
                    }
#endif

The else clause pops (JSOP_SETLOCALPOP => PopOff) the just-pushed (offscreen of the cited code above) exception cookie. The destructuring "then" clause (#if JS_HAS_DESTRUCTURING) calls DecompileDestructuring, OTOH, which leaves handles the JSOP_DUP and leaves the resulting destructuring pattern on top of stack where the dup copied the exception object (in the interpreter; abstractly here), so the exception cookie is still left on-stack.

So the fix is to PopOff(ss, JSOP_NOP) in both cases, not just in the else case.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Comment 3

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsopcode.c 3.231

/be

Comment 4

[georgi - hopefully not receiving bugspam]

is this expected/by design?

javascript:try { throw 2} catch(Function) {alert("func="+Function);var s=new Function();} ;alert('done') 

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

It is, you're allowed to introduce whatever name you want to, it shadows any other names.

Comment 6

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/regress/regress-379483.js,v  <--  regress-379483.js
initial revision: 1.1

Comment 7

[Bob Clary [:bc] (inactive)]

js1_7/regress/regress-379483.js passes but js1_5/extensions/regress-346494-01.js and js1_5/extensions/regress-350312-03.js have the same assert and the same regression range as bug 375794.

-> reopen

Comment 8

[Brendan Eich [:brendan]]

js> function () { try { } catch([e]) { [1]; } }
function () {
    try {
    } catch ([e]) {
        [1];
    }
}

And js1_5/extensions/regress-346494-01.js and js1_5/extensions/regress-350312-03.js do not assertbotch, and seeming pass (they are chatty, aren't they?).

WFM. Someone with more time than I have please pinpoint the fix.

/be