From cd78bd46c394eb4084244988793aab04107fbaed Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Sun, 24 May 2020 22:28:19 +0200 Subject: [PATCH] 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 | 36 --------- cli/tests/integration_tests.rs | 135 ++++++++++++++++++++++++++++----- cli/worker.rs | 44 ++++++----- 3 files changed, 145 insertions(+), 70 deletions(-) diff --git a/cli/state.rs b/cli/state.rs index 46fbdfd0b27191..baac89216ff87c 100644 --- a/cli/state.rs +++ b/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>, } 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) diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index b3f792b4e9bf3b..c643ca68ac6ce4 100644 --- a/cli/tests/integration_tests.rs +++ b/cli/tests/integration_tests.rs @@ -2208,11 +2208,9 @@ fn test_permissions_net_listen_allow_localhost() { } fn extract_ws_url_from_stderr( - stderr: &mut std::process::ChildStderr, + stderr_lines: &mut impl std::iter::Iterator, ) -> url::Url { - let mut stderr = std::io::BufReader::new(stderr); - let mut stderr_first_line = String::from(""); - let _ = stderr.read_line(&mut stderr_first_line).unwrap(); + let stderr_first_line = stderr_lines.next().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); @@ -2233,8 +2231,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) @@ -2266,7 +2268,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"); @@ -2329,7 +2334,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) @@ -2383,8 +2393,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") @@ -2425,7 +2439,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"); @@ -2506,16 +2523,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 - let mut stdout = std::io::BufReader::new(child.stdout.as_mut().unwrap()); - let mut stdout_first_line = String::from(""); - let _ = stdout.read_line(&mut stdout_first_line).unwrap(); - assert_eq!(stdout_first_line, "hello\n"); + // connection. + let stdout = child.stdout.as_mut().unwrap(); + let mut stdout_lines = + std::io::BufReader::new(stdout).lines().map(|r| r.unwrap()); + 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") + // Warning: each inspector test should be on its own port to avoid + // conflicting with another inspector test. + .arg("--inspect=127.0.0.1:9234") + .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 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":"Deno.exit()","objectGroup":"console","includeCommandLineAPI":true,"silent":false,"contextId":1,"returnByValue":true,"generatePreview":true,"userGesture":true,"awaitPromise":false,"replMode":true}}"#, + ), + ]; + + for step in test_steps { + match step { + 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(), + } + } + + for s in stderr_lines { + assert_eq!(&s, ""); + } + + child.wait().unwrap(); } #[test] diff --git a/cli/worker.rs b/cli/worker.rs index b66c1cb06b1548..66aa98e6021c9e 100644 --- a/cli/worker.rs +++ b/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, + pub inspector: Option>, 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(); - isolate.set_js_error_create_fn(move |core_js_error| { - JSError::create(core_js_error, &global_state.ts_compiler) - }); + let inspector = { + let state = state.borrow(); + 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 inspector(&self) -> RefMut>> { - let state = self.state.borrow_mut(); - RefMut::map(state, |s| &mut s.inspector) - } - - fn wait_for_inspector_session(&self) { - if self.state.should_inspector_break_on_first_statement() { + fn wait_for_inspector_session(&mut self) { + let should_break_on_first_statement = self.inspector.is_some() && { + let state = self.state.borrow(); + state.is_main && state.global_state.flags.inspect_brk.is_some() + }; + if should_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) }