Tests of CallStack unwinding after caught exception (and quick patch)

[Jither]

As mentioned in #852, callstack fails to unwind after catching an exception. Not quite sure where this should be handled. Included two tests, one just showing (by way of nested calls) that it's the entire callstack that fails to unwind - it's not just off by 1.

[Jither]

About this issue... Dug a bit into the Engine, probably with too little understanding of it, but...

I think ideally, Engine.Call (and conversely, Construct)

jint/Jint/Engine.cs

Lines 1274 to 1283 in 5b51036

	var result = functionInstance.Call(thisObject, arguments);
	if (_isDebugMode)
	{
	DebugHandler.PopDebugCallStack();
	}
	CallStack.Pop();
	return result;

... should have something like:

        JsValue result;
        try
        {
            result = functionInstance.Call(thisObject, arguments);
        }
        finally
        {
            CallStack.Pop();
        }
        return result;

... to ensure that the call stack is always popped. This will yield the correct call stack when entering a catch (and anywhere else I can think of). However, such a fix interferes with the stack trace in JavascriptException when there's no catch in the script:

In the case I've checked (the test StackTraceCollectedOnThreeLevels), although the stack trace is built when first throwing the exception, when that exception is caught here:

jint/Jint/Runtime/Interpreter/JintStatementList.cs

Lines 84 to 89 in 5b51036

	catch (JavaScriptException v)
	{
	var location = v.Location == default ? s.Location : v.Location;
	var completion = new Completion(CompletionType.Throw, v.Error, null, location);
	return completion;
	}

... it's converted to a Completion, which is then handled in Engine Execute(Script script) - turning it back into a JavascriptException again, and replacing the original stack trace with a new one, which now no longer fits the call stack at the original site of the error:

jint/Jint/Engine.cs

Lines 379 to 383 in 5b51036

	if (result.Type == CompletionType.Throw)
	{
	var ex = new JavaScriptException(result.GetValueOrDefault()).SetCallstack(this, result.Location);
	ResetCallStack();
	throw ex;

... due to us having unwinded the stack through finally, that is. Currently it works only because the CallStack is left as is, if any Exception is thrown during execution.

But still, not really sure what to do about it - the back and forth of JavascriptException <-> Completion looks fishy to me. 😄 (but again, I don't have enough understanding of the Engine's internals).

I guess an alternative to finally would be to do the stack unwinding specifically in JintTryStatement's catch-handling. But that seems more hack-y and obviously less general than the finally approach. Even if there may be no other cases where the current code would fail in practice.

[lahma]

Yes the current call stack handling is quite tricky if not brittle when it comes to the exception chain and re-throw and completions. It would definitely require some love. I think ideally (JS) exceptions should be allowed to flow through until the very end where at last step the completion would be handled.

[Jither]

Added a quick patch for the catch issue - but like I write in the comment, it's not actually meant as a solution - but may clarify what the issue actually is, more so than my walls of text 😉 (For example, in an actual solution, call stack should also be unwound in finally - but doing so in JintTryStatement would once again break exception stack traces - the only reason the quick patch can be done for catch is that the catch means the exception stack trace won't "be needed").

Agree on the optimal solution being more consistent completions/throws.

[lahma]

This looks like an acceptable hack for time being until someone has the courage to redo some if the nested exception handling.