Skip to content

Commit

Permalink
fix: BorrowMutError when evaluating expression in inspector console (d…
Browse files Browse the repository at this point in the history 
…enoland#5822)

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

Fixes: denoland#5807
  • Loading branch information
@piscisaureus
piscisaureus committed May 25, 2020
1 parent ee0b5bb commit ae81e69
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 70 deletions.
36 changes: 0 additions & 36 deletions cli/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -420,7 +417,6 @@ impl State {
target_lib: TargetLib::Main,
is_main: true,
is_internal,
inspector: None,
}));

Ok(Self(state))
Expand Down Expand Up @@ -457,7 +453,6 @@ impl State {
target_lib: TargetLib::Worker,
is_main: false,
is_internal: false,
inspector: None,
}));

Ok(Self(state))
Expand Down Expand Up @@ -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)
Expand Down
134 changes: 117 additions & 17 deletions cli/tests/integration_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 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);
Expand All @@ -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)
Expand Down Expand Up @@ -2269,7 +2271,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");
Expand Down Expand Up @@ -2330,7 +2335,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)
Expand Down Expand Up @@ -2386,8 +2396,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")
Expand Down Expand Up @@ -2426,7 +2440,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");
Expand Down Expand Up @@ -2505,16 +2522,99 @@ 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")
.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 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":"setTimeout(Deno.exit, 100)","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":""#),
];

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]
Expand Down
44 changes: 27 additions & 17 deletions cli/worker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -173,16 +185,14 @@ impl Worker {
self.external_channels.clone()
}

#[inline(always)]
fn inspector(&self) -> RefMut<Option<Box<DenoInspector>>> {
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()
Expand All @@ -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();
}
}

Expand All @@ -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)
}
Expand Down

0 comments on commit ae81e69

Please sign in to comment.