series of try {} catch() doesn't work

RESOLVED FIXED

Status

Rhino
Core
P3
major
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: David C. Navas, Assigned: Norris Boyd)

Tracking

Details

(Reporter)

Description

17 years ago
The problem appears to be simple (and I hope not a result of my merge ;^/).
The ENDTRY instruction decrements the tryStackTop.  So does the actual 
exception handling code at the bottom of the interpret() function.  The 
following fails:
    try
    {
        throw SOME_EXCEPTION;
    } catch (e) {}
    try
    {
        throw SOME_EXCEPTION;
    } catch(e) {}

because when the second TRY begins, the tryStackTop is -1.

The problem appears to be with the generateICode().  There is a comparison 
between nextSibling and catchTarget and nextSibling and finallyTarget.  If it 
matches one of them, then the ENDTRY is added.  But, nextSibling and 
finallyTarget can be null!  Also, if the ENDTRY has already been added for the 
catchTarget, you don't want to add one again before the finallyTarget (which 
may or may not exist, but will always match nextSibling once...).

The hack I used was to set a boolean before the while() loop, initialized to 
false.  I made the conditional additionally check for falsity of the boolean 
flag, and inside the body set the boolean flag to true.  That way, only one 
ENDTRY statement is added....

Seems to work.
(Assignee)

Comment 1

17 years ago
Can you attach the diff for your change?

Thanks!
Status: NEW → ASSIGNED
(Assignee)

Comment 2

17 years ago
Proposed patch:

Index: Interpreter.java
===================================================================
RCS file: /cvsroot/mozilla/js/rhino/org/mozilla/javascript/Interpreter.java,v
retrieving revision 1.37
diff -u -r1.37 Interpreter.java
--- Interpreter.java    2000/11/20 16:16:32     1.37
+++ Interpreter.java    2000/12/01 15:17:33
@@ -241,8 +241,7 @@
                                         + node.toString());
     }

-    private int generateICode(Node node, int iCodeTop)
-    {
+    private int generateICode(Node node, int iCodeTop) {
         int type = node.getType();
         Node child = node.getFirstChild();
         Node firstChild = child;
@@ -860,8 +859,9 @@
                             before that goto.
                         */
                         Node nextSibling = child.getNextSibling();
-                        if (nextSibling == catchTarget ||
-                            nextSibling == finallyTarget)
+                        if (nextSibling != null &&
+                            (nextSibling == catchTarget ||
+                             nextSibling == finallyTarget))
                         {
                             iCodeTop = addByte((byte) TokenStream.ENDTRY,
                                                iCodeTop);
(Reporter)

Comment 3

17 years ago
The only concern I have with that (and I apologize for not getting you a diff 
yet :( -- I only have a hack ;^/) is that the ENDTRY would seem to be added in 
the catch clause for the following case:
   try
   {
   }
   catch () {}
   finally {}

When you're in the catch node, the next node is the finally node, and an
endtry is added.  Don't think that's right -- you only want to add the ENDTRY 
once.  I may be misremembering the code however, and if that's not the case,
then I see no problem with this.
(Assignee)

Comment 4

17 years ago
Ok, thanks for pointing that out. How about the following, which I think is 
similar to what you did:

Index: Interpreter.java
===================================================================
RCS file: /cvsroot/mozilla/js/rhino/org/mozilla/javascript/Interpreter.java,v
retrieving revision 1.37
diff -u -r1.37 Interpreter.java
--- Interpreter.java	2000/11/20 16:16:32	1.37
+++ Interpreter.java	2000/12/01 15:53:32
@@ -241,8 +241,7 @@
                                         + node.toString());
     }
     
-    private int generateICode(Node node, int iCodeTop)
-    {
+    private int generateICode(Node node, int iCodeTop) {
         int type = node.getType();
         Node child = node.getFirstChild();
         Node firstChild = child;
@@ -846,6 +845,7 @@
                         set the stackDepth to 1 to account for the incoming
                         exception object.
                     */
+                    boolean insertedEndTry = false;
                     while (child != null) {
                         if (catchTarget != null && lastChild == catchTarget) {
                             itsStackDepth = 1;
@@ -860,11 +860,13 @@
                             before that goto.
                         */
                         Node nextSibling = child.getNextSibling();
-                        if (nextSibling == catchTarget ||
-                            nextSibling == finallyTarget)
+                        if (!insertedEndTry && nextSibling != null &&
+                            (nextSibling == catchTarget ||
+                             nextSibling == finallyTarget))
                         {
                             iCodeTop = addByte((byte) TokenStream.ENDTRY,
                                                iCodeTop);
+                            insertedEndTry = true;
                         }
                         iCodeTop = generateICode(child, iCodeTop);
                         lastChild = child;

I verified that only one ENDTRY gets added for the snippet in the last comment.

(Assignee)

Comment 5

17 years ago
Committed that change:

Checking in Interpreter.java;
/cvsroot/mozilla/js/rhino/org/mozilla/javascript/Interpreter.java,v  <--  Interp
reter.java
new revision: 1.38; previous revision: 1.37
done
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 6

17 years ago
This is nearly identical to what I did.
You need to log in before you can comment on or make changes to this bug.