chore: encapsulate more launching logic in BrowserType (#2339)

Author: dgozman

src/server/browserServer.ts

@@ -17,13 +17,11 @@
 import { ChildProcess } from 'child_process';
 import { EventEmitter } from 'events';
 import { helper } from '../helper';
-import { RootLogger } from '../logger';
-import { LaunchOptions, processBrowserArgOptions } from './browserType';
-import { ConnectionTransport } from '../transport';
 
 export class WebSocketWrapper {
   readonly wsEndpoint: string;
   private _bindings: (Map<any, any> | Set<any>)[];
+
   constructor(wsEndpoint: string, bindings: (Map<any, any>|Set<any>)[]) {
     this.wsEndpoint = wsEndpoint;
     this._bindings = bindings;
@@ -53,24 +51,12 @@ export class WebSocketWrapper {
 export class BrowserServer extends EventEmitter {
   private _process: ChildProcess;
   private _gracefullyClose: (() => Promise<void>);
-  private _webSocketWrapper: WebSocketWrapper | null = null;
-  readonly _launchOptions: LaunchOptions;
-  readonly _logger: RootLogger;
-  readonly _downloadsPath: string | undefined;
-  readonly _transport: ConnectionTransport;
-  readonly _headful: boolean;
+  _webSocketWrapper: WebSocketWrapper | null = null;
 
-  constructor(options: LaunchOptions, process: ChildProcess, gracefullyClose: () => Promise<void>, transport: ConnectionTransport, downloadsPath: string | undefined, webSocketWrapper: WebSocketWrapper | null) {
+  constructor(process: ChildProcess, gracefullyClose: () => Promise<void>) {
     super();
-    this._launchOptions = options;
-    this._headful = !processBrowserArgOptions(options).headless;
-    this._logger = new RootLogger(options.logger);
-
     this._process = process;
     this._gracefullyClose = gracefullyClose;
-    this._transport = transport;
-    this._downloadsPath = downloadsPath;
-    this._webSocketWrapper = webSocketWrapper;
   }
 
   process(): ChildProcess {

src/server/browserType.ts

@@ -59,7 +59,6 @@ type ConnectOptions = {
   logger?: Logger,
   timeout?: number,
 };
-export type LaunchType = 'local' | 'server' | 'persistent';
 export type LaunchOptions = LaunchOptionsBase & { slowMo?: number };
 type LaunchServerOptions = LaunchOptionsBase & { port?: number };
 
@@ -73,6 +72,7 @@ export interface BrowserType {
 }
 
 const mkdtempAsync = util.promisify(fs.mkdtemp);
+const DOWNLOADS_FOLDER = path.join(os.tmpdir(), 'playwright_downloads-');
 
 export abstract class BrowserTypeBase implements BrowserType {
   private _name: string;
@@ -100,24 +100,37 @@ export abstract class BrowserTypeBase implements BrowserType {
 
   async launch(options: LaunchOptions = {}): Promise<Browser> {
     assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead');
-    return this._innerLaunch('local', options, undefined);
+    assert(!(options as any).port, 'Cannot specify a port without launching as a server.');
+    return this._innerLaunch(options, undefined);
   }
 
   async launchPersistentContext(userDataDir: string, options: LaunchOptions & PersistentContextOptions = {}): Promise<BrowserContext> {
+    assert(!(options as any).port, 'Cannot specify a port without launching as a server.');
     const persistent = validatePersistentContextOptions(options);
-    const browser = await this._innerLaunch('persistent', options, persistent, userDataDir);
+    const browser = await this._innerLaunch(options, persistent, userDataDir);
     return browser._defaultContext!;
   }
 
-  async _innerLaunch(launchType: LaunchType, options: LaunchOptions, persistent: PersistentContextOptions | undefined, userDataDir?: string): Promise<BrowserBase> {
+  async _innerLaunch(options: LaunchOptions, persistent: PersistentContextOptions | undefined, userDataDir?: string): Promise<BrowserBase> {
     const deadline = TimeoutSettings.computeDeadline(options.timeout, 30000);
     const logger = new RootLogger(options.logger);
     logger.startLaunchRecording();
 
     let browserServer: BrowserServer | undefined;
     try {
-      browserServer = await this._launchServer(options, launchType, logger, deadline, userDataDir);
-      const promise = this._innerLaunchPromise(browserServer, options, persistent);
+      const launched = await this._launchServer(options, !!persistent, logger, deadline, userDataDir);
+      browserServer = launched.browserServer;
+      const browserOptions: BrowserOptions = {
+        slowMo: options.slowMo,
+        persistent,
+        headful: !processBrowserArgOptions(options).headless,
+        logger,
+        downloadsPath: launched.downloadsPath,
+        ownedServer: browserServer,
+      };
+      copyTestHooks(options, browserOptions);
+      const hasCustomArguments = !!options.ignoreDefaultArgs && !Array.isArray(options.ignoreDefaultArgs);
+      const promise = this._innerCreateBrowser(launched.transport, browserOptions, hasCustomArguments);
       const browser = await helper.waitWithDeadline(promise, 'the browser to launch', deadline, 'pw:browser*');
       return browser;
     } catch (e) {
@@ -132,34 +145,23 @@ export abstract class BrowserTypeBase implements BrowserType {
     }
   }
 
-  async _innerLaunchPromise(browserServer: BrowserServer, options: LaunchOptions, persistent: PersistentContextOptions | undefined): Promise<BrowserBase> {
-    if ((options as any).__testHookBeforeCreateBrowser)
-      await (options as any).__testHookBeforeCreateBrowser();
-
-    const browserOptions: BrowserOptions = {
-      slowMo: options.slowMo,
-      persistent,
-      headful: browserServer._headful,
-      logger: browserServer._logger,
-      downloadsPath: browserServer._downloadsPath,
-      ownedServer: browserServer,
-    };
-    for (const [key, value] of Object.entries(options)) {
-      if (key.startsWith('__testHook'))
-        (browserOptions as any)[key] = value;
-    }
-
-    const browser = await this._connectToTransport(browserServer._transport, browserOptions);
-    if (persistent && (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs))) {
-      const context = browser._defaultContext!;
-      await context._loadDefaultContext();
-    }
+  async _innerCreateBrowser(transport: ConnectionTransport, browserOptions: BrowserOptions, hasCustomArguments: boolean): Promise<BrowserBase> {
+    if ((browserOptions as any).__testHookBeforeCreateBrowser)
+      await (browserOptions as any).__testHookBeforeCreateBrowser();
+    const browser = await this._connectToTransport(transport, browserOptions);
+    // We assume no control when using custom arguments, and do not prepare the default context in that case.
+    if (browserOptions.persistent && !hasCustomArguments)
+      await browser._defaultContext!._loadDefaultContext();
     return browser;
   }
 
   async launchServer(options: LaunchServerOptions = {}): Promise<BrowserServer> {
+    assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launchServer`. Use `browserType.launchPersistentContext` instead');
+    const { port = 0 } = options;
     const logger = new RootLogger(options.logger);
-    return this._launchServer(options, 'server', logger, TimeoutSettings.computeDeadline(options.timeout, 30000));
+    const { browserServer, transport } = await this._launchServer(options, false, logger, TimeoutSettings.computeDeadline(options.timeout, 30000));
+    browserServer._webSocketWrapper = this._wrapTransportWithWebSocket(transport, logger, port);
+    return browserServer;
   }
 
   async connect(options: ConnectOptions): Promise<Browser> {
@@ -170,7 +172,12 @@ export abstract class BrowserTypeBase implements BrowserType {
     let transport: ConnectionTransport | undefined;
     try {
       transport = await WebSocketTransport.connect(options.wsEndpoint, logger, deadline);
-      const promise = this._innerConnectPromise(transport, options, logger);
+      const browserOptions: BrowserOptions = {
+        slowMo: options.slowMo,
+        logger,
+      };
+      copyTestHooks(options, browserOptions);
+      const promise = this._innerCreateBrowser(transport, browserOptions, false);
       const browser = await helper.waitWithDeadline(promise, 'connect to browser', deadline, 'pw:browser*');
       logger.stopLaunchRecording();
       return browser;
@@ -189,13 +196,7 @@ export abstract class BrowserTypeBase implements BrowserType {
     }
   }
 
-  async _innerConnectPromise(transport: ConnectionTransport, options: ConnectOptions, logger: RootLogger): Promise<Browser> {
-    if ((options as any).__testHookBeforeCreateBrowser)
-      await (options as any).__testHookBeforeCreateBrowser();
-    return this._connectToTransport(transport, { slowMo: options.slowMo, logger });
-  }
-
-  private async _launchServer(options: LaunchServerOptions, launchType: LaunchType, logger: RootLogger, deadline: number, userDataDir?: string): Promise<BrowserServer> {
+  private async _launchServer(options: LaunchServerOptions, isPersistent: boolean, logger: RootLogger, deadline: number, userDataDir?: string): Promise<{ browserServer: BrowserServer, downloadsPath: string, transport: ConnectionTransport }> {
     const {
       ignoreDefaultArgs = false,
       args = [],
@@ -204,21 +205,20 @@ export abstract class BrowserTypeBase implements BrowserType {
       handleSIGINT = true,
       handleSIGTERM = true,
       handleSIGHUP = true,
-      port = 0,
     } = options;
-    assert(!port || launchType === 'server', 'Cannot specify a port without launching as a server.');
 
-    let temporaryUserDataDir: string | null = null;
+    const downloadsPath = await mkdtempAsync(DOWNLOADS_FOLDER);
+    const tempDirectories = [downloadsPath];
     if (!userDataDir) {
       userDataDir = await mkdtempAsync(path.join(os.tmpdir(), `playwright_${this._name}dev_profile-`));
-      temporaryUserDataDir = userDataDir;
+      tempDirectories.push(userDataDir);
     }
 
     const browserArguments = [];
     if (!ignoreDefaultArgs)
-      browserArguments.push(...this._defaultArgs(options, launchType, userDataDir));
+      browserArguments.push(...this._defaultArgs(options, isPersistent, userDataDir));
     else if (Array.isArray(ignoreDefaultArgs))
-      browserArguments.push(...this._defaultArgs(options, launchType, userDataDir).filter(arg => ignoreDefaultArgs.indexOf(arg) === -1));
+      browserArguments.push(...this._defaultArgs(options, isPersistent, userDataDir).filter(arg => ignoreDefaultArgs.indexOf(arg) === -1));
     else
       browserArguments.push(...args);
 
@@ -230,7 +230,7 @@ export abstract class BrowserTypeBase implements BrowserType {
     // "Cannot access 'browserServer' before initialization" if something went wrong.
     let transport: ConnectionTransport | undefined = undefined;
     let browserServer: BrowserServer | undefined = undefined;
-    const { launchedProcess, gracefullyClose, downloadsPath } = await launchProcess({
+    const { launchedProcess, gracefullyClose } = await launchProcess({
       executablePath: executable,
       args: browserArguments,
       env: this._amendEnvironment(env, userDataDir, executable, browserArguments),
@@ -239,7 +239,7 @@ export abstract class BrowserTypeBase implements BrowserType {
       handleSIGHUP,
       logger,
       pipe: !this._webSocketRegexNotPipe,
-      tempDir: temporaryUserDataDir || undefined,
+      tempDirectories,
       attemptToGracefullyClose: async () => {
         if ((options as any).__testHookGracefullyClose)
           await (options as any).__testHookGracefullyClose();
@@ -269,14 +269,20 @@ export abstract class BrowserTypeBase implements BrowserType {
       helper.killProcess(launchedProcess);
       throw e;
     }
-    browserServer = new BrowserServer(options, launchedProcess, gracefullyClose, transport, downloadsPath,
-        launchType === 'server' ? this._wrapTransportWithWebSocket(transport, logger, port) : null);
-    return browserServer;
+    browserServer = new BrowserServer(launchedProcess, gracefullyClose);
+    return { browserServer, downloadsPath, transport };
   }
 
-  abstract _defaultArgs(options: BrowserArgOptions, launchType: LaunchType, userDataDir: string): string[];
+  abstract _defaultArgs(options: BrowserArgOptions, isPersistent: boolean, userDataDir: string): string[];
   abstract _connectToTransport(transport: ConnectionTransport, options: BrowserOptions): Promise<BrowserBase>;
   abstract _wrapTransportWithWebSocket(transport: ConnectionTransport, logger: InnerLogger, port: number): WebSocketWrapper;
   abstract _amendEnvironment(env: Env, userDataDir: string, executable: string, browserArguments: string[]): Env;
   abstract _attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void;
 }
+
+function copyTestHooks(from: object, to: object) {
+  for (const [key, value] of Object.entries(from)) {
+    if (key.startsWith('__testHook'))
+      (to as any)[key] = value;
+  }
+}

src/server/chromium.ts

@@ -21,7 +21,7 @@ import { CRBrowser } from '../chromium/crBrowser';
 import * as ws from 'ws';
 import { Env } from './processLauncher';
 import { kBrowserCloseMessageId } from '../chromium/crConnection';
-import { BrowserArgOptions, BrowserTypeBase, processBrowserArgOptions, LaunchType } from './browserType';
+import { BrowserArgOptions, BrowserTypeBase, processBrowserArgOptions } from './browserType';
 import { WebSocketWrapper } from './browserServer';
 import { ConnectionTransport, ProtocolRequest } from '../transport';
 import { InnerLogger, logError } from '../logger';
@@ -66,7 +66,7 @@ export class Chromium extends BrowserTypeBase {
     return wrapTransportWithWebSocket(transport, logger, port);
   }
 
-  _defaultArgs(options: BrowserArgOptions, launchType: LaunchType, userDataDir: string): string[] {
+  _defaultArgs(options: BrowserArgOptions, isPersistent: boolean, userDataDir: string): string[] {
     const { devtools, headless } = processBrowserArgOptions(options);
     const { args = [] } = options;
     const userDataDirArg = args.find(arg => arg.startsWith('--user-data-dir'));
@@ -89,7 +89,7 @@ export class Chromium extends BrowserTypeBase {
       );
     }
     chromeArguments.push(...args);
-    if (launchType === 'persistent')
+    if (isPersistent)
       chromeArguments.push('about:blank');
     else
       chromeArguments.push('--no-startup-window');

src/server/electron.ts

@@ -183,7 +183,7 @@ export class Electron  {
       logger,
       pipe: true,
       cwd: options.cwd,
-      omitDownloads: true,
+      tempDirectories: [],
       attemptToGracefullyClose: () => app!.close(),
       onkill: (exitCode, signal) => {
         if (app)
@@ -198,7 +198,7 @@ export class Electron  {
 
     const chromeMatch = await waitForLine(launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/, helper.timeUntilDeadline(deadline), timeoutError);
     const chromeTransport = await WebSocketTransport.connect(chromeMatch[1], logger, deadline);
-    const browserServer = new BrowserServer(options, launchedProcess, gracefullyClose, chromeTransport, undefined, null);
+    const browserServer = new BrowserServer(launchedProcess, gracefullyClose);
     const browser = await CRBrowser.connect(chromeTransport, { headful: true, logger, persistent: { viewport: null }, ownedServer: browserServer });
     app = new ElectronApplication(logger, browser, nodeConnection);
     await app._init();

src/server/firefox.ts

@@ -22,7 +22,7 @@ import { FFBrowser } from '../firefox/ffBrowser';
 import { kBrowserCloseMessageId } from '../firefox/ffConnection';
 import { helper } from '../helper';
 import { WebSocketWrapper } from './browserServer';
-import { BrowserArgOptions, BrowserTypeBase, processBrowserArgOptions, LaunchType } from './browserType';
+import { BrowserArgOptions, BrowserTypeBase, processBrowserArgOptions } from './browserType';
 import { Env } from './processLauncher';
 import { ConnectionTransport, SequenceNumberMixer } from '../transport';
 import { InnerLogger, logError } from '../logger';
@@ -56,7 +56,7 @@ export class Firefox extends BrowserTypeBase {
     return wrapTransportWithWebSocket(transport, logger, port);
   }
 
-  _defaultArgs(options: BrowserArgOptions, launchType: LaunchType, userDataDir: string): string[] {
+  _defaultArgs(options: BrowserArgOptions, isPersistent: boolean, userDataDir: string): string[] {
     const { devtools, headless } = processBrowserArgOptions(options);
     const { args = [] } = options;
     if (devtools)
@@ -77,7 +77,7 @@ export class Firefox extends BrowserTypeBase {
     firefoxArguments.push(`-profile`, userDataDir);
     firefoxArguments.push('-juggler', '0');
     firefoxArguments.push(...args);
-    if (launchType === 'persistent')
+    if (isPersistent)
       firefoxArguments.push('about:blank');
     else
       firefoxArguments.push('-silent');