chore: migrate Promise.race to scopes to prevent leaks (#24160)

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;
     }

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<types.World, ContextData>();
   private _childFrames = new Set<Frame>();
   _name = '';
   _inflightRequests = new Set<network.Request>();
   private _networkIdleTimer: NodeJS.Timer | undefined;
   private _setContentCounter = 0;
-  readonly _detachedPromise: Promise<void>;
-  private _detachedCallback = () => {};
+  readonly _detachedRace = new ScopedRace();
   private _raceAgainstEvaluationStallingEventsPromises = new Set<ManualPromise<any>>();
   readonly _redirectedNavigations = new Map<string, { url: string, gotoPromise: Promise<network.Response | null> }>(); // 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<void>(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<network.Response | null>): Promise<network.Response | null> {
-    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();
   }

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<Error>();
-  readonly _crashedPromise = new ManualPromise<Error>();
+  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) {

packages/playwright-core/src/utils/manualPromise.ts

@@ -59,8 +59,10 @@ export class ManualPromise<T = void> extends Promise<T> {
 export class ScopedRace {
   private _terminateError: Error | undefined;
   private _terminatePromises = new Map<ManualPromise<Error>, 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<T>(promise: Promise<T>): Promise<T> {
-    return this._race([promise], false) as Promise<T>;
+  isDone() {
+    return this._isDone;
+  }
+
+  static async raceMultiple<T>(scopes: ScopedRace[], promise: Promise<T>): Promise<T> {
+    return Promise.race(scopes.map(s => s.race(promise)));
+  }
+
+  async race<T>(promise: Promise<T> | Promise<T>[]): Promise<T> {
+    return this._race(Array.isArray(promise) ? promise : [promise], false) as Promise<T>;
   }
 
   async safeRace<T>(promise: Promise<T>, defaultValue?: T): Promise<T> {

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 }) => {

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 }) => {