346494 - JS_ASSERT(JSVAL_IS_INT(rval)) - js1_5/Regress/regress-104077.js bug: none result: FAILED type: BROWSER|SHELL

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

I show a crash in 1.9 20060728 builds on windows/mac(ppc|tel)/linux

On Windows Debug browser I get:

          BEGIN_CASE(JSOP_RETSUB)
            rval = POP();
=>            JS_ASSERT(JSVAL_IS_INT(rval));
            len = JSVAL_TO_INT(rval);
            pc = script->main;
          END_VARLEN_CASE

		rval	70403432	long

>	js3250.dll!js_Interpret(JSContext * cx=0x03f2d210, unsigned char * pc=0x0505e1b6, long * result=0x0012f718)  Line 5506 + 0x27 bytes	C
 	js3250.dll!js_Execute(JSContext * cx=0x03f2d210, JSObject * chain=0x03ef9868, JSScript * script=0x0505df28, JSStackFrame * down=0x00000000, unsigned int flags=0, long * result=0x0012f844)  Line 1599 + 0x13 bytes	C
 	js3250.dll!JS_EvaluateUCScriptForPrincipals(JSContext * cx=0x03f2d210, JSObject * obj=0x03ef9868, JSPrincipals * principals=0x0390e55c, const unsigned short * chars=0x05032060, unsigned int length=11021, const char * filename=0x05002b90, unsigned int lineno=1, long * rval=0x0012f844)  Line 4330 + 0x19 bytes	C
 	gklayout.dll!nsJSContext::EvaluateString(const nsAString_internal & aScript={...}, void * aScopeObject=0x03ef9868, nsIPrincipal * aPrincipal=0x0390e558, const char * aURL=0x05002b90, unsigned int aLineNo=1, unsigned int aVersion=0, nsAString_internal * aRetValue=0x00000000, int * aIsUndefined=0x0012f92c)  Line 1298 + 0x43 bytes	C++
 	gklayout.dll!nsScriptLoader::EvaluateScript(nsScriptLoadRequest * aRequest=0x04ffe6b8, const nsString & aScript={...})  Line 800 + 0x63 bytes	C++
 	gklayout.dll!nsScriptLoader::ProcessRequest(nsScriptLoadRequest * aRequest=0x04ffe6b8)  Line 704 + 0x13 bytes	C++
 	gklayout.dll!nsScriptLoader::OnStreamComplete(nsIStreamLoader * aLoader=0x05003080, nsISupports * aContext=0x04ffe6b8, unsigned int aStatus=0, unsigned int stringLen=11021, const unsigned char * string=0x05053630)  Line 1068	C++
...

But I crash in opt builds as well, so this isn't just an assert. This regressed since 20060725

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

A minimal testcase is:
try {
    throw "outer throw";
} catch (e) {
} finally {
    try {
        throw "inner throw";
    } catch (e) {
    } finally {
    }
}

when we reach the inner JSOP_RETSUB, the top thing on the stack is the actual exception, the next thing down looks like the length that the code is expecting. I bet this is a regression from bug 346029.

Comment 2

[Bob Clary [:bc] (inactive)]

happens with 1.8 2006080805 builds as well. again the crash also occurs in opt builds so it is not just a debug assert. nominating blocking 1.8.1 to get on the radar.

Comment 3

[Brendan Eich [:brendan]]

Blake and I were poking around.  We have to fix the regression from bug 346029 ASAP, but this requires dealing with guarded catch clauses, where the code generator cannot model the stack depth fully -- cc'ing shaver.  The cases are:

1.  try-finally -- fixed by the patch for bug 346029, but that patch broke case 2.

2.  try-catch-finally -- this bug.

3.  try-catch(guard)-finally -- the finally code generator can't know whether the catch consumed the exception, or whether the exception needs to be saved on the stack and re-thrown after normal completion of the finally in order to propagate the exception.  This case was always broken, even before the patch for bug 346029:

js> function f() {
    try {
        throw "foo";
    } catch (e if e == "bar") {
    } finally {
    }
}
js> f()
uncaught exception: [object Object]

The bogus object on the stack is With(new Object) created for the catch clause's scope.  In this case, but not in the catch-all-after-guarded-catch case, the code generator fails to emit an unbalanced JSOP_LEAVEWITH when the guard expression evaluates to false.

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> The bogus object on the stack is With(new Object) created for the catch
> clause's scope.  In this case, but not in the catch-all-after-guarded-catch

Or in any catch-after-guarded-catch, I mean.  You can get this bogus With object on the stack with any number of guarded catches, so long as all the guards fail to match.  A catch-all at the end fixes things, and a guarded catch after a (required to be) guarded catch emits the unbalanced LEAVEWITH for the earlier catch, but nothing saves the later one unless there's a finall catch-all.  Witness:

js> function g(){try{throw "foo"}catch(e if e == "bar"){}catch(e if e == "baz"){}finally{}}
js> g()
uncaught exception: [object Object]
js> g
Assertion failure: (uintN) js_GetSrcNoteOffset(sn, 0) == ss->top, at jsopcode.c:1421
Trace/BPT trap
Yoyodyne:~/Hacking/trunk/mozilla/js/src brendaneich$ ./Darwin_DBG.OBJ/js
js> function h(){try{throw "foo"}catch(e if e == "bar"){}catch(e){}finally{}}
js> function h(){try{throw "foo"}catch(e if e == "bar"){}catch(e){}finally{}}
js> h()
js> h
Assertion failure: (uintN) js_GetSrcNoteOffset(sn, 0) == ss->top, at jsopcode.c:1421
Trace/BPT trap

Note also the decompiler bug. :-(

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (In reply to comment #3)
> > The bogus object on the stack is With(new Object) created for the catch
> > clause's scope.  In this case, but not in the catch-all-after-guarded-catch
> 
> Or in any catch-after-guarded-catch, I mean.

Oops, I was right in my original statement (cited with > > above), because the guarded-catch-with-no-catch-all-at-the-end case gets an automatically-generated catch-all-gosub-finally-and-rethrow code sequence (the one revised by bug 346029, which goes [setsp, exception, gosub, throw] now that that bug is fixed, to keep the exception safe on the stack, instead of leaving it pending in cx, which was the bad bug fixed by 346029).

So the decompiler bug is a separate problem beyond the one reported here.

So is the missing LEAVEWITH for the last (or only) guarded catch with no unguarded catch after it.

I would like to fix the LEAVEWITH bug here, but mrbkap has kindly offered to spin out the decompiler bug.

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > The bogus object on the stack is With(new Object) created for the catch
> > > clause's scope.  In this case, but not in the catch-all-after-guarded-catch
> > 
> > Or in any catch-after-guarded-catch, I mean.
> 
> Oops, I was right in my original statement (cited with > > above), because the
> guarded-catch-with-no-catch-all-at-the-end case gets an automatically-generated
> catch-all-gosub-finally-and-rethrow code sequence (the one revised by bug
> 346029, which goes [setsp, exception, gosub, throw]

But, alas, I was wrong in my original statement for a different reason.  The branch on false guard condition does not jump to the setsp in the [setsp, exception, gosub, throw] sequence -- it jumps to the gosub!  D'oh.

So the try-catch(guard)-finally case is busted too.

Working on a patch for all of these code generation bugs now.  mrbkap should cite the decompiler spin-off bug shortly.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> Working on a patch for all of these code generation bugs now.  mrbkap should
> cite the decompiler spin-off bug shortly.

That's now bug 348273.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> The
> branch on false guard condition does not jump to the setsp in the [setsp,
> exception, gosub, throw] sequence -- it jumps to the gosub!  D'oh.

Fixing this by targeting the jump at the setsp (the start of the rethrow sequence) also fixes the missing LEAVEWITH, since setsp trims the scope chain.  So this is easy to fix:

Index: jsemit.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsemit.c,v
retrieving revision 3.166
diff -p -u -8 -r3.166 jsemit.c
--- jsemit.c    1 Aug 2006 01:49:22 -0000       3.166
+++ jsemit.c    10 Aug 2006 22:18:53 -0000
@@ -4499,28 +4499,34 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
             /*
              * Emit another stack fixup, because the catch could itself
              * throw an exception in an unbalanced state, and the finally
              * may need to call functions.  If there is no finally, only
              * guarded catches, the rethrow code below nevertheless needs
              * stack fixup.
              */
             finallyCatch = CG_OFFSET(cg);
+
+            /*
+             * Last discriminant jumps to the rethrow code sequence if no
+             * discriminants match.  Target catchJump at the beginning of the
+             * rethrow sequence, just in case a guard expression throws and
+             * leaves the stack unbalanced.
+             */
+            if (catchJump != -1 && iter->pn_kid1->pn_expr)
+                CHECK_AND_SET_JUMP_OFFSET_AT(cx, cg, catchJump);
+
             EMIT_UINT16_IMM_OP(JSOP_SETSP, (jsatomid)depth);
             cg->stackDepth = depth;
 
             if (js_NewSrcNote(cx, cg, SRC_HIDDEN) < 0 ||
                 js_Emit1(cx, cg, JSOP_EXCEPTION) < 0) {
                 return JS_FALSE;
             }
 
-            /* Last discriminant jumps to rethrow if none match. */
-            if (catchJump != -1 && iter->pn_kid1->pn_expr)
-                CHECK_AND_SET_JUMP_OFFSET_AT(cx, cg, catchJump);
-
             if (pn->pn_kid3) {
                 jmp = EmitBackPatchOp(cx, cg, JSOP_BACKPATCH_PUSH,
                                       &stmtInfo.gosub);
                 if (jmp < 0)
                     return JS_FALSE;
 
                 /*
                  * Exception and retsub pc index are modeled as on the stack.

Full patch, which will also fix the try-catch-finally case, coming soon.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: the full code generator fix

This fixes the catchJump to target the rethrow sequence, as described in the last comment (which avoids the need for an unbalanced leavewith), and fixes the finally clause code generator to add 1, not 2, to the model stack depth when handling a try-catch-finally or try-catch(guard)...-catch-finally (since the unconditional catch consumed the exception and it won't be on stack under the retsub pc-index).

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 233159 [details] [diff] [review]
the full code generator fix

I'm landing with r=mrbkap, shaver said he didn't have time to review today.  This should go into 1.8.1, I think before Firefox 2 beta 2 -- this bug could allow control of JS bytecode execution, and there's a heap overflow hiding here too.

/be

Comment 11

[Brendan Eich [:brendan]]

Patch landed on trunk.

/be

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 233159 [details] [diff] [review]
the full code generator fix

a=mconnor on behalf of drivers for 1.8 branch, pending shaver's SR

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060811 windows/mac(ppc|tel)/linux

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 233159 [details] [diff] [review]
the full code generator fix

sr=shaver

That code makes me nostalgic!

Comment 15

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch too.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

Checking in regress-346494.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-346494.js,v  <-- 
regress-346494.js
initial revision: 1.1

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060818 windows/mac*/linux

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 233159 [details] [diff] [review]
the full code generator fix

approved for 1.8.0 branch, a=dveditz for drivers

Please land soon.

Comment 19

[Brendan Eich [:brendan]]

Attachment: 1.8.0 branch version of patch -- this needs testing

Required hand merging.

Also, did the fix to this bug introduce bug 349592 ?  bc or jesse could say.

/be

Comment 20

[Bob Clary [:bc] (inactive)]

(In reply to comment #19)
> 
> Also, did the fix to this bug introduce bug 349592 ? 

I reverted js/src back to 2006-08-10 16:00 PDT with jsemit.c at 3.166 and still saw bug 349592

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 235175 [details] [diff] [review]
1.8.0 branch version of patch -- this needs testing

approved for 1.8.0 branch, a=dveditz for drivers

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #20)
> (In reply to comment #19)
> > 
> > Also, did the fix to this bug introduce bug 349592 ? 
> 
> I reverted js/src back to 2006-08-10 16:00 PDT with jsemit.c at 3.166 and still
> saw bug 349592

Thanks.  Any 1.8.0 test news on this patch?  I haven't had time to pull and build.

/be 

Comment 23

[Brendan Eich [:brendan]]

Attachment: fixed 1.8.0 branch patch

Best way to verify is to diff jsemit.c with this patch applied against the 1.8 branch and look for "case JSOP_TRY:" -- the whole try/catch/finally code generator matches now (and will not when the lexical scope and destructuring catch changes land from the trunk -- so diff quickly).

Soliciting shaver, who knows this code, cuz mrbkap is still on the road.

/be

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 235175 [details] [diff] [review]
1.8.0 branch version of patch -- this needs testing

Merging is hard.

Comment 25

[Brendan Eich [:brendan]]

Attachment: 1.8 branch on the left, 1.8.0 + the last attachment on the right, JSOP_TRY case diff excerpt

Comment 26

[Mike Shaver (:shaver emeritus)]

Comment on attachment 235290 [details] [diff] [review]
fixed 1.8.0 branch patch

I assent to this patch. r=shaver

Comment 27

[Brendan Eich [:brendan]]

Attachment: simple test of all combinations up to two catch guards

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 235290 [details] [diff] [review]
fixed 1.8.0 branch patch

a=dveditz for 1.8.0

Comment 29

[Brendan Eich [:brendan]]

Fixed on the 1.8.0 branch.

/be

Comment 30

[Bob Clary [:bc] (inactive)]

Checking in regress-346494-01.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-346494-01.js,v  <--  regress-346494-01.js
initial revision: 1.1

ubfunky

Comment 31

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.7 20060827 windows/mac*/linux