diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index 5469028964..5ccef970d5 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -190,7 +190,7 @@ export abstract class BrowserContext extends SdkObject { const [, ...otherPages] = this.pages(); for (const p of otherPages) await p.close(metadata); - if (page && page._crashedPromise.isDone()) { + if (page && page._crashedRace.isDone()) { await page.close(metadata); page = undefined; } diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index 4a42a3de35..fe1a363fad 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -29,7 +29,7 @@ import * as types from './types'; import { BrowserContext } from './browserContext'; import type { Progress } from './progress'; import { ProgressController } from './progress'; -import { assert, constructURLBasedOnBaseURL, makeWaitForNextTask, monotonicTime } from '../utils'; +import { ScopedRace, assert, constructURLBasedOnBaseURL, makeWaitForNextTask, monotonicTime } from '../utils'; import { ManualPromise } from '../utils/manualPromise'; import { debugLogger } from '../common/debugLogger'; import type { CallMetadata } from './instrumentation'; @@ -476,15 +476,13 @@ export class Frame extends SdkObject { readonly _page: Page; private _parentFrame: Frame | null; _url = ''; - private _detached = false; private _contextData = new Map(); private _childFrames = new Set(); _name = ''; _inflightRequests = new Set(); private _networkIdleTimer: NodeJS.Timer | undefined; private _setContentCounter = 0; - readonly _detachedPromise: Promise; - private _detachedCallback = () => {}; + readonly _detachedRace = new ScopedRace(); private _raceAgainstEvaluationStallingEventsPromises = new Set>(); readonly _redirectedNavigations = new Map }>(); // documentId -> data readonly selectors: FrameSelectors; @@ -498,8 +496,6 @@ export class Frame extends SdkObject { this._currentDocument = { documentId: undefined, request: undefined }; this.selectors = new FrameSelectors(this); - this._detachedPromise = new Promise(x => this._detachedCallback = x); - this._contextData.set('main', { contextPromise: new ManualPromise(), context: null }); this._contextData.set('utility', { contextPromise: new ManualPromise(), context: null }); this._setContext('main', null); @@ -514,7 +510,7 @@ export class Frame extends SdkObject { } isDetached(): boolean { - return this._detached; + return this._detachedRace.isDone(); } _onLifecycleEvent(event: RegularLifecycleEvent) { @@ -617,21 +613,20 @@ export class Frame extends SdkObject { } async raceNavigationAction(progress: Progress, options: types.GotoOptions, action: () => Promise): Promise { - return Promise.race([ - this._page._disconnectedPromise.then(() => { throw new Error('Navigation failed because page was closed!'); }), - this._page._crashedPromise.then(() => { throw new Error('Navigation failed because page crashed!'); }), - this._detachedPromise.then(() => { throw new Error('Navigating frame was detached!'); }), - action().catch(e => { - if (e instanceof NavigationAbortedError && e.documentId) { - const data = this._redirectedNavigations.get(e.documentId); - if (data) { - progress.log(`waiting for redirected navigation to "${data.url}"`); - return data.gotoPromise; - } + return ScopedRace.raceMultiple([ + this._detachedRace, + this._page._disconnectedRace, + this._page._crashedRace, + ], action().catch(e => { + if (e instanceof NavigationAbortedError && e.documentId) { + const data = this._redirectedNavigations.get(e.documentId); + if (data) { + progress.log(`waiting for redirected navigation to "${data.url}"`); + return data.gotoPromise; } - throw e; - }), - ]); + } + throw e; + })); } redirectNavigation(url: string, documentId: string, referer: string | undefined) { @@ -1057,12 +1052,12 @@ export class Frame extends SdkObject { if (timeout) { // Make sure we react immediately upon page close or frame detach. // We need this to show expected/received values in time. - await Promise.race([ - this._page._disconnectedPromise, - this._page._crashedPromise, - this._detachedPromise, - new Promise(f => setTimeout(f, timeout)), - ]); + const actionPromise = new Promise(f => setTimeout(f, timeout)); + await ScopedRace.raceMultiple([ + this._page._disconnectedRace, + this._page._crashedRace, + this._detachedRace, + ], actionPromise); } progress.throwIfAborted(); try { @@ -1538,9 +1533,8 @@ export class Frame extends SdkObject { _onDetached() { this._stopNetworkIdleTimer(); - this._detached = true; - this._detachedCallback(); const error = new Error('Frame was detached'); + this._detachedRace.scopeClosed(error); for (const data of this._contextData.values()) { if (data.context) data.context.contextDestroyed(error); @@ -1606,7 +1600,7 @@ export class Frame extends SdkObject { _contextDestroyed(context: dom.FrameExecutionContext) { // Sometimes we get this after detach, in which case we should not reset // our already destroyed contexts to something that will never resolve. - if (this._detached) + if (this._detachedRace.isDone()) return; context.contextDestroyed(new Error('Execution context was destroyed, most likely because of a navigation')); for (const [world, data] of this._contextData) { @@ -1620,7 +1614,7 @@ export class Frame extends SdkObject { // We should not start a timer and report networkidle in detached frames. // This happens at least in Firefox for child frames, where we may get requestFinished // after the frame was detached - probably a race in the Firefox itself. - if (this._firedLifecycleEvents.has('networkidle') || this._detached) + if (this._firedLifecycleEvents.has('networkidle') || this._detachedRace.isDone()) return; this._networkIdleTimer = setTimeout(() => { this._firedNetworkIdleSelf = true; @@ -1709,12 +1703,11 @@ class SignalBarrier { this._progress.log(` navigated to "${frame._url}"`); return true; }); - await Promise.race([ - frame._page._disconnectedPromise, - frame._page._crashedPromise, - frame._detachedPromise, - waiter.promise, - ]).catch(e => {}); + await ScopedRace.raceMultiple([ + frame._page._disconnectedRace, + frame._page._crashedRace, + frame._detachedRace, + ], waiter.promise).catch(() => {}); waiter.dispose(); this.release(); } diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index d5cba690f5..c5be62fe4e 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -31,7 +31,7 @@ import * as accessibility from './accessibility'; import { FileChooser } from './fileChooser'; import type { Progress } from './progress'; import { ProgressController } from './progress'; -import { assert, isError } from '../utils'; +import { ScopedRace, assert, isError } from '../utils'; import { ManualPromise } from '../utils/manualPromise'; import { debugLogger } from '../common/debugLogger'; import type { ImageComparatorOptions } from '../utils/comparators'; @@ -142,8 +142,8 @@ export class Page extends SdkObject { private _disconnected = false; private _initialized = false; private _eventsToEmitAfterInitialized: { event: string | symbol, args: any[] }[] = []; - readonly _disconnectedPromise = new ManualPromise(); - readonly _crashedPromise = new ManualPromise(); + readonly _disconnectedRace = new ScopedRace(); + readonly _crashedRace = new ScopedRace(); readonly _browserContext: BrowserContext; readonly keyboard: input.Keyboard; readonly mouse: input.Mouse; @@ -285,7 +285,7 @@ export class Page extends SdkObject { this._frameManager.dispose(); this._frameThrottler.dispose(); this.emit(Page.Events.Crash); - this._crashedPromise.resolve(new Error('Page crashed')); + this._crashedRace.scopeClosed(new Error('Page crashed')); this.instrumentation.onPageClose(this); } @@ -294,7 +294,7 @@ export class Page extends SdkObject { this._frameThrottler.dispose(); assert(!this._disconnected, 'Page disconnected twice'); this._disconnected = true; - this._disconnectedPromise.resolve(new Error('Page closed')); + this._disconnectedRace.scopeClosed(new Error('Page closed')); } async _onFileChooserOpened(handle: dom.ElementHandle) { @@ -632,7 +632,7 @@ export class Page extends SdkObject { } isClosedOrClosingOrCrashed() { - return this._closedState !== 'open' || this._crashedPromise.isDone(); + return this._closedState !== 'open' || this._crashedRace.isDone(); } _addWorker(workerId: string, worker: Worker) { diff --git a/packages/playwright-core/src/utils/manualPromise.ts b/packages/playwright-core/src/utils/manualPromise.ts index 5867840c10..ff6342056b 100644 --- a/packages/playwright-core/src/utils/manualPromise.ts +++ b/packages/playwright-core/src/utils/manualPromise.ts @@ -59,8 +59,10 @@ export class ManualPromise extends Promise { export class ScopedRace { private _terminateError: Error | undefined; private _terminatePromises = new Map, Error>(); + private _isDone = false; scopeClosed(error: Error) { + this._isDone = true; this._terminateError = error; for (const [p, e] of this._terminatePromises) { rewriteErrorMessage(e, error.message); @@ -68,8 +70,16 @@ export class ScopedRace { } } - async race(promise: Promise): Promise { - return this._race([promise], false) as Promise; + isDone() { + return this._isDone; + } + + static async raceMultiple(scopes: ScopedRace[], promise: Promise): Promise { + return Promise.race(scopes.map(s => s.race(promise))); + } + + async race(promise: Promise | Promise[]): Promise { + return this._race(Array.isArray(promise) ? promise : [promise], false) as Promise; } async safeRace(promise: Promise, defaultValue?: T): Promise { diff --git a/tests/library/page-event-crash.spec.ts b/tests/library/page-event-crash.spec.ts index 2da4ef40fc..ea90023630 100644 --- a/tests/library/page-event-crash.spec.ts +++ b/tests/library/page-event-crash.spec.ts @@ -67,7 +67,7 @@ test('should cancel navigation when page crashes', async ({ server, page, crash await page.waitForNavigation({ waitUntil: 'domcontentloaded' }); crash(); const error = await promise; - expect(error.message).toContain('Navigation failed because page crashed'); + expect(error.message).toContain('page.goto: Page crashed'); }); test('should be able to close context when page crashes', async ({ isAndroid, isElectron, isWebView2, page, crash }) => { diff --git a/tests/page/frame-goto.spec.ts b/tests/page/frame-goto.spec.ts index 7eba57711d..83d560a53a 100644 --- a/tests/page/frame-goto.spec.ts +++ b/tests/page/frame-goto.spec.ts @@ -38,9 +38,9 @@ it('should reject when frame detaches', async ({ page, server, browserName }) => await page.$eval('iframe', frame => frame.remove()); const error = await navigationPromise; if (browserName === 'chromium') - expect(error.message.includes('net::ERR_ABORTED') || error.message.includes('frame was detached')).toBe(true); + expect(error.message.includes('net::ERR_ABORTED') || error.message.includes('Frame was detached')).toBe(true); else - expect(error.message).toContain('frame was detached'); + expect(error.message).toContain('Frame was detached'); }); it('should continue after client redirect', async ({ page, server, isAndroid }) => {