From 154405ff3b46b5471d087d3cfec7df4737dd4ed8 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Tue, 14 Oct 2025 23:05:19 -0400 Subject: [PATCH] Fix a couple of bugs in remote browser debugging implementation (#40225) Follow-up to #39248 - Correctly forward ports over SSH, including the port from the debug scenario's `url` - Give the companion time to start up, instead of bailing if the first connection attempt fails Release Notes: - Fixed not being able to launch a browser debugging session in an SSH project. --- crates/project/src/debugger/session.rs | 128 ++++++++++++++++--------- crates/remote/src/remote_client.rs | 30 +++--- crates/remote/src/transport/ssh.rs | 19 ++-- crates/remote/src/transport/wsl.rs | 6 +- 4 files changed, 108 insertions(+), 75 deletions(-) diff --git a/crates/project/src/debugger/session.rs b/crates/project/src/debugger/session.rs index 19c088e6e8767bd56bf19759fbddd9947c4ef0ba..b5fbfd80d6152faf9d04715138859dc565e8cba8 100644 --- a/crates/project/src/debugger/session.rs +++ b/crates/project/src/debugger/session.rs @@ -14,12 +14,13 @@ use super::dap_command::{ TerminateCommand, TerminateThreadsCommand, ThreadsCommand, VariablesCommand, }; use super::dap_store::DapStore; -use anyhow::{Context as _, Result, anyhow}; +use anyhow::{Context as _, Result, anyhow, bail}; use base64::Engine; use collections::{HashMap, HashSet, IndexMap}; use dap::adapters::{DebugAdapterBinary, DebugAdapterName}; use dap::messages::Response; use dap::requests::{Request, RunInTerminal, StartDebugging}; +use dap::transport::TcpTransport; use dap::{ Capabilities, ContinueArguments, EvaluateArgumentsContext, Module, Source, StackFrameId, SteppingGranularity, StoppedEvent, VariableReference, @@ -47,12 +48,14 @@ use remote::RemoteClient; use rpc::ErrorExt; use serde::{Deserialize, Serialize}; use serde_json::Value; -use smol::net::TcpListener; +use smol::net::{TcpListener, TcpStream}; use std::any::TypeId; use std::collections::BTreeMap; +use std::net::Ipv4Addr; use std::ops::RangeInclusive; use std::path::PathBuf; use std::process::Stdio; +use std::time::Duration; use std::u64; use std::{ any::Any, @@ -63,6 +66,7 @@ use std::{ }; use task::TaskContext; use text::{PointUtf16, ToPointUtf16}; +use url::Url; use util::command::new_smol_command; use util::{ResultExt, debug_panic, maybe}; use worktree::Worktree; @@ -2768,31 +2772,42 @@ impl Session { let mut console_output = self.console_output(cx); let task = cx.spawn(async move |this, cx| { - let (dap_port, _child) = - if remote_client.read_with(cx, |client, _| client.shares_network_interface())? { - (request.server_port, None) - } else { - let port = { - let listener = TcpListener::bind("127.0.0.1:0") - .await - .context("getting port for DAP")?; - listener.local_addr()?.port() - }; - let child = remote_client.update(cx, |client, _| { - let command = client.build_forward_port_command( - port, - "localhost".into(), - request.server_port, - )?; - let child = new_smol_command(command.program) - .args(command.args) - .envs(command.env) - .spawn() - .context("spawning port forwarding process")?; - anyhow::Ok(child) - })??; - (port, Some(child)) - }; + let forward_ports_process = if remote_client + .read_with(cx, |client, _| client.shares_network_interface())? + { + request.other.insert( + "proxyUri".into(), + format!("127.0.0.1:{}", request.server_port).into(), + ); + None + } else { + let port = TcpTransport::unused_port(Ipv4Addr::LOCALHOST) + .await + .context("getting port for DAP")?; + request + .other + .insert("proxyUri".into(), format!("127.0.0.1:{port}").into()); + let mut port_forwards = vec![(port, "localhost".to_owned(), request.server_port)]; + + if let Some(value) = request.params.get("url") + && let Some(url) = value.as_str() + && let Some(url) = Url::parse(url).ok() + && let Some(frontend_port) = url.port() + { + port_forwards.push((frontend_port, "localhost".to_owned(), frontend_port)); + } + + let child = remote_client.update(cx, |client, _| { + let command = client.build_forward_ports_command(port_forwards)?; + let child = new_smol_command(command.program) + .args(command.args) + .envs(command.env) + .spawn() + .context("spawning port forwarding process")?; + anyhow::Ok(child) + })??; + Some(child) + }; let mut companion_process = None; let companion_port = @@ -2814,14 +2829,17 @@ impl Session { } } }; - this.update(cx, |this, cx| { - this.companion_port = Some(companion_port); - let Some(mut child) = companion_process else { - return; - }; - if let Some(stderr) = child.stderr.take() { + + let mut background_tasks = Vec::new(); + if let Some(mut forward_ports_process) = forward_ports_process { + background_tasks.push(cx.spawn(async move |_| { + forward_ports_process.status().await.log_err(); + })); + }; + if let Some(mut companion_process) = companion_process { + if let Some(stderr) = companion_process.stderr.take() { let mut console_output = console_output.clone(); - this.background_tasks.push(cx.spawn(async move |_, _| { + background_tasks.push(cx.spawn(async move |_| { let mut stderr = BufReader::new(stderr); let mut line = String::new(); while let Ok(n) = stderr.read_line(&mut line).await @@ -2835,9 +2853,9 @@ impl Session { } })); } - this.background_tasks.push(cx.spawn({ + background_tasks.push(cx.spawn({ let mut console_output = console_output.clone(); - async move |_, _| match child.status().await { + async move |_| match companion_process.status().await { Ok(status) => { if status.success() { console_output @@ -2860,17 +2878,33 @@ impl Session { .ok(); } } - })) - })?; + })); + } - request - .other - .insert("proxyUri".into(), format!("127.0.0.1:{dap_port}").into()); // TODO pass wslInfo as needed + let companion_address = format!("127.0.0.1:{companion_port}"); + let mut companion_started = false; + for _ in 0..10 { + if TcpStream::connect(&companion_address).await.is_ok() { + companion_started = true; + break; + } + cx.background_executor() + .timer(Duration::from_millis(100)) + .await; + } + if !companion_started { + console_output + .send("Browser companion failed to start".into()) + .await + .ok(); + bail!("Browser companion failed to start"); + } + let response = http_client .post_json( - &format!("http://127.0.0.1:{companion_port}/launch-and-attach"), + &format!("http://{companion_address}/launch-and-attach"), serde_json::to_string(&request) .context("serializing request")? .into(), @@ -2895,6 +2929,11 @@ impl Session { } } + this.update(cx, |this, _| { + this.background_tasks.extend(background_tasks); + this.companion_port = Some(companion_port); + })?; + anyhow::Ok(()) }); self.background_tasks.push(cx.spawn(async move |_, _| { @@ -2926,15 +2965,16 @@ impl Session { } } -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug)] #[serde(rename_all = "camelCase")] struct LaunchBrowserInCompanionParams { server_port: u16, + params: HashMap, #[serde(flatten)] other: HashMap, } -#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Debug)] #[serde(rename_all = "camelCase")] struct KillCompanionBrowserParams { launch_id: u64, diff --git a/crates/remote/src/remote_client.rs b/crates/remote/src/remote_client.rs index b294a53639a494844ea7cff94906ead892668158..0fea4726e7aafb5f3f04614b76f1ed5dd2d0a965 100644 --- a/crates/remote/src/remote_client.rs +++ b/crates/remote/src/remote_client.rs @@ -836,16 +836,14 @@ impl RemoteClient { connection.build_command(program, args, env, working_dir, port_forward) } - pub fn build_forward_port_command( + pub fn build_forward_ports_command( &self, - local_port: u16, - host: String, - remote_port: u16, + forwards: Vec<(u16, String, u16)>, ) -> Result { let Some(connection) = self.remote_connection() else { return Err(anyhow!("no ssh connection")); }; - connection.build_forward_port_command(local_port, host, remote_port) + connection.build_forward_ports_command(forwards) } pub fn upload_directory( @@ -1116,11 +1114,9 @@ pub(crate) trait RemoteConnection: Send + Sync { working_dir: Option, port_forward: Option<(u16, String, u16)>, ) -> Result; - fn build_forward_port_command( + fn build_forward_ports_command( &self, - local_port: u16, - remote: String, - remote_port: u16, + forwards: Vec<(u16, String, u16)>, ) -> Result; fn connection_options(&self) -> RemoteConnectionOptions; fn path_style(&self) -> PathStyle; @@ -1551,19 +1547,17 @@ mod fake { }) } - fn build_forward_port_command( + fn build_forward_ports_command( &self, - local_port: u16, - host: String, - remote_port: u16, + forwards: Vec<(u16, String, u16)>, ) -> anyhow::Result { Ok(CommandTemplate { program: "ssh".into(), - args: vec![ - "-N".into(), - "-L".into(), - format!("{local_port}:{host}:{remote_port}"), - ], + args: std::iter::once("-N".to_owned()) + .chain(forwards.into_iter().map(|(local_port, host, remote_port)| { + format!("{local_port}:{host}:{remote_port}") + })) + .collect(), env: Default::default(), }) } diff --git a/crates/remote/src/transport/ssh.rs b/crates/remote/src/transport/ssh.rs index ae2987ec34c4e70535143b791ac3c9b3a1d7364c..40b5b39b68ae043c9d7ae954d1a6862fa1ae8488 100644 --- a/crates/remote/src/transport/ssh.rs +++ b/crates/remote/src/transport/ssh.rs @@ -146,19 +146,20 @@ impl RemoteConnection for SshRemoteConnection { ) } - fn build_forward_port_command( + fn build_forward_ports_command( &self, - local_port: u16, - host: String, - remote_port: u16, + forwards: Vec<(u16, String, u16)>, ) -> Result { + let Self { socket, .. } = self; + let mut args = socket.ssh_args(); + args.push("-N".into()); + for (local_port, host, remote_port) in forwards { + args.push("-L".into()); + args.push(format!("{local_port}:{host}:{remote_port}")); + } Ok(CommandTemplate { program: "ssh".into(), - args: vec![ - "-N".into(), - "-L".into(), - format!("{local_port}:{host}:{remote_port}"), - ], + args, env: Default::default(), }) } diff --git a/crates/remote/src/transport/wsl.rs b/crates/remote/src/transport/wsl.rs index 6a8ed48bc98cacdff8e9fac4872cd9ba494d3402..5d6dadb7adc11331938dd6e4f895877aa8dc3802 100644 --- a/crates/remote/src/transport/wsl.rs +++ b/crates/remote/src/transport/wsl.rs @@ -485,11 +485,9 @@ impl RemoteConnection for WslRemoteConnection { }) } - fn build_forward_port_command( + fn build_forward_ports_command( &self, - _: u16, - _: String, - _: u16, + _: Vec<(u16, String, u16)>, ) -> anyhow::Result { Err(anyhow!("WSL shares a network interface with the host")) }