From 79285fa83bfbbef55d8afa8f28d11ae4a0b21927 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= <biwanczuk@gmail.com>
Date: Tue, 6 Dec 2022 02:00:10 +0100
Subject: [PATCH] npm: ensure runtime exceptions are surfaced when debugger is
 attached (#16943)

Currently runtime exception are only displayed at the program end in
terminal, which makes it only a partial fix, as a full fix requires
https://github.com/denoland/rusty_v8/pull/1149 which adds new bindings
to the inspector that allows to notify it about thrown exceptions.
This will be handled in a follow up commit.
---
 cli/tests/inspector_tests.rs                  | 85 +++++++++++++++++++
 .../inspector/error_with_npm_import.js        |  7 ++
 core/ops_builtin_v8.rs                        | 11 +--
 3 files changed, 98 insertions(+), 5 deletions(-)
 create mode 100644 cli/tests/testdata/inspector/error_with_npm_import.js

diff --git a/cli/tests/inspector_tests.rs b/cli/tests/inspector_tests.rs
index fd3e886e4c..8a1bc141fb 100644
--- a/cli/tests/inspector_tests.rs
+++ b/cli/tests/inspector_tests.rs
@@ -1303,4 +1303,89 @@ mod inspector {
     child.kill().unwrap();
     child.wait().unwrap();
   }
+
+  #[tokio::test]
+  async fn inspector_error_with_npm_import() {
+    let script =
+      util::testdata_path().join("inspector/error_with_npm_import.js");
+    let _server = http_server();
+
+    let mut child = util::deno_cmd()
+      .arg("run")
+      .arg("--quiet")
+      .arg("-A")
+      .arg(inspect_flag_with_unique_port("--inspect-brk"))
+      .arg(script)
+      .envs(util::env_vars_for_npm_tests())
+      .stdout(std::process::Stdio::piped())
+      .stderr(std::process::Stdio::piped())
+      .spawn()
+      .unwrap();
+
+    let stderr = child.stderr.as_mut().unwrap();
+    let mut stderr_lines =
+      std::io::BufReader::new(stderr).lines().map(|r| r.unwrap());
+    let ws_url = extract_ws_url_from_stderr(&mut stderr_lines);
+
+    let (socket, response) =
+      tokio_tungstenite::connect_async(ws_url).await.unwrap();
+    assert_eq!(response.status(), 101); // Switching protocols.
+
+    let (mut socket_tx, socket_rx) = socket.split();
+    let mut socket_rx = socket_rx
+      .map(|msg| msg.unwrap().to_string())
+      .filter(|msg| {
+        let pass = !msg.starts_with(r#"{"method":"Debugger.scriptParsed","#);
+        futures::future::ready(pass)
+      })
+      .boxed_local();
+
+    assert_stderr_for_inspect_brk(&mut stderr_lines);
+
+    assert_inspector_messages(
+      &mut socket_tx,
+      &[
+        r#"{"id":1,"method":"Runtime.enable"}"#,
+        r#"{"id":2,"method":"Debugger.enable"}"#,
+      ],
+      &mut socket_rx,
+      &[
+        r#"{"id":1,"result":{}}"#,
+        r#"{"id":2,"result":{"debuggerId":"#,
+      ],
+      &[
+        r#"{"method":"Runtime.executionContextCreated","params":{"context":{"id":1,"#,
+      ],
+    )
+    .await;
+
+    assert_inspector_messages(
+      &mut socket_tx,
+      &[r#"{"id":3,"method":"Runtime.runIfWaitingForDebugger"}"#],
+      &mut socket_rx,
+      &[r#"{"id":3,"result":{}}"#],
+      &[r#"{"method":"Debugger.paused","#],
+    )
+    .await;
+
+    assert_inspector_messages(
+      &mut socket_tx,
+      &[r#"{"id":4,"method":"Debugger.resume"}"#],
+      &mut socket_rx,
+      &[r#"{"id":4,"result":{}}"#],
+      &[],
+    )
+    .await;
+
+    // TODO(bartlomieju): this is a partial fix, we should assert that
+    // "Runtime.exceptionThrown" notification was sent, but a bindings for this
+    // notification is not yet there
+    assert_eq!(&stderr_lines.next().unwrap(), "Debugger session started.");
+    assert_eq!(
+      &stderr_lines.next().unwrap(),
+      "error: Uncaught Error: boom!"
+    );
+
+    assert_eq!(child.wait().unwrap().code(), Some(1));
+  }
 }
diff --git a/cli/tests/testdata/inspector/error_with_npm_import.js b/cli/tests/testdata/inspector/error_with_npm_import.js
new file mode 100644
index 0000000000..9244f2cf2f
--- /dev/null
+++ b/cli/tests/testdata/inspector/error_with_npm_import.js
@@ -0,0 +1,7 @@
+// deno-lint-ignore-file
+
+import chalk from "npm:chalk";
+
+console.log("hello");
+
+throw new Error("boom!");
diff --git a/core/ops_builtin_v8.rs b/core/ops_builtin_v8.rs
index cfbb3eba48..5f4f875ee6 100644
--- a/core/ops_builtin_v8.rs
+++ b/core/ops_builtin_v8.rs
@@ -770,12 +770,13 @@ fn op_dispatch_exception(
     scope.terminate_execution();
     return;
   }
-  match state.inspector().try_borrow() {
-    Ok(inspector) if !inspector.has_active_sessions() => {
-      scope.terminate_execution();
-    }
+
+  // FIXME(bartlomieju): I'm not sure if this assumption is valid... Maybe when
+  // inspector is polling on pause?
+  if state.inspector().try_borrow().is_ok() {
+    scope.terminate_execution();
+  } else {
     // If the inspector is borrowed at this time, assume an inspector is active.
-    _ => {}
   }
 }