diff --git a/src/browser.ts b/src/browser.ts index 582ab6cc0b..986644afa7 100644 --- a/src/browser.ts +++ b/src/browser.ts @@ -24,6 +24,7 @@ export interface Browser extends EventEmitter { newPage(options?: BrowserContextOptions): Promise; isConnected(): boolean; close(): Promise; + _disconnect(): Promise; _setDebugFunction(debugFunction: (message: string) => void): void; } diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 6eb66ed766..3958fde9c4 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -30,6 +30,7 @@ import { Events } from './events'; import { Protocol } from './protocol'; import { CRExecutionContext } from './crExecutionContext'; import { EventEmitter } from 'events'; +import type { BrowserServer } from '../server/browserServer'; export class CRBrowser extends EventEmitter implements Browser { readonly _connection: CRConnection; @@ -46,6 +47,7 @@ export class CRBrowser extends EventEmitter implements Browser { private _tracingRecording = false; private _tracingPath: string | null = ''; private _tracingClient: CRSession | undefined; + _ownedServer: BrowserServer | null = null; static async connect(transport: ConnectionTransport, isPersistent: boolean, slowMo?: number): Promise { const connection = new CRConnection(SlowMoTransport.wrap(transport, slowMo)); @@ -183,13 +185,20 @@ export class CRBrowser extends EventEmitter implements Browser { await this._session.send('Target.closeTarget', { targetId: crPage._targetId }); } - async close() { + async _disconnect() { const disconnected = new Promise(f => this._connection.once(ConnectionEvents.Disconnected, f)); await Promise.all(this.contexts().map(context => context.close())); this._connection.close(); await disconnected; } + async close() { + if (this._ownedServer) + await this._ownedServer.close(); + else + await this._disconnect(); + } + async newBrowserCDPSession(): Promise { return await this._connection.createBrowserSession(); } diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index 17847207e7..22940a80e4 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -28,6 +28,7 @@ import { headersArray } from './ffNetworkManager'; import { FFPage } from './ffPage'; import { Protocol } from './protocol'; import { EventEmitter } from 'events'; +import type { BrowserServer } from '../server/browserServer'; export class FFBrowser extends EventEmitter implements Browser { _connection: FFConnection; @@ -37,6 +38,7 @@ export class FFBrowser extends EventEmitter implements Browser { private _eventListeners: RegisteredListener[]; readonly _firstPagePromise: Promise; private _firstPageCallback = () => {}; + _ownedServer: BrowserServer | null = null; static async connect(transport: ConnectionTransport, attachToDefaultContext: boolean, slowMo?: number): Promise { const connection = new FFConnection(SlowMoTransport.wrap(transport, slowMo)); @@ -140,7 +142,7 @@ export class FFBrowser extends EventEmitter implements Browser { }); } - async close() { + async _disconnect() { await Promise.all(this.contexts().map(context => context.close())); helper.removeEventListeners(this._eventListeners); const disconnected = new Promise(f => this.once(Events.Browser.Disconnected, f)); @@ -148,6 +150,13 @@ export class FFBrowser extends EventEmitter implements Browser { await disconnected; } + async close() { + if (this._ownedServer) + await this._ownedServer.close(); + else + await this._disconnect(); + } + _setDebugFunction(debugFunction: debug.IDebugger) { this._connection._debugProtocol = debugFunction; } diff --git a/src/server/chromium.ts b/src/server/chromium.ts index 8d763fa17d..a11c9e11e4 100644 --- a/src/server/chromium.ts +++ b/src/server/chromium.ts @@ -22,7 +22,7 @@ import * as util from 'util'; import { debugError, helper, assert } from '../helper'; import { CRBrowser } from '../chromium/crBrowser'; import * as ws from 'ws'; -import { launchProcess } from '../server/processLauncher'; +import { launchProcess } from './processLauncher'; import { kBrowserCloseMessageId } from '../chromium/crConnection'; import { PipeTransport } from './pipeTransport'; import { LaunchOptions, BrowserArgOptions, BrowserType, ConnectOptions, LaunchServerOptions } from './browserType'; @@ -49,7 +49,7 @@ export class Chromium implements BrowserType { assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead'); const { browserServer, transport } = await this._launchServer(options, 'local'); const browser = await CRBrowser.connect(transport!, false, options.slowMo); - (browser as any)['__server__'] = browserServer; + browser._ownedServer = browserServer; return browser; } @@ -62,8 +62,9 @@ export class Chromium implements BrowserType { timeout = 30000, slowMo = 0 } = options; - const { transport } = await this._launchServer(options, 'persistent', userDataDir); + const { transport, browserServer } = await this._launchServer(options, 'persistent', userDataDir); const browser = await CRBrowser.connect(transport!, true, slowMo); + browser._ownedServer = browserServer; await helper.waitWithTimeout(browser._firstPagePromise, 'first page', timeout); return browser._defaultContext; } @@ -110,8 +111,7 @@ export class Chromium implements BrowserType { pipe: true, tempDir: temporaryUserDataDir || undefined, attemptToGracefullyClose: async () => { - if (!browserServer) - return Promise.reject(); + assert(browserServer); // We try to gracefully close to prevent crash reporting and core dumps. // Note that it's fine to reuse the pipe transport, since // our connection ignores kBrowserCloseMessageId. @@ -127,7 +127,7 @@ export class Chromium implements BrowserType { let transport: PipeTransport | undefined = undefined; let browserServer: BrowserServer | undefined = undefined; - transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream, () => browserServer!.close()); + transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream); browserServer = new BrowserServer(launchedProcess, gracefullyClose, launchType === 'server' ? wrapTransportWithWebSocket(transport, port) : null); return { browserServer, transport }; } diff --git a/src/server/firefox.ts b/src/server/firefox.ts index 780a535c63..d1181ce0db 100644 --- a/src/server/firefox.ts +++ b/src/server/firefox.ts @@ -53,9 +53,7 @@ export class Firefox implements BrowserType { const browser = await WebSocketTransport.connect(browserServer.wsEndpoint()!, transport => { return FFBrowser.connect(transport, false, options.slowMo); }); - // Hack: for typical launch scenario, ensure that close waits for actual process termination. - browser.close = () => browserServer.close(); - (browser as any)['__server__'] = browserServer; + browser._ownedServer = browserServer; return browser; } @@ -72,10 +70,9 @@ export class Firefox implements BrowserType { const browser = await WebSocketTransport.connect(browserServer.wsEndpoint()!, transport => { return FFBrowser.connect(transport, true, slowMo); }); + browser._ownedServer = browserServer; await helper.waitWithTimeout(browser._firstPagePromise, 'first page', timeout); - // Hack: for typical launch scenario, ensure that close waits for actual process termination. const browserContext = browser._defaultContext; - browserContext.close = () => browserServer.close(); return browserContext; } @@ -128,11 +125,8 @@ export class Firefox implements BrowserType { pipe: false, tempDir: temporaryProfileDir || undefined, attemptToGracefullyClose: async () => { - if (!browserServer) - return Promise.reject(); + assert(browserServer); // We try to gracefully close to prevent crash reporting and core dumps. - // Note that it's fine to reuse the pipe transport, since - // our connection ignores kBrowserCloseMessageId. const transport = await WebSocketTransport.connect(browserWSEndpoint!, async transport => transport); const message = { method: 'Browser.close', params: {}, id: kBrowserCloseMessageId }; await transport.send(message); diff --git a/src/server/pipeTransport.ts b/src/server/pipeTransport.ts index 8b988f87df..b5b4016e40 100644 --- a/src/server/pipeTransport.ts +++ b/src/server/pipeTransport.ts @@ -23,14 +23,12 @@ export class PipeTransport implements ConnectionTransport { private _pendingMessage = ''; private _eventListeners: RegisteredListener[]; private _waitForNextTask = helper.makeWaitForNextTask(); - private readonly _closeCallback: () => void; onmessage?: (message: ProtocolResponse) => void; onclose?: () => void; - constructor(pipeWrite: NodeJS.WritableStream, pipeRead: NodeJS.ReadableStream, closeCallback: () => void) { + constructor(pipeWrite: NodeJS.WritableStream, pipeRead: NodeJS.ReadableStream) { this._pipeWrite = pipeWrite; - this._closeCallback = closeCallback; this._eventListeners = [ helper.addEventListener(pipeRead, 'data', buffer => this._dispatch(buffer)), helper.addEventListener(pipeRead, 'close', () => { @@ -51,7 +49,7 @@ export class PipeTransport implements ConnectionTransport { } close() { - this._closeCallback(); + throw new Error('unimplemented'); } _dispatch(buffer: Buffer) { diff --git a/src/server/processLauncher.ts b/src/server/processLauncher.ts index b86c8064b3..fd9d0335de 100644 --- a/src/server/processLauncher.ts +++ b/src/server/processLauncher.ts @@ -135,7 +135,7 @@ export async function launchProcess(options: LaunchProcessOptions): Promise`); - options.attemptToGracefullyClose().catch(() => killProcess()); + await options.attemptToGracefullyClose().catch(() => killProcess()); await waitForProcessToClose; debugBrowser(``); } diff --git a/src/server/webkit.ts b/src/server/webkit.ts index 7dfa6b5955..0107750a8d 100644 --- a/src/server/webkit.ts +++ b/src/server/webkit.ts @@ -48,8 +48,8 @@ export class WebKit implements BrowserType { async launch(options: LaunchOptions = {}): Promise { assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead'); const { browserServer, transport } = await this._launchServer(options, 'local'); - const browser = await WKBrowser.connect(transport!, options.slowMo); - (browser as any)['__server__'] = browserServer; + const browser = await WKBrowser.connect(transport!, options.slowMo, false, () => browserServer.close()); + browser._ownedServer = browserServer; return browser; } @@ -62,8 +62,8 @@ export class WebKit implements BrowserType { timeout = 30000, slowMo = 0, } = options; - const { transport } = await this._launchServer(options, 'persistent', userDataDir); - const browser = await WKBrowser.connect(transport!, slowMo, true); + const { transport, browserServer } = await this._launchServer(options, 'persistent', userDataDir); + const browser = await WKBrowser.connect(transport!, slowMo, true, () => browserServer.close()); await helper.waitWithTimeout(browser._waitForFirstPageTarget(), 'first page', timeout); return browser._defaultContext; } @@ -111,12 +111,11 @@ export class WebKit implements BrowserType { pipe: true, tempDir: temporaryUserDataDir || undefined, attemptToGracefullyClose: async () => { - if (!transport) - return Promise.reject(); + assert(transport); // We try to gracefully close to prevent crash reporting and core dumps. // Note that it's fine to reuse the pipe transport, since // our connection ignores kBrowserCloseMessageId. - transport.send({method: 'Playwright.close', params: {}, id: kBrowserCloseMessageId}); + await transport.send({method: 'Playwright.close', params: {}, id: kBrowserCloseMessageId}); }, onkill: (exitCode, signal) => { if (browserServer) @@ -127,7 +126,7 @@ export class WebKit implements BrowserType { // For local launch scenario close will terminate the browser process. let transport: ConnectionTransport | undefined = undefined; let browserServer: BrowserServer | undefined = undefined; - transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream, () => browserServer!.close()); + transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream); browserServer = new BrowserServer(launchedProcess, gracefullyClose, launchType === 'server' ? wrapTransportWithWebSocket(transport, port || 0) : null); return { browserServer, transport }; } diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 515d7a39d9..531662ec34 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -27,6 +27,7 @@ import { Protocol } from './protocol'; import { kPageProxyMessageReceived, PageProxyMessageReceivedPayload, WKConnection, WKSession } from './wkConnection'; import { WKPage } from './wkPage'; import { EventEmitter } from 'events'; +import type { BrowserServer } from '../server/browserServer'; const DEFAULT_USER_AGENT = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_2) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.4 Safari/605.1.15'; @@ -39,19 +40,22 @@ export class WKBrowser extends EventEmitter implements Browser { readonly _wkPages = new Map(); private readonly _eventListeners: RegisteredListener[]; private _popupOpeners: string[] = []; + private _closeOverride?: () => Promise; private _firstPageCallback: () => void = () => {}; private readonly _firstPagePromise: Promise; + _ownedServer: BrowserServer | null = null; - static async connect(transport: ConnectionTransport, slowMo: number = 0, attachToDefaultContext: boolean = false): Promise { - const browser = new WKBrowser(SlowMoTransport.wrap(transport, slowMo), attachToDefaultContext); + static async connect(transport: ConnectionTransport, slowMo: number = 0, attachToDefaultContext: boolean = false, closeOverride?: () => Promise): Promise { + const browser = new WKBrowser(SlowMoTransport.wrap(transport, slowMo), attachToDefaultContext, closeOverride); return browser; } - constructor(transport: ConnectionTransport, attachToDefaultContext: boolean) { + constructor(transport: ConnectionTransport, attachToDefaultContext: boolean, closeOverride?: () => Promise) { super(); this._connection = new WKConnection(transport, this._onDisconnect.bind(this)); this._attachToDefaultContext = attachToDefaultContext; + this._closeOverride = closeOverride; this._browserSession = this._connection.browserSession; this._defaultContext = new WKBrowserContext(this, undefined, validateBrowserContextOptions({})); @@ -178,7 +182,7 @@ export class WKBrowser extends EventEmitter implements Browser { return !this._connection.isClosed(); } - async close() { + async _disconnect() { helper.removeEventListeners(this._eventListeners); const disconnected = new Promise(f => this.once(Events.Browser.Disconnected, f)); await Promise.all(this.contexts().map(context => context.close())); @@ -186,6 +190,13 @@ export class WKBrowser extends EventEmitter implements Browser { await disconnected; } + async close() { + if (this._closeOverride) + await this._closeOverride(); + else + await this._disconnect(); + } + _setDebugFunction(debugFunction: debug.IDebugger) { this._connection._debugProtocol = debugFunction; } diff --git a/test/playwright.spec.js b/test/playwright.spec.js index dece59df9e..feff875f1d 100644 --- a/test/playwright.spec.js +++ b/test/playwright.spec.js @@ -160,7 +160,7 @@ module.exports.addPlaywrightTests = ({testRunner, platform, products, playwright describe('', function() { beforeAll(async state => { state.browser = await browserType.launch(defaultBrowserOptions); - state.browserServer = state.browser.__server__; + state.browserServer = state.browser._ownedServer; state._stdout = readline.createInterface({ input: state.browserServer.process().stdout }); state._stderr = readline.createInterface({ input: state.browserServer.process().stderr }); }); diff --git a/utils/doclint/check_public_api/JSBuilder.js b/utils/doclint/check_public_api/JSBuilder.js index 679f355d6b..4d0cff22d7 100644 --- a/utils/doclint/check_public_api/JSBuilder.js +++ b/utils/doclint/check_public_api/JSBuilder.js @@ -112,7 +112,7 @@ function checkSources(sources) { if (ts.isImportDeclaration(node) && ts.isStringLiteral(node.moduleSpecifier)) { const module = node.moduleSpecifier.text; const isServerDependency = path.resolve(path.dirname(fileName), module).includes('src/server'); - if (isServerDependency) { + if (isServerDependency && !node.importClause.isTypeOnly) { const lac = ts.getLineAndCharacterOfPosition(node.getSourceFile(), node.moduleSpecifier.pos); errors.push(`Disallowed import "${module}" at ${node.getSourceFile().fileName}:${lac.line + 1}`); }