The default bug view has changed. See this FAQ.

Assertion failure: blockDepth < ss->top, at jsopcode.cpp:4127

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: Benjamin)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla16
x86_64
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
The following test asserts on mozilla-central revision 85e31a4bdd41 (options -m -n):


function f(x = let([] = c) 1, ... __call__)  {}
assertEq(this > f, true);


Found by adding combined support for Harmony's parameter default values and rest parameter to the LangFuzz ES3 grammar.
(Assignee)

Comment 1

5 years ago
Created attachment 633592 [details] [diff] [review]
fix
Assignee: general → bpeterson
Attachment #633592 - Flags: review?(jorendorff)
Comment on attachment 633592 [details] [diff] [review]
fix

Review of attachment 633592 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsopcode.cpp
@@ +5453,5 @@
>  
> +    for (unsigned i = 0; i < initialStackDepth; i++) {
> +        if (!PushStr(&ss, "", JSOP_NOP))
> +            return NULL;
> +    }

Hmm. Why not do this in 'case JSOP_REST:'? Wouldn't that avoid the extra argument to DecompileCode? The Tao of the decompiler is for its stack slots to mimic what the interpreter would do, so the code in case JSOP_REST seems wrong to me.

@@ +5462,5 @@
>      bool ok = Decompile(&ss, pc, len) != NULL;
>      jp->script = oldscript;
>  
>      /* If the given code didn't empty the stack, do it now. */
> +    if (ok && ss.top - initialStackDepth) {

This seems like an implicit conversion too far to me. IOCCC flashbacks. ;) Assuming you don't rewrite this part of the patch, please use != or > instead of - here, or else add a courtesy "> 0" at the end.
Attachment #633592 - Flags: review?(jorendorff)
(Assignee)

Comment 3

5 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Comment on attachment 633592 [details] [diff] [review]
> fix
> 
> Review of attachment 633592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsopcode.cpp
> @@ +5453,5 @@
> >  
> > +    for (unsigned i = 0; i < initialStackDepth; i++) {
> > +        if (!PushStr(&ss, "", JSOP_NOP))
> > +            return NULL;
> > +    }
> 
> Hmm. Why not do this in 'case JSOP_REST:'? Wouldn't that avoid the extra
> argument to DecompileCode? The Tao of the decompiler is for its stack slots
> to mimic what the interpreter would do, so the code in case JSOP_REST seems
> wrong to me.

That's a good idea, but we run Decompile "manually" when doing defaults and rest. So, all the stack state is lost across the Decompile call for each argument. When we go to decompile the body, the defaults/rest related opcodes are just skipped, which is what "JSOP_REST" does.

> 
> @@ +5462,5 @@
> >      bool ok = Decompile(&ss, pc, len) != NULL;
> >      jp->script = oldscript;
> >  
> >      /* If the given code didn't empty the stack, do it now. */
> > +    if (ok && ss.top - initialStackDepth) {
> 
> This seems like an implicit conversion too far to me. IOCCC flashbacks. ;)
> Assuming you don't rewrite this part of the patch, please use != or >
> instead of - here, or else add a courtesy "> 0" at the end.

Okay.
(Assignee)

Comment 4

5 years ago
Created attachment 634591 [details] [diff] [review]
now with explicit comparison
Attachment #633592 - Attachment is obsolete: true
Attachment #634591 - Flags: review?(jorendorff)
Whiteboard: js-triage-needed → [js:t]
Comment on attachment 634591 [details] [diff] [review]
now with explicit comparison

> That's a good idea, but we run Decompile "manually" when doing defaults and
> rest. So, all the stack state is lost across the Decompile call for each
> argument.

Right, that makes sense.
Attachment #634591 - Flags: review?(jorendorff) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/f572193f64e9
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/f572193f64e9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 8

5 years ago
Testing some in-testsuite automation :)
Flags: in-testsuite?
(Reporter)

Comment 9

4 years ago
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.