From 9995edb3bad073cd1e8cb83681cb33fa434d07b1 Mon Sep 17 00:00:00 2001 From: alectimison-maker Date: Sat, 1 Aug 2026 03:45:12 +0800 Subject: [PATCH] fix: attribute window.open downloads to opener page --- CHANGELOG.md | 4 + python/rustwright/sync_api.py | 3 + src/lib.rs | 163 +++++++++++++++++++++++++++--- tests/test_rustwright_sync_api.py | 52 ++++++++++ 4 files changed, 210 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 336f88a..034e55a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable user-facing changes to Rustwright are documented in this file. ## [Unreleased] +### Fixed + +- Fixed downloads started by `window.open()` timing out instead of being attributed to the opener page's download event. + ## [0.2.0] - 2026-07-29 ### Added diff --git a/python/rustwright/sync_api.py b/python/rustwright/sync_api.py index 1aae3c1..73553d4 100644 --- a/python/rustwright/sync_api.py +++ b/python/rustwright/sync_api.py @@ -15837,6 +15837,9 @@ def _download_started_for_url(self, waiter: Any, url: str, *, timeout: float) -> return False def _download_payload_belongs_to_page(self, payload: dict[str, Any]) -> bool: + opener_target_id = payload.get("opener_target_id") + if opener_target_id is not None: + return bool(self._target_id) and str(opener_target_id) == self._target_id frame_id = payload.get("frame_id") if frame_id is None: return True diff --git a/src/lib.rs b/src/lib.rs index 6652677..1293ac1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9842,6 +9842,8 @@ struct PyDownloadEventWaiter { browser: Arc, receiver: Mutex>>, active_downloads: Mutex>, + transient_download_targets: Mutex>, + opener_target_id: String, download_path: String, } @@ -17925,9 +17927,17 @@ return true; timeout, ) .await?; - return Ok(()); + } else { + result?; } - result?; + client + .send( + "Target.setDiscoverTargets", + json!({ "discover": true }), + None, + timeout, + ) + .await?; Ok(()) }) .map_err(py_err) @@ -17938,6 +17948,8 @@ return true; browser: Arc::clone(&self.inner.browser), receiver: Mutex::new(Some(self.inner.browser.client.subscribe())), active_downloads: Mutex::new(HashMap::new()), + transient_download_targets: Mutex::new(HashSet::new()), + opener_target_id: self.inner.target_id.clone(), download_path: download_path.to_string(), } } @@ -19139,20 +19151,32 @@ impl PyDownloadEventWaiter { .take() .ok_or_else(|| PyRuntimeError::new_err("download waiter is already waiting"))?; let mut active_downloads = std::mem::take(&mut *self.active_downloads.lock().unwrap()); + let mut transient_download_targets = + std::mem::take(&mut *self.transient_download_targets.lock().unwrap()); let browser = Arc::clone(&self.browser); + let opener_target_id = self.opener_target_id.clone(); let download_path = self.download_path.clone(); let timeout = BrowserInner::command_timeout(timeout_ms); - let (result, receiver, active_downloads) = py.detach(move || { - let result = browser.block_on_raw(wait_for_download_event( - &mut receiver, - &download_path, - &mut active_downloads, - timeout, - )); - (result, receiver, active_downloads) - }); + let (result, receiver, active_downloads, transient_download_targets) = + py.detach(move || { + let result = browser.block_on_raw(wait_for_download_event( + &mut receiver, + &download_path, + &mut active_downloads, + &opener_target_id, + &mut transient_download_targets, + timeout, + )); + ( + result, + receiver, + active_downloads, + transient_download_targets, + ) + }); *self.receiver.lock().unwrap() = Some(receiver); *self.active_downloads.lock().unwrap() = active_downloads; + *self.transient_download_targets.lock().unwrap() = transient_download_targets; result.map_err(py_err) } } @@ -26150,6 +26174,8 @@ async fn wait_for_download_event( events: &mut broadcast::Receiver, download_path: &str, active_downloads: &mut HashMap, + opener_target_id: &str, + transient_download_targets: &mut HashSet, timeout: Duration, ) -> RwResult { let deadline = tokio::time::Instant::now() + timeout; @@ -26162,8 +26188,23 @@ async fn wait_for_download_event( match tokio::time::timeout(remaining, events.recv()).await { Ok(Ok(event)) => { let method = event.get("method").and_then(Value::as_str).unwrap_or(""); + update_transient_download_targets( + &event, + opener_target_id, + transient_download_targets, + ); if method == "Browser.downloadWillBegin" { - if let Some(payload) = download_from_begin_event(&event, download_path) { + if let Some(mut payload) = download_from_begin_event(&event, download_path) { + let frame_id = payload + .get("frame_id") + .and_then(Value::as_str) + .map(ToString::to_string); + if let Some(frame_id) = frame_id { + if transient_download_targets.remove(&frame_id) { + payload["opener_target_id"] = + Value::String(opener_target_id.to_string()); + } + } if let Some(guid) = payload.get("guid").and_then(Value::as_str) { if !guid.is_empty() { active_downloads.insert(guid.to_string(), payload); @@ -26893,6 +26934,104 @@ fn download_from_begin_event(event: &Value, download_path: &str) -> Option, +) { + match event.get("method").and_then(Value::as_str) { + Some("Target.targetCreated" | "Target.targetInfoChanged") => { + let info = event.pointer("/params/targetInfo").unwrap_or(&Value::Null); + let Some(target_id) = info.get("targetId").and_then(Value::as_str) else { + return; + }; + let is_initial_popup = info.get("type").and_then(Value::as_str) == Some("page") + && info.get("openerId").and_then(Value::as_str) == Some(opener_target_id) + && info + .get("url") + .and_then(Value::as_str) + .is_none_or(str::is_empty); + if is_initial_popup { + targets.insert(target_id.to_string()); + } else { + targets.remove(target_id); + } + } + Some("Target.targetDestroyed") => { + if let Some(target_id) = event.pointer("/params/targetId").and_then(Value::as_str) { + targets.remove(target_id); + } + } + _ => {} + } +} + +#[cfg(test)] +mod transient_download_target_tests { + use super::*; + + #[test] + fn tracks_only_initial_popup_targets_for_the_waiters_opener() { + let mut targets = HashSet::new(); + update_transient_download_targets( + &json!({ + "method": "Target.targetCreated", + "params": { "targetInfo": { + "targetId": "popup", + "type": "page", + "url": "", + "openerId": "opener" + }} + }), + "opener", + &mut targets, + ); + update_transient_download_targets( + &json!({ + "method": "Target.targetCreated", + "params": { "targetInfo": { + "targetId": "other-popup", + "type": "page", + "url": "", + "openerId": "other" + }} + }), + "opener", + &mut targets, + ); + + assert_eq!(targets, HashSet::from(["popup".to_string()])); + } + + #[test] + fn stops_treating_a_popup_as_transient_after_it_navigates_or_closes() { + let mut targets = HashSet::from(["popup".to_string(), "closed".to_string()]); + update_transient_download_targets( + &json!({ + "method": "Target.targetInfoChanged", + "params": { "targetInfo": { + "targetId": "popup", + "type": "page", + "url": "about:blank", + "openerId": "opener" + }} + }), + "opener", + &mut targets, + ); + update_transient_download_targets( + &json!({ + "method": "Target.targetDestroyed", + "params": { "targetId": "closed" } + }), + "opener", + &mut targets, + ); + + assert!(targets.is_empty()); + } +} + fn console_from_event(event: &Value) -> Option { let params = event.get("params")?; let args = params diff --git a/tests/test_rustwright_sync_api.py b/tests/test_rustwright_sync_api.py index 405a0ea..346d405 100644 --- a/tests/test_rustwright_sync_api.py +++ b/tests/test_rustwright_sync_api.py @@ -5142,6 +5142,19 @@ def test_expect_download_captures_file(page, http_server, tmp_path: Path): assert not saved_path.exists() +def test_expect_download_attributes_window_open_download_to_opener(page, http_server): + page.set_content("
opener
") + + with page.expect_download(timeout=3_000) as download_info: + page.evaluate("url => { window.open(url, '_blank'); }", f"{http_server}/download") + + download = download_info.value + assert download.page is page + assert download.url == f"{http_server}/download" + assert download.suggested_filename == "report.txt" + assert download.failure() is None + + def test_download_save_as_fetches_when_cdp_path_is_not_local(page, http_server, tmp_path: Path): from playwright.sync_api import Download @@ -5174,6 +5187,10 @@ def test_page_download_waiters_ignore_other_pages(browser, http_server): with first.expect_download(timeout=500): second.evaluate("() => document.querySelector('#download').click()") + with pytest.raises(TimeoutError): + with first.expect_download(timeout=500): + second.evaluate("url => { window.open(url, '_blank'); }", f"{http_server}/download") + with first.expect_download(timeout=3_000) as download_info: first.evaluate("() => document.querySelector('#download').click()") @@ -5182,6 +5199,19 @@ def test_page_download_waiters_ignore_other_pages(browser, http_server): context.close() +def test_opener_download_waiter_ignores_established_popup_download(page, http_server): + with page.expect_popup() as popup_info: + page.evaluate("() => { window.open('about:blank'); }") + + popup = popup_info.value + try: + with pytest.raises(TimeoutError): + with page.expect_download(timeout=500): + popup.evaluate("url => { window.location.href = url; }", f"{http_server}/download") + finally: + popup.close() + + def test_overlapping_download_waiters_preserve_page_metadata(page): from benchmarks.automation_cases import overlapping_download_waiters_preserve_page_metadata @@ -33318,6 +33348,28 @@ async def trigger_download() -> None: asyncio.run(run()) +def test_async_expect_download_attributes_window_open_download_to_opener(http_server): + async def run() -> None: + from playwright.async_api import async_playwright + + async with async_playwright() as p: + browser = await p.chromium.launch(headless=True) + page = await browser.new_page() + await page.set_content("
opener
") + + async with page.expect_download(timeout=3_000) as download_info: + await page.evaluate("url => { window.open(url, '_blank'); }", f"{http_server}/download") + + download = await download_info.value + assert download.page is page + assert download.url == f"{http_server}/download" + assert download.suggested_filename == "report.txt" + assert await download.failure() is None + await browser.close() + + asyncio.run(run()) + + def test_async_download_event_handler_can_await_path_and_save_as(http_server, tmp_path: Path): async def run() -> None: from playwright.async_api import async_playwright