From 52f9a0f4c2dd5ab5fd912cb39048b7f06407430d Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 4 Aug 2025 09:14:57 +0100 Subject: [PATCH 1/4] chore: merge various server-side markers for storage state page (#36893) --- .../src/server/bidi/bidiBrowser.ts | 23 ++-------------- .../src/server/bidi/bidiPage.ts | 4 --- .../src/server/browserContext.ts | 26 +++++++++---------- .../src/server/chromium/crBrowser.ts | 7 ++--- .../src/server/chromium/crPage.ts | 7 +++-- .../src/server/debugController.ts | 2 +- .../dispatchers/browserContextDispatcher.ts | 2 +- .../src/server/firefox/ffBrowser.ts | 8 ++---- packages/playwright-core/src/server/page.ts | 11 +++----- .../src/server/webkit/wkBrowser.ts | 7 ++--- .../src/server/webkit/wkPage.ts | 4 +-- 11 files changed, 31 insertions(+), 70 deletions(-) diff --git a/packages/playwright-core/src/server/bidi/bidiBrowser.ts b/packages/playwright-core/src/server/bidi/bidiBrowser.ts index 76847bd5d36f1..d91b3b6ade68d 100644 --- a/packages/playwright-core/src/server/bidi/bidiBrowser.ts +++ b/packages/playwright-core/src/server/bidi/bidiBrowser.ts @@ -180,7 +180,6 @@ export class BidiBrowser extends Browser { export class BidiBrowserContext extends BrowserContext { declare readonly _browser: BidiBrowser; private _originToPermissions = new Map(); - private _blockingPageCreations: Set> = new Set(); private _initScriptIds = new Map(); constructor(browser: BidiBrowser, browserContextId: string | undefined, options: types.BrowserContextOptions) { @@ -212,30 +211,12 @@ export class BidiBrowserContext extends BrowserContext { return this._bidiPages().map(bidiPage => bidiPage._page); } - override async doCreateNewPage(markAsServerSideOnly?: boolean): Promise { - const promise = this._createNewPageImpl(markAsServerSideOnly); - if (markAsServerSideOnly) - this._blockingPageCreations.add(promise); - try { - return await promise; - } finally { - this._blockingPageCreations.delete(promise); - } - } - - private async _createNewPageImpl(markAsServerSideOnly?: boolean): Promise { + override async doCreateNewPage(): Promise { const { context } = await this._browser._browserSession.send('browsingContext.create', { type: bidi.BrowsingContext.CreateType.Window, userContext: this._browserContextId, }); - const page = this._browser._bidiPages.get(context)!._page; - if (markAsServerSideOnly) - page.markAsServerSideOnly(); - return page; - } - - async waitForBlockingPageCreations() { - await Promise.all([...this._blockingPageCreations].map(command => command.catch(() => {}))); + return this._browser._bidiPages.get(context)!._page; } async doGetCookies(urls: string[]): Promise { diff --git a/packages/playwright-core/src/server/bidi/bidiPage.ts b/packages/playwright-core/src/server/bidi/bidiPage.ts index 97d07698f1dd5..d9807955a66be 100644 --- a/packages/playwright-core/src/server/bidi/bidiPage.ts +++ b/packages/playwright-core/src/server/bidi/bidiPage.ts @@ -94,10 +94,6 @@ export class BidiPage implements PageDelegate { this.updateRequestInterception(), // If the page is created by the Playwright client's call, some initialization // may be pending. Wait for it to complete before reporting the page as new. - // - // TODO: ideally we'd wait only for the commands that created this page, but currently - // there is no way in Bidi to track which command created this page. - this._browserContext.waitForBlockingPageCreations(), ]); } diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index d91f2f9668c67..1c52248f9a6a9 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -244,7 +244,7 @@ export abstract class BrowserContext extends SdkObject { // BrowserContext methods. abstract possiblyUninitializedPages(): Page[]; - abstract doCreateNewPage(markAsServerSideOnly?: boolean): Promise; + abstract doCreateNewPage(): Promise; abstract addCookies(cookies: channels.SetNetworkCookie[]): Promise; abstract setGeolocation(geolocation?: types.Geolocation): Promise; abstract setUserAgent(userAgent: string | undefined): Promise; @@ -418,7 +418,7 @@ export abstract class BrowserContext extends SdkObject { // Workaround for: // - chromium fails to change isMobile for existing page; // - webkit fails to change locale for existing page. - await this.newPage(progress, false); + await this.newPage(progress); await defaultPage.close(); } } @@ -538,9 +538,11 @@ export abstract class BrowserContext extends SdkObject { await this._closePromise; } - async newPage(progress: Progress, isServerSide: boolean): Promise { - const page = await progress.race(this.doCreateNewPage(isServerSide)); + async newPage(progress: Progress, forStorageState?: boolean): Promise { + let page: Page | undefined; try { + this._creatingStorageStatePage = !!forStorageState; + page = await progress.race(this.doCreateNewPage()); const pageOrError = await progress.race(page.waitForInitializedOrError()); if (pageOrError instanceof Page) { if (pageOrError.isClosed()) @@ -549,8 +551,10 @@ export abstract class BrowserContext extends SdkObject { } throw pageOrError; } catch (error) { - await page.close({ reason: 'Failed to create page' }).catch(() => {}); + await page?.close({ reason: 'Failed to create page' }).catch(() => {}); throw error; + } finally { + this._creatingStorageStatePage = false; } } @@ -589,7 +593,7 @@ export abstract class BrowserContext extends SdkObject { // If there are still origins to save, create a blank page to iterate over origins. if (originsToSave.size) { - const page = await this.newPage(progress, true); + const page = await this.newPage(progress, true /* forStorageState */); try { await page.addRequestInterceptor(progress, route => { route.fulfill({ body: '' }).catch(() => {}); @@ -629,14 +633,8 @@ export abstract class BrowserContext extends SdkObject { if (allOrigins.size) { if (mode === 'resetForReuse') page = this.pages()[0]; - if (!page) { - try { - this._creatingStorageStatePage = mode !== 'resetForReuse'; - page = await this.newPage(progress, this._creatingStorageStatePage); - } finally { - this._creatingStorageStatePage = false; - } - } + if (!page) + page = await this.newPage(progress, mode !== 'resetForReuse' /* forStorageState */); interceptor = (route: network.Route) => { route.fulfill({ body: '' }).catch(() => {}); diff --git a/packages/playwright-core/src/server/chromium/crBrowser.ts b/packages/playwright-core/src/server/chromium/crBrowser.ts index 37b9a7d47e94b..decbf3a86167f 100644 --- a/packages/playwright-core/src/server/chromium/crBrowser.ts +++ b/packages/playwright-core/src/server/chromium/crBrowser.ts @@ -370,12 +370,9 @@ export class CRBrowserContext extends BrowserContext { return this._crPages().map(crPage => crPage._page); } - override async doCreateNewPage(markAsServerSideOnly?: boolean): Promise { + override async doCreateNewPage(): Promise { const { targetId } = await this._browser._session.send('Target.createTarget', { url: 'about:blank', browserContextId: this._browserContextId }); - const page = this._browser._crPages.get(targetId)!._page; - if (markAsServerSideOnly) - page.markAsServerSideOnly(); - return page; + return this._browser._crPages.get(targetId)!._page; } async doGetCookies(urls: string[]): Promise { diff --git a/packages/playwright-core/src/server/chromium/crPage.ts b/packages/playwright-core/src/server/chromium/crPage.ts index 79cb7d51ba464..34e9e892d24b4 100644 --- a/packages/playwright-core/src/server/chromium/crPage.ts +++ b/packages/playwright-core/src/server/chromium/crPage.ts @@ -446,8 +446,7 @@ class FrameSession { } async _initialize(hasUIWindow: boolean) { - const isSettingStorageState = this._page.browserContext.isCreatingStorageStatePage(); - if (!isSettingStorageState && hasUIWindow && + if (!this._page.isStorageStatePage && hasUIWindow && !this._crPage._browserContext._browser.isClank() && !this._crPage._browserContext._options.noDefaultViewport) { const { windowId } = await this._client.send('Browser.getWindowForTarget'); @@ -455,7 +454,7 @@ class FrameSession { } let screencastOptions: types.PageScreencastOptions | undefined; - if (!isSettingStorageState && this._isMainFrame() && this._crPage._browserContext._options.recordVideo && hasUIWindow) { + if (!this._page.isStorageStatePage && this._isMainFrame() && this._crPage._browserContext._options.recordVideo && hasUIWindow) { const screencastId = createGuid(); const outputFile = path.join(this._crPage._browserContext._options.recordVideo.dir, screencastId + '.webm'); screencastOptions = { @@ -527,7 +526,7 @@ class FrameSession { ] }), ]; - if (!isSettingStorageState) { + if (!this._page.isStorageStatePage) { if (this._crPage._browserContext.needsPlaywrightBinding()) promises.push(this.exposePlaywrightBinding()); if (this._isMainFrame()) diff --git a/packages/playwright-core/src/server/debugController.ts b/packages/playwright-core/src/server/debugController.ts index 89b133d99701b..295b18c13687d 100644 --- a/packages/playwright-core/src/server/debugController.ts +++ b/packages/playwright-core/src/server/debugController.ts @@ -91,7 +91,7 @@ export class DebugController extends SdkObject { if (!pages.length) { const [browser] = this._playwright.allBrowsers(); const context = await browser.newContextForReuse(progress, {}); - await context.newPage(progress, false /* isServerSide */); + await context.newPage(progress); } // Update test id attribute. if (params.testIdAttributeName) { diff --git a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts index 6803d00e52429..a62fcd607f6a8 100644 --- a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts @@ -249,7 +249,7 @@ export class BrowserContextDispatcher extends Dispatcher { - return { page: PageDispatcher.from(this, await this._context.newPage(progress, false /* isServerSide */)) }; + return { page: PageDispatcher.from(this, await this._context.newPage(progress)) }; } async cookies(params: channels.BrowserContextCookiesParams, progress: Progress): Promise { diff --git a/packages/playwright-core/src/server/firefox/ffBrowser.ts b/packages/playwright-core/src/server/firefox/ffBrowser.ts index e019d6dd50adc..b9ae8f2d3656d 100644 --- a/packages/playwright-core/src/server/firefox/ffBrowser.ts +++ b/packages/playwright-core/src/server/firefox/ffBrowser.ts @@ -250,7 +250,7 @@ export class FFBrowserContext extends BrowserContext { return this._ffPages().map(ffPage => ffPage._page); } - override async doCreateNewPage(markAsServerSideOnly?: boolean): Promise { + override async doCreateNewPage(): Promise { const { targetId } = await this._browser.session.send('Browser.newPage', { browserContextId: this._browserContextId }).catch(e => { @@ -258,11 +258,7 @@ export class FFBrowserContext extends BrowserContext { throw new Error(`Invalid timezone ID: ${this._options.timezoneId}`); throw e; }); - const page = this._browser._ffPages.get(targetId)!._page; - if (markAsServerSideOnly) - page.markAsServerSideOnly(); - return page; - + return this._browser._ffPages.get(targetId)!._page; } async doGetCookies(urls: string[]): Promise { diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index 172014360e025..20883814b8d47 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -161,7 +161,7 @@ export class Page extends SdkObject { readonly requestInterceptors: network.RouteHandler[] = []; video: Artifact | null = null; private _opener: Page | undefined; - private _isServerSideOnly = false; + readonly isStorageStatePage: boolean; private _locatorHandlers = new Map }>(); private _lastLocatorHandlerUid = 0; private _locatorHandlerRunningCounter = 0; @@ -186,6 +186,7 @@ export class Page extends SdkObject { if (delegate.pdf) this.pdf = delegate.pdf.bind(delegate); this.coverage = delegate.coverage ? delegate.coverage() : null; + this.isStorageStatePage = browserContext.isCreatingStorageStatePage(); } async reportAsNew(opener: Page | undefined, error: Error | undefined = undefined, contextEvent: string = BrowserContext.Events.Page) { @@ -234,13 +235,13 @@ export class Page extends SdkObject { } emitOnContext(event: string | symbol, ...args: any[]) { - if (this._isServerSideOnly) + if (this.isStorageStatePage) return; this.browserContext.emit(event, ...args); } emitOnContextOnceInitialized(event: string | symbol, ...args: any[]) { - if (this._isServerSideOnly) + if (this.isStorageStatePage) return; // Some events, like console messages, may come before page is ready. // In this case, postpone the event until page is initialized, @@ -813,10 +814,6 @@ export class Page extends SdkObject { await Promise.all(this.frames().map(frame => frame.hideHighlight().catch(() => {}))); } - markAsServerSideOnly() { - this._isServerSideOnly = true; - } - async snapshotForAI(progress: Progress): Promise { this.lastSnapshotFrameIds = []; const snapshot = await snapshotFrameForAI(progress, this.mainFrame(), 0, this.lastSnapshotFrameIds); diff --git a/packages/playwright-core/src/server/webkit/wkBrowser.ts b/packages/playwright-core/src/server/webkit/wkBrowser.ts index f6b26c7e7bce8..5ee778b8301ba 100644 --- a/packages/playwright-core/src/server/webkit/wkBrowser.ts +++ b/packages/playwright-core/src/server/webkit/wkBrowser.ts @@ -244,12 +244,9 @@ export class WKBrowserContext extends BrowserContext { return this._wkPages().map(wkPage => wkPage._page); } - override async doCreateNewPage(markAsServerSideOnly?: boolean): Promise { + override async doCreateNewPage(): Promise { const { pageProxyId } = await this._browser._browserSession.send('Playwright.createPage', { browserContextId: this._browserContextId }); - const page = this._browser._wkPages.get(pageProxyId)!._page; - if (markAsServerSideOnly) - page.markAsServerSideOnly(); - return page; + return this._browser._wkPages.get(pageProxyId)!._page; } async doGetCookies(urls: string[]): Promise { diff --git a/packages/playwright-core/src/server/webkit/wkPage.ts b/packages/playwright-core/src/server/webkit/wkPage.ts index ed27e46710769..0452a225528f9 100644 --- a/packages/playwright-core/src/server/webkit/wkPage.ts +++ b/packages/playwright-core/src/server/webkit/wkPage.ts @@ -113,7 +113,7 @@ export class WKPage implements PageDelegate { } private async _initializePageProxySession() { - if (this._page.browserContext.isCreatingStorageStatePage()) + if (this._page.isStorageStatePage) return; const promises: Promise[] = [ this._pageProxySession.send('Dialog.enable'), @@ -187,7 +187,7 @@ export class WKPage implements PageDelegate { promises.push(session.send('Network.setResourceCachingDisabled', { disabled: true })); promises.push(session.send('Network.addInterception', { url: '.*', stage: 'request', isRegex: true })); } - if (this._page.browserContext.isCreatingStorageStatePage()) { + if (this._page.isStorageStatePage) { await Promise.all(promises); return; } From a73982e7a0ce716f4274144fcf6a68130e46cfe2 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 4 Aug 2025 09:16:08 +0100 Subject: [PATCH 2/4] chore: remove cleanupWhenAborted in favor of try/catch/finally (#36896) --- packages/playwright-core/src/server/dom.ts | 11 ++-- packages/playwright-core/src/server/frames.ts | 37 ++++++----- packages/playwright-core/src/server/page.ts | 61 ++++++++++++------- .../playwright-core/src/server/progress.ts | 26 +------- packages/protocol/src/progress.d.ts | 12 ++-- 5 files changed, 75 insertions(+), 72 deletions(-) diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts index 6f1cc8eb5dcee..a393afc170b16 100644 --- a/packages/playwright-core/src/server/dom.ts +++ b/packages/playwright-core/src/server/dom.ts @@ -447,12 +447,6 @@ export class ElementHandle extends js.JSHandle { return { hitTargetDescription: error }; } hitTargetInterceptionHandle = handle as any; - progress.cleanupWhenAborted(() => { - // Do not await here, just in case the renderer is stuck (e.g. on alert) - // and we won't be able to cleanup. - hitTargetInterceptionHandle!.evaluate(h => h.stop()).catch(e => {}); - hitTargetInterceptionHandle!.dispose(); - }); } const actionResult = await this._page.frameManager.waitForSignalsCreatedBy(progress, options.waitAfter === true, async () => { @@ -484,6 +478,11 @@ export class ElementHandle extends js.JSHandle { if ((options as any).__testHookAfterPointerAction) await progress.race((options as any).__testHookAfterPointerAction()); return 'done'; + }).finally(() => { + // Do not await here, just in case the renderer is stuck (e.g. on alert) + // and we won't be able to cleanup. + const stopPromise = hitTargetInterceptionHandle?.evaluate(h => h.stop()).catch(() => {}); + stopPromise?.then(() => hitTargetInterceptionHandle?.dispose()); }); if (actionResult !== 'done') return actionResult; diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index 9d85416427889..efdd355cd4d0c 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -166,14 +166,16 @@ export class FrameManager { return action(); const barrier = new SignalBarrier(progress); this._signalBarriers.add(barrier); - progress.cleanupWhenAborted(() => this._signalBarriers.delete(barrier)); - const result = await action(); - await progress.race(this._page.delegate.inputActionEpilogue()); - await barrier.waitFor(); - this._signalBarriers.delete(barrier); - // Resolve in the next task, after all waitForNavigations. - await new Promise(makeWaitForNextTask()); - return result; + try { + const result = await action(); + await progress.race(this._page.delegate.inputActionEpilogue()); + await barrier.waitFor(); + // Resolve in the next task, after all waitForNavigations. + await new Promise(makeWaitForNextTask()); + return result; + } finally { + this._signalBarriers.delete(barrier); + } } frameWillPotentiallyRequestNavigation() { @@ -862,10 +864,10 @@ export class Frame extends SdkObject { } async setContent(progress: Progress, html: string, options: types.NavigateOptions): Promise { + const tag = `--playwright--set--content--${this._id}--${++this._setContentCounter}--`; await this.raceNavigationAction(progress, async () => { const waitUntil = options.waitUntil === undefined ? 'load' : options.waitUntil; progress.log(`setting frame content, waiting until "${waitUntil}"`); - const tag = `--playwright--set--content--${this._id}--${++this._setContentCounter}--`; const context = await progress.race(this._utilityContext()); const tagPromise = new ManualPromise(); this._page.frameManager._consoleMessageTags.set(tag, () => { @@ -873,7 +875,6 @@ export class Frame extends SdkObject { this._onClearLifecycle(); tagPromise.resolve(); }); - progress.cleanupWhenAborted(() => this._page.frameManager._consoleMessageTags.delete(tag)); const lifecyclePromise = progress.race(tagPromise).then(() => this._waitForLoadState(progress, waitUntil)); const contentPromise = progress.race(context.evaluate(({ html, tag }) => { document.open(); @@ -883,6 +884,8 @@ export class Frame extends SdkObject { }, { html, tag })); await Promise.all([contentPromise, lifecyclePromise]); return null; + }).finally(() => { + this._page.frameManager._consoleMessageTags.delete(tag); }); } @@ -1489,10 +1492,16 @@ export class Frame extends SdkObject { next(); return { result, abort: () => aborted = true }; }, { expression, isFunction, polling: options.pollingInterval, arg })); - progress.cleanupWhenAborted(() => handle.evaluate(h => h.abort()).finally(() => handle.dispose())); - const result = await progress.race(handle.evaluateHandle(h => h.result)); - handle.dispose(); - return result; + try { + return await progress.race(handle.evaluateHandle(h => h.result)); + } catch (error) { + // Note: it is important to await "abort()" to prevent any side effects + // after this method returns. + await handle.evaluate(h => h.abort()).catch(() => {}); + throw error; + } finally { + handle.dispose(); + } }); } diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index 20883814b8d47..7753982667617 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -325,10 +325,14 @@ export class Page extends SdkObject { await progress.race(this.browserContext.exposePlaywrightBindingIfNeeded()); const binding = new PageBinding(name, playwrightBinding, needsHandle); this._pageBindings.set(name, binding); - progress.cleanupWhenAborted(() => this._pageBindings.delete(name)); - await progress.race(this.delegate.addInitScript(binding.initScript)); - await progress.race(this.safeNonStallingEvaluateInAllFrames(binding.initScript.source, 'main')); - return binding; + try { + await progress.race(this.delegate.addInitScript(binding.initScript)); + await progress.race(this.safeNonStallingEvaluateInAllFrames(binding.initScript.source, 'main')); + return binding; + } catch (error) { + this._pageBindings.delete(name); + throw error; + } } async removeExposedBindings(bindings: PageBinding[]) { @@ -342,12 +346,15 @@ export class Page extends SdkObject { async setExtraHTTPHeaders(progress: Progress, headers: types.HeadersArray) { const oldHeaders = this._extraHTTPHeaders; - this._extraHTTPHeaders = headers; - progress.cleanupWhenAborted(async () => { + try { + this._extraHTTPHeaders = headers; + await progress.race(this.delegate.updateExtraHTTPHeaders()); + } catch (error) { this._extraHTTPHeaders = oldHeaders; - await this.delegate.updateExtraHTTPHeaders(); - }); - await progress.race(this.delegate.updateExtraHTTPHeaders()); + // Note: no await, headers will be updated in the background as soon as possible. + this.delegate.updateExtraHTTPHeaders().catch(() => {}); + throw error; + } } extraHTTPHeaders(): types.HeadersArray | undefined { @@ -502,10 +509,6 @@ export class Page extends SdkObject { async emulateMedia(progress: Progress, options: Partial) { const oldEmulatedMedia = { ...this._emulatedMedia }; - progress.cleanupWhenAborted(async () => { - this._emulatedMedia = oldEmulatedMedia; - await this.delegate.updateEmulateMedia(); - }); if (options.media !== undefined) this._emulatedMedia.media = options.media; @@ -518,7 +521,14 @@ export class Page extends SdkObject { if (options.contrast !== undefined) this._emulatedMedia.contrast = options.contrast; - await progress.race(this.delegate.updateEmulateMedia()); + try { + await progress.race(this.delegate.updateEmulateMedia()); + } catch (error) { + this._emulatedMedia = oldEmulatedMedia; + // Note: no await, emulated media will be updated in the background as soon as possible. + this.delegate.updateEmulateMedia().catch(() => {}); + throw error; + } } emulatedMedia(): EmulatedMedia { @@ -534,13 +544,15 @@ export class Page extends SdkObject { async setViewportSize(progress: Progress, viewportSize: types.Size) { const oldEmulatedSize = this._emulatedSize; - progress.cleanupWhenAborted(async () => { + try { + this._setEmulatedSize({ viewport: { ...viewportSize }, screen: { ...viewportSize } }); + await progress.race(this.delegate.updateEmulatedViewportSize()); + } catch (error) { this._emulatedSize = oldEmulatedSize; - await this.delegate.updateEmulatedViewportSize(); - }); - - this._setEmulatedSize({ viewport: { ...viewportSize }, screen: { ...viewportSize } }); - await progress.race(this.delegate.updateEmulatedViewportSize()); + // Note: no await, emulated size will be updated in the background as soon as possible. + this.delegate.updateEmulatedViewportSize().catch(() => {}); + throw error; + } } setEmulatedSizeFromWindowOpen(emulatedSize: EmulatedSize) { @@ -566,8 +578,13 @@ export class Page extends SdkObject { async addInitScript(progress: Progress, source: string) { const initScript = new InitScript(source); this.initScripts.push(initScript); - progress.cleanupWhenAborted(() => this.removeInitScripts([initScript])); - await progress.race(this.delegate.addInitScript(initScript)); + try { + await progress.race(this.delegate.addInitScript(initScript)); + } catch (error) { + // Note: no await, script will be removed in the background as soon as possible. + this.removeInitScripts([initScript]).catch(() => {}); + throw error; + } return initScript; } diff --git a/packages/playwright-core/src/server/progress.ts b/packages/playwright-core/src/server/progress.ts index e299c38e177c8..7fde3f2557657 100644 --- a/packages/playwright-core/src/server/progress.ts +++ b/packages/playwright-core/src/server/progress.ts @@ -20,18 +20,12 @@ import { ManualPromise } from '../utils/isomorphic/manualPromise'; import type { Progress } from '@protocol/progress'; import type { CallMetadata, SdkObject } from './instrumentation'; -import type { LogName } from './utils/debugLogger'; export type { Progress } from '@protocol/progress'; export class ProgressController { private _forceAbortPromise = new ManualPromise(); private _donePromise = new ManualPromise(); - - // Cleanups to be run only in the case of abort. - private _cleanups: ((error: Error | undefined) => any)[] = []; - - private _logName: LogName; private _state: 'before' | 'running' | { error: Error } | 'finished' = 'before'; private _sdkObject: SdkObject; @@ -40,14 +34,9 @@ export class ProgressController { constructor(metadata: CallMetadata, sdkObject: SdkObject) { this.metadata = metadata; this._sdkObject = sdkObject; - this._logName = sdkObject.logName || 'api'; this._forceAbortPromise.catch(e => null); // Prevent unhandled promise rejection. } - setLogName(logName: LogName) { - this._logName = logName; - } - async abort(error: Error) { if (this._state === 'running') { (error as any)[kAbortErrorSymbol] = true; @@ -66,12 +55,7 @@ export class ProgressController { if (this._state === 'running') this.metadata.log.push(message); // Note: we might be sending logs after progress has finished, for example browser logs. - this._sdkObject.instrumentation.onCallLog(this._sdkObject, this.metadata, this._logName, message); - }, - cleanupWhenAborted: (cleanup: (error: Error | undefined) => any) => { - if (this._state !== 'running') - throw new Error('Internal error: cannot register cleanup after operation has finished.'); - this._cleanups.push(cleanup); + this._sdkObject.instrumentation.onCallLog(this._sdkObject, this.metadata, this._sdkObject.logName || 'api', message); }, metadata: this.metadata, race: (promise: Promise | Promise[]) => { @@ -103,7 +87,6 @@ export class ProgressController { return result; } catch (error) { this._state = { error }; - await Promise.all(this._cleanups.splice(0).map(cleanup => runCleanup(error, cleanup))); throw error; } finally { clearTimeout(timer); @@ -112,13 +95,6 @@ export class ProgressController { } } -async function runCleanup(error: Error | undefined, cleanup: (error: Error | undefined) => any) { - try { - await cleanup(error); - } catch (e) { - } -} - const kAbortErrorSymbol = Symbol('kAbortError'); export function isAbortError(error: Error): boolean { diff --git a/packages/protocol/src/progress.d.ts b/packages/protocol/src/progress.d.ts index 6890b3a048ed6..6e8001629bf07 100644 --- a/packages/protocol/src/progress.d.ts +++ b/packages/protocol/src/progress.d.ts @@ -16,24 +16,26 @@ import type { CallMetadata } from './callMetadata'; -// Most server operations are run inside a Progress instance. +// Most server operations are run inside a Progress instance, which is conceptually +// similar to a cancellation token. +// // Each method that takes a Progress must result in one of the three outcomes: // - It finishes successfully, returning a value, before the Progress is aborted. // - It throws some error, before the Progress is aborted. // - It throws the Progress's aborted error, because the Progress was aborted before // the method could finish. +// // As a rule of thumb, the above is achieved by: // - Passing the Progress instance when awaiting other methods. // - Using `progress.race()` when awaiting other methods that do not take a Progress argument. // In this case, it is important that awaited method has no side effects, for example // it is a read-only browser protocol call. // - In rare cases, when the awaited method does not take a Progress argument, -// but it does have side effects such as creating a page - a proper cleanup -// must be taken in case Progress is aborted before the awaited method finishes. -// That's usually done by `progress.cleanupWhenAborted()`. +// but it does have side effects such as creating a page - a proper try/catch/finally +// should be used to cleanup. Whether the cleanup is awaited or not depends on the method details. +// For the trickiest cases, look at `raceUncancellableOperationWithCleanup()` helper method. export interface Progress { log(message: string): void; - cleanupWhenAborted(cleanup: (error: Error | undefined) => any): void; race(promise: Promise | Promise[]): Promise; wait(timeout: number): Promise; metadata: CallMetadata; From 1beca691a703004430a884119598ce50a42ab7ff Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Mon, 4 Aug 2025 11:12:22 +0200 Subject: [PATCH 3/4] chore: fold PW_BROWSER_SERVER into extension mode (#36906) --- .../src/remote/playwrightServer.ts | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/playwright-core/src/remote/playwrightServer.ts b/packages/playwright-core/src/remote/playwrightServer.ts index dbc418db6eda6..e9309408e1185 100644 --- a/packages/playwright-core/src/remote/playwrightServer.ts +++ b/packages/playwright-core/src/remote/playwrightServer.ts @@ -107,21 +107,21 @@ export class PlaywrightServer { const allowFSPaths = isExtension; launchOptions = filterLaunchOptions(launchOptions, allowFSPaths); - if (process.env.PW_BROWSER_SERVER && url.searchParams.has('connect')) { - const filter = url.searchParams.get('connect'); - if (filter !== 'first') - throw new Error(`Unknown connect filter: ${filter}`); - return new PlaywrightConnection( - browserSemaphore, - ws, - false, - this._playwright, - () => this._initConnectMode(id, filter, browserName, launchOptions), - id, - ); - } - if (isExtension) { + const connectFilter = url.searchParams.get('connect'); + if (connectFilter) { + if (connectFilter !== 'first') + throw new Error(`Unknown connect filter: ${connectFilter}`); + return new PlaywrightConnection( + browserSemaphore, + ws, + false, + this._playwright, + () => this._initConnectMode(id, connectFilter, browserName, launchOptions), + id, + ); + } + if (url.searchParams.has('debug-controller')) { return new PlaywrightConnection( controllerSemaphore, From 2b07f258fe01661cce440fe95e6cc915345fcf34 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 4 Aug 2025 10:59:22 +0100 Subject: [PATCH 4/4] chore: move auto-generated expect computation to injected code (#36894) --- packages/injected/src/ariaSnapshot.ts | 2 +- packages/injected/src/injectedScript.ts | 1 + packages/injected/src/recorder/recorder.ts | 77 +++++++++++++++++-- .../src/server/codegen/language.ts | 24 +++++- .../playwright-core/src/server/recorder.ts | 64 +-------------- .../src/server/recorder/recorderUtils.ts | 4 - .../src/utils/isomorphic/ariaSnapshot.ts | 61 +-------------- packages/recorder/src/actions.d.ts | 1 + 8 files changed, 97 insertions(+), 137 deletions(-) diff --git a/packages/injected/src/ariaSnapshot.ts b/packages/injected/src/ariaSnapshot.ts index fb7ec948f099f..09c3bef843a7d 100644 --- a/packages/injected/src/ariaSnapshot.ts +++ b/packages/injected/src/ariaSnapshot.ts @@ -77,7 +77,7 @@ function toInternalOptions(options: AriaTreeOptions): InternalOptions { } if (options.mode === 'autoexpect') { // To auto-generate assertions on visible elements. - return { visibility: 'ariaAndVisible', refs: 'all' }; + return { visibility: 'ariaAndVisible', refs: 'none' }; } if (options.mode === 'codegen') { // To generate aria assertion with regex heurisitcs. diff --git a/packages/injected/src/injectedScript.ts b/packages/injected/src/injectedScript.ts index baea5f12c83f1..29446799cb80c 100644 --- a/packages/injected/src/injectedScript.ts +++ b/packages/injected/src/injectedScript.ts @@ -107,6 +107,7 @@ export class InjectedScript { isInsideScope, normalizeWhiteSpace, parseAriaSnapshot, + generateAriaTree, // Builtins protect injected code from clock emulation. builtins: null as unknown as Builtins, }; diff --git a/packages/injected/src/recorder/recorder.ts b/packages/injected/src/recorder/recorder.ts index 02e955da06b55..3be1809fdb4a3 100644 --- a/packages/injected/src/recorder/recorder.ts +++ b/packages/injected/src/recorder/recorder.ts @@ -23,6 +23,7 @@ import type { ElementText } from '../selectorUtils'; import type * as actions from '@recorder/actions'; import type { ElementInfo, Mode, OverlayState, UIState } from '@recorder/recorderTypes'; import type { Language } from '@isomorphic/locatorGenerators'; +import type { AriaNode, AriaSnapshot } from '@injected/ariaSnapshot'; const HighlightColors = { multiple: '#f6b26b7f', @@ -543,21 +544,13 @@ class RecordActionTool implements RecorderTool { consumeEvent(event); } - private _captureAriaSnapshotForAction(action: actions.Action) { - const documentElement = this._recorder.injectedScript.document.documentElement; - if (documentElement) - action.ariaSnapshot = this._recorder.injectedScript.ariaSnapshot(documentElement, { mode: 'autoexpect' }); - } - private _recordAction(action: actions.Action) { - this._captureAriaSnapshotForAction(action); this._recorder.recordAction(action); } private _performAction(action: actions.PerformOnRecordAction) { this._recorder.updateHighlight(null, false); - this._captureAriaSnapshotForAction(action); this._performingActions.add(action); const promise = this._recorder.performAction(action).then(() => { @@ -1267,6 +1260,7 @@ export class Recorder { private _tools: Record; private _lastHighlightedSelector: string | undefined = undefined; private _lastHighlightedAriaTemplateJSON: string = 'undefined'; + private _lastActionAutoexpectSnapshot: AriaSnapshot | undefined; readonly highlight: Highlight; readonly overlay: Overlay | undefined; private _stylesheet: CSSStyleSheet; @@ -1585,11 +1579,25 @@ export class Recorder { void this._delegate.setMode?.(mode); } + private _captureAutoExpectSnapshot() { + const documentElement = this.injectedScript.document.documentElement; + return documentElement ? this.injectedScript.utils.generateAriaTree(documentElement, { mode: 'autoexpect' }) : undefined; + } + async performAction(action: actions.PerformOnRecordAction) { + const previousSnapshot = this._lastActionAutoexpectSnapshot; + this._lastActionAutoexpectSnapshot = this._captureAutoExpectSnapshot(); + if (!isAssertAction(action) && this._lastActionAutoexpectSnapshot) { + const element = findNewElement(previousSnapshot, this._lastActionAutoexpectSnapshot); + action.preconditionSelector = element ? this.injectedScript.generateSelector(element, { testIdAttributeName: this.state.testIdAttributeName }).selector : undefined; + if (action.preconditionSelector === action.selector) + action.preconditionSelector = undefined; + } await this._delegate.performAction?.(action).catch(() => {}); } recordAction(action: actions.Action) { + this._lastActionAutoexpectSnapshot = this._captureAutoExpectSnapshot(); void this._delegate.recordAction?.(action); } @@ -1794,3 +1802,56 @@ function createSvgElement(doc: Document, { tagName, attrs, children }: SvgJson): return elem; } + +function isAssertAction(action: actions.Action): action is actions.AssertAction { + return action.name.startsWith('assert'); +} + +function findNewElement(from: AriaSnapshot | undefined, to: AriaSnapshot): Element | undefined { + type ByRoleAndName = Map>; + + function fillMap(root: AriaNode, map: ByRoleAndName, position: number) { + let size = 1; + let childPosition = position + size; + for (const child of root.children || []) { + if (typeof child === 'string') { + size++; + childPosition++; + } else { + size += fillMap(child, map, childPosition); + childPosition += size; + } + } + if (!['none', 'presentation', 'fragment', 'iframe', 'generic'].includes(root.role) && root.name) { + let byRole = map.get(root.role); + if (!byRole) { + byRole = new Map(); + map.set(root.role, byRole); + } + const existing = byRole.get(root.name); + // This heuristic prioritizes elements at the top of the page, even if somewhat smaller. + const sizeAndPosition = size * 100 - position; + if (!existing || existing.sizeAndPosition < sizeAndPosition) + byRole.set(root.name, { node: root, sizeAndPosition }); + } + return size; + } + + const fromMap: ByRoleAndName = new Map(); + if (from) + fillMap(from.root, fromMap, 0); + + const toMap: ByRoleAndName = new Map(); + fillMap(to.root, toMap, 0); + + const result: { node: AriaNode, sizeAndPosition: number }[] = []; + for (const [role, byRole] of toMap) { + for (const [name, byName] of byRole) { + const inFrom = fromMap.get(role)?.get(name); + if (!inFrom) + result.push(byName); + } + } + result.sort((a, b) => b.sizeAndPosition - a.sizeAndPosition); + return result[0]?.node.element; +} diff --git a/packages/playwright-core/src/server/codegen/language.ts b/packages/playwright-core/src/server/codegen/language.ts index 9eba5e6725971..cef97252a80e2 100644 --- a/packages/playwright-core/src/server/codegen/language.ts +++ b/packages/playwright-core/src/server/codegen/language.ts @@ -22,11 +22,33 @@ import type * as actions from '@recorder/actions'; export function generateCode(actions: actions.ActionInContext[], languageGenerator: LanguageGenerator, options: LanguageGeneratorOptions) { const header = languageGenerator.generateHeader(options); const footer = languageGenerator.generateFooter(options.saveStorage); - const actionTexts = actions.map(a => languageGenerator.generateAction(a)).filter(Boolean); + const actionTexts = actions.map(a => generateActionText(languageGenerator, a)).filter(Boolean) as string[]; const text = [header, ...actionTexts, footer].join('\n'); return { header, footer, actionTexts, text }; } +function generateActionText(generator: LanguageGenerator, action: actions.ActionInContext): string | undefined { + let text = generator.generateAction(action); + if (!text) + return; + if (action.action.preconditionSelector) { + const expectAction: actions.ActionInContext = { + frame: action.frame, + startTime: action.startTime, + endTime: action.startTime, + action: { + name: 'assertVisible', + selector: action.action.preconditionSelector, + signals: [], + }, + }; + const expectText = generator.generateAction(expectAction); + if (expectText) + text = expectText + '\n' + text; + } + return text; +} + export function sanitizeDeviceOptions(device: any, options: BrowserContextOptions): BrowserContextOptions { // Filter out all the properties from the device descriptor. const cleanedOptions: Record = {}; diff --git a/packages/playwright-core/src/server/recorder.ts b/packages/playwright-core/src/server/recorder.ts index 152aa9bea12a5..5b4a0d6646cea 100644 --- a/packages/playwright-core/src/server/recorder.ts +++ b/packages/playwright-core/src/server/recorder.ts @@ -20,7 +20,7 @@ import fs from 'fs'; import { isUnderTest } from '../utils'; import { BrowserContext } from './browserContext'; import { Debugger } from './debugger'; -import { buildFullSelector, generateFrameSelector, isAssertAction, metadataToCallLog, shouldMergeAction } from './recorder/recorderUtils'; +import { buildFullSelector, generateFrameSelector, metadataToCallLog } from './recorder/recorderUtils'; import { locatorOrSelectorAsSelector } from '../utils/isomorphic/locatorParser'; import { stringifySelector } from '../utils/isomorphic/selectorParser'; import { ProgressController } from './progress'; @@ -31,8 +31,6 @@ import { eventsHelper, monotonicTime } from './../utils'; import { Frame } from './frames'; import { Page } from './page'; import { performAction } from './recorder/recorderRunner'; -import { findNewElementRef } from '../utils/isomorphic/ariaSnapshot'; -import { yaml } from '../utilsBundle'; import type { Language } from './codegen/types'; import type { CallMetadata, InstrumentationListener, SdkObject } from './instrumentation'; @@ -522,60 +520,9 @@ export class Recorder extends EventEmitter implements Instrume return actionInContext; } - private async _maybeGenerateAssertAction(frame: Frame, actionInContext: actions.ActionInContext) { - const lastAction = getLastFrameAction(frame); - if (isAssertAction(actionInContext.action)) - return; - if (lastAction && (isAssertAction(lastAction.action) || shouldMergeAction(actionInContext, lastAction))) - return; - const newSnapshot = actionInContext.action.ariaSnapshot; - if (!newSnapshot) - return; - const lastSnapshot = lastAction?.action.ariaSnapshot || `- document [ref=e1]\n`; - if (!lastSnapshot) - return; - const callMetadata = serverSideCallMetadata(); - const controller = new ProgressController(callMetadata, frame); - const selector = await controller.run(async progress => { - const ref = findNewElementRef(yaml, lastSnapshot, newSnapshot); - if (!ref) - return; - // Note: recorder runs in the main world, so we need to resolve the ref in the main world. - // Note: resolveSelector always returns a |page|-based selector, not |frame|-based. - const { resolvedSelector } = await frame.resolveSelector(progress, `aria-ref=${ref}`, { mainWorld: true }).catch(() => ({ resolvedSelector: undefined })); - if (!resolvedSelector) - return; - const isVisible = await frame._page.mainFrame().isVisible(progress, resolvedSelector, { strict: true }).catch(() => false); - return isVisible ? resolvedSelector : undefined; - }).catch(() => undefined); - if (!selector) - return; - if (!actionInContext.frame.framePath.length && 'selector' in actionInContext.action && actionInContext.action.selector === selector) - return; // Do not assert the action target, auto-waiting takes care of it. - const assertActionInContext: actions.ActionInContext = { - frame: { - pageGuid: actionInContext.frame.pageGuid, - pageAlias: actionInContext.frame.pageAlias, - framePath: [], - }, - action: { - name: 'assertVisible', - selector, - signals: [], - }, - startTime: actionInContext.startTime, - endTime: actionInContext.startTime, - }; - return assertActionInContext; - } - private async _performAction(frame: Frame, action: actions.PerformOnRecordAction) { const actionInContext = await this._createActionInContext(frame, action); - const assertActionInContext = await this._maybeGenerateAssertAction(frame, actionInContext); - if (assertActionInContext) - this._signalProcessor.addAction(assertActionInContext); this._signalProcessor.addAction(actionInContext); - setLastFrameAction(frame, actionInContext); if (actionInContext.action.name !== 'openPage' && actionInContext.action.name !== 'closePage') await performAction(this._pageAliases, actionInContext); actionInContext.endTime = monotonicTime(); @@ -584,7 +531,6 @@ export class Recorder extends EventEmitter implements Instrume private async _recordAction(frame: Frame, action: actions.Action) { const actionInContext = await this._createActionInContext(frame, action); this._signalProcessor.addAction(actionInContext); - setLastFrameAction(frame, actionInContext); } private _onFrameNavigated(frame: Frame, page: Page) { @@ -624,11 +570,3 @@ function languageForFile(file: string) { return 'csharp'; return 'javascript'; } - -const kLastFrameActionSymbol = Symbol('lastFrameAction'); -function getLastFrameAction(frame: Frame): actions.ActionInContext | undefined { - return (frame as any)[kLastFrameActionSymbol]; -} -function setLastFrameAction(frame: Frame, action: actions.ActionInContext | undefined) { - (frame as any)[kLastFrameActionSymbol] = action; -} diff --git a/packages/playwright-core/src/server/recorder/recorderUtils.ts b/packages/playwright-core/src/server/recorder/recorderUtils.ts index c1026d343e183..03693b3f580d6 100644 --- a/packages/playwright-core/src/server/recorder/recorderUtils.ts +++ b/packages/playwright-core/src/server/recorder/recorderUtils.ts @@ -73,10 +73,6 @@ export async function frameForAction(pageAliases: Map, actionInCon return result.frame; } -export function isAssertAction(action: actions.Action): action is actions.AssertAction { - return action.name.startsWith('assert'); -} - function isSameAction(a: actions.ActionInContext, b: actions.ActionInContext): boolean { return a.action.name === b.action.name && a.frame.pageAlias === b.frame.pageAlias && a.frame.framePath.join('|') === b.frame.framePath.join('|'); } diff --git a/packages/playwright-core/src/utils/isomorphic/ariaSnapshot.ts b/packages/playwright-core/src/utils/isomorphic/ariaSnapshot.ts index 69a2f5819d4d2..a51921bc701e5 100644 --- a/packages/playwright-core/src/utils/isomorphic/ariaSnapshot.ts +++ b/packages/playwright-core/src/utils/isomorphic/ariaSnapshot.ts @@ -32,7 +32,6 @@ export type AriaProps = { level?: number; pressed?: boolean | 'mixed'; selected?: boolean; - ref?: string; }; // We pass parsed template between worlds using JSON, make it easy. @@ -65,7 +64,7 @@ type YamlLibrary = { }; type ParsedYamlPosition = { line: number; col: number; }; -type ParsingOptions = yamlTypes.ParseOptions & { allowRef?: boolean, allowUnknownAttributes?: boolean }; +type ParsingOptions = yamlTypes.ParseOptions; export type ParsedYamlError = { message: string; @@ -434,10 +433,6 @@ export class KeyParser { } private _applyAttribute(node: AriaTemplateRoleNode, key: string, value: string, errorPos: number) { - if (this._options.allowRef && key === 'ref') { - node.ref = value; - return; - } if (key === 'checked') { this._assert(value === 'true' || value === 'false' || value === 'mixed', 'Value of "checked\" attribute must be a boolean or "mixed"', errorPos); node.checked = value === 'true' ? true : value === 'false' ? false : 'mixed'; @@ -473,8 +468,6 @@ export class KeyParser { node.selected = value === 'true'; return; } - if (this._options.allowUnknownAttributes) - return; this._assert(false, `Unsupported attribute [${key}]`, errorPos); } @@ -492,55 +485,3 @@ export class ParserError extends Error { this.pos = pos; } } - -export function findNewElementRef(yaml: YamlLibrary, fromSnapshot: string, toSnapshot: string): string | undefined { - type ByRoleAndName = Map>; - - function fillMap(root: AriaTemplateRoleNode, map: ByRoleAndName, position: number) { - let size = 1; - let childPosition = position + size; - for (const child of root.children || []) { - if (child.kind === 'role') { - size += fillMap(child, map, childPosition); - childPosition += size; - } else { - size++; - childPosition++; - } - } - if (!['none', 'presentation', 'fragment', 'iframe', 'generic'].includes(root.role) && typeof root.name === 'string' && root.name) { - let byRole = map.get(root.role); - if (!byRole) { - byRole = new Map(); - map.set(root.role, byRole); - } - const existing = byRole.get(root.name); - // This heuristic prioritizes elements at the top of the page, even if somewhat smaller. - const sizeAndPosition = size * 100 - position; - if (!existing || existing.sizeAndPosition < sizeAndPosition) - byRole.set(root.name, { node: root, sizeAndPosition }); - } - return size; - } - - const fromMap: ByRoleAndName = new Map(); - const from = parseAriaSnapshotUnsafe(yaml, fromSnapshot, { allowRef: true, allowUnknownAttributes: true }); - if (from.kind === 'role') - fillMap(from, fromMap, 0); - - const toMap: ByRoleAndName = new Map(); - const to = parseAriaSnapshotUnsafe(yaml, toSnapshot, { allowRef: true, allowUnknownAttributes: true }); - if (to.kind === 'role') - fillMap(to, toMap, 0); - - const result: { node: AriaTemplateRoleNode, sizeAndPosition: number }[] = []; - for (const [role, byRole] of toMap) { - for (const [name, byName] of byRole) { - const inFrom = fromMap.get(role)?.get(name); - if (!inFrom) - result.push(byName); - } - } - result.sort((a, b) => b.sizeAndPosition - a.sizeAndPosition); - return result.find(r => r.node.ref)?.node.ref; -} diff --git a/packages/recorder/src/actions.d.ts b/packages/recorder/src/actions.d.ts index e52b420b9041f..4f1a9bb353198 100644 --- a/packages/recorder/src/actions.d.ts +++ b/packages/recorder/src/actions.d.ts @@ -37,6 +37,7 @@ export type ActionBase = { name: ActionName, signals: Signal[], ariaSnapshot?: string, + preconditionSelector?: string, }; export type ActionWithSelector = ActionBase & {