From 43d9c40bc41cb9e9f21fd4df5a46e36117f9162a Mon Sep 17 00:00:00 2001 From: "jin.2" Date: Mon, 30 Mar 2026 02:51:49 +0900 Subject: [PATCH] fix: detect externally opened tabs in --cdp mode (#1042) * fix: detect externally opened tabs in --cdp mode (#1037) Tabs opened outside of agent-browser (e.g. by the user or another CDP client) were invisible to `tab list` because: 1. `Target.targetCreated` with chrome://newtab/ was filtered by `is_internal_chrome_target`, and the subsequent `targetInfoChanged` with the real URL could not update a target that was never tracked. 2. The background drain loop only ran when `request_tracking || har_recording` was active, so target events between commands were silently dropped from the broadcast channel. Fix: promote untracked targets in `targetInfoChanged` to new targets, run the background drain unconditionally (guarded by browser presence), and extract `apply_drained_events` to share target lifecycle processing (attach, domain filter, iframe sessions) between execute_command and the background drain. * refactor: clean up HashSet import and remove call-site duplication - Import HashSet alongside HashMap instead of using fully-qualified path - Replace duplicated drain+apply sequence in execute_command with drain_cdp_events_background call * style: apply cargo fmt --------- Co-authored-by: hyunjinee --- cli/src/native/actions.rs | 209 ++++++++++++++++++++---------------- cli/src/native/daemon.rs | 4 +- cli/src/native/e2e_tests.rs | 74 +++++++++++++ 3 files changed, 190 insertions(+), 97 deletions(-) diff --git a/cli/src/native/actions.rs b/cli/src/native/actions.rs index 4c14004..781a6dc 100644 --- a/cli/src/native/actions.rs +++ b/cli/src/native/actions.rs @@ -1,5 +1,5 @@ use serde_json::{json, Value}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::env; use std::fs; use std::io::Write; @@ -454,8 +454,97 @@ impl DaemonState { recording::stop_recording_task(&mut self.recording_state).await } - pub fn drain_cdp_events_background(&mut self) { - let _ = self.drain_cdp_events(); + pub async fn drain_cdp_events_background(&mut self) { + let drained = self.drain_cdp_events(); + self.apply_drained_events(drained).await; + } + + async fn apply_drained_events(&mut self, drained: DrainedEvents) { + // ACK screencast frames + if !drained.pending_acks.is_empty() { + if let Some(ref browser) = self.browser { + if let Ok(session_id) = browser.active_session_id() { + for ack_sid in drained.pending_acks { + let _ = stream::ack_screencast_frame(&browser.client, session_id, ack_sid) + .await; + } + } + } + } + + // Remove destroyed targets + for target_id in &drained.destroyed_targets { + if let Some(ref mut mgr) = self.browser { + mgr.remove_page_by_target_id(target_id); + } + } + + // Track cross-origin iframe sessions + for (frame_id, iframe_sid) in &drained.attached_iframe_sessions { + self.iframe_sessions + .insert(frame_id.clone(), iframe_sid.clone()); + if let Some(ref mgr) = self.browser { + let _ = mgr + .client + .send_command_no_params("DOM.enable", Some(iframe_sid.as_str())) + .await; + let _ = mgr + .client + .send_command_no_params("Accessibility.enable", Some(iframe_sid.as_str())) + .await; + } + } + for sid in &drained.detached_iframe_sessions { + self.iframe_sessions.retain(|_, v| v != sid); + } + + // Attach and register new targets + for te in &drained.new_targets { + if let Some(ref mut mgr) = self.browser { + let attach_result: Result = mgr + .client + .send_command_typed( + "Target.attachToTarget", + &AttachToTargetParams { + target_id: te.target_info.target_id.clone(), + flatten: true, + }, + None, + ) + .await; + if let Ok(attach) = attach_result { + let _ = mgr.enable_domains_pub(&attach.session_id).await; + + // Install domain filter on new pages + let df = self.domain_filter.read().await; + if let Some(ref filter) = *df { + let has_proxy_creds = self.proxy_credentials.read().await.is_some(); + let _ = network::install_domain_filter( + &mgr.client, + &attach.session_id, + &filter.allowed_domains, + has_proxy_creds, + ) + .await; + } + + mgr.add_page(super::browser::PageInfo { + target_id: te.target_info.target_id.clone(), + session_id: attach.session_id, + url: te.target_info.url.clone(), + title: te.target_info.title.clone(), + target_type: te.target_info.target_type.clone(), + }); + } + } + } + + // Update changed targets + for te in &drained.changed_targets { + if let Some(ref mut mgr) = self.browser { + mgr.update_page_target_info(&te.target_info); + } + } } fn drain_cdp_events(&mut self) -> DrainedEvents { @@ -466,6 +555,7 @@ impl DaemonState { let mut pending_acks: Vec = Vec::new(); let mut new_targets: Vec = Vec::new(); + let mut new_target_ids: HashSet = HashSet::new(); let mut changed_targets: Vec = Vec::new(); let mut destroyed_targets: Vec = Vec::new(); let mut attached_iframe_sessions: Vec<(String, String)> = Vec::new(); @@ -486,6 +576,7 @@ impl DaemonState { .as_ref() .is_none_or(|b| b.has_target(&te.target_info.target_id)); if !already_tracked { + new_target_ids.insert(te.target_info.target_id.clone()); new_targets.push(te); } } @@ -497,7 +588,24 @@ impl DaemonState { event.params.clone(), ) { if should_track_target(&te.target_info) { - changed_targets.push(te); + // If this target is not yet tracked (e.g. it was + // initially filtered because its URL was + // chrome://newtab/), promote it to a new target + // so it gets attached and added to `pages`. + let already_tracked = self + .browser + .as_ref() + .is_some_and(|b| b.has_target(&te.target_info.target_id)); + if already_tracked + || new_target_ids.contains(&te.target_info.target_id) + { + changed_targets.push(te); + } else { + new_target_ids.insert(te.target_info.target_id.clone()); + new_targets.push(TargetCreatedEvent { + target_info: te.target_info, + }); + } } } continue; @@ -870,97 +978,8 @@ pub async fn execute_command(cmd: &Value, state: &mut DaemonState) -> Value { server.broadcast_command(action, &id, cmd); } - // Drain pending CDP events (console, errors, screencast frames, target lifecycle) - let DrainedEvents { - pending_acks, - new_targets, - changed_targets, - destroyed_targets, - attached_iframe_sessions, - detached_iframe_sessions, - } = state.drain_cdp_events(); - if !pending_acks.is_empty() { - if let Some(ref browser) = state.browser { - if let Ok(session_id) = browser.active_session_id() { - for ack_sid in pending_acks { - let _ = - stream::ack_screencast_frame(&browser.client, session_id, ack_sid).await; - } - } - } - } - - for target_id in &destroyed_targets { - if let Some(ref mut mgr) = state.browser { - mgr.remove_page_by_target_id(target_id); - } - } - - // Track cross-origin iframe sessions - for (frame_id, iframe_sid) in &attached_iframe_sessions { - state - .iframe_sessions - .insert(frame_id.clone(), iframe_sid.clone()); - if let Some(ref mgr) = state.browser { - let _ = mgr - .client - .send_command_no_params("DOM.enable", Some(iframe_sid.as_str())) - .await; - let _ = mgr - .client - .send_command_no_params("Accessibility.enable", Some(iframe_sid.as_str())) - .await; - } - } - for sid in &detached_iframe_sessions { - state.iframe_sessions.retain(|_, v| v != sid); - } - - for te in &new_targets { - if let Some(ref mut mgr) = state.browser { - let attach_result: Result = mgr - .client - .send_command_typed( - "Target.attachToTarget", - &AttachToTargetParams { - target_id: te.target_info.target_id.clone(), - flatten: true, - }, - None, - ) - .await; - if let Ok(attach) = attach_result { - let _ = mgr.enable_domains_pub(&attach.session_id).await; - - // Install domain filter on new pages - let df = state.domain_filter.read().await; - if let Some(ref filter) = *df { - let has_proxy_creds = state.proxy_credentials.read().await.is_some(); - let _ = network::install_domain_filter( - &mgr.client, - &attach.session_id, - &filter.allowed_domains, - has_proxy_creds, - ) - .await; - } - - mgr.add_page(super::browser::PageInfo { - target_id: te.target_info.target_id.clone(), - session_id: attach.session_id, - url: te.target_info.url.clone(), - title: te.target_info.title.clone(), - target_type: te.target_info.target_type.clone(), - }); - } - } - } - - for te in &changed_targets { - if let Some(ref mut mgr) = state.browser { - mgr.update_page_target_info(&te.target_info); - } - } + // Drain and apply pending CDP events (console, errors, screencast frames, target lifecycle) + state.drain_cdp_events_background().await; // Hot-reload and check action policy if let Some(ref mut policy) = state.policy { diff --git a/cli/src/native/daemon.rs b/cli/src/native/daemon.rs index 8b1c850..62a93e4 100644 --- a/cli/src/native/daemon.rs +++ b/cli/src/native/daemon.rs @@ -188,8 +188,8 @@ async fn run_socket_server( } _ = drain_interval.tick() => { let mut s = state.lock().await; - if s.request_tracking || s.har_recording { - s.drain_cdp_events_background(); + if s.browser.is_some() { + s.drain_cdp_events_background().await; } } _ = async { diff --git a/cli/src/native/e2e_tests.rs b/cli/src/native/e2e_tests.rs index c7ff8cf..7c9483c 100644 --- a/cli/src/native/e2e_tests.rs +++ b/cli/src/native/e2e_tests.rs @@ -3599,3 +3599,77 @@ async fn e2e_headers_case_insensitive_no_duplicates() { let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await; assert_success(&resp); } + +// --------------------------------------------------------------------------- +// Regression: externally opened tabs must appear in tab_list (#1037) +// +// When connected to Chrome (launched or via --cdp), a tab opened outside of +// agent-browser (e.g. by the user or another CDP client) should be detected +// and listed. Previously, chrome://newtab/ was filtered by +// is_internal_chrome_target, and Target.targetInfoChanged for untracked +// targets was silently ignored. +// --------------------------------------------------------------------------- + +#[tokio::test] +#[ignore] +async fn e2e_externally_opened_tab_detected() { + let mut state = DaemonState::new(); + + // Launch headless Chrome + let resp = execute_command( + &json!({ "id": "1", "action": "launch", "headless": true }), + &mut state, + ) + .await; + assert_success(&resp); + + // Verify initial tab count + let resp = execute_command(&json!({ "id": "2", "action": "tab_list" }), &mut state).await; + assert_success(&resp); + let initial_count = get_data(&resp)["tabs"].as_array().unwrap().len(); + + // Simulate an external client opening a new tab via the browser-level CDP + // session (no sessionId). This mirrors what happens when a user manually + // opens a tab while agent-browser is connected via --cdp. + let browser = state.browser.as_ref().expect("browser should be launched"); + let _: Value = browser + .client + .send_command( + "Target.createTarget", + Some(json!({ "url": "data:text/html,

External Tab

" })), + None, // browser-level session + ) + .await + .expect("Target.createTarget should succeed"); + + // Give Chrome a moment to fire targetCreated / targetInfoChanged events + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + + // Drain events by issuing tab_list — this triggers execute_command's + // drain_cdp_events path which processes new and changed targets. + let resp = execute_command(&json!({ "id": "3", "action": "tab_list" }), &mut state).await; + assert_success(&resp); + let tabs = get_data(&resp)["tabs"].as_array().unwrap(); + + assert_eq!( + tabs.len(), + initial_count + 1, + "Externally opened tab should appear in tab_list, got: {:?}", + tabs, + ); + + // Verify the new tab's URL is the data URL we navigated to + let new_tab = tabs.iter().find(|t| { + t["url"] + .as_str() + .is_some_and(|u| u.starts_with("data:text/html")) + }); + assert!( + new_tab.is_some(), + "Should find the externally opened tab by URL, tabs: {:?}", + tabs, + ); + + let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await; + assert_success(&resp); +}