Repository: denoland/deno_core (rust)

Issue:
`callSite.toString()` returns `"[object Object]"` since 0.299.0
```rs
use deno_core::{JsRuntime, RuntimeOptions};

fn main() {
  let code = r#"
    Error.prepareStackTrace = (error, structuredStackTrace) => {
      let stack = Error.prototype.toString.call(error);
      for (const callSite of structuredStackTrace) {
        stack += `\n    at ${callSite}`;
      }
      return stack;
    };

    Deno.core.ops.op_print(new Error().stack, false);
  "#;
  let mut runtime = JsRuntime::new(RuntimeOptions::default());
  runtime.execute_script("", code).unwrap();
}
```

Expected output:

```text
Error
    at <anonymous>:10:28
```

Actual output:

```text
Error
    at [object Object]
```


Pull request:
fix: Add missing `toString` to callsites, handle user provided eval origin
Fixes #854. Fixes https://github.com/denoland/deno/issues/24782 once this lands there.

The first issue is pretty straightforward, I didn't realize that the `Callsite` object had a `toString` method (though in retrospect it makes sense).

The second issue is that we try to do some parsing of the eval origin so we can source map it, but if the origin wasn't in the form we expected we would return undefined instead of leaving it alone. 

So in this case

```js
const AsyncFunction = Object.getPrototypeOf(async function () {
  // empty
}).constructor;

const func = new AsyncFunction(
  `return doesNotExist();
    //# sourceURL=empty.eval`,
);

func.call({});
```
We try to parse the `evalOrigin` (which is `"empty.eval"`) and since it's not in the expected format (the form that comes from v8 directly) we were returning `None` (which became `null` once it got to JS). Instead, we should just leave it alone and return it directly.

(It turns out that if users specify the `sourceURL` in evaled code it becomes the evalOrigin verbatim)

Patch:
diff --git a/core/error.rs b/core/error.rs
index 79988fd..fd83744 100644
--- a/core/error.rs
+++ b/core/error.rs
@@ -352,7 +352,10 @@ impl JsStackFrame {
 
     // apply source map to the eval origin, if the error originates from `eval`ed code
     let eval_origin = call!(GET_EVAL_ORIGIN: Option<String>).and_then(|o| {
-      let (before, (file, line, col), after) = parse_eval_origin(&o)?;
+      let Some((before, (file, line, col), after)) = parse_eval_origin(&o)
+      else {
+        return Some(o);
+      };
       let (file, line, col) =
         apply_source_map(&mut source_mapper, file.into(), line, col);
       Some(format!("{before}{file}:{line}:{col}{after}"))
@@ -950,6 +953,7 @@ v8_static_strings::v8_static_strings! {
   GET_PROMISE_INDEX = "getPromiseIndex",
   PREPARE_STACK_TRACE = "prepareStackTrace",
   ORIGINAL = "_orig",
+  TO_STRING = "toString",
 }
 
 #[inline(always)]
@@ -971,6 +975,51 @@ fn make_patched_callsite<'s>(
   out_obj
 }
 
+fn make_fn_template<'a, F>(
+  scope: &mut v8::HandleScope<'a>,
+  f: F,
+) -> v8::Local<'a, v8::FunctionTemplate>
+where
+  F: for<'s, 't> FnOnce(
+      &mut v8::HandleScope<'s>,
+      v8::FunctionCallbackArguments<'t>,
+      v8::ReturnValue<'t>,
+    ) + Copy,
+{
+  v8::FunctionBuilder::<v8::FunctionTemplate>::new(
+    move |scope: &mut v8::HandleScope<'_>,
+          args: v8::FunctionCallbackArguments<'_>,
+          rv: v8::ReturnValue<'_>| {
+      f(scope, args, rv);
+    },
+  )
+  .build(scope)
+}
+
+fn maybe_to_path_str(s: &str) -> Option<String> {
+  if s.starts_with("file://") {
+    Some(
+      Url::parse(s)
+        .unwrap()
+        .to_file_path()
+        .unwrap()
+        .to_string_lossy()
+        .into_owned(),
+    )
+  } else {
+    None
+  }
+}
+
+fn original_call_site<'s>(
+  scope: &mut v8::HandleScope<'s>,
+  object: v8::Local<v8::Object>,
+) -> v8::Local<'s, v8::Object> {
+  // lookup the original call site object
+  let orig_key = original_call_site_key(scope);
+  object.get_private(scope, orig_key).unwrap().cast()
+}
+
 /// Creates a template for a `Callsite`-like object, with
 /// a patched `getFileName`.
 /// Effectively:
@@ -996,29 +1045,25 @@ pub(crate) fn make_callsite_template<'s>(
   // effectively
   // `$field() { return this[kOriginalCallsite].$field() }`
   macro_rules! make_delegate {
-    ($scope: ident, $template: ident, [$($field: ident),+ $(,)?]) => {
+    ($($field: ident),+ $(,)?) => {
       $(
         {
-          let key = $field.v8_string($scope).into();
-          $template.set_with_attr(
+          let key = $field.v8_string(scope).into();
+          template.set_with_attr(
             key,
-            v8::FunctionBuilder::<v8::FunctionTemplate>::new(
-              |scope: &mut v8::HandleScope<'_>,
-              args: v8::FunctionCallbackArguments<'_>,
-              mut rv: v8::ReturnValue<'_>| {
-                let orig_key = original_call_site_key(scope);
-                let orig = args.this().get_private(scope, orig_key).unwrap();
+            make_fn_template(
+              scope,
+              |scope, args, mut rv| {
+                let orig = original_call_site(scope, args.this());
                 let key = $field.v8_string(scope).into();
                 let orig_ret = orig
-                  .cast::<v8::Object>()
                   .get(scope, key)
                   .unwrap()
                   .cast::<v8::Function>()
-                  .call(scope, orig, &[]);
+                  .call(scope, orig.into(), &[]);
                 rv.set(orig_ret.unwrap_or_else(|| v8::undefined(scope).into()));
               },
             )
-            .build($scope)
             .into(),
             v8::PropertyAttribute::DONT_DELETE
             | v8::PropertyAttribute::DONT_ENUM
@@ -1031,67 +1076,76 @@ pub(crate) fn make_callsite_template<'s>(
 
   // delegate to the original callsite object
   make_delegate!(
-    scope,
-    template,
-    [
-      // excludes getFileName, which we'll override below
-      GET_THIS,
-      GET_TYPE_NAME,
-      GET_FUNCTION,
-      GET_FUNCTION_NAME,
-      GET_METHOD_NAME,
-      GET_LINE_NUMBER,
-      GET_COLUMN_NUMBER,
-      GET_EVAL_ORIGIN,
-      IS_TOPLEVEL,
-      IS_EVAL,
-      IS_NATIVE,
-      IS_CONSTRUCTOR,
-      IS_ASYNC,
-      IS_PROMISE_ALL,
-      GET_PROMISE_INDEX,
-    ]
+    // excludes getFileName and toString, which we'll override below
+    GET_THIS,
+    GET_TYPE_NAME,
+    GET_FUNCTION,
+    GET_FUNCTION_NAME,
+    GET_METHOD_NAME,
+    GET_LINE_NUMBER,
+    GET_COLUMN_NUMBER,
+    GET_EVAL_ORIGIN,
+    IS_TOPLEVEL,
+    IS_EVAL,
+    IS_NATIVE,
+    IS_CONSTRUCTOR,
+    IS_ASYNC,
+    IS_PROMISE_ALL,
+    GET_PROMISE_INDEX,
   );
 
   let get_file_name_key = GET_FILE_NAME.v8_string(scope).into();
   template.set_with_attr(
     get_file_name_key,
-    v8::FunctionBuilder::<v8::FunctionTemplate>::new(
-      |scope: &mut v8::HandleScope<'_>,
-       args: v8::FunctionCallbackArguments<'_>,
-       mut rv: v8::ReturnValue<'_>| {
-        // lookup the original call site object
-        let orig_key = original_call_site_key(scope);
-        let orig = args.this().get_private(scope, orig_key).unwrap().cast();
-        // call getFileName
-        let orig_ret =
-          call_method::<v8::String>(scope, orig, GET_FILE_NAME, &[]);
-        if let Some(ret_val) = orig_ret {
-          // strip off `file://`
-          let string = ret_val.to_rust_string_lossy(scope);
-          let file_name = if string.starts_with("file://") {
-            Url::parse(&string)
-              .unwrap()
-              .to_file_path()
-              .unwrap()
-              .to_string_lossy()
-              .into_owned()
-          } else {
-            string
-          };
+    make_fn_template(scope, |scope, args, mut rv| {
+      // lookup the original call site object
+      let orig = original_call_site(scope, args.this());
+      // call getFileName
+      let orig_ret = call_method::<v8::String>(scope, orig, GET_FILE_NAME, &[]);
+      if let Some(ret_val) = orig_ret {
+        // strip off `file://`
+        let string = serde_v8::to_utf8(ret_val, scope);
+        let file_name = maybe_to_path_str(&string).unwrap_or(string);
+        let v8_str = crate::FastString::from(file_name).v8_string(scope).into();
+        rv.set(v8_str);
+      }
+    })
+    .into(),
+    v8::PropertyAttribute::DONT_DELETE
+      | v8::PropertyAttribute::DONT_ENUM
+      | v8::PropertyAttribute::READ_ONLY,
+  );
+
+  let to_string_key = TO_STRING.v8_string(scope).into();
+  template.set_with_attr(
+    to_string_key,
+    make_fn_template(scope, |scope, args, mut rv| {
+      let orig = original_call_site(scope, args.this());
+      // `this[kOriginalCallsite].toString()`
+      let orig_to_string_v8 =
+        call_method::<v8::String>(scope, orig, TO_STRING, &[]).unwrap();
+      let orig_to_string = serde_v8::to_utf8(orig_to_string_v8, scope);
+      // `this[kOriginalCallsite].getFileName()`
+      if let Some(orig_file_name) =
+        call_method::<v8::String>(scope, orig, GET_FILE_NAME, &[])
+      {
+        // replace file URL with file path in original `toString`
+        let orig_file_name = serde_v8::to_utf8(orig_file_name, scope);
+        if let Some(file_name) = maybe_to_path_str(&orig_file_name) {
+          let to_string = orig_to_string.replace(&orig_file_name, &file_name);
           let v8_str =
-            crate::FastString::from(file_name).v8_string(scope).into();
+            crate::FastString::from(to_string).v8_string(scope).into();
           rv.set(v8_str);
+          return;
         }
-      },
-    )
-    .build(scope)
+      }
+      rv.set(orig_to_string_v8.into());
+    })
     .into(),
     v8::PropertyAttribute::DONT_DELETE
       | v8::PropertyAttribute::DONT_ENUM
       | v8::PropertyAttribute::READ_ONLY,
   );
-
   template
 }
 


Test patch:
diff --git a/testing/integration/error_callsite/error_callsite.out b/testing/integration/error_callsite/error_callsite.out
new file mode 100644
index 0000000..183e4a1
--- /dev/null
+++ b/testing/integration/error_callsite/error_callsite.out
@@ -0,0 +1,32 @@
+{
+  "getTypeName": null,
+  "getFunctionName": "Foo",
+  "getMethodName": null,
+  "getFileName": "test:///integration/error_callsite/error_callsite.ts",
+  "getLineNumber": 36,
+  "getColumnNumber": 5,
+  "isToplevel": false,
+  "isEval": false,
+  "isNative": false,
+  "isConstructor": true,
+  "isAsync": false,
+  "isPromiseAll": false,
+  "getPromiseIndex": null
+}
+new Foo (test:///integration/error_callsite/error_callsite.ts:36:5)
+{
+  "getTypeName": null,
+  "getFunctionName": null,
+  "getMethodName": null,
+  "getFileName": "test:///integration/error_callsite/error_callsite.ts",
+  "getLineNumber": 39,
+  "getColumnNumber": 1,
+  "isToplevel": true,
+  "isEval": false,
+  "isNative": false,
+  "isConstructor": false,
+  "isAsync": false,
+  "isPromiseAll": false,
+  "getPromiseIndex": null
+}
+test:///integration/error_callsite/error_callsite.ts:39:1
diff --git a/testing/integration/error_callsite/error_callsite.ts b/testing/integration/error_callsite/error_callsite.ts
new file mode 100644
index 0000000..9477f86
--- /dev/null
+++ b/testing/integration/error_callsite/error_callsite.ts
@@ -0,0 +1,38 @@
+// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
+// deno-lint-ignore-file no-explicit-any
+function toObj(callsite: any) {
+  const keys = [
+    "getThis",
+    "getTypeName",
+    "getFunction",
+    "getFunctionName",
+    "getMethodName",
+    "getFileName",
+    "getLineNumber",
+    "getColumnNumber",
+    "getEvalOrigin",
+    "isToplevel",
+    "isEval",
+    "isNative",
+    "isConstructor",
+    "isAsync",
+    "isPromiseAll",
+    "getPromiseIndex",
+  ];
+  return Object.fromEntries(keys.map((key) => [key, callsite[key]()]));
+}
+(Error as any).prepareStackTrace = function (_: any, callsites: any) {
+  callsites.forEach((callsite: any) => {
+    console.log(toObj(callsite));
+    console.log(callsite.toString());
+  });
+  return callsites;
+};
+
+class Foo {
+  constructor() {
+    new Error().stack;
+  }
+}
+
+new Foo();
diff --git a/testing/integration/error_non_existent_eval_source/error_non_existent_eval_source.out b/testing/integration/error_non_existent_eval_source/error_non_existent_eval_source.out
new file mode 100644
index 0000000..c11aac7
--- /dev/null
+++ b/testing/integration/error_non_existent_eval_source/error_non_existent_eval_source.out
@@ -0,0 +1,3 @@
+[ERR] ReferenceError: doesNotExist is not defined
+[ERR]     at Object.eval (empty.eval, <anonymous>:3:1)
+[ERR]     at test:///integration/error_non_existent_eval_source/error_non_existent_eval_source.ts:11:6
diff --git a/testing/integration/error_non_existent_eval_source/error_non_existent_eval_source.ts b/testing/integration/error_non_existent_eval_source/error_non_existent_eval_source.ts
new file mode 100644
index 0000000..07b30ab
--- /dev/null
+++ b/testing/integration/error_non_existent_eval_source/error_non_existent_eval_source.ts
@@ -0,0 +1,11 @@
+// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
+const AsyncFunction = Object.getPrototypeOf(async function () {
+  // empty
+}).constructor;
+
+const func = new AsyncFunction(
+  `return doesNotExist();
+    //# sourceURL=empty.eval`,
+);
+
+func.call({});
diff --git a/testing/lib.rs b/testing/lib.rs
index 6314e80..e97df85 100644
--- a/testing/lib.rs
+++ b/testing/lib.rs
@@ -57,6 +57,8 @@ integration_test!(
   dyn_import_op,
   dyn_import_no_hang,
   error_async_stack,
+  error_callsite,
+  error_non_existent_eval_source,
   error_rejection_catch,
   error_rejection_order,
   error_eval_stack,