From b34d515d8823ea0ee5d490936767837513d1e77f Mon Sep 17 00:00:00 2001 From: ldm0 Date: Sun, 11 Oct 2026 13:56:09 +0800 Subject: [PATCH] fix(browser): preserve permission replies when page owners retire Track the Page and renderer attachment receiving each permission projection. A closed or replaced owner cannot invalidate the already committed browser policy or install its stale PageState on a replacement. Preserve errors for current owners and validate successful retired unit replies. Cover setPermission, grantPermissions and resetPermissions across Page closure before and after the renderer reply and document replacement. --- .../conn/dispatch_tests/command_dispatch.rs | 165 ++++++++++++++++++ moli-protocol/src/domains/browser.rs | 56 +++--- 2 files changed, 198 insertions(+), 23 deletions(-) diff --git a/moli-protocol/src/conn/dispatch_tests/command_dispatch.rs b/moli-protocol/src/conn/dispatch_tests/command_dispatch.rs index b94614edbe..dde392c01e 100644 --- a/moli-protocol/src/conn/dispatch_tests/command_dispatch.rs +++ b/moli-protocol/src/conn/dispatch_tests/command_dispatch.rs @@ -225,6 +225,171 @@ async fn command_dispatch_completes_live_browser_permission_without_legacy_fallb assert_eq!(conn.permission_overrides.len(), 1); } +async fn browser_permission_fixture_connection() -> CdpConnection { + let mut conn = CdpConnection::new(); + let mut context = BrowserContext::new("BID-permission-owner".to_owned()); + context.set_active_target_id("TID-permission-owner".to_owned()); + conn.install_browser_context_fixture_for_test(context); + let seed = json!({ + "id": 90, + "method": "Browser.setPermission", + "params": {"permission": {"name": "geolocation"}, "setting": "prompt"} + }) + .to_string(); + assert_eq!( + complete_messages(conn.start_command_dispatch(&seed)), + vec![json!({"id": 90, "result": {}})] + ); + let page = conn + .load_page_via_runtime_async("data:text/html,

permission owner

") + .await + .expect("permission owner should load"); + conn.browser_context + .as_mut() + .expect("browser context") + .active_page_target_mut() + .runtime_slot + .set_loaded_page_for_test(page); + conn +} + +fn browser_permission_owner_cases() -> [(&'static str, Value, Option<&'static str>); 3] { + [ + ( + "Browser.setPermission", + json!({"permission": {"name": "geolocation"}, "setting": "denied"}), + Some("denied"), + ), + ( + "Browser.grantPermissions", + json!({"permissions": ["geolocation"]}), + Some("granted"), + ), + ("Browser.resetPermissions", json!({}), None), + ] +} + +#[tokio::test(flavor = "multi_thread")] +async fn command_dispatch_completes_browser_permissions_after_page_closes() { + for (method, params, expected_setting) in browser_permission_owner_cases() { + let mut conn = browser_permission_fixture_connection().await; + let raw = json!({"id": 91, "method": method, "params": params}).to_string(); + let pending = match conn.start_command_dispatch(&raw) { + CdpCommandTaskStep::Pending(pending) => pending, + CdpCommandTaskStep::Complete(_) => panic!("{method} should dispatch to the owner"), + }; + let completed = pending.wait().await; + let old_page = conn + .browser_context + .as_mut() + .expect("browser context") + .active_page_target_mut() + .runtime_slot + .clear_loaded_page_for_test_fixture(); + assert!(old_page.is_some()); + drop(old_page); + + let step = conn.complete_pending_command_dispatch(completed).await; + assert_eq!( + complete_messages(step), + vec![json!({"id": 91, "result": {}})], + "a settled {method} must survive removal of its renderer Page" + ); + assert_eq!( + conn.permission_overrides + .last() + .map(|entry| entry.setting.as_str()), + expected_setting, + "the browser retains the requested permission policy" + ); + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn command_dispatch_completes_browser_permissions_when_owner_closes_before_reply() { + for (method, params, expected_setting) in browser_permission_owner_cases() { + let mut conn = browser_permission_fixture_connection().await; + let raw = json!({"id": 93, "method": method, "params": params}).to_string(); + let pending = match conn.start_command_dispatch(&raw) { + CdpCommandTaskStep::Pending(pending) => pending, + CdpCommandTaskStep::Complete(_) => panic!("{method} should dispatch to the owner"), + }; + let old_page = conn + .browser_context + .as_mut() + .expect("browser context") + .active_page_target_mut() + .runtime_slot + .clear_loaded_page_for_test_fixture(); + assert!(old_page.is_some()); + drop(old_page); + + let completed = pending.wait().await; + let step = conn.complete_pending_command_dispatch(completed).await; + assert_eq!( + complete_messages(step), + vec![json!({"id": 93, "result": {}})], + "closing a renderer must not cancel the committed {method} policy" + ); + assert_eq!( + conn.permission_overrides + .last() + .map(|entry| entry.setting.as_str()), + expected_setting + ); + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn command_dispatch_does_not_replay_browser_permission_snapshot_on_replacement() { + for (method, params, expected_setting) in browser_permission_owner_cases() { + let mut conn = browser_permission_fixture_connection().await; + let raw = json!({"id": 92, "method": method, "params": params}).to_string(); + let pending = match conn.start_command_dispatch(&raw) { + CdpCommandTaskStep::Pending(pending) => pending, + CdpCommandTaskStep::Complete(_) => panic!("{method} should dispatch to the owner"), + }; + let completed = pending.wait().await; + let replacement_url = "data:text/html,

replacement permission owner

"; + let replacement = conn + .load_page_via_runtime_async(replacement_url) + .await + .expect("replacement should load"); + let replacement_id = replacement.page_id(); + conn.browser_context + .as_mut() + .expect("browser context") + .active_page_target_mut() + .runtime_slot + .set_loaded_page_for_test(replacement); + + let step = conn.complete_pending_command_dispatch(completed).await; + assert_eq!( + complete_messages(step), + vec![json!({"id": 92, "result": {}})] + ); + let page = conn + .browser_context + .as_ref() + .expect("browser context") + .active_page_target() + .loaded_page() + .expect("replacement should remain loaded"); + assert_eq!(page.page_id(), replacement_id); + assert_eq!( + page.requested_url().as_str(), + replacement_url, + "{method} must not install the old PageState on the replacement" + ); + assert_eq!( + conn.permission_overrides + .last() + .map(|entry| entry.setting.as_str()), + expected_setting + ); + } +} + #[tokio::test] async fn command_dispatch_completes_target_startup_commands_without_legacy_fallback() { let mut conn = CdpConnection::new(); diff --git a/moli-protocol/src/domains/browser.rs b/moli-protocol/src/domains/browser.rs index 504b5237f3..19624ff927 100644 --- a/moli-protocol/src/domains/browser.rs +++ b/moli-protocol/src/domains/browser.rs @@ -77,6 +77,8 @@ struct CompletedBrowserPageCommand { struct PendingBrowserPageTarget { browser_context_id: String, target_id: String, + page_id: u64, + attachment_id: Option, } impl PendingBrowserCommandDispatch { @@ -559,10 +561,7 @@ async fn execute_devtools_set_permission_command_async( unreachable!("set permission can only wait for permission override commands") }; for command in commands { - let completion = command - .completed - .map_err(|error| DevToolsError::new(DevToolsErrorKind::Internal, error))?; - finish_pending_permission_override_command(conn, command.target, completion) + finish_pending_permission_override_command(conn, command) .map_err(|error| DevToolsError::new(DevToolsErrorKind::Internal, error))?; } Ok(AutomationResult::Empty) @@ -760,16 +759,17 @@ fn start_loaded_page_permission_override_commands( let Some(page) = target.loaded_page_mut() else { continue; }; + let page_command = page + .start_set_permission_overrides(&effective_overrides) + .map_err(|error| format!("failed to update page permission overrides: {error}"))?; pending.push(PendingBrowserPageCommand { target: PendingBrowserPageTarget { browser_context_id: browser_context_id.clone(), target_id, + page_id: page.page_id(), + attachment_id: page_command.renderer_agent_attachment_id(), }, - pending: page - .start_set_permission_overrides(&effective_overrides) - .map_err(|error| { - format!("failed to update page permission overrides: {error}") - })?, + pending: page_command, }); } } @@ -792,15 +792,7 @@ pub(crate) fn complete_pending_browser_command( completed: commands, } => { for command in commands { - let completion = match command.completed { - Ok(completion) => completion, - Err(error) => { - return CommandOutputPlan::error(-32000, error); - } - }; - if let Err(error) = - finish_pending_permission_override_command(conn, command.target, completion) - { + if let Err(error) = finish_pending_permission_override_command(conn, command) { return CommandOutputPlan::error(-32000, error); } } @@ -811,16 +803,34 @@ pub(crate) fn complete_pending_browser_command( fn finish_pending_permission_override_command( conn: &mut CdpConnection, - target: PendingBrowserPageTarget, - completion: CompletedPageCommand, + command: CompletedBrowserPageCommand, ) -> Result<(), String> { + let target = command.target; let page = conn .browser_context_by_id_mut(&target.browser_context_id) .and_then(|browser_context| browser_context.page_target_mut(&target.target_id)) .and_then(|target| target.loaded_page_mut()) - .ok_or_else(|| "NoDocumentLoaded".to_owned())?; - page.finish_set_permission_overrides(completion) - .map_err(|error| error.to_string()) + .filter(|page| { + page.page_id() == target.page_id + && page.renderer_agent_attachment_id() == target.attachment_id + }); + if let Some(page) = page { + let completion = command.completed?; + return page + .finish_set_permission_overrides(completion) + .map_err(|error| error.to_string()); + } + + // Browser policy was committed before dispatch and is replayed when a new + // Page is installed. A retired projection may finish or be cancelled; it + // cannot invalidate that policy or install its PageState on a new owner. + match command.completed { + Ok(completion) => completion + .into_unit_page_command_turn() + .map(drop) + .map_err(|error| error.to_string()), + Err(_) => Ok(()), + } } // ────────────────────────────────────────────────────────────────────────────