Closed Bug 355635 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: jruderman, Assigned: brendan)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, verified1.8.1.1)

Attachments

(1 file, 1 obsolete file)

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.
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
Group: security
Attached patch fix (obsolete) — Splinter Review
This special case is required by the empty stack (script depth of 0) condition.

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #241405 - Flags: review?(mrbkap)
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.
Attachment #241405 - Flags: review?(mrbkap) → review+
(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
Blocks: js1.7src
(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.
(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
Attached patch fix, v2Splinter Review
Attachment #241427 - Flags: review?(mrbkap)
OS: Mac OS X 10.4 → All
Priority: -- → P1
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 241427 [details] [diff] [review]
fix, v2

Sure, consistency is good too.
Attachment #241427 - Flags: review?(mrbkap) → review+
Attachment #241405 - Attachment is obsolete: true
Attachment #241405 - Flags: review+
Attachment #241427 - Flags: approval1.8.1.1?
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: blocking1.8.1.1?
Resolution: --- → FIXED
Checking in regress-355635.js;
/cvsroot/mozilla/js/tests/js1_7/regress/regress-355635.js,v  <--  regress-355635.js
initial revision: 1.1
done
Flags: in-testsuite+
verified fixed 1.9 20061007 windows/linux
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment on attachment 241427 [details] [diff] [review]
fix, v2

approved for 1.8 branch, a=dveditz for drivers
Attachment #241427 - Flags: approval1.8.1.1? → approval1.8.1.1+
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
Keywords: fixed1.8.1.1
verified fixed 20061122 1.8.1.1 windows/linux/mac*, 1.9 windows/linux
No longer blocks: 349611
Blocks: 349611
You need to log in before you can comment on or make changes to this bug.