Repository: denoland/deno_core
Pull request #136 (2023-08-06 13:17:33)

fix: use call sites when checking for empty frames
Will fix ... https://github.com/denoland/deno/issues/20034.

As we know, `msg.get_stack_frames()` does not get populated properly for async op errors. That's why we use the 'call sites' passed to `prepareStackTrace()` as a source of truth instead. I had forgotten to account for that in one odd place.

bartlomieju: @nayeemrmn could you check if this change will help with https://github.com/denoland/deno/issues/18946?
nayeemrmn: > could you check if this change will help with [denoland/deno#18946](https://github.com/denoland/deno/issues/18946)?

It doesn't, this only has to do with cases where the original error stack is replaced with the location of a dynamic import it passed through because it was wrongly detected as empty.
bartlomieju: Any chance we could get a regression test for this?

Patch:
diff --git a/core/error.rs b/core/error.rs
index 4972ab9..0652b72 100644
--- a/core/error.rs
+++ b/core/error.rs
@@ -624,6 +624,30 @@ pub(crate) fn is_aggregate_error(
   false
 }
 
+/// Check if the error has a proper stack trace. The stack trace checked is the
+/// one passed to `prepareStackTrace()`, not `msg.get_stack_trace()`.
+pub(crate) fn has_call_site(
+  scope: &mut v8::HandleScope,
+  exception: v8::Local<v8::Value>,
+) -> bool {
+  if !exception.is_object() {
+    return false;
+  }
+  let exception = exception.to_object(scope).unwrap();
+  // Access error.stack to ensure that prepareStackTrace() has been called.
+  // This should populate error.__callSiteEvals.
+  get_property(scope, exception, "stack");
+  let frames_v8 = get_property(scope, exception, "__callSiteEvals");
+  let frames_v8: Option<v8::Local<v8::Array>> =
+    frames_v8.and_then(|a| a.try_into().ok());
+  if let Some(frames_v8) = frames_v8 {
+    if frames_v8.length() > 0 {
+      return true;
+    }
+  }
+  false
+}
+
 const DATA_URL_ABBREV_THRESHOLD: usize = 150;
 
 pub fn format_file_name(file_name: &str) -> String {
diff --git a/core/runtime/bindings.rs b/core/runtime/bindings.rs
index 1191491..6b32b59 100644
--- a/core/runtime/bindings.rs
+++ b/core/runtime/bindings.rs
@@ -6,6 +6,7 @@ use std::option::Option;
 use std::os::raw::c_void;
 use v8::MapFnTo;
 
+use crate::error::has_call_site;
 use crate::error::is_instance_of_error;
 use crate::error::throw_type_error;
 use crate::error::JsStackFrame;
@@ -392,8 +393,8 @@ fn catch_dynamic_import_promise_error(
   if is_instance_of_error(scope, arg) {
     let e: crate::error::NativeJsError = serde_v8::from_v8(scope, arg).unwrap();
     let name = e.name.unwrap_or_else(|| "Error".to_string());
-    let msg = v8::Exception::create_message(scope, arg);
-    if msg.get_stack_trace(scope).unwrap().get_frame_count() == 0 {
+    if !has_call_site(scope, arg) {
+      let msg = v8::Exception::create_message(scope, arg);
       let arg: v8::Local<v8::Object> = arg.try_into().unwrap();
       let message_key =
         v8::String::new_external_onebyte_static(scope, b"message").unwrap();


Test patch:
diff --git a/core/runtime/tests/misc.rs b/core/runtime/tests/misc.rs
index 1531201..66c6e5d 100644
--- a/core/runtime/tests/misc.rs
+++ b/core/runtime/tests/misc.rs
@@ -911,6 +911,52 @@ async fn test_stalled_tla() {
   assert_eq!(js_error.frames[0].column_number, Some(1));
 }
 
+// Regression test for https://github.com/denoland/deno/issues/20034.
+#[tokio::test]
+async fn test_dynamic_import_module_error_stack() {
+  #[op]
+  async fn op_async_error() -> Result<(), Error> {
+    tokio::time::sleep(std::time::Duration::from_millis(1)).await;
+    Err(crate::error::type_error("foo"))
+  }
+  deno_core::extension!(test_ext, ops = [op_async_error]);
+  let loader = StaticModuleLoader::new([
+    (
+      Url::parse("file:///main.js").unwrap(),
+      ascii_str!(
+        "
+        Error.prepareStackTrace = Deno.core.prepareStackTrace;
+        await import(\"file:///import.js\");
+        "
+      ),
+    ),
+    (
+      Url::parse("file:///import.js").unwrap(),
+      ascii_str!("await Deno.core.opAsync(\"op_async_error\");"),
+    ),
+  ]);
+  let mut runtime = JsRuntime::new(RuntimeOptions {
+    extensions: vec![test_ext::init_ops()],
+    module_loader: Some(Rc::new(loader)),
+    ..Default::default()
+  });
+
+  let module_id = runtime
+    .load_main_module(&crate::resolve_url("file:///main.js").unwrap(), None)
+    .await
+    .unwrap();
+  #[allow(clippy::let_underscore_future)]
+  let _ = runtime.mod_evaluate(module_id);
+
+  let error = runtime.run_event_loop(false).await.unwrap_err();
+  let js_error = error.downcast::<JsError>().unwrap();
+  assert_eq!(
+    js_error.to_string(),
+    "Error: foo
+    at async file:///import.js:1:1"
+  );
+}
+
 #[tokio::test]
 #[should_panic(
   expected = "Top-level await is not allowed in extensions (mod:tla:2:1)"