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

()

P1
major
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: bc, Assigned: brendan)

Tracking

(4 keywords)

Trunk
mozilla1.8.1beta2
crash, regression, verified1.8.0.7, verified1.8.1
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)

(Reporter)

Description

12 years ago
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
(Reporter)

Updated

12 years ago
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.
(Reporter)

Comment 2

12 years ago
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?

Updated

12 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 3

12 years ago
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
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Hardware: PC → All
(Assignee)

Comment 4

12 years ago
(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

(Assignee)

Comment 5

12 years ago
(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
(Assignee)

Comment 6

12 years ago
(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.
(Assignee)

Comment 8

12 years ago
(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
(Assignee)

Comment 9

12 years ago
Created attachment 233159 [details] [diff] [review]
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
Attachment #233159 - Flags: superreview?(shaver)
Attachment #233159 - Flags: review?(mrbkap)
Attachment #233159 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 10

12 years ago
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?
(Assignee)

Comment 11

12 years ago
Patch landed on trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.8.0.7?
Resolution: --- → FIXED
(Reporter)

Updated

12 years ago
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+
(Reporter)

Comment 13

12 years ago
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+
(Assignee)

Comment 15

12 years ago
Fixed on the 1.8 branch too.

/be
Keywords: fixed1.8.1
(Reporter)

Comment 16

12 years ago
Checking in regress-346494.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-346494.js,v  <-- 
regress-346494.js
initial revision: 1.1
(Reporter)

Comment 17

12 years ago
verified fixed 1.8 20060818 windows/mac*/linux
Keywords: fixed1.8.1 → verified1.8.1
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+
(Assignee)

Comment 19

12 years ago
Created attachment 235175 [details] [diff] [review]
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
Attachment #235175 - Flags: review+
Attachment #235175 - Flags: approval1.8.0.7?
(Reporter)

Comment 20

12 years ago
(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+
(Assignee)

Comment 22

12 years ago
(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 

(Assignee)

Comment 23

12 years ago
Created attachment 235290 [details] [diff] [review]
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
Attachment #235175 - Attachment is obsolete: true
Attachment #235290 - Flags: review?(shaver)
Attachment #235290 - Flags: approval1.8.0.7?
(Assignee)

Comment 24

12 years ago
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+
(Assignee)

Comment 25

12 years ago
Created attachment 235293 [details] [diff] [review]
1.8 branch on the left, 1.8.0 + the last attachment on the right, JSOP_TRY case diff excerpt
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+
(Assignee)

Comment 27

12 years ago
Created attachment 235295 [details]
simple test of all combinations up to two catch guards
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+
(Assignee)

Comment 29

12 years ago
Fixed on the 1.8.0 branch.

/be
Keywords: fixed1.8.0.7
(Reporter)

Comment 30

12 years ago
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
(Assignee)

Updated

12 years ago
Depends on: 350312
(Reporter)

Comment 31

12 years ago
verified fixed 1.8.0.7 20060827 windows/mac*/linux
Keywords: fixed1.8.0.7 → verified1.8.0.7
(Reporter)

Updated

12 years ago
Blocks: 350553
(Assignee)

Updated

12 years ago
No longer blocks: 350553
You need to log in before you can comment on or make changes to this bug.