fix: wait for cleanup on double SIGINT (#13411)

This is a speculative fix to leftover tmp directories.
When users issues SIGINT twice, we enter `gracefullyClose()`
twice, and shortcut the second time. It turns out, we do
not wait for directories removal.

Note: it is unknown how often we reach this codepath in practice.

packages/playwright-core/src/browserServerImpl.ts

@@ -63,6 +63,7 @@ export class BrowserServerLauncherImpl implements BrowserServerLauncher {
     browserServer.close = () => browser.options.browserProcess.close();
     browserServer.kill = () => browser.options.browserProcess.kill();
     (browserServer as any)._disconnectForTest = () => server.close();
+    (browserServer as any)._artifactsDirForTest = browser.options.artifactsDir;
     browser.options.browserProcess.onclose = async (exitCode, signal) => {
       server.close();
       browserServer.emit('close', exitCode, signal);

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

@@ -110,8 +110,6 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
   });
 
   let processClosed = false;
-  let fulfillClose = () => {};
-  const waitForClose = new Promise<void>(f => fulfillClose = f);
   let fulfillCleanup = () => {};
   const waitForCleanup = new Promise<void>(f => fulfillCleanup = f);
   spawnedProcess.once('exit', (exitCode, signal) => {
@@ -120,7 +118,6 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
     eventsHelper.removeEventListeners(listeners);
     gracefullyCloseSet.delete(gracefullyClose);
     options.onExit(exitCode, signal);
-    fulfillClose();
     // Cleanup as process exits.
     cleanup().then(fulfillCleanup);
   });
@@ -153,7 +150,7 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
     if (gracefullyClosing) {
       options.log(`[pid=${spawnedProcess.pid}] <forecefully close>`);
       killProcess();
-      await waitForClose;  // Ensure the process is dead and we called options.onkill.
+      await waitForCleanup;  // Ensure the process is dead and we have cleaned up.
       return;
     }
     gracefullyClosing = true;

tests/config/remote-server-impl.js

@@ -21,6 +21,7 @@ async function start() {
     console.log(`(exitCode=>${exitCode})`);
     console.log(`(signal=>${signal})`);
   });
+  console.log(`(tempDir=>${browserServer._artifactsDirForTest})`);
   console.log(`(pid=>${browserServer.process().pid})`);
   console.log(`(wsEndpoint=>${browserServer.wsEndpoint()})`);
 }

tests/library/signals.spec.ts

@@ -17,6 +17,7 @@
 
 import { playwrightTest as test, expect } from '../config/browserTest';
 import { execSync } from 'child_process';
+import fs from 'fs';
 
 test.slow();
 
@@ -78,14 +79,19 @@ test.describe('signals', () => {
     expect(await remoteServer.childExitCode()).toBe(0);
   });
 
-  test('should kill the browser on double SIGINT', async ({ startRemoteServer, server }) => {
+  test('should kill the browser on double SIGINT and remove temp dir', async ({ startRemoteServer, server }) => {
     const remoteServer = await startRemoteServer({ stallOnClose: true, url: server.EMPTY_PAGE });
+    const tempDir = await remoteServer.out('tempDir');
+    const before = fs.existsSync(tempDir);
     process.kill(remoteServer.child().pid, 'SIGINT');
     await remoteServer.out('stalled');
     process.kill(remoteServer.child().pid, 'SIGINT');
     expect(await remoteServer.out('exitCode')).toBe('null');
     expect(await remoteServer.out('signal')).toBe('SIGKILL');
     expect(await remoteServer.childExitCode()).toBe(130);
+    const after = fs.existsSync(tempDir);
+    expect(before).toBe(true);
+    expect(after).toBe(false);
   });
 
   test('should kill the browser on SIGINT + SIGTERM', async ({ startRemoteServer, server }) => {