Exclude Error ctor from stack trace (#1217)

* If the current function on the call stack is the Error constructor function, exclude it from the captured stack trace.
* Fix issues with reported location so that JavaScriptException.Location is consistent with the location recorded in the stack trace.
* add new test, adjust existing tests

Jint.Tests/Runtime/ErrorTests.cs

@@ -56,7 +56,7 @@ var c = a(b().Length);
         }
 
         [Fact]
-        public void CanProduceCorrectStackTrace()
+        public void CanProduceCorrectStackTraceForInternalError()
         {
             var engine = new Engine();
 
@@ -82,6 +82,33 @@ var b = function(v) {
    at main.js:1:9", stack);
         }
 
+        [Fact]
+        public void CanProduceCorrectStackTraceForScriptError()
+        {
+            var engine = new Engine();
+
+            engine.Execute(@"
+var a = function(v) {
+  throw new Error('Error thrown from script');
+}
+
+var b = function(v) {
+  return a(v);
+}
+            ", new ParserOptions("custom.js"));
+
+            var e = Assert.Throws<JavaScriptException>(() => engine.Execute("var x = b(7);", new ParserOptions("main.js")));
+            Assert.Equal("Error thrown from script", e.Message);
+            Assert.Equal(3, e.Location.Start.Line);
+            Assert.Equal(8, e.Location.Start.Column);
+            Assert.Equal("custom.js", e.Location.Source);
+
+            var stack = e.JavaScriptStackTrace;
+            EqualIgnoringNewLineDifferences(@"   at a (v) custom.js:3:9
+   at b (v) custom.js:7:10
+   at main.js:1:9", stack);
+        }
+
         [Fact]
         public void ErrorObjectHasTheStackTraceImmediately()
         {
@@ -100,8 +127,7 @@ var b = function(v) {
             var e = engine.Evaluate(@"b(7)", new ParserOptions("main.js")).AsString();
 
             var stack = e;
-            EqualIgnoringNewLineDifferences(@"   at Error custom.js:3:10
-   at a (v) custom.js:3:10
+            EqualIgnoringNewLineDifferences(@"   at a (v) custom.js:3:10
    at b (v) custom.js:7:10
    at main.js:1:1", stack);
         }
@@ -128,8 +154,7 @@ var b = function(v) {
             var e = engine.Evaluate(@"b(7)", new ParserOptions("main.js")).AsString();
 
             var stack = e;
-            EqualIgnoringNewLineDifferences(@"   at Error custom.js:4:11
-   at a (v) custom.js:4:11
+            EqualIgnoringNewLineDifferences(@"   at a (v) custom.js:4:11
    at b (v) custom.js:11:10
    at main.js:1:1", stack);
         }
@@ -249,6 +274,8 @@ var x = b(7);";
    at <anonymous>:9:9";
 
             EqualIgnoringNewLineDifferences(expected, ex.GetJavaScriptErrorString());
+            Assert.Equal(2, ex.Location.Start.Line);
+            Assert.Equal(17, ex.Location.Start.Column);
         }
 
         [Fact]
@@ -282,6 +309,9 @@ var x = b(7);";
    at get-item.js:13:2";
 
             EqualIgnoringNewLineDifferences(expected, ex.GetJavaScriptErrorString());
+
+            Assert.Equal(2, ex.Location.Start.Line);
+            Assert.Equal(21, ex.Location.Start.Column);
         }
 
         // Verify #1202
@@ -307,8 +337,7 @@ try {
 }
 ";
             var stack = engine.Evaluate(Script).AsString();
-            EqualIgnoringNewLineDifferences(@"   at Error <anonymous>:3:21
-   at throwIt (message) <anonymous>:3:15
+            EqualIgnoringNewLineDifferences(@"   at throwIt (message) <anonymous>:3:11
    at <anonymous>:11:5", stack);
 
             static void Handler(Action callback)

Jint/Native/Error/ErrorConstructor.cs

@@ -57,8 +57,7 @@ namespace Jint.Native.Error
                 o.DefinePropertyOrThrow("message", msgDesc);
             }
 
-            var lastSyntaxNode = _engine.GetLastSyntaxNode();
-            var stackString = lastSyntaxNode == null ? Undefined : _engine.CallStack.BuildCallStackString(lastSyntaxNode.Location);
+            var stackString = BuildStackString();
             var stackDesc = new PropertyDescriptor(stackString, PropertyFlag.NonEnumerable);
             o.DefinePropertyOrThrow(CommonProperties.Stack, stackDesc);
 
@@ -67,6 +66,20 @@ namespace Jint.Native.Error
             o.InstallErrorCause(options);
 
             return o;
+
+            JsValue BuildStackString()
+            {
+                var lastSyntaxNode = _engine.GetLastSyntaxNode();
+                if (lastSyntaxNode == null)
+                    return Undefined;
+
+                var callStack = _engine.CallStack;
+                var currentFunction = callStack.TryPeek(out var element) ? element.Function : null;
+
+                // If the current function is the ErrorConstructor itself (i.e. "throw new Error(...)" was called
+                // from script), exclude it from the stack trace, because the trace should begin at the throw point.
+                return callStack.BuildCallStackString(lastSyntaxNode.Location, currentFunction == this ? 1 : 0);
+            }
         }
     }
 }

Jint/Runtime/CallStack/JintCallStack.cs

@@ -1,6 +1,7 @@
 #nullable enable
 
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Text;
 using Esprima;
@@ -63,6 +64,11 @@ namespace Jint.Runtime.CallStack
             return item;
         }
 
+        public bool TryPeek([NotNullWhen(true)] out CallStackElement item)
+        {
+            return _stack.TryPeek(out item);
+        }
+
         public int Count => _stack._size;
 
         public void Clear()
@@ -76,7 +82,7 @@ namespace Jint.Runtime.CallStack
             return string.Join("->", _stack.Select(cse => cse.ToString()).Reverse());
         }
 
-        internal string BuildCallStackString(Location location)
+        internal string BuildCallStackString(Location location, int excludeTop = 0)
         {
             static void AppendLocation(
                 StringBuilder sb,
@@ -124,7 +130,7 @@ namespace Jint.Runtime.CallStack
             using var sb = StringBuilderPool.Rent();
 
             // stack is one frame behind function-wise when we start to process it from expression level
-            var index = _stack._size - 1;
+            var index = _stack._size - 1 - excludeTop;
             var element = index >= 0 ? _stack[index] : (CallStackElement?) null;
             var shortDescription = element?.ToString() ?? "";
 
@@ -171,4 +177,4 @@ namespace Jint.Runtime.CallStack
             return "?";
         }
     }
-}
+}

Jint/Runtime/Interpreter/Expressions/JintNewExpression.cs

@@ -58,9 +58,13 @@ namespace Jint.Runtime.Interpreter.Expressions
                 BuildArguments(context, _jintArguments, arguments);
             }
 
+            // Reset the location to the "new" keyword so that if an Error object is
+            // constructed below, the stack trace will capture the correct location.
+            context.LastSyntaxNode = _expression;
+
             if (!jsValue.IsConstructor)
             {
-                ExceptionHelper.ThrowTypeError(engine.Realm,  _calleeExpression.SourceText + " is not a constructor");
+                ExceptionHelper.ThrowTypeError(engine.Realm, _calleeExpression.SourceText + " is not a constructor");
             }
 
             // construct the new instance using the Function's constructor method

Jint/Runtime/Interpreter/Statements/JintThrowStatement.cs

@@ -21,8 +21,8 @@ namespace Jint.Runtime.Interpreter.Statements
 
         protected override Completion ExecuteInternal(EvaluationContext context)
         {
-            var jsValue = _argument.GetValue(context).Value;
-            return new Completion(CompletionType.Throw, jsValue, null, _statement.Location);
+            var completion = _argument.GetValue(context);
+            return new Completion(CompletionType.Throw, completion.Value, null, completion.Location);
         }
     }
-}
+}

Jint/Runtime/Interpreter/Statements/JintTryStatement.cs

@@ -33,7 +33,6 @@ namespace Jint.Runtime.Interpreter.Statements
         protected override Completion ExecuteInternal(EvaluationContext context)
         {
             var engine = context.Engine;
-            int callStackSizeBeforeExecution = engine.CallStack.Count;
 
             var b = _block.Execute(context);