From 155bb7fcae8f5e1c6fe6f86ed2d8e6e17bfadc70 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Thu, 7 Apr 2022 19:20:54 -0700
Subject: [PATCH] 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     | 1 +
 packages/playwright-core/src/utils/processLauncher.ts | 5 +----
 tests/config/remote-server-impl.js                    | 1 +
 tests/library/signals.spec.ts                         | 8 +++++++-
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/packages/playwright-core/src/browserServerImpl.ts b/packages/playwright-core/src/browserServerImpl.ts
index fae7812921..44d8fe267b 100644
--- a/packages/playwright-core/src/browserServerImpl.ts
+++ b/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);
diff --git a/packages/playwright-core/src/utils/processLauncher.ts b/packages/playwright-core/src/utils/processLauncher.ts
index 057ec81364..4e5ad3a99b 100644
--- a/packages/playwright-core/src/utils/processLauncher.ts
+++ b/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;
diff --git a/tests/config/remote-server-impl.js b/tests/config/remote-server-impl.js
index 65082e7e13..19eeeda9c4 100644
--- a/tests/config/remote-server-impl.js
+++ b/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()})`);
 }
diff --git a/tests/library/signals.spec.ts b/tests/library/signals.spec.ts
index 2c6a43d2f8..5c8fbc71e3 100644
--- a/tests/library/signals.spec.ts
+++ b/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 }) => {