355635 - "Assertion failure: top < ss->printer->script->depth" with "let" that binds nothing

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> function () { let ([] = []) { } }
Assertion failure: top < ss->printer->script->depth, at jsopcode.c:849

In an opt jsshell, I get "out of memory" instead.

Marking security-sensitive out of paranoia; please make this public if it should be public.

Comment 1

[Brendan Eich [:brendan]]

There's no need to mark this security-sensitive, it's just a bug that's defended against in release builds with an "out of memory" error.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

This special case is required by the empty stack (script depth of 0) condition.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 241405 [details] [diff] [review]
fix

I'm willing to accept the assertion the comment makes, but it seems like a *very* large stretch to make.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (From update of attachment 241405 [details] [diff] [review] [edit])
> I'm willing to accept the assertion the comment makes, but it seems like a
> *very* large stretch to make.

It's not just the comment -- the LOCAL_ASSERT insists.  The only way you can have an empty block (stack top up against script depth) is with empty destructuring.  The only way you get into this part of JSOP_SETSP's decompiling code is with group assignment in a let head.  Therefore....

I wondered whether you might object to the "{}" all on one line!

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #4)
> It's not just the comment -- the LOCAL_ASSERT insists.

Well, the comment not botching relies on the assert not firing, or something ;-).

> I wondered whether you might object to the "{}" all on one line!

I think it looks neater that way.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> > I wondered whether you might object to the "{}" all on one line!
> 
> I think it looks neater that way.

js> function f(){let([]=[]){}}
js> f
function f() {
    let ([] = []) {}
}
js> function g(){let([x]=[]){}}
js> g
function g() {
    let ([x] = []) {
    }
}

Is it worthwhile to be consistent?  I think so -- alterna-patch in a few.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 241427 [details] [diff] [review]
fix, v2

Sure, consistency is good too.

Comment 9

[Brendan Eich [:brendan]]

Fixed on trunk:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.192; previous revision: 3.191
done

/be

Comment 10

[Bob Clary [:bc] (inactive)]

Checking in regress-355635.js;
/cvsroot/mozilla/js/tests/js1_7/regress/regress-355635.js,v  <--  regress-355635.js
initial revision: 1.1
done

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061007 windows/linux

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 241427 [details] [diff] [review]
fix, v2

approved for 1.8 branch, a=dveditz for drivers

Comment 13

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.65; previous revision: 3.89.2.64
done

/be

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 20061122 1.8.1.1 windows/linux/mac*, 1.9 windows/linux