From 047fc2698d9ac629a9c5e11008aa6026066dcb55 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Tue, 6 Oct 2020 15:54:53 -0700 Subject: [PATCH] Revert "feat(firefox): migrate to the pipe channel (#4068)" Mac sporadically hangs on browser close. --- browsers.json | 2 +- src/server/browserType.ts | 21 ++++++++++++++++----- src/server/chromium/chromium.ts | 25 ++++++++++++++----------- src/server/electron/electron.ts | 1 + src/server/firefox/firefox.ts | 5 +++-- src/server/processLauncher.ts | 3 ++- src/server/webkit/webkit.ts | 2 +- 7 files changed, 38 insertions(+), 21 deletions(-) diff --git a/browsers.json b/browsers.json index 829e82795bc5d..ec75f9302281c 100644 --- a/browsers.json +++ b/browsers.json @@ -8,7 +8,7 @@ }, { "name": "firefox", - "revision": "1184", + "revision": "1182", "download": true }, { diff --git a/src/server/browserType.ts b/src/server/browserType.ts index 2e60c297f16d7..d5caba695aeba 100644 --- a/src/server/browserType.ts +++ b/src/server/browserType.ts @@ -20,9 +20,9 @@ import * as path from 'path'; import * as util from 'util'; import { BrowserContext, normalizeProxySettings, validateBrowserContextOptions } from './browserContext'; import * as browserPaths from '../utils/browserPaths'; -import { ConnectionTransport } from './transport'; +import { ConnectionTransport, WebSocketTransport } from './transport'; import { BrowserOptions, Browser, BrowserProcess } from './browser'; -import { launchProcess, Env, envArrayToObject } from './processLauncher'; +import { launchProcess, Env, waitForLine, envArrayToObject } from './processLauncher'; import { PipeTransport } from './pipeTransport'; import { Progress, ProgressController } from './progress'; import * as types from './types'; @@ -35,18 +35,22 @@ const mkdtempAsync = util.promisify(fs.mkdtemp); const existsAsync = (path: string): Promise => new Promise(resolve => fs.stat(path, err => resolve(!err))); const DOWNLOADS_FOLDER = path.join(os.tmpdir(), 'playwright_downloads-'); +type WebSocketNotPipe = { webSocketRegex: RegExp, stream: 'stdout' | 'stderr' }; + export abstract class BrowserType { private _name: string; private _executablePath: string; + private _webSocketNotPipe: WebSocketNotPipe | null; private _browserDescriptor: browserPaths.BrowserDescriptor; readonly _browserPath: string; - constructor(packagePath: string, browser: browserPaths.BrowserDescriptor) { + constructor(packagePath: string, browser: browserPaths.BrowserDescriptor, webSocketOrPipe: WebSocketNotPipe | null) { this._name = browser.name; const browsersPath = browserPaths.browsersPath(packagePath); this._browserDescriptor = browser; this._browserPath = browserPaths.browserDirectory(browsersPath, browser); this._executablePath = browserPaths.executablePath(this._browserPath, browser) || ''; + this._webSocketNotPipe = webSocketOrPipe; } executablePath(): string { @@ -171,6 +175,7 @@ export abstract class BrowserType { handleSIGTERM, handleSIGHUP, progress, + pipe: !this._webSocketNotPipe, tempDirectories, attemptToGracefullyClose: async () => { if ((options as any).__testHookGracefullyClose) @@ -193,8 +198,14 @@ export abstract class BrowserType { }; progress.cleanupWhenAborted(() => browserProcess && closeOrKill(browserProcess, progress.timeUntilDeadline())); - const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream]; - transport = new PipeTransport(stdio[3], stdio[4]); + if (this._webSocketNotPipe) { + const match = await waitForLine(progress, launchedProcess, this._webSocketNotPipe.stream === 'stdout' ? launchedProcess.stdout : launchedProcess.stderr, this._webSocketNotPipe.webSocketRegex); + const innerEndpoint = match[1]; + transport = await WebSocketTransport.connect(progress, innerEndpoint); + } else { + const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream]; + transport = new PipeTransport(stdio[3], stdio[4]); + } return { browserProcess, downloadsPath, transport }; } diff --git a/src/server/chromium/chromium.ts b/src/server/chromium/chromium.ts index c580aee0d9fda..3772532cf4c21 100644 --- a/src/server/chromium/chromium.ts +++ b/src/server/chromium/chromium.ts @@ -31,9 +31,18 @@ import { isDebugMode, getFromENV } from '../../utils/utils'; export class Chromium extends BrowserType { private _devtools: CRDevTools | undefined; + private _debugPort: number | undefined; constructor(packagePath: string, browser: BrowserDescriptor) { - super(packagePath, browser); + const debugPortStr = getFromENV('PLAYWRIGHT_CHROMIUM_DEBUG_PORT'); + const debugPort: number | undefined = debugPortStr ? +debugPortStr : undefined; + if (debugPort !== undefined) { + if (Number.isNaN(debugPort)) + throw new Error(`PLAYWRIGHT_CHROMIUM_DEBUG_PORT must be a number, but is set to "${debugPortStr}"`); + } + + super(packagePath, browser, debugPort ? { webSocketRegex: /^DevTools listening on (ws:\/\/.*)$/, stream: 'stderr' } : null); + this._debugPort = debugPort; if (isDebugMode()) this._devtools = this._createDevTools(); } @@ -101,16 +110,10 @@ export class Chromium extends BrowserType { throw new Error('Arguments can not specify page to be opened'); const chromeArguments = [...DEFAULT_ARGS]; chromeArguments.push(`--user-data-dir=${userDataDir}`); - - const debugPortStr = getFromENV('PLAYWRIGHT_CHROMIUM_DEBUG_PORT'); - if (debugPortStr) { - const debugPort = +debugPortStr; - if (Number.isNaN(debugPort)) - throw new Error(`PLAYWRIGHT_CHROMIUM_DEBUG_PORT must be a number, but is set to "${debugPortStr}"`); - chromeArguments.push('--remote-debugging-port=' + debugPort); - } - - chromeArguments.push('--remote-debugging-pipe'); + if (this._debugPort !== undefined) + chromeArguments.push('--remote-debugging-port=' + this._debugPort); + else + chromeArguments.push('--remote-debugging-pipe'); if (options.devtools) chromeArguments.push('--auto-open-devtools-for-tabs'); if (options.headless) { diff --git a/src/server/electron/electron.ts b/src/server/electron/electron.ts index cce972af22ca9..9090371a58cd9 100644 --- a/src/server/electron/electron.ts +++ b/src/server/electron/electron.ts @@ -163,6 +163,7 @@ export class Electron { handleSIGTERM, handleSIGHUP, progress, + pipe: true, cwd: options.cwd, tempDirectories: [], attemptToGracefullyClose: () => app!.close(), diff --git a/src/server/firefox/firefox.ts b/src/server/firefox/firefox.ts index 571b7aa9d9e24..a5facc8bc75a5 100644 --- a/src/server/firefox/firefox.ts +++ b/src/server/firefox/firefox.ts @@ -29,7 +29,8 @@ import * as types from '../types'; export class Firefox extends BrowserType { constructor(packagePath: string, browser: BrowserDescriptor) { - super(packagePath, browser); + const webSocketRegex = /^Juggler listening on (ws:\/\/.*)$/; + super(packagePath, browser, { webSocketRegex, stream: 'stdout' }); } _connectToTransport(transport: ConnectionTransport, options: BrowserOptions): Promise { @@ -81,7 +82,7 @@ export class Firefox extends BrowserType { firefoxArguments.push('-foreground'); } firefoxArguments.push(`-profile`, userDataDir); - firefoxArguments.push('-juggler-pipe'); + firefoxArguments.push('-juggler', '0'); firefoxArguments.push(...args); if (isPersistent) firefoxArguments.push('about:blank'); diff --git a/src/server/processLauncher.ts b/src/server/processLauncher.ts index 47d994a5407fe..3a4d0df206b93 100644 --- a/src/server/processLauncher.ts +++ b/src/server/processLauncher.ts @@ -35,6 +35,7 @@ export type LaunchProcessOptions = { handleSIGINT?: boolean, handleSIGTERM?: boolean, handleSIGHUP?: boolean, + pipe?: boolean, pipeStdin?: boolean, tempDirectories: string[], @@ -82,7 +83,7 @@ export async function launchProcess(options: LaunchProcessOptions): Promise helper.removeFolders(options.tempDirectories); const progress = options.progress; - const stdio: ('ignore' | 'pipe')[] = ['ignore', 'pipe', 'pipe', 'pipe', 'pipe']; + const stdio: ('ignore' | 'pipe')[] = options.pipe ? ['ignore', 'pipe', 'pipe', 'pipe', 'pipe'] : ['ignore', 'pipe', 'pipe']; if (options.pipeStdin) stdio[0] = 'pipe'; progress.log(` ${options.executablePath} ${options.args.join(' ')}`); diff --git a/src/server/webkit/webkit.ts b/src/server/webkit/webkit.ts index fdc747865f06d..8a1bb4929fa55 100644 --- a/src/server/webkit/webkit.ts +++ b/src/server/webkit/webkit.ts @@ -27,7 +27,7 @@ import * as types from '../types'; export class WebKit extends BrowserType { constructor(packagePath: string, browser: BrowserDescriptor) { - super(packagePath, browser); + super(packagePath, browser, null /* use pipe not websocket */); } _connectToTransport(transport: ConnectionTransport, options: BrowserOptions): Promise {