213231 - try / finally not working right

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Mike]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20021213
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20021213

Like in Java, each finally block gets a guaranteed chance to run code upon exit
of a scope.
Things get a little strange when an exception is thrown from inside a finally
block, but, the outer finally blocks still must get a chance to run. 
Presumably, the first exception actually thrown makes it through, not sure.
The thing, is, it's understandable that it is quite tricky to implement a
Javascript interpreter/compiler (or Java compiler) that properly handles this.
Below is a small code fragment how it gets confused.

Reproducible: Always

Steps to Reproduce:
try
{
  print("try");
}
finally
{
  print("finally");
  throw 1;
}

The output is:
try
finally
finally

It ran the print("finally") twice...

Try this in more details, using a counter:

    try
    {
        n=0;
        try
        {
          print("try");
        }
        finally
        {
          print("finally");
          n++;
          throw 1;
        }
    }
    catch (e)
    {
        print("caught " + e);
    }
    print("n=" + n);

The output is:
try
finally
finally
caught 1
n=2

n became 2, proving again that the finally block ran twice.

my guess is that the bug is that Rhino wants to run the finally block even
though the exception is thrown from within that finally block.  It is not
infinite looping because presumably upon the first throw, the finally block gets
marked/flagged, so that next time it knows it already ran.  If this is how it
works, then perhaps a simple fix would be to 'flag' the finally block upon
entering, before any exception.





If there are no developers involved in Rhino anymore, I wouldn't mind getting
involved.  Rhino is a relative untapped resource and is way more useful than
people give it credit for.  Therefore I wouldn't mind getting some exposure this
way.

Comment 1

[Igor Bukanov]

The bug presnts only in the interpreter mode (optimization -1), with
optimization set to 0 or higher I have in the current tip/ 1.5R4.1 / 1.5R3:

try
finally
caught 1
n=1

while the interpreter gives:

try
finally
finally
caught 1
n=2

Comment 2

[Igor Bukanov]

The reason for the bug is that the interpreter maintains a stack of entered try
statements and during execution of a finally handler when code leaves the try
block, the finally handler is executed with the try stack still pointing to the
current try.

It is possible to fix the bug with properly updating the try stack before
calling the finally handler (adding ENDTRY byte code when necessary) but it is
complicated by the fact that finally has to be executed after each
return/break/continue that transfer control outside the try block and each such
case requires to know if the code is inside try or is inside catch with try
stack already updated.

It seems a better way to fix this is to follow JVM byte code in the interpreter
 and construct a table of starting/ending byte codes for the try blocks and
instead of looking at the top of the try stack search this table when looking
for catch or finally handler. In this way finally code will never be executed
twice since it will be outside code for the try block.

As far as I can see the biggest obstacle to such implementation comes from
"with" statements and changes in the scope that exception handler has to
restore. Currently the proper scope is placed on the try stack providing a
trivial way to restore the scope. 

Without the try stack a counter has to be maintained of current "with" statement
 nesting and exception handler should store a static information about its
"with" level. The exception handling can then unwind with scopes until the
current and expected levels matches.

Comment 3

[Igor Bukanov]

Attachment: First fix part: when handling exceptions, restore proper scope without using try stack

The patch implements the above idea of restoring catch/finally scope without
using try stack

Comment 4

[Igor Bukanov]

Attachment: The seconf fix part: replacing try stack by static table of exception handlers

With the patch the above test case behaves properly under any optimization
level.

Comment 5

[Igor Bukanov]

Attachment: Additional cleanup: store catch/finally/with depth in the exception handler

This is an addition to the previous patch to completely remove generation f TRY
byte code. Since the second patch makes TRY byte code to hold only
catch/finally offsets and width depth, this patch moves that information into 
exception table removing the need for TRY byte code.

Comment 6

[Igor Bukanov]

I committed the above patches to the interpreter. They passes the test suite and
fixes the above problem but it is a significant change still.

Comment 7

[Igor Bukanov]

Targeting as resolved against 1.5R5