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

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.

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.

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

(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

(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

(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

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

(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

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

Patch landed on trunk. /be

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

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

Comment on attachment 233159 [details] [diff] [review] the full code generator fix sr=shaver That code makes me nostalgic!

Fixed on the 1.8 branch too. /be

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

verified fixed 1.8 20060818 windows/mac*/linux

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.

Required hand merging. Also, did the fix to this bug introduce bug 349592 ? bc or jesse could say. /be

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

(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

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 on attachment 235175 [details] [diff] [review] 1.8.0 branch version of patch -- this needs testing Merging is hard.

Comment on attachment 235290 [details] [diff] [review] fixed 1.8.0 branch patch I assent to this patch. r=shaver

Comment on attachment 235290 [details] [diff] [review] fixed 1.8.0 branch patch a=dveditz for 1.8.0

Fixed on the 1.8.0 branch. /be

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

verified fixed 1.8.0.7 20060827 windows/mac*/linux