From 8dede71382a87d3caf16e1676df991374d1acc08 Mon Sep 17 00:00:00 2001 From: Saim Shafique Date: Tue, 10 Nov 2026 08:42:41 +0000 Subject: [PATCH 1/3] fix(dialog): auto-dismiss alerts reliably and fail fast on dialog-blocked reads On Windows, alert() under the default auto-dialog handling wedged the tab: the auto-dismiss task read dialog-opening events from the shared broadcast receiver, which can lag and drop the opening event. A dropped event meant the dialog was never dismissed, so the triggering command and every later command timed out after ~30 s with os error 10060 until close reported page_wedged. `dialog status` also reported no dialog, since auto-handled dialogs were never tracked. Listen for Page.javascriptDialogOpening on a lossless method tap instead of the shared broadcast, and track every dialog in pending_dialog, including the ones the background task auto-dismisses. The dismiss usually lands first and the closed event clears the tracking on the next drain; but if the dismiss is missed or fails, the dialog stays visible to `dialog status`, the fail-fast gate, and the timeout path, so an open alert no longer looks like a wedged tab. Also fail fast for `url` and `title` while a dialog is open: both evaluate JavaScript in the page, so they hung until the client read timeout instead of reporting the blocking dialog. `dialog`, `screenshot`, and tab management stay usable during a dialog. Fixes #2511 --- cli/src/native/actions.rs | 163 ++++++++++++++++++++---------------- cli/src/native/e2e_tests.rs | 123 +++++++++++++++++++++++++++ 2 files changed, 215 insertions(+), 71 deletions(-) diff --git a/cli/src/native/actions.rs b/cli/src/native/actions.rs index e7829cd093..84e160cdc6 100644 --- a/cli/src/native/actions.rs +++ b/cli/src/native/actions.rs @@ -1155,43 +1155,44 @@ impl DaemonState { }; let client = browser.client.clone(); - let mut rx = browser.client.subscribe(); + // A lossless method tap, not the shared broadcast: a broadcast + // receiver can lag and drop the opening event, and a dropped event + // means the dialog is never dismissed and the tab wedges (#2511). + let mut rx = browser + .client + .subscribe_methods(&["Page.javascriptDialogOpening"]); self.dialog_handler_task = Some(tokio::spawn(async move { - loop { - match rx.recv().await { - Ok(event) if event.method == "Page.javascriptDialogOpening" => { - let dialog_type = event - .params - .get("type") - .and_then(|v| v.as_str()) - .unwrap_or(""); - if matches!(dialog_type, "beforeunload" | "alert") { - let message = event - .params - .get("message") - .and_then(|v| v.as_str()) - .unwrap_or(""); - eprintln!("[auto-dismiss] {} dialog: {}", dialog_type, message); - let sid = event.session_id.clone().unwrap_or_default(); - if let Err(e) = client - .send_command( - "Page.handleJavaScriptDialog", - Some(json!({ "accept": true })), - Some(&sid), - ) - .await - { - eprintln!( - "[auto-dismiss] failed to dismiss {} dialog: {}", - dialog_type, e - ); - } - } + while let Some(event) = rx.recv().await { + if event.method != "Page.javascriptDialogOpening" { + continue; + } + let dialog_type = event + .params + .get("type") + .and_then(|v| v.as_str()) + .unwrap_or(""); + if matches!(dialog_type, "beforeunload" | "alert") { + let message = event + .params + .get("message") + .and_then(|v| v.as_str()) + .unwrap_or(""); + eprintln!("[auto-dismiss] {} dialog: {}", dialog_type, message); + let sid = event.session_id.clone().unwrap_or_default(); + if let Err(e) = client + .send_command( + "Page.handleJavaScriptDialog", + Some(json!({ "accept": true })), + Some(&sid), + ) + .await + { + eprintln!( + "[auto-dismiss] failed to dismiss {} dialog: {}", + dialog_type, e + ); } - Ok(_) => continue, - Err(broadcast::error::RecvError::Lagged(_)) => continue, - Err(_) => break, } } })); @@ -2260,23 +2261,22 @@ impl DaemonState { event.params.clone(), ) { - // When auto_dialog is enabled, alert and beforeunload - // dialogs are handled by the background dialog_handler_task. - // Skip tracking them to avoid a stale warning. - let auto_handled = self.auto_dialog - && matches!( - dialog_event.dialog_type.as_str(), - "beforeunload" | "alert" - ); - if !auto_handled { - self.pending_dialog = Some(PendingDialog { - dialog_type: dialog_event.dialog_type, - message: dialog_event.message, - url: dialog_event.url, - default_prompt: dialog_event.default_prompt, - session_id: event.session_id.clone(), - }); - } + // Track every dialog, including the ones the + // background dialog_handler_task auto-dismisses. + // The dismiss usually lands first and the + // closed event clears this on the next drain; + // but if the dismiss is missed or fails, the + // dialog must still be visible to `dialog + // status`, the fail-fast gate, and the timeout + // path, or an open alert looks exactly like a + // wedged tab (#2511). + self.pending_dialog = Some(PendingDialog { + dialog_type: dialog_event.dialog_type, + message: dialog_event.message, + url: dialog_event.url, + default_prompt: dialog_event.default_prompt, + session_id: event.session_id.clone(), + }); } } "Page.javascriptDialogClosed" => { @@ -3452,7 +3452,10 @@ async fn execute_command_inner(cmd: &Value, state: &mut DaemonState) -> Value { // A pending confirm/prompt dialog blocks the renderer's main thread, so // any command that touches the page would hang until the client read // timeout. Fail fast with instructions instead. Actions in skip_launch - // never touch the page; dialog/screenshot/url/title are browser-side. + // never touch the page; dialog/screenshot are browser-side. url and + // title evaluate JavaScript in the page, so they hang on a + // dialog-blocked renderer like any other page command and must fail + // fast too (#2511). // Only a dialog on the ACTIVE tab blocks: one on a background tab leaves // the active tab's renderer responsive. if let Some(ref dialog) = state.pending_dialog { @@ -3477,14 +3480,7 @@ async fn execute_command_inner(cmd: &Value, state: &mut DaemonState) -> Value { let safe_during_dialog = (skip_launch && !read_touches_active_tab) || matches!( action, - "dialog" - | "screenshot" - | "url" - | "title" - | "tab_list" - | "tab_new" - | "tab_switch" - | "tab_close" + "dialog" | "screenshot" | "tab_list" | "tab_new" | "tab_switch" | "tab_close" ); if on_active_tab && !safe_during_dialog { let mut resp = error_response( @@ -22078,17 +22074,42 @@ printf '%s' '{"protocol":"agent-browser.plugin.v1","success":true,"browser":{"cd } #[test] - fn test_pending_dialog_not_set_for_auto_handled_alert() { - // Simulate what handle_browser_event does: when auto_dialog is true, - // alert/beforeunload should NOT populate pending_dialog. - let auto_dialog = true; - for dialog_type in &["alert", "beforeunload"] { - let auto_handled = auto_dialog && matches!(*dialog_type, "beforeunload" | "alert"); - assert!( - auto_handled, - "{dialog_type} should be auto-handled when auto_dialog is true" - ); - } + fn test_pending_dialog_tracked_for_auto_handled_alert() { + // Auto-dismissed dialogs are still tracked in pending_dialog (#2511): + // the background dismiss usually lands first and the closed event + // clears this on the next drain, but a missed or failed dismiss must + // stay visible to `dialog status` and the fail-fast gate. + let mut state = DaemonState::new(); + let (events, receiver) = broadcast::channel(16); + state.event_rx = Some(receiver); + + events + .send(CdpEvent { + method: "Page.javascriptDialogOpening".to_string(), + params: json!({"type": "alert", "message": "hi", "url": "https://example.com/"}), + session_id: None, + }) + .unwrap(); + let _ = state.drain_cdp_events(); + let dialog = state + .pending_dialog + .as_ref() + .expect("an auto-handled alert must still be tracked"); + assert_eq!(dialog.dialog_type, "alert"); + assert_eq!(dialog.message, "hi"); + + events + .send(CdpEvent { + method: "Page.javascriptDialogClosed".to_string(), + params: json!({}), + session_id: None, + }) + .unwrap(); + let _ = state.drain_cdp_events(); + assert!( + state.pending_dialog.is_none(), + "a closed dialog must clear tracking" + ); } #[test] diff --git a/cli/src/native/e2e_tests.rs b/cli/src/native/e2e_tests.rs index 70939b19a7..bbcd8fa987 100644 --- a/cli/src/native/e2e_tests.rs +++ b/cli/src/native/e2e_tests.rs @@ -14834,6 +14834,129 @@ async fn e2e_dialog_blocked_tab_is_not_treated_as_wedged() { assert_success(&resp); } +#[tokio::test] +#[ignore] +async fn e2e_auto_dialog_accepts_alert_without_wedging_tab() { + let mut state = DaemonState::new(); + assert!(state.auto_dialog, "auto_dialog should be on by default"); + let resp = execute_command( + &json!({ "id": "1", "action": "launch", "headless": true }), + &mut state, + ) + .await; + assert_success(&resp); + let resp = execute_command( + &json!({ "id": "2", "action": "navigate", "url": "data:text/html,livelive" }), + &mut state, + ) + .await; + assert_success(&resp); + let target_before = state + .browser + .as_ref() + .unwrap() + .active_target_id() + .unwrap() + .to_string(); + + // alert() is auto-accepted: the eval must return instead of timing out, + // and the tab must survive instead of being replaced as wedged (#2511). + let resp = execute_command( + &json!({ "id": "3", "action": "evaluate", "script": "alert('hi'); 'after-alert'" }), + &mut state, + ) + .await; + assert_ne!( + resp["code"], "page_wedged", + "an auto-dismissed alert is not a wedge: {}", + resp + ); + assert_success(&resp); + assert_eq!(get_data(&resp)["result"], "after-alert"); + assert_eq!( + state.browser.as_ref().unwrap().active_target_id().unwrap(), + target_before, + "the alert must not replace the tab" + ); + + let resp = execute_command(&json!({ "id": "4", "action": "title" }), &mut state).await; + assert_success(&resp); + assert_eq!(get_data(&resp)["title"], "live"); + + let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await; + assert_success(&resp); +} + +#[tokio::test] +#[ignore] +async fn e2e_title_fails_fast_while_dialog_open() { + let mut state = DaemonState::new(); + state.auto_dialog = false; + let resp = execute_command( + &json!({ "id": "1", "action": "launch", "headless": true }), + &mut state, + ) + .await; + assert_success(&resp); + let resp = execute_command( + &json!({ "id": "2", "action": "navigate", "url": "data:text/html,live" }), + &mut state, + ) + .await; + assert_success(&resp); + + // The click opens a confirm dialog and returns; the dialog stays open. + let resp = execute_command( + &json!({ "id": "3", "action": "click", "selector": "#zz" }), + &mut state, + ) + .await; + assert_success(&resp); + let resp = execute_command( + &json!({ "id": "4", "action": "dialog", "response": "status" }), + &mut state, + ) + .await; + assert_success(&resp); + assert_eq!( + get_data(&resp)["hasDialog"], + true, + "the confirm dialog must be tracked: {}", + resp + ); + + // title evaluates JS in the page, so a dialog-blocked renderer would hang + // it until the CDP timeout. It must fail fast with the blocking hint + // instead (#2511). + let resp = execute_command(&json!({ "id": "5", "action": "title" }), &mut state).await; + assert_eq!( + resp.get("success").and_then(Value::as_bool), + Some(false), + "title must fail while a dialog blocks the page: {}", + resp + ); + let error = resp.get("error").and_then(Value::as_str).unwrap_or(""); + assert!( + error.contains("dialog is blocking"), + "title must fail fast with the dialog hint, got: {}", + error + ); + + // The dialog can still be resolved and the tab keeps working. + let resp = execute_command( + &json!({ "id": "6", "action": "dialog", "response": "dismiss" }), + &mut state, + ) + .await; + assert_success(&resp); + let resp = execute_command(&json!({ "id": "7", "action": "title" }), &mut state).await; + assert_success(&resp); + assert_eq!(get_data(&resp)["title"], "live"); + + let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await; + assert_success(&resp); +} + #[tokio::test] #[ignore] async fn e2e_slow_finite_script_keeps_its_tab() { From 77455508adcc1dedf0d0020509ac78321269ce40 Mon Sep 17 00:00:00 2001 From: Saim Shafique Date: Tue, 10 Nov 2026 10:14:13 +0000 Subject: [PATCH 2/3] fix(dialog): drop stale auto-dismissed dialog tracking on broadcast lag Vercel review on #2528 found two issues in the dialog tracking: 1. If broadcast lag dropped an auto-dismissed alert's close event after its opening was drained, pending_dialog stayed set forever and page commands kept failing fast on a dialog that no longer exists. The background task saw the same opening on its lossless tap and already dismissed it, so on lag the tracked alert/beforeunload entry is stale: clear it. Confirm/prompt dialogs are never auto-dismissed, so their entries stay tracked. 2. The dialog-drain unit test always panicked: the drain's session filter discarded dialog events with no browser attached. Dialog open/close is daemon-global state, so dialog events bypass the session filter (the fail-fast gate still scopes to the active tab), and a close only clears the dialog it closed. --- cli/src/native/actions.rs | 107 +++++++++++++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 2 deletions(-) diff --git a/cli/src/native/actions.rs b/cli/src/native/actions.rs index 84e160cdc6..c49e34c9ca 100644 --- a/cli/src/native/actions.rs +++ b/cli/src/native/actions.rs @@ -1938,7 +1938,19 @@ impl DaemonState { || self.iframe_sessions.values().any(|known| known == sid))) }); - if !session_matches && !iframe_network_event && !webmcp_event { + // Dialog open/close is daemon-global state, not + // page-session state: the fail-fast gate does its own + // active-tab scoping from the tracked session id, and + // `dialog status` must see a dialog on any tab. Let + // dialog events through even with no browser attached + // (unit tests) or when the dialog is on a background + // tab, or the drain would silently discard them. + let dialog_event = matches!( + event.method.as_str(), + "Page.javascriptDialogOpening" | "Page.javascriptDialogClosed" + ); + + if !session_matches && !iframe_network_event && !webmcp_event && !dialog_event { continue; } @@ -2280,7 +2292,21 @@ impl DaemonState { } } "Page.javascriptDialogClosed" => { - self.pending_dialog = None; + // Only clear the dialog this event closed: a + // close on another tab must not hide a + // still-open dialog on the tracked tab. A + // missing session id means a top-level page + // dialog. + let closed_session = event.session_id.as_deref(); + let same_dialog = match (&self.pending_dialog, closed_session) { + (Some(tracked), Some(closed)) => { + tracked.session_id.as_deref() == Some(closed) + } + _ => true, + }; + if same_dialog { + self.pending_dialog = None; + } } // Fetch.requestPaused is handled by the background // fetch_handler_task — no need to collect here. @@ -2290,6 +2316,20 @@ impl DaemonState { Err(broadcast::error::TryRecvError::Empty) => break, Err(broadcast::error::TryRecvError::Lagged(n)) => { eprintln!("[agent-browser] Warning: CDP event buffer overflowed, {} events dropped. Network requests may be missing from HAR output.", n); + // A dropped close event would leave an auto-dismissed + // dialog tracked forever: the background task saw the + // same opening on its lossless tap and already + // dismissed it, so the tracked entry is stale and page + // commands would fail fast on a dialog that no longer + // exists (#2528). Confirm/prompt dialogs are never + // auto-dismissed, so their entries stay tracked. + if self.auto_dialog { + if let Some(dialog) = &self.pending_dialog { + if matches!(dialog.dialog_type.as_str(), "beforeunload" | "alert") { + self.pending_dialog = None; + } + } + } continue; } Err(broadcast::error::TryRecvError::Closed) => { @@ -22112,6 +22152,69 @@ printf '%s' '{"protocol":"agent-browser.plugin.v1","success":true,"browser":{"cd ); } + #[test] + fn test_lagged_broadcast_clears_auto_handled_dialog() { + // Broadcast lag can drop the close event of an auto-dismissed + // alert; the tracked entry must not block page commands forever + // (#2528). A confirm dialog is never auto-dismissed, so lag must + // not clear it. + let mut state = DaemonState::new(); + state.auto_dialog = true; + let (events, receiver) = broadcast::channel(16); + state.event_rx = Some(receiver); + + // Flood the channel past its capacity so the next drain reports lag. + let flood = |events: &broadcast::Sender| { + for _ in 0..20 { + events + .send(CdpEvent { + method: "Page.frameNavigated".to_string(), + params: json!({}), + session_id: None, + }) + .unwrap(); + } + }; + + events + .send(CdpEvent { + method: "Page.javascriptDialogOpening".to_string(), + params: json!({"type": "alert", "message": "hi", "url": "https://example.com/"}), + session_id: None, + }) + .unwrap(); + let _ = state.drain_cdp_events(); + assert!( + state.pending_dialog.is_some(), + "an alert must be tracked before the lag" + ); + flood(&events); + let _ = state.drain_cdp_events(); + assert!( + state.pending_dialog.is_none(), + "lag must clear an auto-dismissed alert whose close event was dropped" + ); + + events + .send(CdpEvent { + method: "Page.javascriptDialogOpening".to_string(), + params: json!({"type": "confirm", "message": "sure?", "url": "https://example.com/"}), + session_id: None, + }) + .unwrap(); + let _ = state.drain_cdp_events(); + assert!( + state.pending_dialog.is_some(), + "a confirm must be tracked before the lag" + ); + flood(&events); + let _ = state.drain_cdp_events(); + assert!( + state.pending_dialog.is_some(), + "lag must not clear a confirm dialog: only the agent resolves it" + ); + } + #[test] fn test_pending_dialog_set_for_confirm_prompt() { // confirm and prompt should NOT be auto-handled even when auto_dialog is true. From 1c6b41b8400bed906bbb40ced5063d2f318e2bc2 Mon Sep 17 00:00:00 2001 From: Saim Shafique Date: Tue, 10 Nov 2026 11:49:29 +0000 Subject: [PATCH 3/3] fix(dialog): answer a dialog on the session it opened on Vercel review on #2528 (comment 4523632585): tracking a background-tab dialog without updating dialog resolution made `dialog accept` and `dialog dismiss` target the active tab instead of the reported dialog. BrowserManager::handle_dialog always used the active session. It now takes the tracked dialog's session (falling back to the active session when none is recorded, e.g. a top-level page dialog), and the CLI `dialog` handler passes pending_dialog's session_id. The fail-fast gate already scoped blocking to the active tab; only the resolution path was still active-tab bound. New mock-CDP unit test pins both halves: a background-tab dialog is answered on its own session, and a missing session falls back to the active tab. --- cli/src/native/actions.rs | 13 ++++++- cli/src/native/browser.rs | 71 +++++++++++++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/cli/src/native/actions.rs b/cli/src/native/actions.rs index c49e34c9ca..dbc2bd6e79 100644 --- a/cli/src/native/actions.rs +++ b/cli/src/native/actions.rs @@ -9490,10 +9490,21 @@ async fn handle_dialog(cmd: &Value, state: &mut DaemonState) -> Result, + session_id: Option<&str>, ) -> Result<(), String> { - let session_id = self.active_session_id()?; + let session_id = match session_id { + Some(sid) => sid.to_string(), + None => self.active_session_id()?.to_string(), + }; let mut params = json!({ "accept": accept }); if let Some(text) = prompt_text { params["promptText"] = Value::String(text.to_string()); @@ -2347,7 +2356,7 @@ impl BrowserManager { .send_command( "Page.handleJavaScriptDialog", Some(params), - Some(session_id), + Some(&session_id), ) .await?; Ok(()) @@ -5196,9 +5205,14 @@ mod tests { /// dialog: it never answers Runtime.evaluate (its main thread is paused) /// but still answers domain-enable and other browser-served commands. /// Returns a counter of Target.activateTarget calls so a test can assert a - /// blocked tab is treated as live without reactivation. - async fn start_mock_cdp_browser_with_dialog_blocked_tab( - ) -> (String, std::sync::Arc) { + /// blocked tab is treated as live without reactivation, plus the session + /// ids that received Page.handleJavaScriptDialog, so a test can assert a + /// dialog is answered on its own tab's session. + async fn start_mock_cdp_browser_with_dialog_blocked_tab() -> ( + String, + std::sync::Arc, + std::sync::Arc>>, + ) { use futures_util::{SinkExt, StreamExt}; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; @@ -5206,6 +5220,8 @@ mod tests { let activations = Arc::new(AtomicUsize::new(0)); let activations_task = activations.clone(); + let dialog_sessions = Arc::new(std::sync::Mutex::new(Vec::::new())); + let dialog_sessions_task = dialog_sessions.clone(); let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let url = format!( @@ -5241,6 +5257,12 @@ mod tests { if method == "Target.activateTarget" { activations_task.fetch_add(1, Ordering::SeqCst); } + if method == "Page.handleJavaScriptDialog" { + dialog_sessions_task + .lock() + .unwrap() + .push(session.to_string()); + } let reply = match method { "Target.getTargets" => Some(respond(json!({ @@ -5269,7 +5291,7 @@ mod tests { } }); - (url, activations) + (url, activations, dialog_sessions) } /// A live tab blocked by a JavaScript dialog must not be misread as @@ -5278,7 +5300,7 @@ mod tests { /// without reactivation and without hanging on script evaluation. #[tokio::test] async fn test_tab_switch_does_not_misclassify_dialog_blocked_tab() { - let (url, activations) = start_mock_cdp_browser_with_dialog_blocked_tab().await; + let (url, activations, _) = start_mock_cdp_browser_with_dialog_blocked_tab().await; let mut mgr = BrowserManager::connect_cdp(&url).await.expect("connect"); assert_eq!(mgr.pages.len(), 2); @@ -5307,6 +5329,41 @@ mod tests { assert_eq!(mgr.active_page_index, 1); } + /// `dialog accept` / `dialog dismiss` must answer the dialog on the + /// session it opened on, not the active tab (#2528): a dialog on a + /// background tab would otherwise be resolved against the wrong tab. + #[tokio::test] + async fn test_handle_dialog_targets_tracked_dialog_session() { + let (url, _activations, dialog_sessions) = + start_mock_cdp_browser_with_dialog_blocked_tab().await; + let mgr = BrowserManager::connect_cdp(&url).await.expect("connect"); + assert_eq!(mgr.pages.len(), 2); + assert_eq!(mgr.active_page_index, 0); + + // The dialog is on the background tab (S-T-BLOCKED); answering it + // must target that session even though T-ALIVE is active. + mgr.handle_dialog(true, None, Some("S-T-BLOCKED")) + .await + .expect("resolving a background-tab dialog must succeed"); + assert_eq!( + *dialog_sessions.lock().unwrap(), + vec!["S-T-BLOCKED".to_string()], + "the dialog must be answered on its own session, not the active tab" + ); + + // No recorded session falls back to the active tab, as before. + mgr.handle_dialog(false, None, None) + .await + .expect("resolving with no session must fall back to the active tab"); + let sessions = dialog_sessions.lock().unwrap(); + assert_eq!(sessions.len(), 2); + assert_eq!( + sessions[1], + mgr.active_session_id().expect("active session"), + "no session must fall back to the active tab" + ); + } + /// Closing the active tab must not hang when the tab that becomes active is /// itself discarded: the successor is revived before domains are enabled, /// rather than enable_domains blocking on a dead renderer.