From 7d1e4d0e2bfb77bd47203b4f962e4f034678dc75 Mon Sep 17 00:00:00 2001 From: Ysopwnable <161784452+N0zoM1z0@users.noreply.github.com> Date: Thu, 15 Oct 2026 09:21:56 +0800 Subject: [PATCH] fix(browser): release attached emulation overrides (#3778) * fix(browser): release attached emulation overrides * fix(browser): preserve unowned environment state * fix(browser): coordinate attached environment cleanup --- src/browser/PlaywrightScenarioBrowser.test.ts | 273 +++++++++++++++++- src/browser/PlaywrightScenarioBrowser.ts | 239 ++++++++++++--- 2 files changed, 466 insertions(+), 46 deletions(-) diff --git a/src/browser/PlaywrightScenarioBrowser.test.ts b/src/browser/PlaywrightScenarioBrowser.test.ts index 0d25442c24..36b9e753e1 100644 --- a/src/browser/PlaywrightScenarioBrowser.test.ts +++ b/src/browser/PlaywrightScenarioBrowser.test.ts @@ -1,5 +1,10 @@ import { afterEach, expect, it, vi } from "vitest"; -import type { Browser } from "playwright-core"; +import type { + Browser, + BrowserContext, + CDPSession, + Page, +} from "playwright-core"; import { browserScenarioSchema } from "../domain/browserScenario.js"; import { @@ -157,11 +162,12 @@ it("keeps attached-target ownership after the cleanup response timeout", async ( it("does not configure a late attached connection after cancellation", async () => { const connection = - deferred>(); + deferred>(); const connectionStarted = deferred(); const connectedBrowser = { close: vi.fn(async () => undefined), contexts: vi.fn(() => []), + isConnected: vi.fn(() => true), version: () => "fixture", }; const controller = new AbortController(); @@ -211,6 +217,7 @@ it("closes an attached connection when cancellation starts target discovery", as controller.abort(); return []; }), + isConnected: vi.fn(() => true), version: () => "fixture", }; const launcher = { @@ -245,6 +252,265 @@ it("closes an attached connection when cancellation starts target discovery", as expect(connectedBrowser.close).toHaveBeenCalledOnce(); }); +const attachedBrowserFixture = ( + callbacks: { + readonly send?: (method: string, parameters?: unknown) => Promise; + readonly emulateMedia?: ( + options: Parameters[0], + ) => Promise; + readonly detach?: () => Promise; + readonly closeBrowser?: () => Promise; + readonly isTargetClosed?: () => boolean; + readonly isBrowserConnected?: () => boolean; + } = {}, +) => { + const discoverySession = { + send: vi.fn(async () => ({ targetInfo: { targetId: "page-1" } })), + detach: vi.fn(async () => undefined), + } as unknown as CDPSession; + const environmentSession = { + send: vi.fn(callbacks.send ?? (async () => undefined)), + detach: vi.fn(callbacks.detach ?? (async () => undefined)), + } as unknown as CDPSession; + const page = { + emulateMedia: vi.fn(callbacks.emulateMedia ?? (async () => undefined)), + isClosed: vi.fn(callbacks.isTargetClosed ?? (() => false)), + } as unknown as Page; + const context = { + pages: () => [page], + newCDPSession: vi + .fn() + .mockResolvedValueOnce(discoverySession) + .mockResolvedValueOnce(environmentSession), + } as unknown as BrowserContext; + const browser = { + close: vi.fn(callbacks.closeBrowser ?? (async () => undefined)), + contexts: () => [context], + isConnected: vi.fn(callbacks.isBrowserConnected ?? (() => true)), + version: () => "fixture", + } as unknown as Pick< + Browser, + "contexts" | "close" | "isConnected" | "version" + >; + const scenario = browserScenarioSchema.parse({ + browser: { + mode: "connect", + cdp_endpoint: "http://127.0.0.1:9222", + target_id: "page-1", + }, + start_url: { url: "https://example.test" }, + actions: [{ step_id: "wait", action: "wait_for_timeout", duration_ms: 1 }], + }); + return { + browser, + environmentSession, + page, + open: (signal?: AbortSignal) => + openPlaywrightScenarioBrowser( + scenario, + {}, + { + launcher: { + connectOverCDP: async () => browser, + launchPersistentContext: async () => { + throw new Error("Unexpected launch"); + }, + }, + signal, + }, + ), + }; +}; + +it("restores its environment overrides before releasing an attached browser", async () => { + const cleanupOrder: string[] = []; + const fixture = attachedBrowserFixture({ + send: async (method, parameters) => { + if ( + method === "Emulation.setTimezoneOverride" && + (parameters as { timezoneId?: string }).timezoneId === "" + ) + cleanupOrder.push("timezone"); + if ( + method === "Emulation.setLocaleOverride" && + (parameters as { locale?: string }).locale === "" + ) + cleanupOrder.push("locale"); + if (method === "Emulation.clearDeviceMetricsOverride") + cleanupOrder.push("device-metrics"); + }, + emulateMedia: async (options) => { + if (options?.colorScheme === null) cleanupOrder.push("media"); + }, + detach: async () => { + cleanupOrder.push("session"); + }, + closeBrowser: async () => { + cleanupOrder.push("browser"); + }, + }); + const opened = await fixture.open(); + await opened.cleanup.close(); + + expect(fixture.environmentSession.send).toHaveBeenCalledWith( + "Emulation.setLocaleOverride", + { locale: "en-US" }, + ); + expect(fixture.environmentSession.send).toHaveBeenCalledWith( + "Emulation.setLocaleOverride", + { locale: "" }, + ); + expect(fixture.environmentSession.send).toHaveBeenCalledWith( + "Emulation.setTimezoneOverride", + { timezoneId: "" }, + ); + expect(fixture.environmentSession.send).toHaveBeenCalledWith( + "Emulation.clearDeviceMetricsOverride", + ); + expect(fixture.page.emulateMedia).toHaveBeenLastCalledWith({ + colorScheme: null, + reducedMotion: null, + }); + expect(fixture.environmentSession.detach).toHaveBeenCalledOnce(); + expect(fixture.browser.close).toHaveBeenCalledOnce(); + expect(cleanupOrder).toEqual([ + "timezone", + "locale", + "device-metrics", + "media", + "session", + "browser", + ]); +}); + +it("retains attached environment ownership until a failed reset can be retried", async () => { + let localeResetAttempts = 0; + const fixture = attachedBrowserFixture({ + send: async (method, parameters) => { + if ( + method === "Emulation.setLocaleOverride" && + (parameters as { locale?: string }).locale === "" + ) { + localeResetAttempts += 1; + if (localeResetAttempts === 1) throw new Error("reset interrupted"); + } + }, + }); + const opened = await fixture.open(); + + await expect(opened.cleanup.close()).rejects.toMatchObject({ + reason: "cleanup_failed", + cleanup: { reason: "reset interrupted" }, + }); + expect(fixture.browser.close).not.toHaveBeenCalled(); + expect(fixture.environmentSession.detach).not.toHaveBeenCalled(); + + await opened.cleanup.close(); + expect(localeResetAttempts).toBe(2); + expect(fixture.environmentSession.detach).toHaveBeenCalledOnce(); + expect(fixture.browser.close).toHaveBeenCalledOnce(); +}); + +it("does not clear an attached environment override it failed to acquire", async () => { + const fixture = attachedBrowserFixture({ + send: async (method, parameters) => { + if ( + method === "Emulation.setLocaleOverride" && + (parameters as { locale?: string }).locale === "en-US" + ) + throw new Error("caller already owns locale"); + }, + }); + + await expect(fixture.open()).rejects.toThrow("caller already owns locale"); + expect(fixture.environmentSession.send).not.toHaveBeenCalledWith( + "Emulation.setLocaleOverride", + { locale: "" }, + ); + expect(fixture.environmentSession.send).toHaveBeenCalledWith( + "Emulation.clearDeviceMetricsOverride", + ); + expect(fixture.environmentSession.detach).toHaveBeenCalledOnce(); + expect(fixture.browser.close).toHaveBeenCalledOnce(); +}); + +it("waits for a cancelled environment mutation before restoring it", async () => { + const localeStarted = deferred(); + const localeResult = deferred(); + const fixture = attachedBrowserFixture({ + send: async (method, parameters) => { + if ( + method === "Emulation.setLocaleOverride" && + (parameters as { locale?: string }).locale === "en-US" + ) { + localeStarted.resolve(undefined); + await localeResult.promise; + } + }, + }); + const controller = new AbortController(); + const opening = fixture.open(controller.signal); + await localeStarted.promise; + const cancelled = expect(opening).rejects.toMatchObject({ + _tag: "AnalysisCancelledError", + }); + controller.abort(); + await new Promise((resolve) => setImmediate(resolve)); + expect(fixture.environmentSession.send).not.toHaveBeenCalledWith( + "Emulation.setLocaleOverride", + { locale: "" }, + ); + + localeResult.resolve(undefined); + await cancelled; + expect(fixture.environmentSession.send).toHaveBeenCalledWith( + "Emulation.setLocaleOverride", + { locale: "" }, + ); + expect(fixture.environmentSession.detach).toHaveBeenCalledOnce(); + expect(fixture.browser.close).toHaveBeenCalledOnce(); +}); + +it("releases cleanup ownership when the attached target is destroyed", async () => { + let targetClosed = false; + const fixture = attachedBrowserFixture({ + send: async (method, parameters) => { + if ( + targetClosed && + method === "Emulation.setTimezoneOverride" && + (parameters as { timezoneId?: string }).timezoneId === "" + ) + throw new Error("Target closed"); + }, + isTargetClosed: () => targetClosed, + }); + const opened = await fixture.open(); + targetClosed = true; + + await opened.cleanup.close(); + expect(fixture.browser.close).toHaveBeenCalledOnce(); +}); + +it("releases cleanup ownership when the attached browser disconnects", async () => { + let connected = true; + const fixture = attachedBrowserFixture({ + send: async (method, parameters) => { + if ( + !connected && + method === "Emulation.setTimezoneOverride" && + (parameters as { timezoneId?: string }).timezoneId === "" + ) + throw new Error("Browser disconnected"); + }, + isBrowserConnected: () => connected, + }); + const opened = await fixture.open(); + connected = false; + + await opened.cleanup.close(); + expect(fixture.browser.close).not.toHaveBeenCalled(); +}); + it("cancels same-target queued connection before acquiring a browser", async () => { const makeScenario = (targetId: string) => browserScenarioSchema.parse({ @@ -350,7 +616,7 @@ it("serializes concurrent attach across loopback spellings of one browser", asyn ], }); const firstConnection = - deferred>(); + deferred>(); let rejectFirstConnection: ((cause: Error) => void) | undefined; const failingConnection = new Promise((_resolve, reject) => { rejectFirstConnection = reject; @@ -399,6 +665,7 @@ it("serializes concurrent attach across loopback spellings of one browser", asyn firstConnection.resolve({ close: vi.fn(async () => undefined), contexts: vi.fn(() => []), + isConnected: vi.fn(() => true), version: () => "fixture", }); await expect(first).rejects.toThrow(); diff --git a/src/browser/PlaywrightScenarioBrowser.ts b/src/browser/PlaywrightScenarioBrowser.ts index 567f0ad6c2..fe151d7a96 100644 --- a/src/browser/PlaywrightScenarioBrowser.ts +++ b/src/browser/PlaywrightScenarioBrowser.ts @@ -2,7 +2,13 @@ import { chmod, mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; -import type { Browser, BrowserContext, Page, chromium } from "playwright-core"; +import type { + Browser, + BrowserContext, + CDPSession, + Page, + chromium, +} from "playwright-core"; import type { BrowserScenario } from "../domain/browserScenario.js"; import { AnalysisError } from "../domain/analysisErrorBase.js"; @@ -314,39 +320,148 @@ const findConnectedPage = async ( throw new BrowserObservationError(OPERATION, "target_not_found"); }; -const configureAttachedEnvironment = async ( - context: BrowserContext, - page: Page, - environment: BrowserScenario["environment"], - signal: AbortSignal | undefined, -): Promise => { - // setViewportSize also resizes the external browser window. The scenario's - // CDP metrics override below supplies the viewport and expires on disconnect. - await page.emulateMedia({ - colorScheme: environment.color_scheme, - reducedMotion: environment.reduced_motion, - }); - throwIfScenarioCancelled(signal); - const session = await context.newCDPSession(page); - throwIfScenarioCancelled(signal); - // Chromium resets these overrides when the CDP session detaches. Keep it on - // the scenario transport until cleanup disconnects the attached browser. - await session.send("Emulation.setDeviceMetricsOverride", { - width: environment.viewport.width, - height: environment.viewport.height, - deviceScaleFactor: environment.viewport.device_scale_factor, - mobile: false, - dontSetVisibleSize: true, - }); - throwIfScenarioCancelled(signal); - await session.send("Emulation.setLocaleOverride", { - locale: environment.locale, - }); - throwIfScenarioCancelled(signal); - await session.send("Emulation.setTimezoneOverride", { - timezoneId: environment.timezone, - }); -}; +/** Owns the target state changed while one scenario uses an attached page. */ +class AttachedScenarioEnvironmentOwner { + #mediaOverride = false; + #deviceMetricsOverride = false; + #localeOverride = false; + #timezoneOverride = false; + #detached = false; + #application: Promise | undefined; + + private constructor( + private readonly page: Page, + private readonly session: CDPSession, + ) {} + + static async create( + context: BrowserContext, + page: Page, + ): Promise { + return new AttachedScenarioEnvironmentOwner( + page, + await context.newCDPSession(page), + ); + } + + get restored(): boolean { + return ( + !this.#mediaOverride && + !this.#deviceMetricsOverride && + !this.#localeOverride && + !this.#timezoneOverride + ); + } + + get targetClosed(): boolean { + return this.page.isClosed(); + } + + apply( + environment: BrowserScenario["environment"], + signal: AbortSignal | undefined, + ): Promise { + const application = + this.#application ?? this.#applyEnvironment(environment, signal); + this.#application = application; + return application; + } + + async #applyEnvironment( + environment: BrowserScenario["environment"], + signal: AbortSignal | undefined, + ): Promise { + await this.page.emulateMedia({ + colorScheme: environment.color_scheme, + reducedMotion: environment.reduced_motion, + }); + this.#mediaOverride = true; + throwIfScenarioCancelled(signal); + + // setViewportSize also resizes the external browser window. CDP metrics + // provide the scenario viewport without changing that caller-owned window. + await this.session.send("Emulation.setDeviceMetricsOverride", { + width: environment.viewport.width, + height: environment.viewport.height, + deviceScaleFactor: environment.viewport.device_scale_factor, + mobile: false, + dontSetVisibleSize: true, + }); + this.#deviceMetricsOverride = true; + throwIfScenarioCancelled(signal); + + await this.session.send("Emulation.setLocaleOverride", { + locale: environment.locale, + }); + this.#localeOverride = true; + throwIfScenarioCancelled(signal); + + await this.session.send("Emulation.setTimezoneOverride", { + timezoneId: environment.timezone, + }); + this.#timezoneOverride = true; + throwIfScenarioCancelled(signal); + } + + async close(): Promise { + if (this.#detached) return; + // The execution boundary can report cancellation before a pending CDP + // command settles. Wait for that producer so every successful mutation is + // recorded before deciding which state this owner must restore. + try { + await this.#application; + } catch { + // The operation path reports application failures. Cleanup still owns + // and restores every mutation that completed before the failure. + } + const failures: unknown[] = []; + if (this.#timezoneOverride) { + try { + await this.session.send("Emulation.setTimezoneOverride", { + timezoneId: "", + }); + this.#timezoneOverride = false; + } catch (cause: unknown) { + failures.push(cause); + } + } + if (this.#localeOverride) { + try { + await this.session.send("Emulation.setLocaleOverride", { locale: "" }); + this.#localeOverride = false; + } catch (cause: unknown) { + failures.push(cause); + } + } + if (this.#deviceMetricsOverride) { + try { + await this.session.send("Emulation.clearDeviceMetricsOverride"); + this.#deviceMetricsOverride = false; + } catch (cause: unknown) { + failures.push(cause); + } + } + if (this.#mediaOverride) { + try { + await this.page.emulateMedia({ + colorScheme: null, + reducedMotion: null, + }); + this.#mediaOverride = false; + } catch (cause: unknown) { + failures.push(cause); + } + } + if (failures.length === 1) throw failures[0]; + if (failures.length > 1) + throw new AggregateError( + failures, + "Attached browser environment restoration failed", + ); + await this.session.detach(); + this.#detached = true; + } +} /** Preserve an operation failure while reporting any incomplete cleanup. */ export const failBrowserScenarioOperation = async ( @@ -389,7 +504,7 @@ type ScenarioBrowserLauncher = Pick< > & { readonly connectOverCDP: ( endpoint: string, - ) => Promise>; + ) => Promise>; }; type AttachedBrowserRequest = Extract< @@ -406,6 +521,7 @@ const openAttachedScenarioBrowser = async ( ): Promise => { const lease = await acquireAttachedScenario(browserRequest, signal); let cleanupOwner: PlaywrightScenarioBrowserCleanupOwner | undefined; + let environmentOwner: AttachedScenarioEnvironmentOwner | undefined; try { const priorOwner = lease.admission.cleanupOwner; if (priorOwner !== undefined) await priorOwner.close(); @@ -414,7 +530,43 @@ const openAttachedScenarioBrowser = async ( const browser = await launcher.connectOverCDP(browserRequest.cdp_endpoint); const cleanup: PlaywrightScenarioBrowserCleanupOwner = new PlaywrightScenarioBrowserCleanupOwner({ - closeBrowser: () => browser.close(), + closeBrowser: async () => { + let environmentFailure: unknown; + const ownedEnvironment = environmentOwner; + if (ownedEnvironment !== undefined) { + try { + await ownedEnvironment.close(); + environmentOwner = undefined; + } catch (cause: unknown) { + environmentFailure = cause; + const browserConnected = browser.isConnected(); + if (ownedEnvironment.targetClosed || !browserConnected) { + // A destroyed target has no remaining emulation state. A lost + // transport cannot be retained or retried; release its local + // owner so later discovery can establish current target state. + environmentOwner = undefined; + if (!browserConnected) return; + } else if (!ownedEnvironment.restored) { + // Keep the live transport and target session for a cleanup + // retry while any target override remains. + throw cause; + } + } + } + try { + await browser.close(); + // Once target state is restored, disconnecting the transport is a + // definitive fallback for an explicit session detach failure. + environmentOwner = undefined; + } catch (browserFailure: unknown) { + if (environmentFailure !== undefined) + throw new AggregateError( + [environmentFailure, browserFailure], + "Attached browser environment cleanup failed", + ); + throw browserFailure; + } + }, onSettled: (browserClosed) => { if (browserClosed && lease.admission.cleanupOwner === cleanup) lease.admission.cleanupOwner = undefined; @@ -431,14 +583,15 @@ const openAttachedScenarioBrowser = async ( signal, ); throwIfScenarioCancelled(signal); - await withPlaywrightExecutionBoundary( + const configuredEnvironment = await withPlaywrightExecutionBoundary( () => - configureAttachedEnvironment( - target.context, - target.page, - environment, - signal, - ), + AttachedScenarioEnvironmentOwner.create(target.context, target.page), + undefined, + signal, + ); + environmentOwner = configuredEnvironment; + await withPlaywrightExecutionBoundary( + () => configuredEnvironment.apply(environment, signal), undefined, signal, );