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

VERIFIED FIXED in mozilla1.8.1beta2

Status

()

defect
P1
major
VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: bc, Assigned: brendan)

Tracking

(4 keywords)

Trunk
mozilla1.8.1beta2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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
Keywords: crash
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.
Flags: blocking1.8.1?
Flags: blocking1.8.1? → blocking1.8.1+
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
Assignee: general → brendan
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1beta2
Status: NEW → ASSIGNED
Hardware: PC → All
(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
Blocks: 346029
(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
Attachment #233159 - Flags: superreview?(shaver)
Attachment #233159 - Flags: review?(mrbkap)
Attachment #233159 - Flags: review?(mrbkap) → review+
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
Attachment #233159 - Flags: approval1.8.1?
Patch landed on trunk.

/be
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: blocking1.8.0.7?
Resolution: --- → FIXED
Flags: in-testsuite+
Flags: blocking1.8.0.7? → blocking1.8.0.7+
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
Attachment #233159 - Flags: approval1.8.1? → approval1.8.1+
verified fixed 1.9 20060811 windows/mac(ppc|tel)/linux
Status: RESOLVED → VERIFIED
Comment on attachment 233159 [details] [diff] [review]
the full code generator fix

sr=shaver

That code makes me nostalgic!
Attachment #233159 - Flags: superreview?(shaver) → superreview+
Fixed on the 1.8 branch too.

/be
Keywords: fixed1.8.1
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.
Attachment #233159 - Flags: approval1.8.0.8+
Required hand merging.

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

/be
Attachment #235175 - Flags: review+
Attachment #235175 - Flags: approval1.8.0.7?
(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
Attachment #233159 - Flags: approval1.8.0.8+
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
Attachment #235175 - Flags: approval1.8.0.7? → approval1.8.0.7+
(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
Attachment #235175 - Attachment is obsolete: true
Attachment #235290 - Flags: review?(shaver)
Attachment #235290 - Flags: approval1.8.0.7?
Comment on attachment 235175 [details] [diff] [review]
1.8.0 branch version of patch -- this needs testing

Merging is hard.
Attachment #235175 - Flags: review-
Attachment #235175 - Flags: review+
Attachment #235175 - Flags: approval1.8.0.7-
Attachment #235175 - Flags: approval1.8.0.7+
Comment on attachment 235290 [details] [diff] [review]
fixed 1.8.0 branch patch

I assent to this patch. r=shaver
Attachment #235290 - Flags: review?(shaver) → review+
Comment on attachment 235290 [details] [diff] [review]
fixed 1.8.0 branch patch

a=dveditz for 1.8.0
Attachment #235290 - Flags: approval1.8.0.7? → approval1.8.0.7+
Fixed on the 1.8.0 branch.

/be
Keywords: fixed1.8.0.7
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
Depends on: 350312
verified fixed 1.8.0.7 20060827 windows/mac*/linux
Blocks: 350553
No longer blocks: 350553
You need to log in before you can comment on or make changes to this bug.