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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[:Benjamin Peterson]

Attachment: fix

Comment 2

[Jason Orendorff [: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.

Comment 3

[:Benjamin Peterson]

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

Comment 4

[:Benjamin Peterson]

Attachment: now with explicit comparison

Comment 5

[Jason Orendorff [:jorendorff]]

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.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f572193f64e9

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f572193f64e9

Comment 8

[Christian Holler (:decoder)]

Testing some in-testsuite automation :)

Comment 9

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929