From 6f38468fe0ea42d803830940e96c8c98be5e95d9 Mon Sep 17 00:00:00 2001 From: Kesava Date: Tue, 14 Jul 2026 22:29:44 +0530 Subject: [PATCH 1/2] fix tab cleanup ownership and session safety --- SKILL.md | 9 ++ src/browser_harness/helpers.py | 129 +++++++++++++++++++- src/browser_harness/run.py | 17 ++- tests/unit/test_run.py | 131 ++++++++++++++++++++ tests/unit/test_tab_cleanup.py | 216 +++++++++++++++++++++++++++++++++ 5 files changed, 500 insertions(+), 2 deletions(-) create mode 100644 tests/unit/test_tab_cleanup.py diff --git a/SKILL.md b/SKILL.md index a11d3683..f4bce34e 100644 --- a/SKILL.md +++ b/SKILL.md @@ -93,6 +93,15 @@ Cloud profile cookie sync reference: https://github.com/browser-use/browser-harn - Login walls: stop and ask. Exception: use available SSO automatically when Chrome is already signed in; still stop for passwords, MFA, consent, or ambiguous account choice. - Raw CDP is available with `cdp("Domain.method", ...)`. +## Tab Lifecycle + +- Tabs created by `new_tab()` are closed automatically when the CLI invocation ends, including after an error. +- If `new_tab()` reuses an existing blank page, that page is restored to `about:blank` instead of being closed. +- To keep new tabs for a later invocation, call `keep_opened_tabs()` or set `BH_KEEP_TABS=1`. The later workflow is responsible for closing them explicitly. +- Preservation applies to tabs created by the invocation. A pre-existing blank tab borrowed by `new_tab()` is still restored to `about:blank`. +- Existing tabs and tabs opened by another process are never automatically closed. +- Popups and pages opened indirectly with `window.open()` or `target=_blank` are not process-owned and are left open. + ## Interaction Skills If you get stuck on a browser mechanic, check https://github.com/browser-use/browser-harness/tree/main/interaction-skills. diff --git a/src/browser_harness/helpers.py b/src/browser_harness/helpers.py index 89cbc5f4..fc49ae19 100644 --- a/src/browser_harness/helpers.py +++ b/src/browser_harness/helpers.py @@ -255,6 +255,11 @@ def capture_screenshot(path=None, full=False, max_dim=None): # --- tabs --- +_OPENED_TABS = set() +_REUSED_BLANK_TABS = {} +_KEEP_OPENED_TABS = False + + def _is_agent_startup_placeholder(title, url): url = str(url or "") return str(title or "").startswith("Starting agent ") and ( @@ -314,16 +319,138 @@ def new_tab(url="about:blank"): cur = current_tab() cur_url = cur.get("url") or "" if cur_url in ("", "about:blank") or cur_url.startswith("about:blank#"): + target_id = cur.get("targetId") or cur.get("target_id") + _REUSED_BLANK_TABS.setdefault(target_id, cur_url or "about:blank") goto_url(url) - return cur.get("targetId") or cur.get("target_id") + return target_id except Exception: pass tid = cdp("Target.createTarget", url="about:blank")["targetId"] + _OPENED_TABS.add(tid) switch_tab(tid) if url != "about:blank": goto_url(url) return tid + +def opened_tabs(): + """Return target IDs created by new_tab() in this CLI process.""" + return list(_OPENED_TABS) + + +def keep_opened_tabs(keep=True): + """Opt out of automatic cleanup for tabs owned by this CLI process.""" + global _KEEP_OPENED_TABS + _KEEP_OPENED_TABS = keep + + +def _restore_reused_blank_tabs(): + restored = [] + failures = [] + for target_id, original_url in list(_REUSED_BLANK_TABS.items()): + try: + switch_tab(target_id) + goto_url(original_url or "about:blank") + restored.append(target_id) + _REUSED_BLANK_TABS.pop(target_id, None) + except Exception as exc: + failures.append((target_id, exc)) + return restored, failures + + +def _move_to_keeper(target_ids): + """Move off an owned target before it closes; return protected IDs and failures.""" + try: + current_id = current_tab()["targetId"] + except Exception as exc: + # Without the active target ID, any owned target could be the daemon's + # current session. Fail closed instead of risking a stale attachment. + return set(target_ids), [("current target", exc)] + if current_id not in target_ids: + return set(), [] + + keeper_id = None + try: + keeper_id = cdp("Target.createTarget", url="about:blank")["targetId"] + switch_tab(keeper_id) + # The keeper becomes the daemon's neutral anchor. It is intentionally + # not owned by this invocation and can be reused by the next new_tab(). + except Exception as exc: + failures = [("keeper handoff", exc)] + if keeper_id: + try: + result = cdp("Target.closeTarget", targetId=keeper_id) + if not result.get("success", True): + raise RuntimeError("Target.closeTarget returned false") + except Exception as close_exc: + # Retain ownership so a later invocation can retry cleanup. + _OPENED_TABS.add(keeper_id) + failures.append((keeper_id, close_exc)) + # Keeping one page is better than leaving the daemon attached to a dead + # target. Other owned tabs can still be cleaned up safely. + return {current_id}, failures + return set(), [] + + +def _cleanup_error_message(failures): + details = ", ".join(f"{target}: {error}" for target, error in failures) + return f"tab cleanup incomplete ({details})" + + +def close_opened_tabs(force=False): + """Close created tabs and restore blank tabs reused by this CLI process.""" + _, failures = _restore_reused_blank_tabs() + + if _KEEP_OPENED_TABS and not force: + # Keeping a tab releases it from this invocation's ownership. Otherwise + # an embedder calling main() again would close it on the next run. + _OPENED_TABS.clear() + if failures: + raise RuntimeError(_cleanup_error_message(failures)) + return [] + + target_ids = set(_OPENED_TABS) + if not target_ids: + if failures: + raise RuntimeError(_cleanup_error_message(failures)) + return [] + + protected_ids, handoff_failures = _move_to_keeper(target_ids) + failures.extend(handoff_failures) + target_ids.difference_update(protected_ids) + + closed = [] + for target_id in list(target_ids): + last_error = None + for _ in range(2): + try: + result = cdp("Target.closeTarget", targetId=target_id) + if result.get("success", True): + closed.append(target_id) + last_error = None + break + last_error = RuntimeError("Target.closeTarget returned false") + except Exception as exc: + last_error = exc + if last_error is None: + _OPENED_TABS.discard(target_id) + else: + failures.append((target_id, last_error)) + + if closed: + deadline = time.time() + 2.0 + while time.time() < deadline: + try: + remaining = {tab["targetId"] for tab in list_tabs(include_chrome=True)} + except Exception: + break + if not remaining.intersection(closed): + break + time.sleep(0.05) + if failures: + raise RuntimeError(_cleanup_error_message(failures)) + return closed + def close_tab(target=None): """Close a tab. If `target` is omitted, closes the currently attached tab. Accepts a raw targetId string or a dict from list_tabs()/current_tab().""" diff --git a/src/browser_harness/run.py b/src/browser_harness/run.py index 16607e1e..37e29e38 100644 --- a/src/browser_harness/run.py +++ b/src/browser_harness/run.py @@ -28,6 +28,7 @@ sync_local_profile, ) from . import auth, telemetry +from . import helpers as helper_module from .helpers import * HELP = """Browser Harness @@ -343,7 +344,21 @@ def _run(args): start_remote_daemon(NAME) ensure_daemon() _install_helper_trace() - exec(code, globals()) + try: + exec(code, globals()) + finally: + keep_tabs = os.environ.get("BH_KEEP_TABS", "").lower() + try: + if keep_tabs in {"1", "true", "yes"}: + helper_module.keep_opened_tabs() + helper_module.close_opened_tabs() + except Exception as exc: + # Cleanup must never replace the browser task's result or exception. + print(f"Warning: automatic tab cleanup failed: {exc}", file=sys.stderr) + finally: + # main() can be exercised repeatedly in one Python process by tests + # and embedders, so keep the opt-out scoped to one invocation. + helper_module.keep_opened_tabs(False) if __name__ == "__main__": diff --git a/tests/unit/test_run.py b/tests/unit/test_run.py index ec3ef95e..77240e28 100644 --- a/tests/unit/test_run.py +++ b/tests/unit/test_run.py @@ -7,6 +7,17 @@ from browser_harness import run +@pytest.fixture(autouse=True) +def reset_tab_ownership(): + run.helper_module._OPENED_TABS.clear() + run.helper_module._REUSED_BLANK_TABS.clear() + run.helper_module.keep_opened_tabs(False) + yield + run.helper_module._OPENED_TABS.clear() + run.helper_module._REUSED_BLANK_TABS.clear() + run.helper_module.keep_opened_tabs(False) + + def test_stdin_executes_code(): stdout = StringIO() fake_stdin = StringIO("print('hello from stdin')") @@ -21,6 +32,126 @@ def test_stdin_executes_code(): assert stdout.getvalue().strip() == "hello from stdin" +def test_stdin_closes_owned_tabs_after_success(): + with patch.object(sys, "argv", ["browser-harness"]), \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"), \ + patch("browser_harness.run.helper_module.close_opened_tabs") as cleanup, \ + patch("sys.stdin", StringIO("x = 1")): + run.main() + + cleanup.assert_called_once_with() + + +def test_stdin_closes_owned_tabs_after_error(): + with patch.object(sys, "argv", ["browser-harness"]), \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"), \ + patch("browser_harness.run.helper_module.close_opened_tabs") as cleanup, \ + patch("sys.stdin", StringIO("raise RuntimeError('boom')")), \ + pytest.raises(RuntimeError, match="boom"): + run.main() + + cleanup.assert_called_once_with() + + +def test_cleanup_failure_does_not_mask_task_error(): + stderr = StringIO() + with patch.object(sys, "argv", ["browser-harness"]), \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"), \ + patch( + "browser_harness.run.helper_module.close_opened_tabs", + side_effect=RuntimeError("cleanup failed"), + ), \ + patch("sys.stdin", StringIO("raise RuntimeError('task failed')")), \ + patch("sys.stderr", stderr), \ + pytest.raises(RuntimeError, match="task failed"): + run.main() + + assert "automatic tab cleanup failed: cleanup failed" in stderr.getvalue() + + +def test_real_cleanup_runs_after_task_error(): + closed = [] + + def fake_cdp(method, **kwargs): + if method == "Target.closeTarget": + closed.append(kwargs["targetId"]) + return {"success": True} + + with patch.object(sys, "argv", ["browser-harness"]), \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"), \ + patch.object(run.helper_module, "_OPENED_TABS", {"owned"}), \ + patch.object(run.helper_module, "_REUSED_BLANK_TABS", {}), \ + patch.object(run.helper_module, "_KEEP_OPENED_TABS", False), \ + patch("browser_harness.helpers.current_tab", return_value={"targetId": "survivor"}), \ + patch("browser_harness.helpers.cdp", side_effect=fake_cdp), \ + patch("browser_harness.helpers.list_tabs", return_value=[]), \ + patch("sys.stdin", StringIO("raise RuntimeError('task failed')")), \ + pytest.raises(RuntimeError, match="task failed"): + run.main() + + assert closed == ["owned"] + + +def test_keep_opened_tabs_is_scoped_to_one_main_invocation(): + with patch.object(sys, "argv", ["browser-harness"]), \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"), \ + patch("sys.stdin", StringIO("keep_opened_tabs()")): + run.main() + + assert run.helper_module._KEEP_OPENED_TABS is False + + +@pytest.mark.parametrize("value", ["1", "true", "TRUE", "yes", "YES"]) +def test_bh_keep_tabs_skips_automatic_cleanup(monkeypatch, value): + monkeypatch.setenv("BH_KEEP_TABS", value) + with patch.object(sys, "argv", ["browser-harness"]), \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"), \ + patch("browser_harness.run.helper_module.keep_opened_tabs") as keep, \ + patch("browser_harness.run.helper_module.close_opened_tabs") as cleanup, \ + patch("sys.stdin", StringIO("x = 1")): + run.main() + + keep.assert_any_call() + keep.assert_called_with(False) + cleanup.assert_called_once_with() + + +def test_cloud_admin_code_still_runs_tab_finalizer(): + with patch.object(sys, "argv", ["browser-harness"]), \ + patch("browser_harness.run.print_update_banner"), \ + patch("browser_harness.run.start_remote_daemon"), \ + patch("browser_harness.run.helper_module.close_opened_tabs") as cleanup, \ + patch("sys.stdin", StringIO("start_remote_daemon('profile')")): + run.main() + + cleanup.assert_called_once_with() + + +def test_bh_keep_tabs_releases_owned_tabs_and_restores_borrowed_blank(monkeypatch): + monkeypatch.setenv("BH_KEEP_TABS", "1") + run.helper_module._OPENED_TABS.add("created") + run.helper_module._REUSED_BLANK_TABS["blank"] = "about:blank" + + with patch.object(sys, "argv", ["browser-harness"]), \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"), \ + patch("browser_harness.helpers.switch_tab") as switch, \ + patch("browser_harness.helpers.goto_url") as restore, \ + patch("sys.stdin", StringIO("x = 1")): + run.main() + + switch.assert_called_once_with("blank") + restore.assert_called_once_with("about:blank") + assert run.helper_module.opened_tabs() == [] + assert run.helper_module._REUSED_BLANK_TABS == {} + + def test_c_flag_is_rejected(): with patch.object(sys, "argv", ["browser-harness", "-c", "print('old path')"]), \ patch("sys.stdin", StringIO("print('ignored')")): diff --git a/tests/unit/test_tab_cleanup.py b/tests/unit/test_tab_cleanup.py new file mode 100644 index 00000000..8c94953d --- /dev/null +++ b/tests/unit/test_tab_cleanup.py @@ -0,0 +1,216 @@ +from unittest.mock import patch + +import pytest + +from browser_harness import helpers + + +@pytest.fixture(autouse=True) +def reset_tab_ownership(): + helpers._OPENED_TABS.clear() + helpers._REUSED_BLANK_TABS.clear() + helpers.keep_opened_tabs(False) + yield + helpers._OPENED_TABS.clear() + helpers._REUSED_BLANK_TABS.clear() + helpers.keep_opened_tabs(False) + + +def test_new_tab_tracks_only_targets_it_creates(): + def fake_cdp(method, **kwargs): + if method == "Target.createTarget": + return {"targetId": "created"} + return {} + + with patch("browser_harness.helpers.current_tab", side_effect=RuntimeError("no tab")), \ + patch("browser_harness.helpers.cdp", side_effect=fake_cdp), \ + patch("browser_harness.helpers.switch_tab"), \ + patch("browser_harness.helpers.goto_url"): + assert helpers.new_tab("https://example.com") == "created" + + assert helpers.opened_tabs() == ["created"] + + +def test_reused_blank_tab_is_restored_instead_of_closed(): + with patch("browser_harness.helpers.current_tab", return_value={ + "targetId": "blank", "target_id": "blank", "url": "about:blank", "title": "" + }), patch("browser_harness.helpers.goto_url"): + assert helpers.new_tab("https://example.com") == "blank" + + assert helpers.opened_tabs() == [] + + with patch("browser_harness.helpers.switch_tab") as switch, \ + patch("browser_harness.helpers.goto_url") as restore, \ + patch("browser_harness.helpers.cdp") as cdp: + assert helpers.close_opened_tabs() == [] + + switch.assert_called_once_with("blank") + restore.assert_called_once_with("about:blank") + assert not any(call.args[0] == "Target.closeTarget" for call in cdp.call_args_list) + + +def test_cleanup_closes_only_owned_tabs_and_uses_fresh_keeper(): + helpers._OPENED_TABS.update({"owned-a", "owned-b"}) + events = [] + + def fake_cdp(method, **kwargs): + events.append((method, kwargs)) + if method == "Target.createTarget": + return {"targetId": "created"} + return {"success": True} + + def fake_switch(target): + events.append(("switch", {"targetId": target})) + + with patch("browser_harness.helpers.current_tab", return_value={"targetId": "owned-b"}), \ + patch("browser_harness.helpers.switch_tab", side_effect=fake_switch), \ + patch("browser_harness.helpers.cdp", side_effect=fake_cdp): + closed = helpers.close_opened_tabs() + + assert set(closed) == {"owned-a", "owned-b"} + assert events[:2] == [ + ("Target.createTarget", {"url": "about:blank"}), + ("switch", {"targetId": "created"}), + ] + close_ids = { + kwargs["targetId"] for method, kwargs in events if method == "Target.closeTarget" + } + assert close_ids == {"owned-a", "owned-b"} + assert helpers.opened_tabs() == [] + + +def test_cleanup_creates_keeper_before_closing_only_remaining_tab(): + helpers._OPENED_TABS.add("owned") + events = [] + + def fake_cdp(method, **kwargs): + events.append((method, kwargs)) + if method == "Target.createTarget": + return {"targetId": "keeper"} + return {"success": True} + + def fake_switch(target): + events.append(("switch", {"targetId": target})) + + with patch("browser_harness.helpers.current_tab", return_value={"targetId": "owned"}), \ + patch("browser_harness.helpers.switch_tab", side_effect=fake_switch), \ + patch("browser_harness.helpers.cdp", side_effect=fake_cdp): + assert helpers.close_opened_tabs() == ["owned"] + + assert events[:3] == [ + ("Target.createTarget", {"url": "about:blank"}), + ("switch", {"targetId": "keeper"}), + ("Target.closeTarget", {"targetId": "owned"}), + ] + + +def test_cleanup_keeps_current_tab_if_safe_session_handoff_fails(): + helpers._OPENED_TABS.update({"current", "other"}) + + def fake_cdp(method, **kwargs): + if method == "Target.createTarget": + return {"targetId": "keeper"} + return {"success": True} + + with patch("browser_harness.helpers.current_tab", return_value={"targetId": "current"}), \ + patch("browser_harness.helpers.switch_tab", side_effect=RuntimeError("attach failed")), \ + patch("browser_harness.helpers.cdp", side_effect=fake_cdp) as cdp, \ + pytest.raises(RuntimeError, match="keeper handoff"): + helpers.close_opened_tabs() + + closed = [ + call.kwargs["targetId"] for call in cdp.call_args_list + if call.args[0] == "Target.closeTarget" + ] + assert closed == ["keeper", "other"] + assert "current" in helpers.opened_tabs() + + +def test_keep_opened_tabs_requires_force_to_clean_up(): + helpers._OPENED_TABS.add("owned") + helpers.keep_opened_tabs() + + with patch("browser_harness.helpers.cdp") as cdp: + assert helpers.close_opened_tabs() == [] + cdp.assert_not_called() + assert helpers.opened_tabs() == [] + + helpers._OPENED_TABS.add("owned") + with patch("browser_harness.helpers.current_tab", return_value={"targetId": "survivor"}), \ + patch("browser_harness.helpers.cdp", return_value={"success": True}), \ + patch("browser_harness.helpers.list_tabs", return_value=[]): + assert helpers.close_opened_tabs(force=True) == ["owned"] + + +def test_keep_opened_tabs_still_restores_reused_blank(): + helpers._OPENED_TABS.add("created") + helpers._REUSED_BLANK_TABS["blank"] = "about:blank" + helpers.keep_opened_tabs() + + with patch("browser_harness.helpers.switch_tab") as switch, \ + patch("browser_harness.helpers.goto_url") as restore: + assert helpers.close_opened_tabs() == [] + + switch.assert_called_once_with("blank") + restore.assert_called_once_with("about:blank") + assert helpers.opened_tabs() == [] + assert helpers._REUSED_BLANK_TABS == {} + + +def test_restore_failure_is_reported_and_retained_for_retry(): + helpers._REUSED_BLANK_TABS["blank"] = "about:blank" + + with patch("browser_harness.helpers.switch_tab", side_effect=RuntimeError("attach failed")), \ + pytest.raises(RuntimeError, match="blank: attach failed"): + helpers.close_opened_tabs() + + assert helpers._REUSED_BLANK_TABS == {"blank": "about:blank"} + + +def test_unknown_current_target_fails_closed(): + helpers._OPENED_TABS.update({"owned-a", "owned-b"}) + + with patch("browser_harness.helpers.current_tab", side_effect=RuntimeError("unknown")), \ + patch("browser_harness.helpers.cdp") as cdp, \ + pytest.raises(RuntimeError, match="current target: unknown"): + helpers.close_opened_tabs() + + cdp.assert_not_called() + assert set(helpers.opened_tabs()) == {"owned-a", "owned-b"} + + +def test_failed_close_is_retried_and_retained(): + helpers._OPENED_TABS.add("owned") + + with patch("browser_harness.helpers.current_tab", return_value={"targetId": "survivor"}), \ + patch("browser_harness.helpers.cdp", side_effect=RuntimeError("close failed")) as cdp, \ + pytest.raises(RuntimeError, match="owned: close failed"): + helpers.close_opened_tabs() + + assert cdp.call_count == 2 + assert helpers.opened_tabs() == ["owned"] + + with patch("browser_harness.helpers.current_tab", return_value={"targetId": "survivor"}), \ + patch("browser_harness.helpers.cdp", return_value={"success": True}), \ + patch("browser_harness.helpers.list_tabs", return_value=[]): + assert helpers.close_opened_tabs() == ["owned"] + assert helpers.opened_tabs() == [] + + +def test_failed_keeper_cleanup_is_retained_for_retry(): + helpers._OPENED_TABS.update({"current", "other"}) + + def fake_cdp(method, **kwargs): + if method == "Target.createTarget": + return {"targetId": "keeper"} + if method == "Target.closeTarget" and kwargs["targetId"] == "keeper": + raise RuntimeError("keeper close failed") + return {"success": True} + + with patch("browser_harness.helpers.current_tab", return_value={"targetId": "current"}), \ + patch("browser_harness.helpers.switch_tab", side_effect=RuntimeError("attach failed")), \ + patch("browser_harness.helpers.cdp", side_effect=fake_cdp), \ + pytest.raises(RuntimeError, match="keeper: keeper close failed"): + helpers.close_opened_tabs() + + assert set(helpers.opened_tabs()) == {"current", "keeper"} From d57c9216aa17c70118ed87208ff26ce8424979d5 Mon Sep 17 00:00:00 2001 From: Kesava Date: Sat, 18 Jul 2026 14:13:41 +0530 Subject: [PATCH 2/2] fix: keep_opened_tabs() must also protect reused-blank tabs new_tab() usually reuses the current blank tab instead of creating a new one via Target.createTarget, since the daemon is commonly parked on a blank keeper tab between invocations. close_opened_tabs() only ever checked _KEEP_OPENED_TABS after unconditionally restoring reused-blank tabs, so keep_opened_tabs() silently failed to protect exactly the common single-tab case callers rely on it for. Move the keep-mode early return above _restore_reused_blank_tabs() so both owned and reused-blank tabs are left untouched when keeping, matching the actual promise of the API. force=True still restores as before. --- src/browser_harness/helpers.py | 16 ++++++++++------ tests/unit/test_run.py | 9 ++++++--- tests/unit/test_tab_cleanup.py | 24 ++++++++++++++++++++++-- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/browser_harness/helpers.py b/src/browser_harness/helpers.py index fc49ae19..839af215 100644 --- a/src/browser_harness/helpers.py +++ b/src/browser_harness/helpers.py @@ -399,16 +399,20 @@ def _cleanup_error_message(failures): def close_opened_tabs(force=False): """Close created tabs and restore blank tabs reused by this CLI process.""" - _, failures = _restore_reused_blank_tabs() - if _KEEP_OPENED_TABS and not force: - # Keeping a tab releases it from this invocation's ownership. Otherwise - # an embedder calling main() again would close it on the next run. + # Keeping means hands off every tab this invocation touched, not just + # ones it created via Target.createTarget. new_tab() usually reuses + # the current blank tab instead of creating a new one (the daemon is + # commonly parked on a blank keeper between invocations), so most + # "kept" tabs in practice are reused-blank ones. Restoring them to + # blank here would silently defeat keep_opened_tabs() for exactly the + # common case a caller relies on it for. _OPENED_TABS.clear() - if failures: - raise RuntimeError(_cleanup_error_message(failures)) + _REUSED_BLANK_TABS.clear() return [] + _, failures = _restore_reused_blank_tabs() + target_ids = set(_OPENED_TABS) if not target_ids: if failures: diff --git a/tests/unit/test_run.py b/tests/unit/test_run.py index 77240e28..daf694ef 100644 --- a/tests/unit/test_run.py +++ b/tests/unit/test_run.py @@ -133,7 +133,10 @@ def test_cloud_admin_code_still_runs_tab_finalizer(): cleanup.assert_called_once_with() -def test_bh_keep_tabs_releases_owned_tabs_and_restores_borrowed_blank(monkeypatch): +def test_bh_keep_tabs_releases_owned_and_borrowed_blank_tabs_untouched(monkeypatch): + # Regression: BH_KEEP_TABS must protect reused-blank tabs too, since + # new_tab() usually reuses the current blank tab instead of creating a + # new one, so most "kept" tabs in practice go through that path. monkeypatch.setenv("BH_KEEP_TABS", "1") run.helper_module._OPENED_TABS.add("created") run.helper_module._REUSED_BLANK_TABS["blank"] = "about:blank" @@ -146,8 +149,8 @@ def test_bh_keep_tabs_releases_owned_tabs_and_restores_borrowed_blank(monkeypatc patch("sys.stdin", StringIO("x = 1")): run.main() - switch.assert_called_once_with("blank") - restore.assert_called_once_with("about:blank") + switch.assert_not_called() + restore.assert_not_called() assert run.helper_module.opened_tabs() == [] assert run.helper_module._REUSED_BLANK_TABS == {} diff --git a/tests/unit/test_tab_cleanup.py b/tests/unit/test_tab_cleanup.py index 8c94953d..9a64408f 100644 --- a/tests/unit/test_tab_cleanup.py +++ b/tests/unit/test_tab_cleanup.py @@ -142,7 +142,11 @@ def test_keep_opened_tabs_requires_force_to_clean_up(): assert helpers.close_opened_tabs(force=True) == ["owned"] -def test_keep_opened_tabs_still_restores_reused_blank(): +def test_keep_opened_tabs_does_not_restore_reused_blank(): + # Regression: new_tab() usually reuses the current blank tab instead of + # creating a new one, so most "kept" tabs in practice are reused-blank + # ones. keep_opened_tabs() must protect those too, or it silently fails + # for the common single-tab case a caller relies on it for. helpers._OPENED_TABS.add("created") helpers._REUSED_BLANK_TABS["blank"] = "about:blank" helpers.keep_opened_tabs() @@ -151,9 +155,25 @@ def test_keep_opened_tabs_still_restores_reused_blank(): patch("browser_harness.helpers.goto_url") as restore: assert helpers.close_opened_tabs() == [] + switch.assert_not_called() + restore.assert_not_called() + assert helpers.opened_tabs() == [] + assert helpers._REUSED_BLANK_TABS == {} + + +def test_keep_opened_tabs_force_still_restores_reused_blank(): + helpers._REUSED_BLANK_TABS["blank"] = "about:blank" + helpers.keep_opened_tabs() + + with patch("browser_harness.helpers.switch_tab") as switch, \ + patch("browser_harness.helpers.goto_url") as restore, \ + patch("browser_harness.helpers.current_tab", return_value={"targetId": "survivor"}), \ + patch("browser_harness.helpers.cdp", return_value={"success": True}), \ + patch("browser_harness.helpers.list_tabs", return_value=[]): + assert helpers.close_opened_tabs(force=True) == [] + switch.assert_called_once_with("blank") restore.assert_called_once_with("about:blank") - assert helpers.opened_tabs() == [] assert helpers._REUSED_BLANK_TABS == {}