Keep C# stack trace in exceptions thrown in JS, provide JS stack in inner exception instead (#1171)

Jint.Tests/Runtime/EngineTests.cs

@@ -1982,20 +1982,16 @@ var prep = function (fn) { fn(); };
         [Fact]
         public void ExceptionShouldHaveLocationOfInnerFunction()
         {
-            try
-            {
-                new Engine()
-                    .Evaluate(@"
-                    function test(s) {
-                        o.boom();
-                    }
-                    test('arg');
-                ");
-            }
-            catch (JavaScriptException ex)
-            {
-                Assert.Equal(3, ex.LineNumber);
-            }
+            var engine = new Engine();
+            const string source = @"
+                function test(s) {
+                    o.boom();
+                }
+                test('arg');
+            ";
+            
+            var ex = Assert.Throws<JavaScriptException>(() => engine.Evaluate(source));
+            Assert.Equal(3, ex.Location.Start.Line);
         }
 
         [Fact]

Jint.Tests/Runtime/ErrorTests.cs

@@ -76,7 +76,7 @@ var b = function(v) {
             Assert.Equal(15, e.Location.Start.Column);
             Assert.Equal("custom.js", e.Location.Source);
 
-            var stack = e.StackTrace;
+            var stack = e.JavaScriptStackTrace;
             EqualIgnoringNewLineDifferences(@"   at a (v) custom.js:3:16
    at b (v) custom.js:7:10
    at main.js:1:9", stack);
@@ -218,7 +218,7 @@ var b = function(v) {
    at recursive (folderInstance) <anonymous>:8:32
    at recursive (folderInstance) <anonymous>:8:32
    at recursive (folderInstance) <anonymous>:8:32
-   at <anonymous>:12:17", javaScriptException.StackTrace);
+   at <anonymous>:12:17", javaScriptException.JavaScriptStackTrace);
 
             var expected = new List<string>
             {
@@ -243,12 +243,12 @@ var x = b(7);";
 
             var ex = Assert.Throws<JavaScriptException>(() => engine.Execute(script));
 
-            const string expected = @"Jint.Runtime.JavaScriptException: Cannot read property 'yyy' of undefined
+            const string expected = @"Error: Cannot read property 'yyy' of undefined
    at a (v) <anonymous>:2:18
    at b (v) <anonymous>:6:12
    at <anonymous>:9:9";
 
-            EqualIgnoringNewLineDifferences(expected, ex.ToString());
+            EqualIgnoringNewLineDifferences(expected, ex.GetJavaScriptErrorString());
         }
 
         [Fact]
@@ -276,12 +276,12 @@ var x = b(7);";
             };
             var ex = Assert.Throws<JavaScriptException>(() => engine.Execute(script, parserOptions));
 
-            const string expected = @"Jint.Runtime.JavaScriptException: Cannot read property '5' of null
+            const string expected = @"Error: Cannot read property '5' of null
    at getItem (items, itemIndex) get-item.js:2:22
    at (anonymous) (getItem) get-item.js:9:16
    at get-item.js:13:2";
 
-            EqualIgnoringNewLineDifferences(expected, ex.ToString());
+            EqualIgnoringNewLineDifferences(expected, ex.GetJavaScriptErrorString());
         }
 
         [Fact]
@@ -326,95 +326,5 @@ var x = b(7);";
             actualString = actualString.Replace("\r\n", "\n");
             Assert.Contains(expectedSubstring, actualString);
         }
-
-        [Fact]
-        public void CustomException()
-        {
-            var engine = new Engine();
-            const string filename = "someFile.js";
-            JintJsException jsException = Assert.Throws<JintJsException>(() =>
-            {
-                try
-                {
-                    const string script = @"
-                        var test = 42; // just adding a line for a non zero line offset
-                        throw new Error('blah');
-                    ";
-
-                    engine.Execute(script);
-                }
-                catch (JavaScriptException ex)
-                {
-                    throw new JintJsException(filename, ex);
-                }
-            });
-
-            Assert.Equal(24, jsException.Column);
-            Assert.Equal(3, jsException.LineNumber);
-            Assert.Equal(filename, jsException.Module);
-        }
-
-        [Fact]
-        public void CustomExceptionUsesCopyConstructor()
-        {
-            var engine = new Engine();
-            const string filename = "someFile.js";
-            JintJsException2 jsException = Assert.Throws<JintJsException2>(() =>
-            {
-                try
-                {
-                    const string script = @"
-                        var test = 42; // just adding a line for a non zero line offset
-                        throw new Error('blah');
-                    ";
-
-                    engine.Execute(script);
-                }
-                catch (JavaScriptException ex)
-                {
-                    throw new JintJsException2(filename, ex);
-                }
-            });
-
-            Assert.Equal(24, jsException.Column);
-            Assert.Equal(3, jsException.LineNumber);
-            Assert.Equal(filename, jsException.Module);
-        }
-    }
-
-    public class JintJsException : JavaScriptException
-    {
-        private readonly JavaScriptException _jsException;
-
-        public JintJsException(string moduleName, JavaScriptException jsException) : base(jsException.Error)
-        {
-            Module = moduleName;
-            _jsException = jsException;
-            Location = jsException.Location;
-        }
-
-        public string Module { get; }
-        
-        public override string Message
-        {
-            get
-            {
-                var scriptFilename = (Module != null) ? "Filepath: " + Module + " " : "";
-                var errorMsg = $"{scriptFilename}{_jsException.Message}";
-                return errorMsg;
-            }
-        }
-
-        public override string StackTrace => _jsException.StackTrace;
-    }
-
-    public class JintJsException2 : JavaScriptException
-    {
-        public JintJsException2(string moduleName, JavaScriptException jsException) : base(jsException)
-        {
-            Module = moduleName;
-        }
-
-        public string Module { get; }
     }
 }

Jint.Tests/Runtime/InteropTests.cs

@@ -2695,11 +2695,14 @@ namespace Jint.Tests.Runtime
             engine.Realm.GlobalObject.FastAddProperty("global", engine.Realm.GlobalObject, true, true, true);
             engine.Realm.GlobalObject.FastAddProperty("test", new DelegateWrapper(engine, (Action<string, object>) Test), true, true, true);
 
-            var ex = Assert.Throws<JavaScriptException>(() => engine.Realm.GlobalObject.ToObject());
-            Assert.Equal("Cyclic reference detected.", ex.Message);
+            {
+                var ex = Assert.Throws<JavaScriptException>(() => engine.Realm.GlobalObject.ToObject());
+                Assert.Equal("Cyclic reference detected.", ex.Message);
+            }
 
-            ex = Assert.Throws<JavaScriptException>(() =>
-                engine.Execute(@"
+            {
+                var ex = Assert.Throws<JavaScriptException>(() =>
+                    engine.Execute(@"
                     var demo={};
                     demo.value=1;
                     test('Test 1', demo.value===1);
@@ -2707,10 +2710,10 @@ namespace Jint.Tests.Runtime
                     demo.demo=demo;
                     test('Test 3', demo);
                     test('Test 4', global);"
-                )
-            );
-
-            Assert.Equal("Cyclic reference detected.", ex.Message);
+                    )
+                );
+                Assert.Equal("Cyclic reference detected.", ex.Message);
+            }
         }
 
         [Fact]

Jint.Tests/Runtime/ModuleTests.cs

@@ -3,6 +3,7 @@ using System.Reflection;
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
 using Jint.Native;
 using Jint.Runtime;
 using Xunit;
@@ -117,6 +118,7 @@ public class ModuleTests
 
         var exc = Assert.Throws<JavaScriptException>(() => _engine.ImportModule("my-module"));
         Assert.Equal("Error while loading module: error in module 'imported': Line 1: Missing initializer in const declaration", exc.Message);
+        Assert.Equal("imported", exc.Location.Source);
     }
 
     [Fact]
@@ -127,7 +129,7 @@ public class ModuleTests
 
         var exc = Assert.Throws<JavaScriptException>(() => _engine.ImportModule("my-module"));
         Assert.Equal("Error while loading module: error in module 'imported': Line 1: Unexpected identifier", exc.Message);
-        Assert.Equal("my-module", exc.Location.Source);
+        Assert.Equal("imported", exc.Location.Source);
     }
 
     [Fact]
@@ -306,7 +308,8 @@ export const count = globals.counter;
     {
         var engine = new Engine(opts => opts.TimeoutInterval(TimeSpan.FromTicks(1)));
 
-        engine.AddModule("my-module", @"for(var i = 0; i < 100000; i++) { } export const result = 'ok';");
+        engine.AddModule("sleep", builder => builder.ExportFunction("sleep", () => Thread.Sleep(100)));
+        engine.AddModule("my-module", @"import { sleep } from 'sleep'; for(var i = 0; i < 100; i++) { sleep(); } export const result = 'ok';");
         Assert.Throws<TimeoutException>(() => engine.ImportModule("my-module"));
     }
 

Jint/Engine.Modules.cs

@@ -101,21 +101,12 @@ namespace Jint
 
             if (module is not CyclicModuleRecord cyclicModule)
             {
-                module.Link();
+                LinkModule(specifier, module);
                 EvaluateModule(specifier, module);
             }
             else if (cyclicModule.Status == ModuleStatus.Unlinked)
             {
-                try
-                {
-                    cyclicModule.Link();
-                }
-                catch (JavaScriptException ex)
-                {
-                    if (ex.Location.Source == null)
-                        ex.SetLocation(new Location(new Position(), new Position(), specifier));
-                    throw;
-                }
+                LinkModule(specifier, cyclicModule);
 
                 if (cyclicModule.Status == ModuleStatus.Linked)
                 {
@@ -133,6 +124,11 @@ namespace Jint
             return ModuleRecord.GetModuleNamespace(module);
         }
 
+        private static void LinkModule(string specifier, ModuleRecord module)
+        {
+            module.Link();
+        }
+
         private JsValue EvaluateModule(string specifier, ModuleRecord cyclicModule)
         {
             var ownsContext = _activeEvaluationContext is null;

Jint/Engine.cs

@@ -303,7 +303,7 @@ namespace Jint
 
                 if (result.Type == CompletionType.Throw)
                 {
-                    var ex = new JavaScriptException(result.GetValueOrDefault()).SetCallstack(this, result.Location);
+                    var ex = new JavaScriptException(result.GetValueOrDefault()).SetJavaScriptCallstack(this, result.Location);
                     ResetCallStack();
                     throw ex;
                 }
@@ -616,7 +616,7 @@ namespace Jint
             var callable = value as ICallable;
             if (callable is null)
             {
-                ExceptionHelper.ThrowTypeError(Realm, "Can only invoke functions");
+                ExceptionHelper.ThrowJavaScriptException(Realm.Intrinsics.TypeError, "Can only invoke functions");
             }
 
             JsValue DoInvoke()

Jint/ModuleBuilder.cs

@@ -121,12 +121,7 @@ public sealed class ModuleBuilder
         }
         catch (ParserException ex)
         {
-            ExceptionHelper.ThrowSyntaxError(_engine.Realm, $"Error while loading module: error in module '{_specifier}': {ex.Error}");
-            return null!;
-        }
-        catch (Exception)
-        {
-            ExceptionHelper.ThrowJavaScriptException(_engine, $"Could not load module {_specifier}", Completion.Empty());
+            ExceptionHelper.ThrowSyntaxError(_engine.Realm, $"Error while loading module: error in module '{_specifier}': {ex.Error}", new Location(new Position(ex.LineNumber, ex.Column), new Position(ex.LineNumber, ex.Column), _specifier));
             return null!;
         }
     }

Jint/Native/Function/EvalFunctionInstance.cs

@@ -143,8 +143,8 @@ namespace Jint.Native.Function
 
                     if (result.Type == CompletionType.Throw)
                     {
-                        var ex = new JavaScriptException(value).SetCallstack(_engine, result.Location);
-                        throw ex;
+                        ExceptionHelper.ThrowJavaScriptException(_engine, value, result);
+                        return null!;
                     }
                     else
                     {

Jint/Runtime/Debugger/DebugHandler.cs

@@ -101,7 +101,7 @@ namespace Jint.Runtime.Debugger
                 // TODO: Should we return an error here? (avoid exception overhead, since e.g. breakpoint
                 // evaluation may be high volume.
                 var error = result.GetValueOrDefault();
-                var ex = new JavaScriptException(error).SetCallstack(_engine, result.Location);
+                var ex = new JavaScriptException(error).SetJavaScriptCallstack(_engine, result.Location);
                 throw new DebugEvaluationException("An error occurred during debugger evaluation", ex);
             }
 

Jint/Runtime/Environments/DeclarativeEnvironmentRecord.cs

@@ -137,7 +137,7 @@ namespace Jint.Runtime.Environments
         [MethodImpl(MethodImplOptions.NoInlining)]
         private void ThrowUninitializedBindingError(string name)
         {
-            throw new JavaScriptException(_engine.Realm.Intrinsics.ReferenceError, $"Cannot access '{name}' before initialization");
+            ExceptionHelper.ThrowReferenceError(_engine.Realm, $"Cannot access '{name}' before initialization");
         }
 
         public sealed override bool DeleteBinding(string name)

Jint/Runtime/ExceptionHelper.cs

@@ -4,6 +4,7 @@ using System.Reflection;
 using System.Runtime.ExceptionServices;
 using Esprima;
 using Jint.Native;
+using Jint.Native.Error;
 using Jint.Runtime.CallStack;
 using Jint.Runtime.Modules;
 using Jint.Runtime.References;
@@ -21,7 +22,7 @@ namespace Jint.Runtime
         [DoesNotReturn]
         public static void ThrowSyntaxError(Realm realm, string message, Location location)
         {
-            throw new JavaScriptException(realm.Intrinsics.SyntaxError, message).SetLocation(location);
+            throw new JavaScriptException(realm.Intrinsics.SyntaxError, message).SetJavaScriptLocation(location);
         }
 
         [DoesNotReturn]
@@ -56,15 +57,15 @@ namespace Jint.Runtime
         }
 
         [DoesNotReturn]
-        public static void ThrowTypeErrorNoEngine(string message = null, Exception exception = null)
+        public static void ThrowTypeErrorNoEngine(string message = null)
         {
             throw new TypeErrorException(message);
         }
 
         [DoesNotReturn]
-        public static void ThrowTypeError(Realm realm, string message = null, Exception exception = null)
+        public static void ThrowTypeError(Realm realm, string message = null)
         {
-            throw new JavaScriptException(realm.Intrinsics.TypeError, message, exception);
+            throw new JavaScriptException(realm.Intrinsics.TypeError, message);
         }
 
         [DoesNotReturn]
@@ -116,9 +117,9 @@ namespace Jint.Runtime
         }
 
         [DoesNotReturn]
-        public static void ThrowInvalidOperationException(string message = null)
+        public static void ThrowInvalidOperationException(string message = null, Exception exception = null)
         {
-            throw new InvalidOperationException(message);
+            throw new InvalidOperationException(message, exception);
         }
 
         [DoesNotReturn]
@@ -126,11 +127,29 @@ namespace Jint.Runtime
         {
             throw new PromiseRejectedException(error);
         }
+        
+        [DoesNotReturn]
+        public static void ThrowJavaScriptException(JsValue value)
+        {
+            throw new JavaScriptException(value);
+        }
 
         [DoesNotReturn]
         public static void ThrowJavaScriptException(Engine engine, JsValue value, in Completion result)
         {
-            throw new JavaScriptException(value).SetCallstack(engine, result.Location);
+            throw new JavaScriptException(value).SetJavaScriptCallstack(engine, result.Location);
+        }
+        
+        [DoesNotReturn]
+        public static void ThrowJavaScriptException(ErrorConstructor errorConstructor, string message)
+        {
+            throw new JavaScriptException(errorConstructor, message);
+        }
+        
+        [DoesNotReturn]
+        public static void ThrowJavaScriptException(ErrorConstructor errorConstructor, string message, Engine engine, Location location)
+        {
+            throw new JavaScriptException(errorConstructor, message).SetJavaScriptCallstack(engine, location);
         }
 
         [DoesNotReturn]

Jint/Runtime/Interop/NamespaceReference.cs

@@ -66,7 +66,7 @@ namespace Jint.Runtime.Interop
             }
             catch (Exception e)
             {
-                ExceptionHelper.ThrowTypeError(_engine.Realm, "Invalid generic type parameter on " + _path + ", if this is not a generic type / method, are you missing a lookup assembly?", e);
+                ExceptionHelper.ThrowInvalidOperationException($"Invalid generic type parameter on {_path}, if this is not a generic type / method, are you missing a lookup assembly?", e);
                 return null;
             }
         }

Jint/Runtime/JavaScriptException.cs

@@ -7,55 +7,82 @@ using Jint.Native.Error;
 using Jint.Native.Object;
 using Jint.Pooling;
 
-namespace Jint.Runtime
+namespace Jint.Runtime;
+
+public class JavaScriptException : JintException
 {
-    public class JavaScriptException : JintException
+    private static string GetMessage(JsValue error)
     {
-        private string? _callStack;
-
-        public JavaScriptException(ErrorConstructor errorConstructor) : base("")
+        string? ret;
+        if (error is ObjectInstance oi)
         {
-            Error = errorConstructor.Construct(Arguments.Empty);
+            ret = oi.Get(CommonProperties.Message).ToString();
         }
-
-        public JavaScriptException(ErrorConstructor errorConstructor, string? message, Exception? innerException)
-            : base(message, innerException)
+        else
         {
-            Error = errorConstructor.Construct(new JsValue[] { message });
+            ret = null;
         }
 
-        public JavaScriptException(ErrorConstructor errorConstructor, string? message)
-            : base(message)
-        {
-            Error = errorConstructor.Construct(new JsValue[] { message });
-        }
+        return ret ?? TypeConverter.ToString(error);
+    }
 
-        public JavaScriptException(JsValue error)
-        {
-            Error = error;
-        }
+    private readonly JavaScriptErrorWrapperException _jsErrorException;
 
-        // Copy constructors
-        public JavaScriptException(JavaScriptException exception, Exception? innerException) : base(exception.Message, exception.InnerException)
-        {
-            Error = exception.Error;
-            Location = exception.Location;
-        }
+    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), ""))
+    {
+        _jsErrorException = (JavaScriptErrorWrapperException) InnerException!;
+    }
+
+    internal JavaScriptException(ErrorConstructor errorConstructor, string? message)
+        : base(message, new JavaScriptErrorWrapperException(errorConstructor.Construct(new JsValue[] { message }), 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)
+    {
+        _jsErrorException.SetCallstack(engine, location);
+        return this;
+    }
+    
+    public JavaScriptException SetJavaScriptLocation(Location location)
+    {
+        _jsErrorException.SetLocation(location);
+        return this;
+    }
+
+    private class JavaScriptErrorWrapperException : JintException
+    {
+        private string? _callStack;
+
+        public JsValue Error { get; }
+        public Location Location { get; private set; }
 
-        public JavaScriptException(JavaScriptException exception) : base(exception.Message)
+        internal JavaScriptErrorWrapperException(JsValue error, string? message = null)
+            : base(message ?? GetMessage(error))
         {
-            Error = exception.Error;
-            Location = exception.Location;
+            Error = error;
         }
 
-        internal JavaScriptException SetLocation(Location location)
+        internal void SetLocation(Location location)
         {
             Location = location;
-
-            return this;
         }
 
-        internal JavaScriptException SetCallstack(Engine engine, Location location)
+        internal void SetCallstack(Engine engine, Location location)
         {
             Location = location;
             var value = engine.CallStack.BuildCallStackString(location);
@@ -65,27 +92,10 @@ namespace Jint.Runtime
                 Error.AsObject()
                     .FastAddProperty(CommonProperties.Stack, new JsString(value), false, false, false);
             }
-
-            return this;
-        }
-
-        private string? GetErrorMessage()
-        {
-            if (Error is ObjectInstance oi)
-            {
-                return oi.Get(CommonProperties.Message).ToString();
-            }
-
-            return null;
         }
 
-        public JsValue Error { get; }
-
-        public override string Message => GetErrorMessage() ?? TypeConverter.ToString(Error);
-
         /// <summary>
-        /// Returns the call stack of the exception. Requires that engine was built using
-        /// <see cref="Options.CollectStackTrace"/>.
+        /// Returns the call stack of the JavaScript exception.
         /// </summary>
         public override string? StackTrace
         {
@@ -109,40 +119,20 @@ namespace Jint.Runtime
             }
         }
 
-        public Location Location { get; protected set; }
-
-        public int LineNumber => Location.Start.Line;
-
-        public int Column => Location.Start.Column;
-
         public override string ToString()
         {
-            // adapted custom version as logic differs between full framework and .NET Core
-            var className = GetType().ToString();
-            var message = Message;
-            var innerExceptionString = InnerException?.ToString() ?? "";
-            const string endOfInnerExceptionResource = "--- End of inner exception stack trace ---";
-            var stackTrace = StackTrace;
-
             using var rent = StringBuilderPool.Rent();
             var sb = rent.Builder;
-            sb.Append(className);
+
+            sb.Append("Error");
+            var message = Message;
             if (!string.IsNullOrEmpty(message))
             {
                 sb.Append(": ");
                 sb.Append(message);
             }
 
-            if (InnerException != null)
-            {
-                sb.Append(Environment.NewLine);
-                sb.Append(" ---> ");
-                sb.Append(innerExceptionString);
-                sb.Append(Environment.NewLine);
-                sb.Append("   ");
-                sb.Append(endOfInnerExceptionResource);
-            }
-
+            var stackTrace = StackTrace;
             if (stackTrace != null)
             {
                 sb.Append(Environment.NewLine);
@@ -152,4 +142,4 @@ namespace Jint.Runtime
             return rent.ToString();
         }
     }
-}
+}

Jint/Runtime/TypeConverter.cs

@@ -1030,7 +1030,7 @@ namespace Jint.Runtime
         {
             referencedName ??= "unknown";
             var message = $"Cannot read property '{referencedName}' of {o}";
-            throw new JavaScriptException(engine.Realm.Intrinsics.TypeError, message).SetCallstack(engine, sourceNode.Location);
+            throw new JavaScriptException(engine.Realm.Intrinsics.TypeError, message).SetJavaScriptCallstack(engine, sourceNode.Location);
         }
 
         public static void CheckObjectCoercible(Engine engine, JsValue o)