Fix interop stack trace unwind (#1202)

Jint.Tests/Runtime/ErrorTests.cs

@@ -284,6 +284,46 @@ public void StackTraceCollectedForImmediatelyInvokedFunctionExpression()
             EqualIgnoringNewLineDifferences(expected, ex.GetJavaScriptErrorString());
         }
 
+        // Verify #1202
+        [Fact]
+        public void StackIsUnwoundWhenExceptionHandledByInteropCode()
+        {
+            var engine = new Engine()
+                .SetValue("handle", new Action<Action>(Handler));
+
+            const string Script = @"
+function throwIt(message) {
+    throw new Error(message);
+}
+
+handle(() => throwIt('e1'));
+handle(() => throwIt('e2'));
+handle(() => throwIt('e3'));
+
+try {
+    throwIt('e4');
+} catch(x){
+    x.stack; // return stack trace string
+}
+";
+            var stack = engine.Evaluate(Script).AsString();
+            EqualIgnoringNewLineDifferences(@"   at Error <anonymous>:3:21
+   at throwIt (message) <anonymous>:3:15
+   at <anonymous>:11:5", stack);
+
+            static void Handler(Action callback)
+            {
+                try
+                {
+                    callback();
+                }
+                catch (JavaScriptException)
+                {
+                    // handle JS error
+                }
+            }
+        }
+
         [Fact]
         public void StackTraceIsForOriginalException()
         {
@@ -327,4 +367,4 @@ private static void ContainsIgnoringNewLineDifferences(string expectedSubstring,
             Assert.Contains(expectedSubstring, actualString);
         }
     }
-}
+}

Jint/Engine.cs

@@ -1353,9 +1353,15 @@ ObjectInstance Callback()
                 ExceptionHelper.ThrowRecursionDepthOverflowException(CallStack, callStackElement.ToString());
             }
 
-            var result = functionInstance.Call(thisObject, arguments);
-
-            CallStack.Pop();
+            JsValue result;
+            try
+            {
+                result = functionInstance.Call(thisObject, arguments);
+            }
+            finally
+            {
+                CallStack.Pop();
+            }
 
             return result;
         }
@@ -1376,9 +1382,15 @@ ObjectInstance Callback()
                 ExceptionHelper.ThrowRecursionDepthOverflowException(CallStack, callStackElement.ToString());
             }
 
-            var result = ((IConstructor) functionInstance).Construct(arguments, newTarget);
-
-            CallStack.Pop();
+            ObjectInstance result;
+            try
+            {
+                result = ((IConstructor) functionInstance).Construct(arguments, newTarget);
+            }
+            finally
+            {
+                CallStack.Pop();
+            }
 
             return result;
         }

Jint/Runtime/Interpreter/Statements/JintTryStatement.cs

@@ -48,16 +48,6 @@ protected override Completion ExecuteInternal(EvaluationContext context)
                 // execute catch
                 if (_statement.Handler is not null)
                 {
-                    // Quick-patch for call stack not being unwinded when an exception is caught.
-                    // Ideally, this should instead be solved by always popping the stack when returning
-                    // from a call, regardless of whether it throws (i.e. CallStack.Pop() in finally clause
-                    // in Engine.Call/Engine.Construct - however, that method currently breaks stack traces
-                    // in error messages.
-                    while (callStackSizeBeforeExecution < engine.CallStack.Count)
-                    {
-                        engine.CallStack.Pop();
-                    }
-
                     // https://tc39.es/ecma262/#sec-runtime-semantics-catchclauseevaluation
 
                     var thrownValue = b.Value;

Jint/Runtime/JavaScriptException.cs

@@ -31,7 +31,7 @@ private static string GetMessage(JsValue error)
     public string? JavaScriptStackTrace => _jsErrorException.StackTrace;
     public Location Location => _jsErrorException.Location;
     public JsValue Error => _jsErrorException.Error;
-    
+
     internal JavaScriptException(ErrorConstructor errorConstructor)
         : base("", new JavaScriptErrorWrapperException(errorConstructor.Construct(Arguments.Empty), ""))
     {
@@ -43,21 +43,21 @@ internal JavaScriptException(ErrorConstructor errorConstructor, string? message)
     {
         _jsErrorException = (JavaScriptErrorWrapperException) InnerException!;
     }
-    
+
     internal JavaScriptException(JsValue error)
         : base(GetMessage(error), new JavaScriptErrorWrapperException(error, GetMessage(error)))
     {
         _jsErrorException = (JavaScriptErrorWrapperException) InnerException!;
     }
-    
+
     public string GetJavaScriptErrorString() => _jsErrorException.ToString();
-    
-    public JavaScriptException SetJavaScriptCallstack(Engine engine, Location location)
+
+    public JavaScriptException SetJavaScriptCallstack(Engine engine, Location location, bool overwriteExisting = false)
     {
-        _jsErrorException.SetCallstack(engine, location);
+        _jsErrorException.SetCallstack(engine, location, overwriteExisting);
         return this;
     }
-    
+
     public JavaScriptException SetJavaScriptLocation(Location location)
     {
         _jsErrorException.SetLocation(location);
@@ -82,15 +82,26 @@ internal void SetLocation(Location location)
             Location = location;
         }
 
-        internal void SetCallstack(Engine engine, Location location)
+        internal void SetCallstack(Engine engine, Location location, bool overwriteExisting)
         {
             Location = location;
-            var value = engine.CallStack.BuildCallStackString(location);
-            _callStack = value;
-            if (Error.IsObject())
+
+            var errObj = Error.IsObject() ? Error.AsObject() : null;
+            if (errObj == null)
+            {
+                _callStack = engine.CallStack.BuildCallStackString(location);
+                return;
+            }
+
+            // Does the Error object already have a stack property?
+            if (errObj.HasProperty(CommonProperties.Stack) && !overwriteExisting)
+            {
+                _callStack = errObj.Get(CommonProperties.Stack).AsString();
+            }
+            else
             {
-                Error.AsObject()
-                    .FastAddProperty(CommonProperties.Stack, new JsString(value), false, false, false);
+                _callStack = engine.CallStack.BuildCallStackString(location);
+                errObj.FastAddProperty(CommonProperties.Stack, _callStack, false, false, false);
             }
         }
 

Jint/Runtime/TypeConverter.cs

@@ -1030,7 +1030,8 @@ private static ObjectInstance ToObjectNonObject(Realm realm, JsValue value)
         {
             referencedName ??= "unknown";
             var message = $"Cannot read property '{referencedName}' of {o}";
-            throw new JavaScriptException(engine.Realm.Intrinsics.TypeError, message).SetJavaScriptCallstack(engine, sourceNode.Location);
+            throw new JavaScriptException(engine.Realm.Intrinsics.TypeError, message)
+                .SetJavaScriptCallstack(engine, sourceNode.Location, overwriteExisting: true);
         }
 
         public static void CheckObjectCoercible(Engine engine, JsValue o)