chore: replace more cleanupWhenAborted calls with try/catch (microsoft#36833)

Author: dgozman

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

@@ -91,26 +91,26 @@ export class DragManager {
       };
     }
 
-    await this._crPage._page.safeNonStallingEvaluateInAllFrames(`(${setupDragListeners.toString()})()`, 'utility');
-    progress.cleanupWhenAborted(() => this._crPage._page.safeNonStallingEvaluateInAllFrames('window.__cleanupDrag && window.__cleanupDrag()', 'utility'));
-
-    client.on('Input.dragIntercepted', onDragIntercepted!);
     try {
+      let expectingDrag = false;
+      await progress.race(this._crPage._page.safeNonStallingEvaluateInAllFrames(`(${setupDragListeners.toString()})()`, 'utility'));
+      client.on('Input.dragIntercepted', onDragIntercepted!);
       await client.send('Input.setInterceptDrags', { enabled: true });
-    } catch {
-      // If Input.setInterceptDrags is not supported, just do a regular move.
-      // This can be removed once we stop supporting old Electron.
-      client.off('Input.dragIntercepted', onDragIntercepted!);
-      return moveCallback();
+      try {
+        await progress.race(moveCallback());
+        expectingDrag = (await Promise.all(this._crPage._page.frames().map(async frame => {
+          return frame.nonStallingEvaluateInExistingContext('window.__cleanupDrag?.()', 'utility').catch(() => false);
+        }))).some(x => x);
+      } finally {
+        client.off('Input.dragIntercepted', onDragIntercepted!);
+        await client.send('Input.setInterceptDrags', { enabled: false });
+      }
+      this._dragState = expectingDrag ? (await dragInterceptedPromise).data : null;
+    } catch (error) {
+      // Cleanup without blocking, it will be done before the next playwright action.
+      this._crPage._page.safeNonStallingEvaluateInAllFrames('window.__cleanupDrag?.()', 'utility').catch(() => {});
+      throw error;
     }
-    await moveCallback();
-
-    const expectingDrag = (await Promise.all(this._crPage._page.frames().map(async frame => {
-      return frame.nonStallingEvaluateInExistingContext('window.__cleanupDrag && window.__cleanupDrag()', 'utility').catch(() => false);
-    }))).some(x => x);
-    this._dragState = expectingDrag ? (await dragInterceptedPromise).data : null;
-    client.off('Input.dragIntercepted', onDragIntercepted!);
-    await progress.race(client.send('Input.setInterceptDrags', { enabled: false }));
 
     if (this._dragState) {
       await progress.race(this._crPage._mainFrameSession._client.send('Input.dispatchDragEvent', {

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

@@ -19,7 +19,7 @@ import os from 'os';
 import path from 'path';
 import * as readline from 'readline';
 
-import { ManualPromise, removeFolders } from '../../utils';
+import { ManualPromise } from '../../utils';
 import { wrapInASCIIBox } from '../utils/ascii';
 import { RecentLogsCollector } from '../utils/debugLogger';
 import { eventsHelper } from '../utils/eventsHelper';
@@ -166,8 +166,6 @@ export class Electron extends SdkObject {
     }
 
     const artifactsDir = await progress.race(fs.promises.mkdtemp(ARTIFACTS_FOLDER));
-    progress.cleanupWhenAborted(() => removeFolders([artifactsDir]));
-
     const browserLogsCollector = new RecentLogsCollector();
     const env = options.env ? envArrayToObject(options.env) : process.env;
 
@@ -243,76 +241,76 @@ export class Electron extends SdkObject {
     const chromeMatchPromise = waitForLine(progress, launchedProcess, /^DevTools listening on (ws:\/\/.*)$/);
     const debuggerDisconnectPromise = waitForLine(progress, launchedProcess, /Waiting for the debugger to disconnect\.\.\./);
 
-    const nodeMatch = await nodeMatchPromise;
-    const nodeTransport = await WebSocketTransport.connect(progress, nodeMatch[1]);
-    progress.cleanupWhenAborted(() => nodeTransport.close());
-    const nodeConnection = new CRConnection(this, nodeTransport, helper.debugProtocolLogger(), browserLogsCollector);
+    try {
+      const nodeMatch = await nodeMatchPromise;
+      const nodeTransport = await WebSocketTransport.connect(progress, nodeMatch[1]);
+      const nodeConnection = new CRConnection(this, nodeTransport, helper.debugProtocolLogger(), browserLogsCollector);
+      // Immediately release exiting process under debug.
+      debuggerDisconnectPromise.then(() => {
+        nodeTransport.close();
+      }).catch(() => {});
 
-    // Immediately release exiting process under debug.
-    debuggerDisconnectPromise.then(() => {
-      nodeTransport.close();
-    }).catch(() => {});
-    const chromeMatch = await Promise.race([
-      chromeMatchPromise,
-      waitForXserverError,
-    ]) as RegExpMatchArray;
-    const chromeTransport = await WebSocketTransport.connect(progress, chromeMatch[1]);
-    progress.cleanupWhenAborted(() => chromeTransport.close());
-    const browserProcess: BrowserProcess = {
-      onclose: undefined,
-      process: launchedProcess,
-      close: gracefullyClose,
-      kill
-    };
-    const contextOptions: types.BrowserContextOptions = {
-      ...options,
-      noDefaultViewport: true,
-    };
-    const browserOptions: BrowserOptions = {
-      name: 'electron',
-      isChromium: true,
-      headful: true,
-      persistent: contextOptions,
-      browserProcess,
-      protocolLogger: helper.debugProtocolLogger(),
-      browserLogsCollector,
-      artifactsDir,
-      downloadsPath: artifactsDir,
-      tracesDir: options.tracesDir || artifactsDir,
-      originalLaunchOptions: {},
-    };
-    validateBrowserContextOptions(contextOptions, browserOptions);
-    const browser = await progress.race(CRBrowser.connect(this.attribution.playwright, chromeTransport, browserOptions));
-    app = new ElectronApplication(this, browser, nodeConnection, launchedProcess);
-    await progress.race(app.initialize());
-    return app;
+      const chromeMatch = await Promise.race([
+        chromeMatchPromise,
+        waitForXserverError,
+      ]);
+      const chromeTransport = await WebSocketTransport.connect(progress, chromeMatch[1]);
+      const browserProcess: BrowserProcess = {
+        onclose: undefined,
+        process: launchedProcess,
+        close: gracefullyClose,
+        kill
+      };
+      const contextOptions: types.BrowserContextOptions = {
+        ...options,
+        noDefaultViewport: true,
+      };
+      const browserOptions: BrowserOptions = {
+        name: 'electron',
+        isChromium: true,
+        headful: true,
+        persistent: contextOptions,
+        browserProcess,
+        protocolLogger: helper.debugProtocolLogger(),
+        browserLogsCollector,
+        artifactsDir,
+        downloadsPath: artifactsDir,
+        tracesDir: options.tracesDir || artifactsDir,
+        originalLaunchOptions: {},
+      };
+      validateBrowserContextOptions(contextOptions, browserOptions);
+      const browser = await progress.race(CRBrowser.connect(this.attribution.playwright, chromeTransport, browserOptions));
+      app = new ElectronApplication(this, browser, nodeConnection, launchedProcess);
+      await progress.race(app.initialize());
+      return app;
+    } catch (error) {
+      await kill();
+      throw error;
+    }
   }
 }
 
-function waitForLine(progress: Progress, process: childProcess.ChildProcess, regex: RegExp): Promise<RegExpMatchArray> {
-  return progress.race(new Promise((resolve, reject) => {
-    const rl = readline.createInterface({ input: process.stderr! });
-    const failError = new Error('Process failed to launch!');
-    const listeners = [
-      eventsHelper.addEventListener(rl, 'line', onLine),
-      eventsHelper.addEventListener(rl, 'close', reject.bind(null, failError)),
-      eventsHelper.addEventListener(process, 'exit', reject.bind(null, failError)),
-      // It is Ok to remove error handler because we did not create process and there is another listener.
-      eventsHelper.addEventListener(process, 'error', reject.bind(null, failError))
-    ];
+async function waitForLine(progress: Progress, process: childProcess.ChildProcess, regex: RegExp) {
+  const promise = new ManualPromise<RegExpMatchArray>();
+  const rl = readline.createInterface({ input: process.stderr! });
+  const failError = new Error('Process failed to launch!');
+  const listeners = [
+    eventsHelper.addEventListener(rl, 'line', onLine),
+    eventsHelper.addEventListener(rl, 'close', () => promise.reject(failError)),
+    eventsHelper.addEventListener(process, 'exit', () => promise.reject(failError)),
+    // It is Ok to remove error handler because we did not create process and there is another listener.
+    eventsHelper.addEventListener(process, 'error', () => promise.reject(failError)),
+  ];
 
-    progress.cleanupWhenAborted(cleanup);
-
-    function onLine(line: string) {
-      const match = line.match(regex);
-      if (!match)
-        return;
-      cleanup();
-      resolve(match);
-    }
+  function onLine(line: string) {
+    const match = line.match(regex);
+    if (match)
+      promise.resolve(match);
+  }
 
-    function cleanup() {
-      eventsHelper.removeEventListeners(listeners);
-    }
-  }));
+  try {
+    return await progress.race(promise);
+  } finally {
+    eventsHelper.removeEventListeners(listeners);
+  }
 }

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

@@ -298,6 +298,7 @@ export abstract class APIRequestContext extends SdkObject {
     };
     this.emit(APIRequestContext.Events.Request, requestEvent);
 
+    let destroyRequest: (() => void) | undefined;
     const resultPromise = new Promise<SendRequestResult>((fulfill, reject) => {
       const requestConstructor: ((url: URL, options: http.RequestOptions, callback?: (res: http.IncomingMessage) => void) => http.ClientRequest)
         = (url.protocol === 'https:' ? https : http).request;
@@ -483,7 +484,7 @@ export abstract class APIRequestContext extends SdkObject {
         body.on('end', notifyBodyFinished);
       });
       request.on('error', reject);
-      progress.cleanupWhenAborted(() => request.destroy());
+      destroyRequest = () => request.destroy();
 
       listeners.push(
           eventsHelper.addEventListener(this, APIRequestContext.Events.Dispose, () => {
@@ -539,7 +540,11 @@ export abstract class APIRequestContext extends SdkObject {
         request.write(postData);
       request.end();
     });
-    return progress.race(resultPromise);
+
+    return progress.race(resultPromise).catch(error => {
+      destroyRequest?.();
+      throw error;
+    });
   }
 
   private _getHttpCredentials(url: URL) {

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

@@ -57,22 +57,19 @@ class Helper {
 
   static waitForEvent(progress: Progress, emitter: EventEmitter, event: string | symbol, predicate?: Function): { promise: Promise<any>, dispose: () => void } {
     const listeners: RegisteredListener[] = [];
-    const promise = new Promise((resolve, reject) => {
+    const dispose = () => eventsHelper.removeEventListeners(listeners);
+    const promise = progress.race(new Promise((resolve, reject) => {
       listeners.push(eventsHelper.addEventListener(emitter, event, eventArg => {
         try {
           if (predicate && !predicate(eventArg))
             return;
-          eventsHelper.removeEventListeners(listeners);
           resolve(eventArg);
         } catch (e) {
-          eventsHelper.removeEventListeners(listeners);
           reject(e);
         }
       }));
-    });
-    const dispose = () => eventsHelper.removeEventListeners(listeners);
-    progress.cleanupWhenAborted(dispose);
-    return { promise: progress.race(promise), dispose };
+    })).finally(() => dispose());
+    return { promise, dispose };
   }
 
   static secondsToRoundishMillis(value: number): number {

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

@@ -179,16 +179,18 @@ export function harClose(harBackends: Map<string, HarBackend>, params: channels.
 export async function harUnzip(progress: Progress, params: channels.LocalUtilsHarUnzipParams): Promise<void> {
   const dir = path.dirname(params.zipFile);
   const zipFile = new ZipFile(params.zipFile);
-  progress.cleanupWhenAborted(() => zipFile.close());
-  for (const entry of await progress.race(zipFile.entries())) {
-    const buffer = await progress.race(zipFile.read(entry));
-    if (entry === 'har.har')
-      await progress.race(fs.promises.writeFile(params.harFile, buffer));
-    else
-      await progress.race(fs.promises.writeFile(path.join(dir, entry), buffer));
+  try {
+    for (const entry of await progress.race(zipFile.entries())) {
+      const buffer = await progress.race(zipFile.read(entry));
+      if (entry === 'har.har')
+        await progress.race(fs.promises.writeFile(params.harFile, buffer));
+      else
+        await progress.race(fs.promises.writeFile(path.join(dir, entry), buffer));
+    }
+    await progress.race(fs.promises.unlink(params.zipFile));
+  } finally {
+    zipFile.close();
   }
-  await progress.race(fs.promises.unlink(params.zipFile));
-  zipFile.close();
 }
 
 export async function tracingStarted(progress: Progress, stackSessions: Map<string, StackSession>, params: channels.LocalUtilsTracingStartedParams): Promise<channels.LocalUtilsTracingStartedResult> {