Nashorn sandbox does not catch Error when using an ExecutorService

[lukehutch]

The Nashorn sandbox does not catch Error instances, e.g. ExceptionInInitializerError, when running with an ExecutorService.

This means that if you run JS code that calls back to Java methods, and one of the calls throws an Error, the sandbox.eval call will simply return null rather than throwing ScriptException, and the Error will be thrown as an uncaught exception in the ExecutorService thread. (If you're not using an ExecutorService, then the Error is simply thrown to the caller of `sandbox.eval as expected.)

Presumably the fix is to catch Error, not Exception, in sandbox.eval, and then wrap the Error object in a ScriptException.

[mxro]

Thank you for raising this issue as well!

I think I understand what the issue is but I in order to be sure can you have a look at this test case:

https://github.com/javadelight/delight-nashorn-sandbox/blob/issue-168-error-returns-null/src/test/java/delight/nashornsandbox/TestIssue168_Error_Returns_Null.java#L24

How can this best be modified to replicate the sought after behaviour? I think we would need to add a call into Java where a RuntimeError occurs?

Anything that catches your eye in these places where some execution occurs.

https://github.com/javadelight/delight-nashorn-sandbox/blob/issue-168-error-returns-null/src/main/java/delight/nashornsandbox/internal/JsEvaluator.java#L66

https://github.com/javadelight/delight-nashorn-sandbox/blob/issue-168-error-returns-null/src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java#L470

Does this look like the right direction? Once I can confirm the test case, can look further where best to patch the code.

[lukehutch]

Correct, you can't catch Exception, you have to catch Throwable in order to also catch Error (both share the superclass of Throwable -- I made a mistake in how I described this in the original bug report, and I neglected to mention Throwable). Otherwise, if you only catch Exception, then any .allowed Java code that is called by JS cannot throw Error, it can only throw Exception. If Java code throws Error, then the Nashorn Sandbox cannot catch that currently.

So the fix requires:

1. Catching Throwable as the catch-all, not Exception.
2. Changing the field Exception exception to Throwable throwable wherever the exception object is captured.

[mxro]

Thank you for the further information! I extended the test case now.

Currently it looks like if we throw an Error in the Java code, it will be thrown as an Error to the Java code calling sandbox.eval() - even when using the Executor service.

Before I make any changes, could you confirm:

1. Would this be acceptable behaviour (e.g. to throw Error instead of ScriptException?
2. Do you know what I would need to change in the test case to make it return null instead of the thrown Error (as it does seem to do at the moment)? (I understand in your case it was throwing the error in the wrong thread and returning null, correct?)

public class TestIssue168_Error_Returns_Null {

  public static class IFail {
    public void doIt() {
      throw new Error("I tried my best but I failed");
    }
  }

  @Test(expected = java.lang.Error.class)
  public void test() throws ScriptCPUAbuseException, ScriptException, NoSuchMethodException {

    NashornSandbox sandbox = NashornSandboxes.create();
    try {
      sandbox.setExecutor(Executors.newSingleThreadExecutor());
      sandbox.inject("iFail", new IFail());
      String code = "iFail.doIt();";
      sandbox.eval(code);
    } finally {
      sandbox.getExecutor().shutdown();
    }
  }

}

[lukehutch]

• Would this be acceptable behaviour (e.g. to throw Error instead of ScriptException?

That's certainly the most backwards-compatible solution (and having Exception in the name implies that you probably shouldn't be wrapping Error anyway). So yes, that's fine.

• Do you know what I would need to change in the test case to make it return null instead of the thrown Error (as it does seem to do at the moment)? (I understand in your case it was throwing the error in the wrong thread and returning null, correct?)

I don't understand -- you want sandbox.eval to return null rather than re-throwing Error when Error is thrown? That's what the current behavior is, when there is an ExecutorService, and it's bad behavior, so no, I don't recommend doing that. Catching Error in the ExecutorService and then re-throwing it in the calling thread (which is I assume what you changed the code to do) is the right thing to do here; you don't want to return null.

(Or maybe I misunderstood your question...)