chore: simplify client-side browserType.connect routine (#8596)

src/client/browserType.ts

@@ -23,10 +23,10 @@ import { Connection } from './connection';
 import { Events } from './events';
 import { ChildProcess } from 'child_process';
 import { envObjectToArray } from './clientHelper';
-import { assert, headersObjectToArray, getUserAgent } from '../utils/utils';
+import { assert, headersObjectToArray, getUserAgent, monotonicTime } from '../utils/utils';
 import * as api from '../../types/types';
 import { kBrowserClosedError } from '../utils/errors';
-import { ManualPromise } from '../utils/async';
+import { raceAgainstDeadline } from '../utils/async';
 
 export interface BrowserServerLauncher {
   launchServer(options?: LaunchServerOptions): Promise<api.BrowserServer>;
@@ -128,12 +128,25 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
   async _connect(wsEndpoint: string, params: Partial<ConnectOptions> = {}): Promise<Browser> {
     const logger = params.logger;
     return await this._wrapApiCall(async (channel: channels.BrowserTypeChannel) => {
-      const timeoutPromise = new ManualPromise<Browser>();
-      const timer = params.timeout ? setTimeout(() => timeoutPromise.reject(new Error(`Timeout ${params.timeout}ms exceeded.`)), params.timeout) : undefined;
-
+      const deadline = params.timeout ? monotonicTime() + params.timeout : 0;
+      let browser: Browser;
       const { pipe } = await channel.connect({ wsEndpoint, headers: params.headers, timeout: params.timeout });
-      const connection = new Connection(() => pipe.close().catch(() => {}));
-      connection.onmessage = message => pipe.send({ message }).catch(() => { });
+      const closePipe = () => pipe.close().catch(() => {});
+      const connection = new Connection(closePipe);
+
+      const onPipeClosed = () => {
+        // Emulate all pages, contexts and the browser closing upon disconnect.
+        for (const context of browser?.contexts() || []) {
+          for (const page of context.pages())
+            page._onClose();
+          context._onClose();
+        }
+        browser?._didClose();
+        connection.didDisconnect(kBrowserClosedError);
+      };
+      pipe.on('closed', onPipeClosed);
+      connection.onmessage = message => pipe.send({ message }).catch(onPipeClosed);
+
       pipe.on('message', ({ message }) => {
         try {
           if (!connection!.isDisconnected())
@@ -141,64 +154,42 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
         } catch (e) {
           console.error(`Playwright: Connection dispatch error`);
           console.error(e);
-          pipe.close().catch(() => {});
+          closePipe();
         }
       });
 
-      const successPromise = new Promise<Browser>(async (fulfill, reject) => {
-        if ((params as any).__testHookBeforeCreateBrowser) {
-          try {
+      const createBrowserPromise = new Promise<Browser>(async (fulfill, reject) => {
+        try {
+          // For tests.
+          if ((params as any).__testHookBeforeCreateBrowser)
             await (params as any).__testHookBeforeCreateBrowser();
-          } catch (e) {
-            reject(e);
-          }
-        }
-        const prematureCloseListener = (params: { error?: channels.SerializedError }) => {
-          reject(new Error(`WebSocket server disconnected ${params.error!.error?.message}`));
-        };
-        pipe.on('closed', prematureCloseListener);
 
-        pipe.on('opened', async () => {
           const playwright = await connection!.initializePlaywright();
-
           if (!playwright._initializer.preLaunchedBrowser) {
             reject(new Error('Malformed endpoint. Did you use launchServer method?'));
-            pipe.close().catch(() => {});
+            closePipe();
             return;
           }
-
-          const browser = Browser.from(playwright._initializer.preLaunchedBrowser!);
+          browser = Browser.from(playwright._initializer.preLaunchedBrowser!);
           browser._logger = logger;
           browser._remoteType = 'owns-connection';
           browser._setBrowserType((playwright as any)[browser._name]);
-          const closeListener = (param: { error?: Error }) => {
-            // Emulate all pages, contexts and the browser closing upon disconnect.
-            for (const context of browser.contexts()) {
-              for (const page of context.pages())
-                page._onClose();
-              context._onClose();
-            }
-            browser._didClose();
-            connection.didDisconnect(kBrowserClosedError);
-            if (param.error)
-              reject(new Error(param.error + '. Most likely ws endpoint is incorrect'));
-          };
-          pipe.off('closed', prematureCloseListener);
-          pipe.on('closed', closeListener);
           browser.on(Events.Browser.Disconnected, () => {
             playwright._cleanup();
-            pipe.off('closed', closeListener);
-            pipe.close().catch(() => {});
+            closePipe();
           });
           fulfill(browser);
-        });
+        } catch (e) {
+          reject(e);
+        }
       });
 
-      try {
-        return await Promise.race([successPromise, timeoutPromise]);
-      } finally {
-        if (timer)
-          clearTimeout(timer);
+      const result = await raceAgainstDeadline(createBrowserPromise, deadline);
+      if (result.result) {
+        return result.result;
+      } else {
+        closePipe();
+        throw new Error(`Timeout ${params.timeout}ms exceeded`);
       }
     }, logger);
   }

src/dispatchers/browserTypeDispatcher.ts

@@ -23,6 +23,7 @@ import { CallMetadata } from '../server/instrumentation';
 import WebSocket from 'ws';
 import { JsonPipeDispatcher } from '../dispatchers/jsonPipeDispatcher';
 import { getUserAgent, makeWaitForNextTask } from '../utils/utils';
+import { ManualPromise } from '../utils/async';
 
 export class BrowserTypeDispatcher extends Dispatcher<BrowserType, channels.BrowserTypeInitializer, channels.BrowserTypeEvents> implements channels.BrowserTypeChannel {
   constructor(scope: DispatcherScope, browserType: BrowserType) {
@@ -59,13 +60,21 @@ export class BrowserTypeDispatcher extends Dispatcher<BrowserType, channels.Brow
     const ws = new WebSocket(params.wsEndpoint, [], {
       perMessageDeflate: false,
       maxPayload: 256 * 1024 * 1024, // 256Mb,
-      handshakeTimeout: params.timeout || 30000,
+      handshakeTimeout: params.timeout,
       headers: paramsHeaders,
     });
     const pipe = new JsonPipeDispatcher(this._scope);
-    ws.on('open', () => pipe.wasOpened());
+    const openPromise = new ManualPromise<{ pipe: JsonPipeDispatcher }>();
+    ws.on('open', () => openPromise.resolve({ pipe }));
     ws.on('close', () => pipe.wasClosed());
-    ws.on('error', error => pipe.wasClosed(error));
+    ws.on('error', error => {
+      if (openPromise.isDone()) {
+        pipe.wasClosed(error);
+      } else {
+        pipe.dispose();
+        openPromise.reject(error);
+      }
+    });
     pipe.on('close', () => ws.close());
     pipe.on('message', message => ws.send(JSON.stringify(message)));
     ws.addEventListener('message', event => {
@@ -77,6 +86,6 @@ export class BrowserTypeDispatcher extends Dispatcher<BrowserType, channels.Brow
         }
       });
     });
-    return { pipe };
+    return openPromise;
   }
 }

src/dispatchers/jsonPipeDispatcher.ts

@@ -36,11 +36,6 @@ export class JsonPipeDispatcher extends Dispatcher<{ guid: string }, channels.Js
     }
   }
 
-  wasOpened(): void {
-    if (!this._disposed)
-      this._dispatchEvent('opened');
-  }
-
   dispatch(message: Object) {
     if (!this._disposed)
       this._dispatchEvent('message', { message });
@@ -53,4 +48,8 @@ export class JsonPipeDispatcher extends Dispatcher<{ guid: string }, channels.Js
       this._dispose();
     }
   }
+
+  dispose() {
+    this._dispose();
+  }
 }

src/protocol/channels.ts

@@ -3524,13 +3524,11 @@ export type AndroidElementInfo = {
 // ----------- JsonPipe -----------
 export type JsonPipeInitializer = {};
 export interface JsonPipeChannel extends Channel {
-  on(event: 'opened', callback: (params: JsonPipeOpenedEvent) => void): this;
   on(event: 'message', callback: (params: JsonPipeMessageEvent) => void): this;
   on(event: 'closed', callback: (params: JsonPipeClosedEvent) => void): this;
   send(params: JsonPipeSendParams, metadata?: Metadata): Promise<JsonPipeSendResult>;
   close(params?: JsonPipeCloseParams, metadata?: Metadata): Promise<JsonPipeCloseResult>;
 }
-export type JsonPipeOpenedEvent = {};
 export type JsonPipeMessageEvent = {
   message: any,
 };
@@ -3547,7 +3545,6 @@ export type JsonPipeCloseOptions = {};
 export type JsonPipeCloseResult = void;
 
 export interface JsonPipeEvents {
-  'opened': JsonPipeOpenedEvent;
   'message': JsonPipeMessageEvent;
   'closed': JsonPipeClosedEvent;
 }

src/protocol/protocol.yml

@@ -2852,8 +2852,6 @@ JsonPipe:
 
   events:
 
-    opened:
-
     message:
       parameters:
         message: json

tests/browsertype-connect.spec.ts

@@ -64,12 +64,20 @@ test('should be able to connect two browsers at the same time', async ({browserT
   await browser2.close();
 });
 
-test('should timeout while connecting', async ({browserType, startRemoteServer, server}) => {
+test('should timeout in socket while connecting', async ({browserType, startRemoteServer, server}) => {
+  const e = await browserType.connect({
+    wsEndpoint: `ws://localhost:${server.PORT}/ws`,
+    timeout: 1,
+  }).catch(e => e);
+  expect(e.message).toContain('browserType.connect: Opening handshake has timed out');
+});
+
+test('should timeout in connect while connecting', async ({browserType, startRemoteServer, server}) => {
   const e = await browserType.connect({
     wsEndpoint: `ws://localhost:${server.PORT}/ws`,
     timeout: 100,
   }).catch(e => e);
-  expect(e.message).toContain('browserType.connect: Timeout 100ms exceeded.');
+  expect(e.message).toContain('browserType.connect: Timeout 100ms exceeded');
 });
 
 test('should send extra headers with connect request', async ({browserType, startRemoteServer, server}) => {

tests/browsertype-launch-server.spec.ts

@@ -57,7 +57,7 @@ it.describe('launch server', () => {
     const browserServer = await browserType.launchServer(browserOptions);
     const error = await browserType.connect({ wsEndpoint: browserServer.wsEndpoint() + '-foo' }).catch(e => e);
     await browserServer.close();
-    expect(error.message).toContain('WebSocket server disconnected');
+    expect(error.message).toContain('Unexpected server response: 400');
   });
 
   it('should fire "close" event during kill', async ({browserType, browserOptions}) => {