From bd5a23f88f3c54b6fd15ff1cde0693babfc86285 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 4 Jun 2025 16:44:12 -0700 Subject: [PATCH] chore: get rid of ready state type (#36177) --- .../playwright-core/src/server/bidi/DEPS.list | 1 + .../src/server/bidi/bidiChromium.ts | 19 +++--------- .../src/server/bidi/bidiFirefox.ts | 23 +++++++------- .../playwright-core/src/server/browserType.ts | 31 ++++++------------- .../src/server/chromium/chromium.ts | 22 +++++++------ .../src/server/firefox/firefox.ts | 19 ++++++------ .../src/server/utils/debugLogger.ts | 9 ++++++ 7 files changed, 55 insertions(+), 69 deletions(-) diff --git a/packages/playwright-core/src/server/bidi/DEPS.list b/packages/playwright-core/src/server/bidi/DEPS.list index b0ff3a6403..ab509f181c 100644 --- a/packages/playwright-core/src/server/bidi/DEPS.list +++ b/packages/playwright-core/src/server/bidi/DEPS.list @@ -11,3 +11,4 @@ [bidiChromium.ts] ../chromium/chromiumSwitches.ts +../chromium/chromium.ts diff --git a/packages/playwright-core/src/server/bidi/bidiChromium.ts b/packages/playwright-core/src/server/bidi/bidiChromium.ts index 57506a4571..53fc05cb9c 100644 --- a/packages/playwright-core/src/server/bidi/bidiChromium.ts +++ b/packages/playwright-core/src/server/bidi/bidiChromium.ts @@ -17,11 +17,12 @@ import os from 'os'; import { wrapInASCIIBox } from '../utils/ascii'; -import { BrowserReadyState, BrowserType, kNoXServerRunningError } from '../browserType'; +import { BrowserType, kNoXServerRunningError } from '../browserType'; import { BidiBrowser } from './bidiBrowser'; import { kBrowserCloseMessageId } from './bidiConnection'; import { chromiumSwitches } from '../chromium/chromiumSwitches'; import { RecentLogsCollector } from '../utils/debugLogger'; +import { waitForReadyState } from '../chromium/chromium'; import type { BrowserOptions } from '../browser'; import type { SdkObject } from '../instrumentation'; @@ -102,8 +103,8 @@ export class BidiChromium extends BrowserType { return chromeArguments; } - override readyState(options: types.LaunchOptions): BrowserReadyState | undefined { - return new ChromiumReadyState(); + override async waitForReadyState(options: types.LaunchOptions, browserLogsCollector: RecentLogsCollector): Promise<{ wsEndpoint?: string }> { + return waitForReadyState({ ...options, cdpPort: 0 }, browserLogsCollector); } private _innerDefaultArgs(options: types.LaunchOptions): string[] { @@ -164,16 +165,4 @@ export class BidiChromium extends BrowserType { } } -class ChromiumReadyState extends BrowserReadyState { - override onBrowserOutput(message: string): void { - if (message.includes('Failed to create a ProcessSingleton for your profile directory.')) { - this._wsEndpoint.reject(new Error('Failed to create a ProcessSingleton for your profile directory. ' + - 'This usually means that the profile is already in use by another instance of Chromium.')); - } - const match = message.match(/DevTools listening on (.*)/); - if (match) - this._wsEndpoint.resolve(match[1]); - } -} - const kBidiOverCdpWrapper = Symbol('kBidiConnectionWrapper'); diff --git a/packages/playwright-core/src/server/bidi/bidiFirefox.ts b/packages/playwright-core/src/server/bidi/bidiFirefox.ts index 4a3c027a31..28c2af7c1d 100644 --- a/packages/playwright-core/src/server/bidi/bidiFirefox.ts +++ b/packages/playwright-core/src/server/bidi/bidiFirefox.ts @@ -18,10 +18,11 @@ import os from 'os'; import path from 'path'; import { wrapInASCIIBox } from '../utils/ascii'; -import { BrowserReadyState, BrowserType, kNoXServerRunningError } from '../browserType'; +import { BrowserType, kNoXServerRunningError } from '../browserType'; import { BidiBrowser } from './bidiBrowser'; import { kBrowserCloseMessageId } from './bidiConnection'; import { createProfile } from './third_party/firefoxPrefs'; +import { ManualPromise } from '../../utils/isomorphic/manualPromise'; import type { BrowserOptions } from '../browser'; import type { SdkObject } from '../instrumentation'; @@ -29,6 +30,7 @@ import type { Env } from '../utils/processLauncher'; import type { ProtocolError } from '../protocolError'; import type { ConnectionTransport } from '../transport'; import type * as types from '../types'; +import type { RecentLogsCollector } from '../utils/debugLogger'; export class BidiFirefox extends BrowserType { @@ -101,16 +103,13 @@ export class BidiFirefox extends BrowserType { return firefoxArguments; } - override readyState(options: types.LaunchOptions): BrowserReadyState | undefined { - return new FirefoxReadyState(); - } -} - -class FirefoxReadyState extends BrowserReadyState { - override onBrowserOutput(message: string): void { - // Bidi WebSocket in Firefox. - const match = message.match(/WebDriver BiDi listening on (ws:\/\/.*)$/); - if (match) - this._wsEndpoint.resolve(match[1] + '/session'); + override async waitForReadyState(options: types.LaunchOptions, browserLogsCollector: RecentLogsCollector): Promise<{ wsEndpoint?: string }> { + const result = new ManualPromise<{ wsEndpoint?: string }>(); + browserLogsCollector.onMessage(message => { + const match = message.match(/WebDriver BiDi listening on (ws:\/\/.*)$/); + if (match) + result.resolve({ wsEndpoint: match[1] + '/session' }); + }); + return result; } } diff --git a/packages/playwright-core/src/server/browserType.ts b/packages/playwright-core/src/server/browserType.ts index 4a8600c557..8564f44081 100644 --- a/packages/playwright-core/src/server/browserType.ts +++ b/packages/playwright-core/src/server/browserType.ts @@ -49,22 +49,6 @@ import type * as channels from '@protocol/channels'; export const kNoXServerRunningError = 'Looks like you launched a headed browser without having a XServer running.\n' + 'Set either \'headless: true\' or use \'xvfb-run \' before running Playwright.\n\n<3 Playwright Team'; - -export abstract class BrowserReadyState { - protected readonly _wsEndpoint = new ManualPromise(); - - onBrowserExit(): void { - // Unblock launch when browser prematurely exits. - this._wsEndpoint.resolve(undefined); - } - async waitUntilReady(): Promise<{ wsEndpoint?: string }> { - const wsEndpoint = await this._wsEndpoint; - return { wsEndpoint }; - } - - abstract onBrowserOutput(message: string): void; -} - export abstract class BrowserType extends SdkObject { private _name: BrowserName; @@ -216,11 +200,11 @@ export abstract class BrowserType extends SdkObject { await registry.validateHostRequirementsForExecutablesIfNeeded([registryExecutable], this.attribution.playwright.options.sdkLanguage); } - const readyState = this.readyState(options); // Note: it is important to define these variables before launchProcess, so that we don't get // "Cannot access 'browserServer' before initialization" if something went wrong. let transport: ConnectionTransport | undefined = undefined; let browserProcess: BrowserProcess | undefined = undefined; + const exitPromise = new ManualPromise(); const { launchedProcess, gracefullyClose, kill } = await launchProcess({ command: executable, args: browserArguments, @@ -229,7 +213,6 @@ export abstract class BrowserType extends SdkObject { handleSIGTERM, handleSIGHUP, log: (message: string) => { - readyState?.onBrowserOutput(message); progress.log(message); browserLogsCollector.log(message); }, @@ -245,11 +228,12 @@ export abstract class BrowserType extends SdkObject { }, onExit: (exitCode, signal) => { // Unblock launch when browser prematurely exits. - readyState?.onBrowserExit(); + exitPromise.resolve(); if (browserProcess && browserProcess.onclose) browserProcess.onclose(exitCode, signal); }, }); + async function closeOrKill(timeout: number): Promise { let timer: NodeJS.Timeout; try { @@ -270,7 +254,10 @@ export abstract class BrowserType extends SdkObject { kill }; progress.cleanupWhenAborted(() => closeOrKill(progress.timeUntilDeadline())); - const wsEndpoint = (await readyState?.waitUntilReady())?.wsEndpoint; + const { wsEndpoint } = await Promise.race([ + this.waitForReadyState(options, browserLogsCollector), + exitPromise.then(() => ({ wsEndpoint: undefined })), + ]); if (options.cdpPort !== undefined || !this.supportsPipeTransport()) { transport = await WebSocketTransport.connect(progress, wsEndpoint!); } else { @@ -326,8 +313,8 @@ export abstract class BrowserType extends SdkObject { return this.doRewriteStartupLog(error); } - readyState(options: types.LaunchOptions): BrowserReadyState|undefined { - return undefined; + async waitForReadyState(options: types.LaunchOptions, browserLogsCollector: RecentLogsCollector): Promise<{ wsEndpoint?: string }> { + return {}; } async prepareUserDataDir(options: types.LaunchOptions, userDataDir: string): Promise { diff --git a/packages/playwright-core/src/server/chromium/chromium.ts b/packages/playwright-core/src/server/chromium/chromium.ts index 91f4fd6f9b..61c4b0cc72 100644 --- a/packages/playwright-core/src/server/chromium/chromium.ts +++ b/packages/playwright-core/src/server/chromium/chromium.ts @@ -30,7 +30,6 @@ import { fetchData } from '../utils/network'; import { getUserAgent } from '../utils/userAgent'; import { validateBrowserContextOptions } from '../browserContext'; import { BrowserType, kNoXServerRunningError } from '../browserType'; -import { BrowserReadyState } from '../browserType'; import { helper } from '../helper'; import { registry } from '../registry'; import { WebSocketTransport } from '../transport'; @@ -353,10 +352,8 @@ export class Chromium extends BrowserType { return chromeArguments; } - override readyState(options: types.LaunchOptions): BrowserReadyState | undefined { - if (options.cdpPort !== undefined || options.args?.some(a => a.startsWith('--remote-debugging-port'))) - return new ChromiumReadyState(); - return undefined; + override async waitForReadyState(options: types.LaunchOptions, browserLogsCollector: RecentLogsCollector): Promise<{ wsEndpoint?: string }> { + return waitForReadyState(options, browserLogsCollector); } override getExecutableName(options: types.LaunchOptions): string { @@ -366,16 +363,21 @@ export class Chromium extends BrowserType { } } -class ChromiumReadyState extends BrowserReadyState { - override onBrowserOutput(message: string): void { +export async function waitForReadyState(options: types.LaunchOptions, browserLogsCollector: RecentLogsCollector): Promise<{ wsEndpoint?: string }> { + if (options.cdpPort === undefined && !options.args?.some(a => a.startsWith('--remote-debugging-port'))) + return {}; + + const result = new ManualPromise<{ wsEndpoint?: string }>(); + browserLogsCollector.onMessage(message => { if (message.includes('Failed to create a ProcessSingleton for your profile directory.')) { - this._wsEndpoint.reject(new Error('Failed to create a ProcessSingleton for your profile directory. ' + + result.reject(new Error('Failed to create a ProcessSingleton for your profile directory. ' + 'This usually means that the profile is already in use by another instance of Chromium.')); } const match = message.match(/DevTools listening on (.*)/); if (match) - this._wsEndpoint.resolve(match[1]); - } + result.resolve({ wsEndpoint: match[1] }); + }); + return result; } async function urlToWSEndpoint(progress: Progress, endpointURL: string, headers: { [key: string]: string; }) { diff --git a/packages/playwright-core/src/server/firefox/firefox.ts b/packages/playwright-core/src/server/firefox/firefox.ts index 8c6959c554..5e51688977 100644 --- a/packages/playwright-core/src/server/firefox/firefox.ts +++ b/packages/playwright-core/src/server/firefox/firefox.ts @@ -22,7 +22,7 @@ import { FFBrowser } from './ffBrowser'; import { kBrowserCloseMessageId } from './ffConnection'; import { wrapInASCIIBox } from '../utils/ascii'; import { BrowserType, kNoXServerRunningError } from '../browserType'; -import { BrowserReadyState } from '../browserType'; +import { ManualPromise } from '../../utils/isomorphic/manualPromise'; import type { BrowserOptions } from '../browser'; import type { SdkObject } from '../instrumentation'; @@ -30,6 +30,7 @@ import type { Env } from '../utils/processLauncher'; import type { ProtocolError } from '../protocolError'; import type { ConnectionTransport } from '../transport'; import type * as types from '../types'; +import type { RecentLogsCollector } from '../utils/debugLogger'; export class Firefox extends BrowserType { constructor(parent: SdkObject) { @@ -92,14 +93,12 @@ export class Firefox extends BrowserType { return firefoxArguments; } - override readyState(options: types.LaunchOptions): BrowserReadyState | undefined { - return new JugglerReadyState(); - } -} - -class JugglerReadyState extends BrowserReadyState { - override onBrowserOutput(message: string): void { - if (message.includes('Juggler listening to the pipe')) - this._wsEndpoint.resolve(undefined); + override waitForReadyState(options: types.LaunchOptions, browserLogsCollector: RecentLogsCollector): Promise<{ wsEndpoint?: string }> { + const result = new ManualPromise<{ wsEndpoint?: string }>(); + browserLogsCollector.onMessage(message => { + if (message.includes('Juggler listening to the pipe')) + result.resolve({}); + }); + return result; } } diff --git a/packages/playwright-core/src/server/utils/debugLogger.ts b/packages/playwright-core/src/server/utils/debugLogger.ts index 84a0f53d82..ec2cef4a1c 100644 --- a/packages/playwright-core/src/server/utils/debugLogger.ts +++ b/packages/playwright-core/src/server/utils/debugLogger.ts @@ -72,11 +72,14 @@ export const debugLogger = new DebugLogger(); const kLogCount = 150; export class RecentLogsCollector { private _logs: string[] = []; + private _listeners: ((log: string) => void)[] = []; log(message: string) { this._logs.push(message); if (this._logs.length === kLogCount * 2) this._logs.splice(0, kLogCount); + for (const listener of this._listeners) + listener(message); } recentLogs(): string[] { @@ -84,4 +87,10 @@ export class RecentLogsCollector { return this._logs.slice(-kLogCount); return this._logs; } + + onMessage(listener: (message: string) => void) { + for (const message of this._logs) + listener(message); + this._listeners.push(listener); + } }