104077 - JS crash: with/finally/return.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chingfa Wu]

I checked out the lastest main branch source codes from both mozilla/js and 
mozilla/nsprpub on Sun Solaris 2.6(10/09/2001), built js following the 
instructions. But if there is a return statement in the finally block inside of 
with block, js always coredump at jsinterp.c. Please help ASAP.

Thanks!


js test.js
Assertion failure: JSVAL_IS_OBJECT(rval), at jsinterp.c:1395
Abort (core dumped)


The following is the test program:




function addValues(dialog)
{
 var sum;
 with (dialog)
  {
   try
    {
      // doing something else
      sum = arg1 + arg2;
      //return sum;
    }
   finally
    {
     //Print(" sum = " + sum);
     return sum;
    }
  }
}


var i;
var _nextDialog = new Object();
_nextDialog.arg1 = 1;
_nextDialog.arg2 = 2;

i = addValues(_nextDialog);

//Print(" i = " + i);

Comment 1

[Phil Schwartau]

Confirming crash in WinNT JS shell 2001-10-08. OS : Solaris --> All 

cc'ing Brendan on this one - 

Comment 2

[Brendan Eich [:brendan]]

Ouch.  We need this in our testsuite!  Thanks, chwu.  I'll diagnose.

/be

Comment 3

[Phil Schwartau]

Chingfa's testcase has been added to JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-104077.js

As reported, it crashes in both the debug and optimized SpiderMonkey shells.
Test passes in the rhino and rhinoi shells.

Comment 4

[Phil Schwartau]

Attachment: WinNT stack trace

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This is probably my fault.  Sucky.

Comment 6

[Brendan Eich [:brendan]]

I crave shaver's help.  Here's the disassembly of addValues:

 HEAVYWEIGHT
main:
00000:  getvar 0
00003:  pop
00004:  getarg 0
00007:  enterwith
00008:  try
00009:  bindname "sum"
00012:  name "arg1"
00015:  name "arg2"
00018:  add
00019:  setname "sum"
00022:  popv
00023:  gosub 37 (14)
00026:  goto 46 (20)
00029:  setsp 1
00032:  gosub 37 (5)
00035:  exception
00036:  throw
00037:  finally
00038:  name "sum"
00041:  swap
00042:  leavewith
00043:  return
00044:  retsub
00045:  nop
00046:  leavewith

Source notes:
  0:     0 [   0] newline 
  1:     0 [   0] var     
  2:     4 [   4] newline 
  3:     8 [   4] newline 
  4:     8 [   0] newline 
  5:     9 [   1] newline 
  6:     9 [   0] setline  lineno 9
  8:    23 [  14] xdelta  
  9:    23 [   0] hidden  
 10:    26 [   3] hidden  
 11:    35 [   9] xdelta  
 12:    35 [   0] hidden  
 13:    36 [   1] hidden  
 14:    37 [   1] setline  lineno 13
 16:    38 [   1] setline  lineno 15
 18:    42 [   4] hidden  
 19:    45 [   3] endbrace

Exception table:
start
end
catch
  9	32	32

The dead code after bytecode 26 (goto 46) is one problem.  The lack of a setsp
before the finally can safely do its leavewith at 42 (which is where we assert
and die in jsinterp.c) is another, or could it be that the setsp is misplaced?

/be
/be

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Exceptions are my baby.

Comment 8

[Phil Schwartau]

cc'ing Kenton - 

Comment 9

[Phil Schwartau]

OOPS - wrong field; try again -

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: swap-n-pop the retsub PC for non-local jumps, and misc tidying

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This baby better go for 0.9.6.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 54595 [details] [diff] [review]
swap-n-pop the retsub PC for non-local jumps, and misc tidying

sr=[e-mail], much appreciated and overdue!

/be

Comment 13

[Phil Schwartau]

Just need r= to get this in. Anyone? 

Comment 14

[Chingfa Wu]

After I merged attachment 54595 [details] [diff] [review], my application does not work anymore. Finally 
I sorted out the following test program, whick did not work with the fix.

function test(a, b)
{
  var sum ;
  var i = 0;

  sum = a + b;
  while (sum < 10)
  {
    try
    {
     sum += 1;
     i += 1;
     print("sum = ", sum, "i = ", i);
    }
    finally
    {
     print("finally");
    }
  }
  return i;
}


var loop;

loop = test(1, 3);
print("loop = " , loop);

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yurk.  We're not backpatching correctly now.  I'm on it.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: repair stmtInfo.catchJump before backpatching

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 54595 [details] [diff] [review]
swap-n-pop the retsub PC for non-local jumps, and misc tidying

The important parts of the new patch are the new bits in these patch
stanzas: save and restore stmtInfo.catchJump so that we backpatch
correctly.

Question for reviewers: better to just store the catchJump target in a
local, and use that local as an argument to BackPatch, rather than do the
shuffling here?  Or should I use a savedStmtInfo structure and restore the
whole thing to make the reuse for STMT_SUBROUTINE undetectable outside that
block?

@@ -2935,11 +2940,14 @@
          * gosubs that might have been emitted before non-local jumps.
          */
         if (pn->pn_kid3) {
+            ptrdiff_t savedCatchJump = stmtInfo.catchJump;
             if (!BackPatch(cx, cg, &stmtInfo, stmtInfo.gosub, CG_NEXT(cg),
                            JSOP_GOSUB)) {
                 return JS_FALSE;
             }
-            js_PopStatementCG(cx, cg);
+            /* Now indicate that we're emitting a subroutine body. */
+            stmtInfo.type = STMT_SUBROUTINE;
+            SET_STATEMENT_TOP(&stmtInfo, CG_OFFSET(cg));
             if (!UpdateLinenoNotes(cx, cg, pn->pn_kid3))
                 return JS_FALSE;
             if (js_Emit1(cx, cg, JSOP_FINALLY) < 0 ||
@@ -2947,9 +2955,13 @@
                 js_Emit1(cx, cg, JSOP_RETSUB) < 0) {
                 return JS_FALSE;
             }
-        } else {
-            js_PopStatementCG(cx, cg);
+            /*
+             * Now repair catchJump, which is nuked by SET_STATEMENT_TOP.
+             * We need it for backpatching below.
+             */
+            stmtInfo.catchJump = savedCatchJump;
         }
+        js_PopStatementCG(cx, cg);
 
         if (js_NewSrcNote(cx, cg, SRC_ENDBRACE) < 0 ||
             js_Emit1(cx, cg, JSOP_NOP) < 0) {


I also removed a dead-code JSOP_SETSP that the previous patch added before
the gosub to finally at the end of the try block: the stack had better be
balanced when we get to the end of that block, or we have problems that
JSOP_SETSP would just be papering over.

Review me, boys!

Comment 18

[Chingfa Wu]

Attachment: After merging the lastest fix, two test programs passed, but my aplication got coredump, any clue? I have not got a test program for this problem yet.

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Your stack trace shows an assert blowing at line 3753 of jsemit.c, which in my
patched tree is a case label (case TOK_PRIMARY).

Is your tree up-to-CVS-date?

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 55744 [details] [diff] [review]
repair stmtInfo.catchJump before backpatching

+            ptrdiff_t savedCatchJump = stmtInfo.catchJump;

Newline here please.

             if (!BackPatch(cx, cg, &stmtInfo, stmtInfo.gosub, CG_NEXT(cg),
                            JSOP_GOSUB)) {
                 return JS_FALSE;
             }
-            js_PopStatementCG(cx, cg);

And here, too.

+            /* Now indicate that we're emitting a subroutine body. */

I also mentioned the idea over IRC of inlining a specialized
SET_STATEMENT_TOP, which sets only the needed members -- then no need for
savedCatchJump.

/be

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Added newlines, lost savedCatchJump.

Comment 22

[Brendan Eich [:brendan]]

I pushed further and found that there's no reason to set top or update in the
stmtInfo struct when changing type from STMT_FINALLY to STMT_SUBROUTINE.  What's
more, top is unused now (it once was, by an old version of the function now
called BackPatch).  shaver, bless him, is gonna eliminate JSStmtInfo.top, saving
a word per js_EmitTree stack frame.

/be

Comment 23

[Chingfa Wu]

Attachment: The lastest fix is great, but one of my applications is stilll coredump on "top".

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

OK, I'll see your suggestion of removing JSStmtInfo.top, and raise you the
removal of the unused JSStmtInfo * parameter to BackPatch!

Now, to fix up the decompiler...

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Can you give me the source to the function it's trying to decompile when it
crashes?  I'm not able to reproduce it easily here.

Comment 26

[Chingfa Wu]

Attachment: The lastest fix is great, but one of my applications is stilll coredump on "top".

Comment 27

[Chingfa Wu]

Attachment: A test program can coredump js

Comment 28

[Chingfa Wu]

Attachment: Maybe this is a new bug, test program attached

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Latest fix, looking to slip into 0.9.6.

This one also loses the unused StmtInfo parameter to BackPatch.

Comment 30

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

You should file another bug about the decompiler crash, I think.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 57229 [details] [diff] [review]
Latest fix, looking to slip into 0.9.6.

Nit: no need for multi-line condition and braces now:

-	     if (!BackPatch(cx, cg, &stmtInfo, stmtInfo.gosub, CG_NEXT(cg),
+	     if (!BackPatch(cx, cg, stmtInfo.gosub, CG_NEXT(cg),
			    JSOP_GOSUB)) {
		 return JS_FALSE;
	     }

Naughty of you not to fix the decompiler!  File that sequel bug for 0.9.7, ok? 
Or keep this one open.

sr=[e-mail]

/be

Comment 32

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The decompiler is a different bug, I think, and I was hoping that Chingfa would
file it.  But I'll do that now, sure.

Comment 33

[Chingfa Wu]

Sorry that I have not filed a new bug for the decompile problem, if Mike have 
not done it, I prefer do it myself.

The lastest fix does pass my test program, but I get another coredump. I will 
try to create a test case for this problem, right now I can only publish the 
core stack.

(dbx) where
current thread: t@1
  [1] __sigprocmask(0x0, 0x60946f88, 0x0, 0xffffffff, 0xffffffff, 0x0), at 0xef7
04c90
  [2] _resetsig(0xef717814, 0xef716b60, 0x0, 0x0, 0x1db544, 0x1db548), at 0xef6f
b9c8
  [3] _sigon(0xef71b600, 0xef71b5e0, 0x1db540, 0xefffc47c, 0x6, 0x1), at 0xef6fb
184
  [4] _thrp_kill(0x0, 0x1, 0x6, 0xef716b60, 0x1db4d8, 0x0), at 0xef6fdec0
  [5] abort(0xef6a563c, 0x53, 0xef6ace3c, 0xef6a8fa8, 0x58a, 0x0), at 0xef63b328
=>[6] JS_Assert(s = 0x19fe80 "OBJ_GET_CLASS(cx, withobj) == &js_WithClass", file
 = 0x19feac "jsinterp.c", ln = 1418), line 174 in "jsutil.c"
  [7] js_Interpret(cx = 0x1e1208, result = 0xefffc988), line 1418 in "jsinterp.c
"
  [8] js_Invoke(cx = 0x1e1208, argc = 3U, flags = 0), line 849 in "jsinterp.c"
  [9] js_Interpret(cx = 0x1e1208, result = 0xefffdef8), line 2791 in "jsinterp.c
"
  [10] js_Execute(cx = 0x1e1208, chain = 0x1e2b40, script = 0x293d58, down = (ni
l), special = 0, result = 0xefffdef8), line 1012 in "jsinterp.c"
  [11] JS_ExecuteScript(cx = 0x1e1208, obj = 0x1e2b40, script = 0x293d58, rval =
 0xefffdef8), line 3263 in "jsapi.c"
  [12] Process(cx = 0x1e1208, obj = 0x1e2b40, filename = 0xefffe2a7 "ftappEvent.
jsv"), line 407 in "js.c"
  [13] ProcessArgs(cx = 0x1e1208, obj = 0x1e2b40, argv = 0xefffe080, argc = 1),
line 607 in "js.c"
  [14] main(argc = 1, argv = 0xefffe080), line 2268 in "js.c"

Comment 34

[Brendan Eich [:brendan]]

Attachment: the needed decompiler fix (yes, it was hard; let's go shopping!)

Comment 35

[Chingfa Wu]

First, I have to take back my last comment, apparently the latest fix does not 
fix the coredump in attachment 55929 [details].

More urgent problem related to the with statment, the test program is as 
follows:

function mytest(_doc)
{
var document = new Object();
var _ex;
var _arg;

with (document)
 {
  _arg = (_doc != null) ? "application" : "document";
  print("_arg = " + _arg);

  try
   {
     throw "test";
   }
  catch (_ex)
   {
     print("exception = " + _ex);
   }
 }
}
print(0);
mytest(null);
print(1);


Notice that if we took out the with statement, no coredump. If we took out 
the ?: statement, no coredump. If we took out the try block, no coredump.

Comment 36

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Son of a spidermonkey.

I'm sick right now, but I'll print some jsemit.c and sit in bed with them for a
bit.  I fear we're going to miss 0.9.6, unless nyquil inspires me more than is
its habit.

Comment 37

[Phil Schwartau]

Chingfa's recent examples added to testcase in JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-104077.js 

Note: the example at Comment 35 crashes in the current JS debug shell
on WinNT (no patches from this bug applied). It does not crash in 
the optimized JS shell.

Comment 38

[Chingfa Wu]

I know this is not the place to ask this question, but YOU are the ONLY experts 
that can provide the info I am looking for.

I'd like to get familiar with js interpreter source code. Becides the public 
API reference, the guide and the tutorial, is there any other document 
available, especially those about how js script gets to be compiled? THANKS!

Comment 39

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

There isn't really any documentation other than the source, sadly.

I (finally, heh) looked at this again today, and found that if I change the ?:
to an explicit if-else, it works.

In the if-else case, the stack depth is 1 (correct) when we start generating the
try block, but in the ?: case the stack depth is 2 (incorrect) when we start
generating the try block.  This leads to the JSOP_SETSP resetting the stack
depth to the wrong value when we hit the catch block, and much hilarity ensues.

I'll look more at that codegen path in a bit.

Comment 40

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, this looks like an old, old bug.  Both branches of the ?: expression
produce a value, and that means that we adjust cg->stackDepth upwards by two
when emitting the expression.  But execution will only take one path, so the
real stackDepth is one lower, and that's where our SETSP woes come from.

with-hook-try combinations have always been busted, and I wonder if there aren't
other such combinations lurking.  I couldn't find any, but then I didn't see
this back in 1999 either.

I'm going to merge in brendan's decompiler wizardry and post a new patch this
afternoon.  (Been up all night, because I'm stupid.)

Phil, I'll send along a minimal test case sample, in hopes that you can add it
to the test suite.

Brendan: does anything other than JSOP_SETSP generation for exceptions depend on
the accuracy of cg->stackDepth's value?

Comment 41

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: now with merged decompiler fix and adjusted cg->stackDepth from TOK_HOOK!

FWIW, none of the attached test cases do anything unexpected for me anymore. 
Dare I dream that this is The One?

Comment 42

[Chingfa Wu]

Attachment: I noticed the attachment fixed all bugs in my submitted test cases, but still coredump on this one.

Comment 43

[Brendan Eich [:brendan]]

shaver and I chatted via IRC and I believe we have a fix for chingfa's latest
testcase.  Shaver'll attach it soon, I bet (and I hope he changes "net-produce a
value" in that TOK_HOOK comment to "push a single value", after changing the if
(--cg->stackDepth < 0) {...} to a JS_ASSERT(cg->stackDepth >= 2);
cg->stackDepth--; sequence :-).

/be

Comment 44

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Add JSOP_{GET,RET}RVAL, to get the return value off the stack before running the finally.

I can now run 59359.  Chingfa, hit me again!

(The thing brendan and I talked about in IRC wasn't a fix, it turned out. 
Alas.)

Comment 45

[Chingfa Wu]

Attachment: The coredump trace

Mike, really APPRECIATED your swift response, THANK YOU VERY MUCH. Your latest
fix does fix my all testcases.	However, one of our application is still
coredump in the same place seems due to the same season. I will try to get
another test case for this problem. Right now I have problem to figure out
where the coredump happened in my js scripts, could you please tell me how I
can figure it out? Thanks!

Comment 46

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

No, thank _you_ for your excellent test cases, exposing years of bogusness in my
exception code. =)

From js_Interpret, looking at "*script" and "(pc - script->code)" should tell us
enough to see what line it's crashing on, and of what script.

Comment 47

[timeless]

w/ and w/o attachment 59568 [details] [diff] [review] this crashes xpcshell:
try {throw new A()}catch(e){}finally {try {throw new A()}catch(e){}finally
{print("a")}print("a")}

should i file a new bug?

Comment 48

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

No, just attach the test case here so we don't forget to add it to the set.

*sigh*

Comment 49

[timeless]

Attachment: testcase nested finallies. please don't shoot me for being evil. this just blows a JS_ASSERT

Comment 50

[Chingfa Wu]

Attachment: decompiler error

As I said before, one of my application is still coredump, I try to figure out
a testcase for this one but have not got it yet. In this application, there is
no return stmtment in finaly block, no conditional operator(use if-else
instead), only an exception travels through many functions in many files. In
the way to search for it, I find your lastest fix has introduced a decompiler
error(see the attached testcase). Sorry about it, Mike.

Comment 51

[Chingfa Wu]

Attachment: bug with js

Nothing is special about this testcase as I said before, but js is coredump.
Notice that without with statement, no coredump, if the exception is thrown in
if block, no coredump. 

Although the place that our appication is curedump is different from the
testcase, hope they are caused by the same reason. 

Mike, could you please treat this one as higher priority than others? Thanks!

Comment 52

[Chingfa Wu]

function testfunc()
{
 var document = new Object();
 with (document)
  {
   var _nextDialog = 100;
   var _doc = "abc" ;
   //var _doc = null ;

   if (_doc == null)
    {
     try
      {
       throw "authentication.0";
      }
     catch (_ex)
      {
      }
     finally
      {
      }
     return _nextDialog;
    }
   else
    {
     try
      {
        throw "authentication.0";
        //mytest();
      }
     catch (_ex)
      {
      }
     finally
      {
      }
     return _nextDialog;
    }
  }
}

var abc = testfunc();


A simpler testcase for the attach 59900. Notice if I comment throw and 
uncomment mytest(), js is also coredump.

Comment 53

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

No need to apologize.  I think I've found one part of the problem (SETSP is
using an incorrect stack depth), but I haven't figured out why yet.

Something must come of this bug over this weekend.  Thanks for the test cases,
Chingfa!

Comment 54

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Take that, Chingfa's test cases!

Brendan found that I was only adjusting cg->stackDepth half as often as I
should have been, and I can now run all your deviant JS code from the shell.

Haven't looked at the decompiler error, yet.

Next!

Comment 55

[Phil Schwartau]

Chingfa's and timeless' recent examples added to testcase in JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-104077.js 


Note: to run the test without using the Perl test driver, just do this:

[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js
js> load('../../tests/js1_5/shell.js')
js> load('../../tests/js1_5/Regress/regress-104077.js')

Comment 56

[Chingfa Wu]

Attachment: Looks like the lastest fix broke this testcase.

Comment 57

[Phil Schwartau]

Chingfa's latest example added to testcase in JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-104077.js 

This now contains 7 of his examples, plus the one from timeless above. 
This testcase is currently passing in the rhino, rhinoi shells. 

I'd like to report how the patch in Comment #54 is doing on this,
bug I'm having NO LUCK in applying the patch either on WinNT or Linux. 
I get errors like this:

[ ] patch < 104077_59920.patch
patching file `js.c'
Hunk #1 succeeded at 1123 (offset -3 lines).
patching file `jsemit.c'
Hunk #1 succeeded at 324 (offset -865 lines).
Hunk #2 FAILED at 336.
Hunk #4 succeeded at 412 (offset -865 lines).
Hunk #5 FAILED at 430.
Hunk #6 FAILED at 479.
         etc.
         etc.

Comment 58

[Brendan Eich [:brendan]]

Attachment: my latest take on shaver+brendan's patch, which fixes the decompiler

Testing and review needed.  Feel free to diff against the last posted patch (I
hope I based my work off that!).

/be

Comment 59

[Brendan Eich [:brendan]]

Attachment: diff of shaver's last patch against my last patch

Yes, my patch was based on shaver's last one.

/be

Comment 60

[Brendan Eich [:brendan]]

Attachment: slightly better comments (diff from my last patch coming next)

Comment 61

[Brendan Eich [:brendan]]

Attachment: plain diff of my last two patches, for easier review

Comment 62

[Chingfa Wu]

Just like reiterating that the lastest attach have introduced a bug.

function entry_menu()
{
 var document = new Object();
 var dialog = new Object();
 var _nextDialog = 100;

 with (document)
  {
   //var dialog = new Object();
   with (dialog)
    {
     try
      {
       while (true)
        {
          return _nextDialog;
        }
      }
     finally
      {
      }
    }
  }
}

var abc;
abc = entry_menu();
//Print ("abc = " + abc);

Comment 63

[Brendan Eich [:brendan]]

Attachment: best patch yet! thanks, chingfa, for reminding me again

Comment 64

[Brendan Eich [:brendan]]

Attachment: plain diff of my last two patches, for easier reviewing

Comment 65

[Brendan Eich [:brendan]]

Attachment: commented plain diff of shaver's last patch against my latest, for reviewers

Comment 66

[Brendan Eich [:brendan]]

Attachment: best ever fix, don't generate JSOP_SWAP at all from EmitNonLocalJumpFixup

We don't need JSOP_SWAP now that we have JSOP_SETRVAL/JSOP_RETRVAL.  Either we
need the latter pair, with properly interleaved fixup ops (LEAVEWITH, POP2,
POP) and GOSUBs, for return-from-try-with-finally; or we aren't returning from
a try with a finally clause, so we can simply JSOP_RETURN and let the non-empty
stack die when its frame is popped.

/be

Comment 67

[Phil Schwartau]

Is the patch above (id=60704) self-contained? I would like to run the
testsuite against it, but I'm not sure if I have to apply one of the
previous patches first. Thanks - 

Comment 68

[Brendan Eich [:brendan]]

Yes, attachment 60704 [details] [diff] [review] is self-contained
(http://bugzilla.mozilla.org/attachment.cgi?id=60704&action=view).

/be

Comment 69

[Brendan Eich [:brendan]]

Attachment: comments-only change, replacement patch for jsopcode.c only

The comment in JSOP_FINALLY's case code in Decompile lied, referring to the
now-unused JSOP_SWAP opcode.

/be

Comment 70

[Brendan Eich [:brendan]]

Attachment: truly the finest wine ever bottled!

Boy, talk about compensating fudge terms!  As I read the jsopcode.c
comments-only patch, the inconsistency of the decompiler's JSOP_SETSP case
leapt out at me.  The comment there babbled happily about how the decompiler
must adapt the code generator's stack depth model by subtracting the number of
open finally clauses, yet just above this comment, JSOP_FINALLY and JSOP_RETSUB
push and pop a cookie on the decompiler's string stack!  This cookie stands in
for the return address pushed by one or more JSOP_GOSUBs that call the
finally-clause subroutine.

So the adapting subtraction of ss->finallyLevel from the JSOP_SETSP operand
(which the code generator computed based on its model) must have been wrong. 
But removing it lead to string stack imbalance.  Therefore, I reviewed the code
in jsemit.c that generates catch and finally bytecode, and found that it had a
fudging adjustment upward of cg->stackDepth in two places, just before emitting
hidden JSOP_LEAVEWITHs.  The second LEAVEWITH was preceded in straight-line
fashion by an ENTERWITH, so the second fudge of cg->stackDepth was clearly
wrong.	Removing it still didn't save me, because the finally-clause code
generator failed to bump and (here a max computation is necessary; elsewhere it
can be asserted that the max is big enough already) compute the max stack depth
to include space for the JSOP_RETSUB return address.

I still wasn't out of the woods, because there was squirrelly code in the
try-catches-with-guards-only-[optional-finally] case that sets the last
(guarded) catch's JSOP_IFEQ jump offset to target rethrow code.  In this odd
case of guarded-only catch clauses, the last catch clause's scope object is not
modeled as on-stack, so we need to bump cg->stackDepth and emit a LEAVEWITH. 
The logic here was strange: it would set the jump offset for the weird case,
but it would emit the LEAVEWITH if *any* catch clauses (guarded or not) were
present -- and that would be bad for try-catch[noguards]-finally, which runs
through the same rethrow-generating logic and which would trigger a spurious
LEAVEWITH generation.

Testcases with catch-with-guard and two-catches-with-guards, based on chingfa's
testcase, coming up.

/be

Comment 71

[Brendan Eich [:brendan]]

Attachment: single-catch-with-guard testcase

Comment 72

[Brendan Eich [:brendan]]

Attachment: two-catches-with-guards-in-finally testcase

The point about these testcases is that they add try-catch[with-guard]-finally
in a finally clause, which tests whether the compiler and decompiler stack
models agree.  There's a simpler testcase that adds a guard-free
try-catch-finally in the finally clause of chingfa's testcase, I'll attach that
next.

/be

Comment 73

[Brendan Eich [:brendan]]

Attachment: one-catch-without-guard-in-finally testcase

Comment 74

[Brendan Eich [:brendan]]

attachment 60774 [details] [diff] [review] is the complete patch to apply, test, pour, nose, sip, savor,
and swallow!

/be

Comment 75

[Phil Schwartau]

Brendan's recent examples added to testcase in JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-104077.js 

This testcase now contains a total of 11 different scenarios.


Attachment 60774 [details] [diff] tested out well on my WinNT box. No regressions
were introduced by the patch, and the testcase for this bug ran
without crashing, and passed, to boot. I'm getting one crazy 
date test failure, but it has nothing to do with the patch: 
I'm getting it with the unpatched engine as well. Will investigate.


One thing I don't understand: the last three test scenarios
all have:

function addValues(dialog)
{
  var sum = 0;
  with (dialog)
  {
    try
    {
      sum = arg1 + arg2;
      with (arg3)
      {
        while (sum < 10)
        {
          try 
          {
            if (sum > 5)
              return sum;
            sum += 1;
          }

             etc.



Now, sum increments up to 6 in each scenario, where I would have thought
the return statement would cause function execution to terminate.
Yet it doesn't; control passes to the finally block later on. 

Is that the way it's supposed to be? I thought ECMA specified that
a return statement causes function execution to terminate. 

Comment 76

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

finally trumps return, and all other control-flow constructs that cause program
execution to jump out of the try block: throw, break, etc.  Once you enter a try
block, you will execute the finally block after leaving the try, regardless of
what happens to make you leave the try.

Comment 77

[Brendan Eich [:brendan]]

Attachment: last patch, but with jsemit.c fix to set finallyCatch before emitting SETSP

Old bug, caught it when reviewing the patch.  The "rethrow exception after
gosub finally" code that's emitted right after a try block's closing goto
around the catch and/or finally clauses, which is also hooked up via the last
catch's ifeq jump offset if all catches are guarded, begins with a JSOP_SETSP. 
But alas, the long-standing code did not update its finallyCatch local to
capture the pc offset of this setsp, so something like the next testcase could
overflow the stack in its finally clause.

/be

Comment 78

[Brendan Eich [:brendan]]

Attachment: example testcase whose trynote "missed" the setsp at start of the rethrow block

Comment 79

[Phil Schwartau]

Brendan's latest example added to testcase in JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-104077.js

Comment 80

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 60893 [details] [diff] [review]
last patch, but with jsemit.c fix to set finallyCatch before emitting SETSP

I like it.  Assuming that it passes the Chingfa Test, sr=shaver.  Thanks a ton,
/be.

Comment 81

[Chingfa Wu]

The lastest fix is great, it has passed all my testcases, and passed the 
applications that were crashed before.

Great job, thanks to all of you! Cheer!  --From Chingfa Wu
 

Comment 82

[Phil Schwartau]

On WinNT, I have also had luck with the latest patch (id=60893):
In both the debug and optimized JS shell, the testcase for this bug
passed, and no new regressions were introduced by the patch.

Comment 83

[Brendan Eich [:brendan]]

I can't seem to get an r= yet from the people I've asked, but I'm going to check
this in today (and not at the last minute).  Grumble.  Maybe I can cite
r=chingfa if he's read the patch and understands it to some extent?

/be

Comment 84

[John Bandhauer]

Comment on attachment 60893 [details] [diff] [review]
last patch, but with jsemit.c fix to set finallyCatch before emitting SETSP

r=jband

Comment 85

[Brendan Eich [:brendan]]

Fix is in, thanks all, esp. chingfa for the great testing, shaver for the tag
teaming, and jband for the last minute r=.

/be

Comment 86

[Phil Schwartau]

Verified FIXED.

Using JS source pulled at 5PM PST today, the above testcase passes
in the debug, optimized JS shells built on WinNT, Linux, and Mac9.1.

Comment 87

[Bob Clary [:bc] (inactive)]

fixed by Bug 336373 on the 1.8.1 branch. 
verified fixed 1.8.1 with windows/macppc/linux 20060707