From 0ff7aa5c3df7b1ba290aebc514c5d56fe6cb3cdf Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 25 Apr 2026 06:43:22 +0100 Subject: [PATCH] fix(browser): retry stale cached playwright attach once (cherry picked from commit ec252ebd7b45d54e431e0d7599532e5a0c1a9b73) # Conflicts: # extensions/browser/src/browser/pw-session.ts --- .../browser/pw-session.connections.test.ts | 57 +++++++- ...ge-for-targetid.extension-fallback.test.ts | 130 +++++++++++++++++- extensions/browser/src/browser/pw-session.ts | 56 +++++++- 3 files changed, 238 insertions(+), 5 deletions(-) diff --git a/extensions/browser/src/browser/pw-session.connections.test.ts b/extensions/browser/src/browser/pw-session.connections.test.ts index abb6946d610..f8771b983de 100644 --- a/extensions/browser/src/browser/pw-session.connections.test.ts +++ b/extensions/browser/src/browser/pw-session.connections.test.ts @@ -1,7 +1,11 @@ import { chromium } from "playwright-core"; import { afterEach, describe, expect, it, vi } from "vitest"; import * as chromeModule from "./chrome.js"; -import { closePlaywrightBrowserConnection, listPagesViaPlaywright } from "./pw-session.js"; +import { + closePlaywrightBrowserConnection, + getPageForTargetId, + listPagesViaPlaywright, +} from "./pw-session.js"; const connectOverCdpSpy = vi.spyOn(chromium, "connectOverCDP"); const getChromeWebSocketUrlSpy = vi.spyOn(chromeModule, "getChromeWebSocketUrl"); @@ -42,6 +46,24 @@ function makeBrowser(targetId: string, url: string): BrowserMockBundle { return { browser, browserClose }; } +function makeEmptyBrowser(): BrowserMockBundle { + const browserClose = vi.fn(async () => {}); + const context = { + pages: () => [], + on: vi.fn(), + newCDPSession: vi.fn(), + } as unknown as import("playwright-core").BrowserContext; + + const browser = { + contexts: () => [context], + on: vi.fn(), + off: vi.fn(), + close: browserClose, + } as unknown as import("playwright-core").Browser; + + return { browser, browserClose }; +} + afterEach(async () => { connectOverCdpSpy.mockReset(); getChromeWebSocketUrlSpy.mockReset(); @@ -116,4 +138,37 @@ describe("pw-session connection scoping", () => { expect(browserA.browserClose).toHaveBeenCalledTimes(1); expect(browserB.browserClose).not.toHaveBeenCalled(); }); + + it("evicts only the stale cdpUrl when getPageForTargetId retries a cached connection", async () => { + const staleA = makeEmptyBrowser(); + const refreshedA = makeBrowser("A", "https://a.example/recovered"); + const browserB = makeBrowser("B", "https://b.example"); + let callsForA = 0; + + connectOverCdpSpy.mockImplementation((async (...args: unknown[]) => { + const endpointText = String(args[0]); + if (endpointText === "http://127.0.0.1:9222") { + callsForA += 1; + return callsForA === 1 ? staleA.browser : refreshedA.browser; + } + if (endpointText === "http://127.0.0.1:9333") { + return browserB.browser; + } + throw new Error(`unexpected endpoint: ${endpointText}`); + }) as never); + getChromeWebSocketUrlSpy.mockResolvedValue(null); + + await listPagesViaPlaywright({ cdpUrl: "http://127.0.0.1:9222" }); + await listPagesViaPlaywright({ cdpUrl: "http://127.0.0.1:9333" }); + + const recoveredA = await getPageForTargetId({ cdpUrl: "http://127.0.0.1:9222" }); + const stillCachedB = await getPageForTargetId({ cdpUrl: "http://127.0.0.1:9333" }); + + expect(recoveredA.url()).toBe("https://a.example/recovered"); + expect(stillCachedB.url()).toBe("https://b.example"); + expect(staleA.browserClose).toHaveBeenCalledTimes(1); + expect(refreshedA.browserClose).not.toHaveBeenCalled(); + expect(browserB.browserClose).not.toHaveBeenCalled(); + expect(connectOverCdpSpy).toHaveBeenCalledTimes(3); + }); }); diff --git a/extensions/browser/src/browser/pw-session.get-page-for-targetid.extension-fallback.test.ts b/extensions/browser/src/browser/pw-session.get-page-for-targetid.extension-fallback.test.ts index 36c5dd24e37..c9d41c06fc7 100644 --- a/extensions/browser/src/browser/pw-session.get-page-for-targetid.extension-fallback.test.ts +++ b/extensions/browser/src/browser/pw-session.get-page-for-targetid.extension-fallback.test.ts @@ -1,14 +1,69 @@ import { chromium } from "playwright-core"; import { afterEach, describe, expect, it, vi } from "vitest"; import * as chromeModule from "./chrome.js"; -import { closePlaywrightBrowserConnection, getPageForTargetId } from "./pw-session.js"; +import { + closePlaywrightBrowserConnection, + getPageForTargetId, + listPagesViaPlaywright, +} from "./pw-session.js"; const connectOverCdpSpy = vi.spyOn(chromium, "connectOverCDP"); const getChromeWebSocketUrlSpy = vi.spyOn(chromeModule, "getChromeWebSocketUrl"); +type MockPageSpec = { + targetId?: string; + url?: string; + title?: string; +}; + +type BrowserMockBundle = { + browser: import("playwright-core").Browser; + browserClose: ReturnType; + pages: import("playwright-core").Page[]; +}; + +function makeBrowser(pages: MockPageSpec[]): BrowserMockBundle { + let context: import("playwright-core").BrowserContext; + const browserClose = vi.fn(async () => {}); + const targetIdByPage = new Map(); + + const pageObjects = pages.map((spec, index) => { + const page = { + on: vi.fn(), + context: () => context, + title: vi.fn(async () => spec.title ?? spec.targetId ?? `page-${index + 1}`), + url: vi.fn(() => spec.url ?? `https://page-${index + 1}.example`), + } as unknown as import("playwright-core").Page; + targetIdByPage.set(page, spec.targetId); + return page; + }); + + context = { + pages: () => pageObjects, + on: vi.fn(), + newCDPSession: vi.fn(async (page: import("playwright-core").Page) => ({ + send: vi.fn(async (method: string) => + method === "Target.getTargetInfo" + ? { targetInfo: { targetId: targetIdByPage.get(page) } } + : {}, + ), + detach: vi.fn(async () => {}), + })), + } as unknown as import("playwright-core").BrowserContext; + + const browser = { + contexts: () => [context], + on: vi.fn(), + off: vi.fn(), + close: browserClose, + } as unknown as import("playwright-core").Browser; + + return { browser, browserClose, pages: pageObjects }; +} + afterEach(async () => { - connectOverCdpSpy.mockClear(); - getChromeWebSocketUrlSpy.mockClear(); + connectOverCdpSpy.mockReset(); + getChromeWebSocketUrlSpy.mockReset(); await closePlaywrightBrowserConnection().catch(() => {}); }); @@ -119,4 +174,73 @@ describe("pw-session getPageForTargetId", () => { fetchSpy.mockRestore(); } }); + + it("evicts a stale cached page-less browser once and succeeds on a fresh reconnect", async () => { + const stale = makeBrowser([]); + const fresh = makeBrowser([{ targetId: "TARGET_OK", url: "https://fresh.example" }]); + + connectOverCdpSpy.mockResolvedValueOnce(stale.browser).mockResolvedValueOnce(fresh.browser); + getChromeWebSocketUrlSpy.mockResolvedValue(null); + + await listPagesViaPlaywright({ cdpUrl: "http://127.0.0.1:9222" }); + + const resolved = await getPageForTargetId({ cdpUrl: "http://127.0.0.1:9222" }); + + expect(resolved).toBe(fresh.pages[0]); + expect(connectOverCdpSpy).toHaveBeenCalledTimes(2); + expect(stale.browserClose).toHaveBeenCalledTimes(1); + }); + + it("evicts a stale cached tab-selection miss once and succeeds on a fresh reconnect", async () => { + const stale = makeBrowser([ + { targetId: "TARGET_A", url: "https://alpha.example" }, + { targetId: "TARGET_C", url: "https://charlie.example" }, + ]); + const fresh = makeBrowser([ + { targetId: "TARGET_A", url: "https://alpha.example" }, + { targetId: "TARGET_B", url: "https://beta.example" }, + ]); + + connectOverCdpSpy.mockResolvedValueOnce(stale.browser).mockResolvedValueOnce(fresh.browser); + getChromeWebSocketUrlSpy.mockResolvedValue(null); + + await getPageForTargetId({ cdpUrl: "http://127.0.0.1:9333" }); + + const resolved = await getPageForTargetId({ + cdpUrl: "http://127.0.0.1:9333", + targetId: "TARGET_B", + }); + + expect(resolved).toBe(fresh.pages[1]); + expect(connectOverCdpSpy).toHaveBeenCalledTimes(2); + expect(stale.browserClose).toHaveBeenCalledTimes(1); + }); + + it("fails after a single reconnect when the refreshed browser is still page-less", async () => { + const stale = makeBrowser([]); + const stillBroken = makeBrowser([]); + + connectOverCdpSpy + .mockResolvedValueOnce(stale.browser) + .mockResolvedValueOnce(stillBroken.browser); + getChromeWebSocketUrlSpy.mockResolvedValue(null); + + await listPagesViaPlaywright({ cdpUrl: "http://127.0.0.1:9444" }); + + await expect(getPageForTargetId({ cdpUrl: "http://127.0.0.1:9444" })).rejects.toThrow( + "No pages available in the connected browser.", + ); + expect(connectOverCdpSpy).toHaveBeenCalledTimes(2); + expect(stale.browserClose).toHaveBeenCalledTimes(1); + }); + + it("does not add an extra top-level retry for non-recoverable connect failures", async () => { + connectOverCdpSpy.mockRejectedValue(new Error("connectOverCDP exploded")); + getChromeWebSocketUrlSpy.mockResolvedValue(null); + + await expect(getPageForTargetId({ cdpUrl: "http://127.0.0.1:9555" })).rejects.toThrow( + "connectOverCDP exploded", + ); + expect(connectOverCdpSpy).toHaveBeenCalledTimes(3); + }); }); diff --git a/extensions/browser/src/browser/pw-session.ts b/extensions/browser/src/browser/pw-session.ts index 6a240581473..6a81cfd04f9 100644 --- a/extensions/browser/src/browser/pw-session.ts +++ b/extensions/browser/src/browser/pw-session.ts @@ -123,6 +123,27 @@ function normalizeCdpUrl(raw: string) { return raw.replace(/\/$/, ""); } +function hasCachedPlaywrightBrowserConnection(cdpUrl: string): boolean { + return cachedByCdpUrl.has(normalizeCdpUrl(cdpUrl)); +} + +function isRecoverableStalePageSelectionError(err: unknown, reusedCachedBrowser: boolean): boolean { + if (!reusedCachedBrowser) { + return false; + } + if ( + err instanceof Error && + err.message.includes("No pages available in the connected browser.") + ) { + return true; + } + if (err instanceof BrowserTabNotFoundError) { + return true; + } + const message = err instanceof Error ? err.message : formatErrorMessage(err); + return message.toLowerCase().includes("tab not found"); +} + function findNetworkRequestById(state: PageState, id: string): BrowserNetworkRequest | undefined { for (let i = state.requests.length - 1; i >= 0; i -= 1) { const candidate = state.requests[i]; @@ -625,7 +646,7 @@ async function resolvePageByTargetIdOrThrow(opts: { return page; } -export async function getPageForTargetId(opts: { +async function getPageForTargetIdOnce(opts: { cdpUrl: string; targetId?: string; ssrfPolicy?: SsrFPolicy; @@ -671,6 +692,23 @@ export async function getPageForTargetId(opts: { throw new BrowserTabNotFoundError(); } +export async function getPageForTargetId(opts: { + cdpUrl: string; + targetId?: string; + ssrfPolicy?: SsrFPolicy; +}): Promise { + const reusedCachedBrowser = hasCachedPlaywrightBrowserConnection(opts.cdpUrl); + try { + return await getPageForTargetIdOnce(opts); + } catch (err) { + if (!isRecoverableStalePageSelectionError(err, reusedCachedBrowser)) { + throw err; + } + await closePlaywrightBrowserConnection({ cdpUrl: opts.cdpUrl }); + return await getPageForTargetIdOnce(opts); + } +} + function isTopLevelNavigationRequest(page: Page, request: Request): boolean { let sameMainFrame = false; try { @@ -848,6 +886,22 @@ export async function gotoPageWithNavigationGuard( } } +export async function getPageForTargetId(opts: { + cdpUrl: string; + targetId?: string; +}): Promise { + const reusedCachedBrowser = hasCachedPlaywrightBrowserConnection(opts.cdpUrl); + try { + return await getPageForTargetIdOnce(opts); + } catch (err) { + if (!isRecoverableStalePageSelectionError(err, reusedCachedBrowser)) { + throw err; + } + await closePlaywrightBrowserConnection({ cdpUrl: opts.cdpUrl }); + return await getPageForTargetIdOnce(opts); + } +} + export function refLocator(page: Page, ref: string) { const normalized = ref.startsWith("@") ? ref.slice(1)