fix: BorrowMutError when evaluating expression in inspector console (#5822)

Note that this does not fix the 'Uncaught ReferenceError' issue that
happens when 'eager evaluation' is enabled in the inspector.

Fixes: #5807

cli/state.rs

@@ -3,7 +3,6 @@ use crate::file_fetcher::SourceFileFetcher;
 use crate::global_state::GlobalState;
 use crate::global_timer::GlobalTimer;
 use crate::import_map::ImportMap;
-use crate::inspector::DenoInspector;
 use crate::metrics::Metrics;
 use crate::op_error::OpError;
 use crate::ops::JsonOp;
@@ -12,7 +11,6 @@ use crate::permissions::Permissions;
 use crate::tsc::TargetLib;
 use crate::web_worker::WebWorkerHandle;
 use deno_core::Buf;
-use deno_core::CoreIsolate;
 use deno_core::ErrBox;
 use deno_core::ModuleLoadId;
 use deno_core::ModuleLoader;
@@ -61,7 +59,6 @@ pub struct StateInner {
   pub target_lib: TargetLib,
   pub is_main: bool,
   pub is_internal: bool,
-  pub inspector: Option<Box<DenoInspector>>,
 }
 
 impl State {
@@ -420,7 +417,6 @@ impl State {
       target_lib: TargetLib::Main,
       is_main: true,
       is_internal,
-      inspector: None,
     }));
 
     Ok(Self(state))
@@ -457,7 +453,6 @@ impl State {
       target_lib: TargetLib::Worker,
       is_main: false,
       is_internal: false,
-      inspector: None,
     }));
 
     Ok(Self(state))
@@ -526,37 +521,6 @@ impl State {
     }
   }
 
-  pub fn maybe_init_inspector(&self, isolate: &mut CoreIsolate) {
-    let mut state = self.borrow_mut();
-
-    if state.is_internal {
-      return;
-    };
-
-    let inspector_host = {
-      let global_state = &state.global_state;
-      match global_state
-        .flags
-        .inspect
-        .or(global_state.flags.inspect_brk)
-      {
-        Some(host) => host,
-        None => return,
-      }
-    };
-
-    let inspector = DenoInspector::new(isolate, inspector_host);
-    state.inspector.replace(inspector);
-  }
-
-  #[inline]
-  pub fn should_inspector_break_on_first_statement(&self) -> bool {
-    let state = self.borrow();
-    state.inspector.is_some()
-      && state.is_main
-      && state.global_state.flags.inspect_brk.is_some()
-  }
-
   #[cfg(test)]
   pub fn mock(main_module: &str) -> State {
     let module_specifier = ModuleSpecifier::resolve_url_or_path(main_module)

cli/tests/integration_tests.rs

@@ -2215,11 +2215,9 @@ fn inspect_flag_with_unique_port(flag_prefix: &str) -> String {
 }
 
 fn extract_ws_url_from_stderr(
-  stderr: &mut std::process::ChildStderr,
+  stderr_lines: &mut impl std::iter::Iterator<Item = String>,
 ) -> url::Url {
-  let mut stderr = std::io::BufReader::new(stderr);
+  let stderr_first_line = stderr_lines.next().unwrap();
-  let mut stderr_first_line = String::from("");
-  let _ = stderr.read_line(&mut stderr_first_line).unwrap();
   assert!(stderr_first_line.starts_with("Debugger listening on "));
   let v: Vec<_> = stderr_first_line.match_indices("ws:").collect();
   assert_eq!(v.len(), 1);
@@ -2238,8 +2236,12 @@ async fn inspector_connect() {
     .stderr(std::process::Stdio::piped())
     .spawn()
     .unwrap();
-  let ws_url = extract_ws_url_from_stderr(child.stderr.as_mut().unwrap());
+
-  println!("ws_url {}", ws_url);
+  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);
+
   // We use tokio_tungstenite as a websocket client because warp (which is
   // a dependency of Deno) uses it.
   let (_socket, response) = tokio_tungstenite::connect_async(ws_url)
@@ -2252,6 +2254,7 @@ async fn inspector_connect() {
 
 enum TestStep {
   StdOut(&'static str),
+  StdErr(&'static str),
   WsRecv(&'static str),
   WsSend(&'static str),
 }
@@ -2269,7 +2272,10 @@ async fn inspector_break_on_first_line() {
     .unwrap();
 
   let stderr = child.stderr.as_mut().unwrap();
-  let ws_url = extract_ws_url_from_stderr(stderr);
+  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
     .expect("Can't connect");
@@ -2313,6 +2319,7 @@ async fn inspector_break_on_first_line() {
       StdOut(s) => assert_eq!(&stdout_lines.next().unwrap(), s),
       WsRecv(s) => assert!(socket_rx.next().await.unwrap().starts_with(s)),
       WsSend(s) => socket_tx.send(s.into()).await.unwrap(),
+      _ => unreachable!(),
     }
   }
 
@@ -2330,7 +2337,12 @@ async fn inspector_pause() {
     .stderr(std::process::Stdio::piped())
     .spawn()
     .unwrap();
-  let ws_url = extract_ws_url_from_stderr(child.stderr.as_mut().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);
+
   // We use tokio_tungstenite as a websocket client because warp (which is
   // a dependency of Deno) uses it.
   let (mut socket, _) = tokio_tungstenite::connect_async(ws_url)
@@ -2386,8 +2398,12 @@ async fn inspector_port_collision() {
     .stderr(std::process::Stdio::piped())
     .spawn()
     .unwrap();
-  let ws_url_1 = extract_ws_url_from_stderr(child1.stderr.as_mut().unwrap());
+
-  println!("ws_url {}", ws_url_1);
+  let stderr_1 = child1.stderr.as_mut().unwrap();
+  let mut stderr_lines_1 = std::io::BufReader::new(stderr_1)
+    .lines()
+    .map(|r| r.unwrap());
+  let _ = extract_ws_url_from_stderr(&mut stderr_lines_1);
 
   let mut child2 = util::deno_cmd()
     .arg("run")
@@ -2426,7 +2442,10 @@ async fn inspector_does_not_hang() {
     .unwrap();
 
   let stderr = child.stderr.as_mut().unwrap();
-  let ws_url = extract_ws_url_from_stderr(stderr);
+  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
     .expect("Can't connect");
@@ -2505,16 +2524,100 @@ async fn inspector_without_brk_runs_code() {
     .stderr(std::process::Stdio::piped())
     .spawn()
     .unwrap();
-  extract_ws_url_from_stderr(child.stderr.as_mut().unwrap());
+
+  let stderr = child.stderr.as_mut().unwrap();
+  let mut stderr_lines =
+    std::io::BufReader::new(stderr).lines().map(|r| r.unwrap());
+  let _ = extract_ws_url_from_stderr(&mut stderr_lines);
 
   // Check that inspector actually runs code without waiting for inspector
-  // connection
+  // connection.
-  let mut stdout = std::io::BufReader::new(child.stdout.as_mut().unwrap());
+  let stdout = child.stdout.as_mut().unwrap();
-  let mut stdout_first_line = String::from("");
+  let mut stdout_lines =
-  let _ = stdout.read_line(&mut stdout_first_line).unwrap();
+    std::io::BufReader::new(stdout).lines().map(|r| r.unwrap());
-  assert_eq!(stdout_first_line, "hello\n");
+  let stdout_first_line = stdout_lines.next().unwrap();
+  assert_eq!(stdout_first_line, "hello");
 
   child.kill().unwrap();
+  child.wait().unwrap();
+}
+
+#[tokio::test]
+async fn inspector_runtime_evaluate_does_not_crash() {
+  let mut child = util::deno_cmd()
+    .arg("repl")
+    .arg(inspect_flag_with_unique_port("--inspect"))
+    .stdin(std::process::Stdio::piped())
+    .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())
+    .filter(|s| s.as_str() != "Debugger session started.");
+  let ws_url = extract_ws_url_from_stderr(&mut stderr_lines);
+
+  let (socket, response) = tokio_tungstenite::connect_async(ws_url)
+    .await
+    .expect("Can't connect");
+  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)
+    });
+
+  let stdin = child.stdin.take().unwrap();
+
+  let stdout = child.stdout.as_mut().unwrap();
+  let mut stdout_lines = std::io::BufReader::new(stdout)
+    .lines()
+    .map(|r| r.unwrap())
+    .filter(|s| !s.starts_with("Deno "));
+
+  use TestStep::*;
+  let test_steps = vec![
+    WsSend(r#"{"id":1,"method":"Runtime.enable"}"#),
+    WsSend(r#"{"id":2,"method":"Debugger.enable"}"#),
+    WsRecv(
+      r#"{"method":"Runtime.executionContextCreated","params":{"context":{"id":1,"#,
+    ),
+    WsRecv(r#"{"id":1,"result":{}}"#),
+    WsRecv(r#"{"id":2,"result":{"debuggerId":"#),
+    WsSend(r#"{"id":3,"method":"Runtime.runIfWaitingForDebugger"}"#),
+    WsRecv(r#"{"id":3,"result":{}}"#),
+    StdOut("exit using ctrl+d or close()"),
+    WsSend(
+      r#"{"id":4,"method":"Runtime.compileScript","params":{"expression":"Deno.cwd()","sourceURL":"","persistScript":false,"executionContextId":1}}"#,
+    ),
+    WsRecv(r#"{"id":4,"result":{}}"#),
+    WsSend(
+      r#"{"id":5,"method":"Runtime.evaluate","params":{"expression":"Deno.cwd()","objectGroup":"console","includeCommandLineAPI":true,"silent":false,"contextId":1,"returnByValue":true,"generatePreview":true,"userGesture":true,"awaitPromise":false,"replMode":true}}"#,
+    ),
+    WsRecv(r#"{"id":5,"result":{"result":{"type":"string","value":""#),
+    WsSend(
+      r#"{"id":6,"method":"Runtime.evaluate","params":{"expression":"console.error('done');","objectGroup":"console","includeCommandLineAPI":true,"silent":false,"contextId":1,"returnByValue":true,"generatePreview":true,"userGesture":true,"awaitPromise":false,"replMode":true}}"#,
+    ),
+    WsRecv(r#"{"id":6,"result":{"result":{"type":"undefined"}}}"#),
+    StdErr("done"),
+  ];
+
+  for step in test_steps {
+    match step {
+      StdOut(s) => assert_eq!(&stdout_lines.next().unwrap(), s),
+      StdErr(s) => assert_eq!(&stderr_lines.next().unwrap(), s),
+      WsRecv(s) => assert!(socket_rx.next().await.unwrap().starts_with(s)),
+      WsSend(s) => socket_tx.send(s.into()).await.unwrap(),
+    }
+  }
+
+  std::mem::drop(stdin);
+  child.wait().unwrap();
 }
 
 #[test]

cli/worker.rs

@@ -12,7 +12,6 @@ use futures::channel::mpsc;
 use futures::future::FutureExt;
 use futures::stream::StreamExt;
 use futures::task::AtomicWaker;
-use std::cell::RefMut;
 use std::env;
 use std::future::Future;
 use std::ops::Deref;
@@ -88,6 +87,7 @@ fn create_channels() -> (WorkerChannelsInternal, WorkerHandle) {
 pub struct Worker {
   pub name: String,
   pub isolate: Box<deno_core::EsIsolate>,
+  pub inspector: Option<Box<DenoInspector>>,
   pub state: State,
   pub waker: AtomicWaker,
   pub(crate) internal_channels: WorkerChannelsInternal,
@@ -99,18 +99,30 @@ impl Worker {
     let loader = Rc::new(state.clone());
     let mut isolate = deno_core::EsIsolate::new(loader, startup_data, false);
 
-    state.maybe_init_inspector(&mut isolate);
+    {
+      let global_state = state.borrow().global_state.clone();
+      isolate.set_js_error_create_fn(move |core_js_error| {
+        JSError::create(core_js_error, &global_state.ts_compiler)
+      });
+    }
 
-    let global_state = state.borrow().global_state.clone();
+    let inspector = {
-    isolate.set_js_error_create_fn(move |core_js_error| {
+      let state = state.borrow();
-      JSError::create(core_js_error, &global_state.ts_compiler)
+      let global_state = &state.global_state;
-    });
+      global_state
+        .flags
+        .inspect
+        .or(global_state.flags.inspect_brk)
+        .filter(|_| !state.is_internal)
+        .map(|inspector_host| DenoInspector::new(&mut isolate, inspector_host))
+    };
 
     let (internal_channels, external_channels) = create_channels();
 
     Self {
       name,
       isolate,
+      inspector,
       state,
       waker: AtomicWaker::new(),
       internal_channels,
@@ -173,16 +185,14 @@ impl Worker {
     self.external_channels.clone()
   }
 
-  #[inline(always)]
+  fn wait_for_inspector_session(&mut self) {
-  fn inspector(&self) -> RefMut<Option<Box<DenoInspector>>> {
+    let should_break_on_first_statement = self.inspector.is_some() && {
-    let state = self.state.borrow_mut();
+      let state = self.state.borrow();
-    RefMut::map(state, |s| &mut s.inspector)
+      state.is_main && state.global_state.flags.inspect_brk.is_some()
-  }
+    };
-
+    if should_break_on_first_statement {
-  fn wait_for_inspector_session(&self) {
-    if self.state.should_inspector_break_on_first_statement() {
       self
-        .inspector()
+        .inspector
         .as_mut()
         .unwrap()
         .wait_for_session_and_break_on_next_statement()
@@ -194,7 +204,7 @@ impl Drop for Worker {
   fn drop(&mut self) {
     // The Isolate object must outlive the Inspector object, but this is
     // currently not enforced by the type system.
-    self.inspector().take();
+    self.inspector.take();
   }
 }
 
@@ -205,7 +215,7 @@ impl Future for Worker {
     let inner = self.get_mut();
 
     // We always poll the inspector if it exists.
-    let _ = inner.inspector().as_mut().map(|i| i.poll_unpin(cx));
+    let _ = inner.inspector.as_mut().map(|i| i.poll_unpin(cx));
     inner.waker.register(cx.waker());
     inner.isolate.poll_unpin(cx)
   }