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 <leehj0110@kakao.com>
This commit is contained in:
jin.2
+114
-95
@@ -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<AttachToTargetResult, String> = 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<i64> = Vec::new();
|
||||
let mut new_targets: Vec<TargetCreatedEvent> = Vec::new();
|
||||
let mut new_target_ids: HashSet<String> = HashSet::new();
|
||||
let mut changed_targets: Vec<TargetInfoChangedEvent> = Vec::new();
|
||||
let mut destroyed_targets: Vec<String> = 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<AttachToTargetResult, String> = 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 {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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,<h1>External Tab</h1>" })),
|
||||
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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user