Last Comment Bug 765280 - Assertion failure: blockDepth < ss->top, at jsopcode.cpp:4127
: Assertion failure: blockDepth < ss->top, at jsopcode.cpp:4127
Status: RESOLVED FIXED
[js:t]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla16
Assigned To: :Benjamin Peterson
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-06-15 09:47 PDT by Christian Holler (:decoder)
Modified: 2013-01-19 13:59 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (3.53 KB, patch)
2012-06-15 11:21 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
now with explicit comparison (3.53 KB, patch)
2012-06-19 14:30 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-06-15 09:47:20 PDT
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 2012-06-15 11:21:41 PDT
Created attachment 633592 [details] [diff] [review]
fix
Comment 2 Jason Orendorff [:jorendorff] 2012-06-19 14:05:41 PDT
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 2012-06-19 14:29:11 PDT
(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 2012-06-19 14:30:15 PDT
Created attachment 634591 [details] [diff] [review]
now with explicit comparison
Comment 5 Jason Orendorff [:jorendorff] 2012-06-20 01:05:34 PDT
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] 2012-06-20 13:51:15 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f572193f64e9
Comment 7 Ed Morley [:emorley] 2012-06-21 04:05:47 PDT
https://hg.mozilla.org/mozilla-central/rev/f572193f64e9
Comment 8 Christian Holler (:decoder) 2012-06-28 17:19:39 PDT
Testing some in-testsuite automation :)
Comment 9 Christian Holler (:decoder) 2013-01-19 13:59:03 PST
Automatically extracted testcase for this bug was committed:

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

Note You need to log in before you can comment on or make changes to this bug.