fix(selenium connect): register in gracefullyCloseAll for driver cleanup (#9218)

Otherwise, killing the driver does not cleanup sessions in the grid.

src/outofprocess.ts

@@ -20,8 +20,8 @@ import { Playwright } from './client/playwright';
 import * as childProcess from 'child_process';
 import * as path from 'path';
 
-export async function start() {
-  const client = new PlaywrightClient();
+export async function start(env: any = {}) {
+  const client = new PlaywrightClient(env);
   const playwright = await client._playwright;
   (playwright as any).stop = () => client.stop();
   (playwright as any).driverProcess = client._driverProcess;
@@ -34,7 +34,7 @@ class PlaywrightClient {
   private _closePromise: Promise<void>;
   private _onExit: (exitCode: number | null, signal: string | null) => {};
 
-  constructor() {
+  constructor(env: any) {
     this._onExit = (exitCode: number | null, signal: string | null) => {
       throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`);
     };
@@ -42,6 +42,10 @@ class PlaywrightClient {
     this._driverProcess = childProcess.fork(path.join(__dirname, 'cli', 'cli.js'), ['run-driver'], {
       stdio: 'pipe',
       detached: true,
+      env: {
+        ...process.env,
+        ...env
+      },
     });
     this._driverProcess.unref();
     this._driverProcess.on('exit', this._onExit);

src/server/chromium/chromium.ts

@@ -19,7 +19,7 @@ import fs from 'fs';
 import os from 'os';
 import path from 'path';
 import { CRBrowser } from './crBrowser';
-import { Env } from '../../utils/processLauncher';
+import { Env, gracefullyCloseSet } from '../../utils/processLauncher';
 import { kBrowserCloseMessageId } from './crConnection';
 import { rewriteErrorMessage } from '../../utils/stackTrace';
 import { BrowserType } from '../browserType';
@@ -169,7 +169,9 @@ export class Chromium extends BrowserType {
         method: 'DELETE',
       }).catch(error => progress.log(`<error disconnecting from selenium>: ${error}`));
       progress.log(`<disconnected from selenium> sessionId=${sessionId}`);
+      gracefullyCloseSet.delete(disconnectFromSelenium);
     };
+    gracefullyCloseSet.add(disconnectFromSelenium);
 
     try {
       const capabilities = value.capabilities;

src/utils/processLauncher.ts

@@ -48,7 +48,7 @@ type LaunchResult = {
   kill: () => Promise<void>,
 };
 
-const gracefullyCloseSet = new Set<() => Promise<void>>();
+export const gracefullyCloseSet = new Set<() => Promise<void>>();
 
 export async function gracefullyCloseAll() {
   await Promise.all(Array.from(gracefullyCloseSet).map(gracefullyClose => gracefullyClose().catch(e => {})));

tests/browsertype-launch-selenium.spec.ts

@@ -18,6 +18,7 @@ import { playwrightTest as test, expect } from './config/browserTest';
 import type { TestInfo } from '../types/test';
 import path from 'path';
 import fs from 'fs';
+import { start } from '../lib/outofprocess';
 
 const chromeDriver = require('chromedriver').path;
 const brokenDriver = path.join(__dirname, 'assets', 'selenium-grid', 'broken-selenium-driver.js');
@@ -34,6 +35,7 @@ function writeSeleniumConfig(testInfo: TestInfo, port: number) {
 
 test.skip(({ mode }) => mode !== 'default', 'Using test hooks');
 test.skip(() => !!process.env.INSIDE_DOCKER, 'Docker image does not have Java');
+test.slow();
 
 test('selenium grid 3.141.59 standalone chromium', async ({ browserOptions, browserName, childProcess, waitForPort, browserType }, testInfo) => {
   test.skip(browserName !== 'chromium');
@@ -105,3 +107,31 @@ test('selenium grid 3.141.59 standalone non-chromium', async ({ browserName, bro
   const error = await browserType.launch({ __testHookSeleniumRemoteURL } as any).catch(e => e);
   expect(error.message).toContain('Connecting to SELENIUM_REMOTE_URL is only supported by Chromium');
 });
+
+test('selenium grid 3.141.59 standalone chromium through driver', async ({ browserOptions, browserName, childProcess, waitForPort }, testInfo) => {
+  test.skip(browserName !== 'chromium');
+
+  const port = testInfo.workerIndex + 15123;
+  const grid = childProcess({
+    command: ['java', `-Dwebdriver.chrome.driver=${chromeDriver}`, '-jar', standalone_3_141_59, '-config', writeSeleniumConfig(testInfo, port)],
+    cwd: __dirname,
+  });
+  await waitForPort(port);
+
+  const pw = await start({
+    SELENIUM_REMOTE_URL: `http://localhost:${port}/wd/hub`,
+  });
+  const browser = await pw.chromium.launch(browserOptions);
+  const page = await browser.newPage();
+  await page.setContent('<title>Hello world</title><div>Get Started</div>');
+  await page.click('text=Get Started');
+  await expect(page).toHaveTitle('Hello world');
+  // Note: it is important to stop the driver without explicitly closing the browser.
+  // It should terminate selenium session in this case.
+  await pw.stop();
+
+  expect(grid.output).toContain('Starting ChromeDriver');
+  expect(grid.output).toContain('Started new session');
+  // It is important that selenium session is terminated.
+  await grid.waitForOutput('Removing session');
+});

tests/config/commonFixtures.ts

@@ -117,7 +117,7 @@ export const commonFixtures: Fixtures<CommonFixtures, {}> = {
       return process;
     });
     await Promise.all(processes.map(child => child.close()));
-    if (testInfo.status !== 'passed' && !process.env.PW_RUNNER_DEBUG) {
+    if (testInfo.status !== 'passed' && !process.env.PWTEST_DEBUG) {
       for (const process of processes) {
         console.log('====== ' + process.params.command.join(' '));
         console.log(process.output);