fix: kill browser if process doesnt exit for 30s after close (#5968)

src/server/browserType.ts

@@ -26,7 +26,7 @@ import { launchProcess, Env, envArrayToObject } from './processLauncher';
 import { PipeTransport } from './pipeTransport';
 import { Progress, ProgressController } from './progress';
 import * as types from './types';
-import { TimeoutSettings } from '../utils/timeoutSettings';
+import { DEFAULT_TIMEOUT, TimeoutSettings } from '../utils/timeoutSettings';
 import { validateHostRequirements } from './validateDependencies';
 import { isDebugMode } from '../utils/utils';
 import { helper } from './helper';
@@ -219,13 +219,26 @@ export abstract class BrowserType extends SdkObject {
           browserProcess.onclose(exitCode, signal);
       },
     });
+    async function closeOrKill(timeout: number): Promise<void> {
+      let timer: NodeJS.Timer;
+      try {
+        await Promise.race([
+          gracefullyClose(),
+          new Promise((resolve, reject) => timer = setTimeout(reject, timeout)),
+        ]);
+      } catch (ignored) {
+        await kill().catch(ignored => {}); // Make sure to await actual process exit.
+      } finally {
+        clearTimeout(timer!);
+      }
+    }
     browserProcess = {
       onclose: undefined,
       process: launchedProcess,
-      close: gracefullyClose,
+      close: () => closeOrKill((options as any).__testHookBrowserCloseTimeout || DEFAULT_TIMEOUT),
       kill
     };
-    progress.cleanupWhenAborted(() => browserProcess && closeOrKill(browserProcess, progress.timeUntilDeadline()));
+    progress.cleanupWhenAborted(() => closeOrKill(progress.timeUntilDeadline()));
     if (options.useWebSocket) {
       transport = await WebSocketTransport.connect(progress, await wsEndpoint!);
     } else {
@@ -260,17 +273,3 @@ function validateLaunchOptions<Options extends types.LaunchOptions>(options: Opt
     headless = false;
   return { ...options, devtools, headless };
 }
-
-async function closeOrKill(browserProcess: BrowserProcess, timeout: number): Promise<void> {
-  let timer: NodeJS.Timer;
-  try {
-    await Promise.race([
-      browserProcess.close(),
-      new Promise((resolve, reject) => timer = setTimeout(reject, timeout)),
-    ]);
-  } catch (ignored) {
-    await browserProcess.kill().catch(ignored => {}); // Make sure to await actual process exit.
-  } finally {
-    clearTimeout(timer!);
-  }
-}

test/launcher.spec.ts

@@ -27,3 +27,18 @@ it('should require top-level DeviceDescriptors', async ({playwright}) => {
   expect(Devices['iPhone 6']).toBeTruthy();
   expect(Devices['iPhone 6']).toEqual(playwright.devices['iPhone 6']);
 });
+
+it('should kill browser process on timeout after close', (test, { mode }) => {
+  test.skip(mode !== 'default', 'Test passes server hooks via options');
+}, async ({browserType, browserOptions}) => {
+  const launchOptions = { ...browserOptions };
+  let stalled = false;
+  (launchOptions as any).__testHookGracefullyClose = () => {
+    stalled = true;
+    return new Promise(() => {});
+  };
+  (launchOptions as any).__testHookBrowserCloseTimeout = 1_000;
+  const browser = await browserType.launch(launchOptions);
+  await browser.close();
+  expect(stalled).toBeTruthy();
+});