210605 - Script catches all Throwables when inside reflected Java method

Status: VERIFIED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Hannes Wallnoefer]

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

If a java method is called through reflection by
org.mozilla.javascript.NativeJavaMethod.call(), all instances of
java.lang.Throwable thrown while executing that method are caught by a
surrounding JavaScript try/catch statement.

Reproducible: Always

Steps to Reproduce:
1. Compile this class and add it to your classpath:
public class Test {
    public static void stop() {
        Thread.currentThread().stop();
    }
}

2. Execute this script: 
try {
        Packages.Test.stop();
} catch (x) {
        java.lang.System.err.println("cought: "+x);
}

Actual Results:  
The following line is printed: 
cought: java.lang.ThreadDeath

Expected Results:  
JavaScript shouldn't be able to catch the java.lang.ThreadDeath.

The cause of the problem seems to be that all Errors/Throwables are caught by
NativeJavaMethod.call() wrapped inside a InvocationTargetException, which is
then again wrapped inside a JavaScriptException. I guess the right thing would
be to unwrap and check the Throwable wrapped by the InvocationTargetException
and either wrap that in the JavaScriptException or rethrow it. The following
code in line 272 of NativeJavaMethod fixes the problem for me:

        } catch (InvocationTargetException e) {
            Throwable t = e.getTargetException();
            if (!(t instanceof Exception)) {
                Thread.currentThread().stop(t);
            }
            throw JavaScriptException.wrapException(cx, scope, e);
        }

I'm not sure if this is the way to go, though, and I don't understand why
NativeJavaMethod behaves differently than FunctionObject, which handles
InvocationTargetExceptions in the same way.

Comment 1

[Phil Schwartau]

cc'ing Igor -

Comment 2

[Igor Bukanov]

Attachment: Consistent exception handling

This big patch changes exception handling in Rhino in the following way:

1. JavaScriptException is used only to represent exceptions thrown by the
script throw statement. Previously the class was used as well to wrap instances
of Throwable that script can catch.

2. WrappedException is always used to wrap instances of Throwable that script
can catch by the catch statement. The new method
ScriptRutime.throwAsUncheckedException() that replaces
WrappedException.wrapException() will either re-throw the exception wrapped as
WrappedException or re-throw it as is. The method unwrap
InvocationTargetExceptions and if its target is Error instance, it is re-thrown
as is. In this way the script catch statement will never see instances of Error
including ThreadDeath.

3. Since the change effectively means that the script catch can see instances
of JavaScriptException, EcmaError and WrappedException, the new method
ScriptRutime.getCatchObject is used to extract JS object that is passed as the
catch argument. For JavaScriptException and EcmaError the method simply
extracts the corresponding JS object. For WrappedException the method wraps the
original exception as a LiveConnect object. In this way a potentially expensive
LiveConnect wrapping is avoided if the exception is never caught by a script.

Interpreter.interpret and optimizer/Codegen.generateCatchBlock is updated to
use the new method.

Comment 3

[Igor Bukanov]

Attachment: Patch updates with more comments

Comment 4

[Hannes Wallnoefer]

Thanks Igor. I'm running the patched version, everything looks right now.

Comment 5

[Igor Bukanov]

I committed the patch.

Comment 6

[Hannes Wallnoefer]

This bug has resurfaced with revision 1.173 of Context.java committed on
01/16/2004: By removing the lines 

    if (e instanceof Error) { 
        throw (Error)e;                                                     
    }

from Context.throwAsUncheckedException(), java.lang.Errors are thrown as
WrappedExceptions, which will be cought by JavaScript catch statements.

Comment 7

[Igor Bukanov]

That was caused by my attempt to get script source/line number information for
all exceptions including LinkageError and StackOverflow. For I now I will revert
the check for Error instances and after 1.5R1.5 release implement better solution.

Comment 8

[Igor Bukanov]

Should be fixed in 1.5R5

Comment 9

[Igor Bukanov]

Attachment: Fix to restore special handling of Error

Comment 10

[Igor Bukanov]

I committed the fix.

Comment 11

[Hannes Wallnoefer]

Thanks Igor. From your comments it seems like 1.5R5 is just around the corner,
which is also good to hear.

Comment 12

[Igor Bukanov]

Trageting as resolved against 1.5R5