chore: remove Progress.raceWithCleanup

Now that legacy timeout mechanism is gone, we can replace
all kinds of cleanups with an explicit try/catch.

Author: dgozman

packages/playwright-core/src/server/android/android.ts

@@ -79,8 +79,7 @@ export class Android extends SdkObject {
       newSerials.add(d.serial);
       if (this._devices.has(d.serial))
         continue;
-      const device = await progress.raceWithCleanup(AndroidDevice.create(this, d, options), device => device.close());
-      this._devices.set(d.serial, device);
+      await progress.race(AndroidDevice.create(this, d, options).then(device => this._devices.set(d.serial, device)));
     }
     for (const d of this._devices.keys()) {
       if (!newSerials.has(d))
@@ -152,7 +151,7 @@ export class AndroidDevice extends SdkObject {
   }
 
   async open(progress: Progress, command: string): Promise<SocketBackend> {
-    return await progress.raceWithCleanup(this._backend.open(`${command}`), socket => socket.close());
+    return await this._open(progress, command);
   }
 
   async screenshot(): Promise<Buffer> {
@@ -216,7 +215,7 @@ export class AndroidDevice extends SdkObject {
     debug('pw:android')(`Polling the socket localabstract:${socketName}`);
     while (!socket) {
       try {
-        socket = await progress.raceWithCleanup(this._backend.open(`localabstract:${socketName}`), socket => socket.close());
+        socket = await this._open(progress, `localabstract:${socketName}`);
       } catch (e) {
         if (isAbortError(e))
           throw e;
@@ -354,14 +353,29 @@ export class AndroidDevice extends SdkObject {
     return defaultContext;
   }
 
+  private async _open(progress: Progress, command: string): Promise<SocketBackend> {
+    let aborted = false;
+    try {
+      return await progress.race(this._backend.open(command).then(socket => {
+        // Make sure to close the opened socket if progress was aborted before that.
+        if (aborted)
+          socket.close();
+        return socket;
+      }));
+    } catch (error) {
+      aborted = true;
+      throw error;
+    }
+  }
+
   webViews(): channels.AndroidWebView[] {
     return [...this._webViews.values()];
   }
 
   async installApk(progress: Progress, content: Buffer, options?: { args?: string[] }): Promise<void> {
     const args = options && options.args ? options.args : ['-r', '-t', '-S'];
     debug('pw:android')('Opening install socket');
-    const installSocket = await progress.raceWithCleanup(this._backend.open(`shell:cmd package install ${args.join(' ')} ${content.length}`), socket => socket.close());
+    const installSocket = await this._open(progress, `shell:cmd package install ${args.join(' ')} ${content.length}`);
     debug('pw:android')('Writing driver bytes: ' + content.length);
     await progress.race(installSocket.write(content));
     const success = await progress.race(new Promise(f => installSocket.on('data', f)));
@@ -370,7 +384,7 @@ export class AndroidDevice extends SdkObject {
   }
 
   async push(progress: Progress, content: Buffer, path: string, mode = 0o644): Promise<void> {
-    const socket = await progress.raceWithCleanup(this._backend.open(`sync:`), socket => socket.close());
+    const socket = await this._open(progress, `sync:`);
     const sendHeader = async (command: string, length: number) => {
       const buffer = Buffer.alloc(command.length + 4);
       buffer.write(command, 0);

packages/playwright-core/src/server/bidi/bidiChromium.ts

@@ -82,6 +82,7 @@ export class BidiChromium extends BrowserType {
   }
 
   override attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void {
+    // Note that it's fine to reuse the transport, since our connection ignores kBrowserCloseMessageId.
     const bidiTransport = (transport as any)[kBidiOverCdpWrapper];
     if (bidiTransport)
       transport = bidiTransport;

packages/playwright-core/src/server/bidi/bidiFirefox.ts

@@ -78,6 +78,7 @@ export class BidiFirefox extends BrowserType {
   }
 
   override attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void {
+    // Note that it's fine to reuse the transport, since our connection ignores kBrowserCloseMessageId.
     transport.send({ method: 'browser.close', params: {}, id: kBrowserCloseMessageId });
   }
 

packages/playwright-core/src/server/browser.ts

@@ -100,20 +100,27 @@ export abstract class Browser extends SdkObject {
   async newContext(progress: Progress, options: types.BrowserContextOptions): Promise<BrowserContext> {
     validateBrowserContextOptions(options, this.options);
     let clientCertificatesProxy: ClientCertificatesProxy | undefined;
-    if (options.clientCertificates?.length) {
-      clientCertificatesProxy = await progress.raceWithCleanup(ClientCertificatesProxy.create(options), proxy => proxy.close());
-      options = { ...options };
-      options.proxyOverride = clientCertificatesProxy.proxySettings();
-      options.internalIgnoreHTTPSErrors = true;
+    let context: BrowserContext | undefined;
+    try {
+      if (options.clientCertificates?.length) {
+        clientCertificatesProxy = await ClientCertificatesProxy.create(progress, options);
+        options = { ...options };
+        options.proxyOverride = clientCertificatesProxy.proxySettings();
+        options.internalIgnoreHTTPSErrors = true;
+      }
+      context = await progress.race(this.doCreateNewContext(options));
+      context._clientCertificatesProxy = clientCertificatesProxy;
+      if ((options as any).__testHookBeforeSetStorageState)
+        await progress.race((options as any).__testHookBeforeSetStorageState());
+      if (options.storageState)
+        await context.setStorageState(progress, options.storageState);
+      this.emit(Browser.Events.Context, context);
+      return context;
+    } catch (error) {
+      await context?.close({ reason: 'Failed to create context' }).catch(() => {});
+      await clientCertificatesProxy?.close().catch(() => {});
+      throw error;
     }
-    const context = await progress.raceWithCleanup(this.doCreateNewContext(options), context => context.close({ reason: 'Failed to create context' }));
-    context._clientCertificatesProxy = clientCertificatesProxy;
-    if ((options as any).__testHookBeforeSetStorageState)
-      await progress.race((options as any).__testHookBeforeSetStorageState());
-    if (options.storageState)
-      await context.setStorageState(progress, options.storageState);
-    this.emit(Browser.Events.Context, context);
-    return context;
   }
 
   async newContextForReuse(progress: Progress, params: channels.BrowserNewContextForReuseParams): Promise<BrowserContext> {

packages/playwright-core/src/server/browserContext.ts

@@ -528,14 +528,20 @@ export abstract class BrowserContext extends SdkObject {
   }
 
   async newPage(progress: Progress, isServerSide: boolean): Promise<Page> {
-    const page = await progress.raceWithCleanup(this.doCreateNewPage(isServerSide), page => page.close());
-    const pageOrError = await progress.race(page.waitForInitializedOrError());
-    if (pageOrError instanceof Page) {
-      if (pageOrError.isClosed())
-        throw new Error('Page has been closed.');
-      return pageOrError;
+    let page: Page | undefined;
+    try {
+      page = await progress.race(this.doCreateNewPage(isServerSide));
+      const pageOrError = await progress.race(page.waitForInitializedOrError());
+      if (pageOrError instanceof Page) {
+        if (pageOrError.isClosed())
+          throw new Error('Page has been closed.');
+        return pageOrError;
+      }
+      throw pageOrError;
+    } catch (error) {
+      await page?.close({ reason: 'Failed to create page' }).catch(() => {});
+      throw error;
     }
-    throw pageOrError;
   }
 
   addVisitedOrigin(origin: string) {

packages/playwright-core/src/server/browserType.ts

@@ -23,7 +23,7 @@ import { debugMode } from './utils/debug';
 import { assert } from '../utils/isomorphic/assert';
 import { ManualPromise } from '../utils/isomorphic/manualPromise';
 import { DEFAULT_PLAYWRIGHT_TIMEOUT } from '../utils/isomorphic/time';
-import { existsAsync, removeFolders } from './utils/fileUtils';
+import { existsAsync } from './utils/fileUtils';
 import { helper } from './helper';
 import { SdkObject } from './instrumentation';
 import { PipeTransport } from './pipeTransport';
@@ -78,14 +78,19 @@ export abstract class BrowserType extends SdkObject {
     // Note: Any initial TLS requests will fail since we rely on the Page/Frames initialize which sets ignoreHTTPSErrors.
     let clientCertificatesProxy: ClientCertificatesProxy | undefined;
     if (options.clientCertificates?.length) {
-      clientCertificatesProxy = await progress.raceWithCleanup(ClientCertificatesProxy.create(options), proxy => proxy.close());
+      clientCertificatesProxy = await ClientCertificatesProxy.create(progress, options);
       launchOptions.proxyOverride = clientCertificatesProxy.proxySettings();
       options = { ...options };
       options.internalIgnoreHTTPSErrors = true;
     }
-    const browser = await this._innerLaunchWithRetries(progress, launchOptions, options, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); });
-    browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy;
-    return browser._defaultContext!;
+    try {
+      const browser = await this._innerLaunchWithRetries(progress, launchOptions, options, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); });
+      browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy;
+      return browser._defaultContext!;
+    } catch (error) {
+      await clientCertificatesProxy?.close().catch(() => {});
+      throw error;
+    }
   }
 
   async _innerLaunchWithRetries(progress: Progress, options: types.LaunchOptions, persistent: types.BrowserContextOptions | undefined, protocolLogger: types.ProtocolLogger, userDataDir?: string): Promise<Browser> {
@@ -106,35 +111,40 @@ export abstract class BrowserType extends SdkObject {
     options.proxy = options.proxy ? normalizeProxySettings(options.proxy) : undefined;
     const browserLogsCollector = new RecentLogsCollector();
     const { browserProcess, userDataDir, artifactsDir, transport } = await this._launchProcess(progress, options, !!persistent, browserLogsCollector, maybeUserDataDir);
-    if ((options as any).__testHookBeforeCreateBrowser)
-      await progress.race((options as any).__testHookBeforeCreateBrowser());
-    const browserOptions: BrowserOptions = {
-      name: this._name,
-      isChromium: this._name === 'chromium',
-      channel: options.channel,
-      slowMo: options.slowMo,
-      persistent,
-      headful: !options.headless,
-      artifactsDir,
-      downloadsPath: (options.downloadsPath || artifactsDir)!,
-      tracesDir: (options.tracesDir || artifactsDir)!,
-      browserProcess,
-      customExecutablePath: options.executablePath,
-      proxy: options.proxy,
-      protocolLogger,
-      browserLogsCollector,
-      wsEndpoint: transport instanceof WebSocketTransport ? transport.wsEndpoint : undefined,
-      originalLaunchOptions: options,
-    };
-    if (persistent)
-      validateBrowserContextOptions(persistent, browserOptions);
-    copyTestHooks(options, browserOptions);
-    const browser = await progress.race(this.connectToTransport(transport, browserOptions, browserLogsCollector));
-    (browser as any)._userDataDirForTest = userDataDir;
-    // We assume no control when using custom arguments, and do not prepare the default context in that case.
-    if (persistent && !options.ignoreAllDefaultArgs)
-      await browser._defaultContext!._loadDefaultContext(progress);
-    return browser;
+    try {
+      if ((options as any).__testHookBeforeCreateBrowser)
+        await progress.race((options as any).__testHookBeforeCreateBrowser());
+      const browserOptions: BrowserOptions = {
+        name: this._name,
+        isChromium: this._name === 'chromium',
+        channel: options.channel,
+        slowMo: options.slowMo,
+        persistent,
+        headful: !options.headless,
+        artifactsDir,
+        downloadsPath: (options.downloadsPath || artifactsDir)!,
+        tracesDir: (options.tracesDir || artifactsDir)!,
+        browserProcess,
+        customExecutablePath: options.executablePath,
+        proxy: options.proxy,
+        protocolLogger,
+        browserLogsCollector,
+        wsEndpoint: transport instanceof WebSocketTransport ? transport.wsEndpoint : undefined,
+        originalLaunchOptions: options,
+      };
+      if (persistent)
+        validateBrowserContextOptions(persistent, browserOptions);
+      copyTestHooks(options, browserOptions);
+      const browser = await progress.race(this.connectToTransport(transport, browserOptions, browserLogsCollector));
+      (browser as any)._userDataDirForTest = userDataDir;
+      // We assume no control when using custom arguments, and do not prepare the default context in that case.
+      if (persistent && !options.ignoreAllDefaultArgs)
+        await browser._defaultContext!._loadDefaultContext(progress);
+      return browser;
+    } catch (error) {
+      await browserProcess.close().catch(() => {});
+      throw error;
+    }
   }
 
   private async _prepareToLaunch(options: types.LaunchOptions, isPersistent: boolean, userDataDir: string | undefined) {
@@ -194,7 +204,6 @@ export abstract class BrowserType extends SdkObject {
 
     const env = options.env ? envArrayToObject(options.env) : process.env;
     const prepared = await progress.race(this._prepareToLaunch(options, isPersistent, userDataDir));
-    progress.cleanupWhenAborted(() => removeFolders(prepared.tempDirectories));
 
     // 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.
@@ -217,10 +226,12 @@ export abstract class BrowserType extends SdkObject {
       attemptToGracefullyClose: async () => {
         if ((options as any).__testHookGracefullyClose)
           await (options as any).__testHookGracefullyClose();
-        // We try to gracefully close to prevent crash reporting and core dumps.
-        // Note that it's fine to reuse the pipe transport, since
-        // our connection ignores kBrowserCloseMessageId.
-        this.attemptToGracefullyCloseBrowser(transport!);
+        if (transport) {
+          // We try to gracefully close to prevent crash reporting and core dumps.
+          this.attemptToGracefullyCloseBrowser(transport);
+        } else {
+          throw new Error('Force-killing the browser because no transport is available to gracefully close it.');
+        }
       },
       onExit: (exitCode, signal) => {
         // Unblock launch when browser prematurely exits.
@@ -249,19 +260,22 @@ export abstract class BrowserType extends SdkObject {
       close: () => closeOrKill((options as any).__testHookBrowserCloseTimeout || DEFAULT_PLAYWRIGHT_TIMEOUT),
       kill
     };
-    progress.cleanupWhenAborted(() => closeOrKill(DEFAULT_PLAYWRIGHT_TIMEOUT));
-    const { wsEndpoint } = await progress.race([
-      this.waitForReadyState(options, browserLogsCollector),
-      exitPromise.then(() => ({ wsEndpoint: undefined })),
-    ]);
-    if (options.cdpPort !== undefined || !this.supportsPipeTransport()) {
-      transport = await WebSocketTransport.connect(progress, wsEndpoint!);
-    } else {
-      const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream];
-      transport = new PipeTransport(stdio[3], stdio[4]);
+    try {
+      const { wsEndpoint } = await progress.race([
+        this.waitForReadyState(options, browserLogsCollector),
+        exitPromise.then(() => ({ wsEndpoint: undefined })),
+      ]);
+      if (options.cdpPort !== undefined || !this.supportsPipeTransport()) {
+        transport = await WebSocketTransport.connect(progress, wsEndpoint!);
+      } else {
+        const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream];
+        transport = new PipeTransport(stdio[3], stdio[4]);
+      }
+      return { browserProcess, artifactsDir: prepared.artifactsDir, userDataDir: prepared.userDataDir, transport };
+    } catch (error) {
+      await closeOrKill(DEFAULT_PLAYWRIGHT_TIMEOUT).catch(() => {});
+      throw error;
     }
-    progress.cleanupWhenAborted(() => transport.close());
-    return { browserProcess, artifactsDir: prepared.artifactsDir, userDataDir: prepared.userDataDir, transport };
   }
 
   async _createArtifactDirs(options: types.LaunchOptions): Promise<void> {

packages/playwright-core/src/server/chromium/chromium.ts

@@ -163,6 +163,7 @@ export class Chromium extends BrowserType {
   }
 
   override attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void {
+    // Note that it's fine to reuse the transport, since our connection ignores kBrowserCloseMessageId.
     const message: ProtocolRequest = { method: 'Browser.close', id: kBrowserCloseMessageId, params: {} };
     transport.send(message);
   }

packages/playwright-core/src/server/firefox/firefox.ts

@@ -65,6 +65,7 @@ export class Firefox extends BrowserType {
   }
 
   override attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void {
+    // Note that it's fine to reuse the transport, since our connection ignores kBrowserCloseMessageId.
     const message = { method: 'Browser.close', params: {}, id: kBrowserCloseMessageId };
     transport.send(message);
   }

packages/playwright-core/src/server/frames.ts

@@ -761,7 +761,7 @@ export class Frame extends SdkObject {
           return null;
         return continuePolling;
       }
-      const result = await progress.raceWithCleanup(resolved.injected.evaluateHandle((injected, { info, root }) => {
+      const result = await progress.race(resolved.injected.evaluateHandle((injected, { info, root }) => {
         if (root && !root.isConnected)
           throw injected.createStacklessError('Element is not attached to the DOM');
         const elements = injected.querySelectorAll(info.parsed, root || document);
@@ -776,7 +776,7 @@ export class Frame extends SdkObject {
           log = `  locator resolved to ${visible ? 'visible' : 'hidden'} ${injected.previewNode(element)}`;
         }
         return { log, element, visible, attached: !!element };
-      }, { info: resolved.info, root: resolved.frame === this ? scope : undefined }), handle => handle.dispose());
+      }, { info: resolved.info, root: resolved.frame === this ? scope : undefined }));
       const { log, visible, attached } = await progress.race(result.evaluate(r => ({ log: r.log, visible: r.visible, attached: r.attached })));
       if (log)
         progress.log(log);
@@ -1085,7 +1085,7 @@ export class Frame extends SdkObject {
       const resolved = await progress.race(this.selectors.resolveInjectedForSelector(selector, { strict }));
       if (!resolved)
         return continuePolling;
-      const result = await progress.raceWithCleanup(resolved.injected.evaluateHandle((injected, { info, callId }) => {
+      const result = await progress.race(resolved.injected.evaluateHandle((injected, { info, callId }) => {
         const elements = injected.querySelectorAll(info.parsed, document);
         if (callId)
           injected.markTargetElements(new Set(elements), callId);
@@ -1099,7 +1099,7 @@ export class Frame extends SdkObject {
           log = `  locator resolved to ${injected.previewNode(element)}`;
         }
         return { log, success: !!element, element };
-      }, { info: resolved.info, callId: progress.metadata.id }), handle => handle.dispose());
+      }, { info: resolved.info, callId: progress.metadata.id }));
       const { log, success } = await progress.race(result.evaluate(r => ({ log: r.log, success: r.success })));
       if (log)
         progress.log(log);