fix(connect): include websocket close reason in the error message (#30203)

When websocket disconnects during `browserType.connect()` call, the
error looks like this now:

```
browserType.connect: Custom error message received over WebSocket
```

Previously, it was a generic error:
```
browserType.connect: Target page, context or browser has been closed
```
This commit is contained in:
Dmitry Gozman 2024-04-01 21:05:33 -07:00 committed by GitHub
parent 6e799fdfa8
commit 6d56b453ff
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
16 changed files with 52 additions and 34 deletions

View file

@ -76,7 +76,7 @@ export class Android extends ChannelOwner<channels.AndroidChannel> implements ap
connection.on('close', closePipe); connection.on('close', closePipe);
let device: AndroidDevice; let device: AndroidDevice;
let closeError: Error | undefined; let closeError: string | undefined;
const onPipeClosed = () => { const onPipeClosed = () => {
device?._didClose(); device?._didClose();
connection.close(closeError); connection.close(closeError);
@ -88,7 +88,7 @@ export class Android extends ChannelOwner<channels.AndroidChannel> implements ap
try { try {
connection!.dispatch(message); connection!.dispatch(message);
} catch (e) { } catch (e) {
closeError = e; closeError = String(e);
closePipe(); closePipe();
} }
}); });

View file

@ -143,8 +143,8 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel> imple
connection.on('close', closePipe); connection.on('close', closePipe);
let browser: Browser; let browser: Browser;
let closeError: Error | undefined; let closeError: string | undefined;
const onPipeClosed = () => { const onPipeClosed = (reason?: string) => {
// Emulate all pages, contexts and the browser closing upon disconnect. // Emulate all pages, contexts and the browser closing upon disconnect.
for (const context of browser?.contexts() || []) { for (const context of browser?.contexts() || []) {
for (const page of context.pages()) for (const page of context.pages())
@ -152,16 +152,16 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel> imple
context._onClose(); context._onClose();
} }
browser?._didClose(); browser?._didClose();
connection.close(closeError); connection.close(reason || closeError);
}; };
pipe.on('closed', onPipeClosed); pipe.on('closed', params => onPipeClosed(params.reason));
connection.onmessage = message => pipe.send({ message }).catch(onPipeClosed); connection.onmessage = message => pipe.send({ message }).catch(() => onPipeClosed());
pipe.on('message', ({ message }) => { pipe.on('message', ({ message }) => {
try { try {
connection!.dispatch(message); connection!.dispatch(message);
} catch (e) { } catch (e) {
closeError = e; closeError = String(e);
closePipe(); closePipe();
} }
}); });

View file

@ -191,8 +191,8 @@ export class Connection extends EventEmitter {
(object._channel as any).emit(method, validator(params, '', { tChannelImpl: this._tChannelImplFromWire.bind(this), binary: this._rawBuffers ? 'buffer' : 'fromBase64' })); (object._channel as any).emit(method, validator(params, '', { tChannelImpl: this._tChannelImplFromWire.bind(this), binary: this._rawBuffers ? 'buffer' : 'fromBase64' }));
} }
close(cause?: Error) { close(cause?: string) {
this._closedError = new TargetClosedError(cause?.toString()); this._closedError = new TargetClosedError(cause);
for (const callback of this._callbacks.values()) for (const callback of this._callbacks.values())
callback.reject(this._closedError); callback.reject(this._closedError);
this._callbacks.clear(); this._callbacks.clear();

View file

@ -2586,7 +2586,7 @@ scheme.JsonPipeMessageEvent = tObject({
message: tAny, message: tAny,
}); });
scheme.JsonPipeClosedEvent = tObject({ scheme.JsonPipeClosedEvent = tObject({
error: tOptional(tType('SerializedError')), reason: tOptional(tString),
}); });
scheme.JsonPipeSendParams = tObject({ scheme.JsonPipeSendParams = tObject({
message: tAny, message: tAny,

View file

@ -75,11 +75,11 @@ export class CRConnection extends EventEmitter {
session._onMessage(message); session._onMessage(message);
} }
_onClose() { _onClose(reason?: string) {
this._closed = true; this._closed = true;
this._transport.onmessage = undefined; this._transport.onmessage = undefined;
this._transport.onclose = undefined; this._transport.onclose = undefined;
this._browserDisconnectedLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs()); this._browserDisconnectedLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs(), reason);
this.rootSession.dispose(); this.rootSession.dispose();
Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected)); Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected));
} }

View file

@ -17,7 +17,6 @@
import type * as channels from '@protocol/channels'; import type * as channels from '@protocol/channels';
import { Dispatcher } from './dispatcher'; import { Dispatcher } from './dispatcher';
import { createGuid } from '../../utils'; import { createGuid } from '../../utils';
import { serializeError } from '../errors';
import type { LocalUtilsDispatcher } from './localUtilsDispatcher'; import type { LocalUtilsDispatcher } from './localUtilsDispatcher';
export class JsonPipeDispatcher extends Dispatcher<{ guid: string }, channels.JsonPipeChannel, LocalUtilsDispatcher> implements channels.JsonPipeChannel { export class JsonPipeDispatcher extends Dispatcher<{ guid: string }, channels.JsonPipeChannel, LocalUtilsDispatcher> implements channels.JsonPipeChannel {
@ -43,10 +42,9 @@ export class JsonPipeDispatcher extends Dispatcher<{ guid: string }, channels.Js
this._dispatchEvent('message', { message }); this._dispatchEvent('message', { message });
} }
wasClosed(error?: Error): void { wasClosed(reason?: string): void {
if (!this._disposed) { if (!this._disposed) {
const params = error ? { error: serializeError(error) } : {}; this._dispatchEvent('closed', { reason });
this._dispatchEvent('closed', params);
this._dispose(); this._dispose();
} }
} }

View file

@ -230,9 +230,9 @@ export class LocalUtilsDispatcher extends Dispatcher<{ guid: string }, channels.
pipe.on('message', message => { pipe.on('message', message => {
transport.send(message); transport.send(message);
}); });
transport.onclose = () => { transport.onclose = (reason?: string) => {
socksInterceptor?.cleanup(); socksInterceptor?.cleanup();
pipe.wasClosed(); pipe.wasClosed(reason);
}; };
pipe.on('close', () => transport.close()); pipe.on('close', () => transport.close());
return { pipe, headers: transport.headers }; return { pipe, headers: transport.headers };

View file

@ -77,11 +77,11 @@ export class FFConnection extends EventEmitter {
session.dispatchMessage(message); session.dispatchMessage(message);
} }
_onClose() { _onClose(reason?: string) {
this._closed = true; this._closed = true;
this._transport.onmessage = undefined; this._transport.onmessage = undefined;
this._transport.onclose = undefined; this._transport.onclose = undefined;
this._browserDisconnectedLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs()); this._browserDisconnectedLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs(), reason);
this.rootSession.dispose(); this.rootSession.dispose();
Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected)); Promise.resolve().then(() => this.emit(ConnectionEvents.Disconnected));
} }

View file

@ -95,10 +95,10 @@ class Helper {
}; };
} }
static formatBrowserLogs(logs: string[]) { static formatBrowserLogs(logs: string[], disconnectReason?: string) {
if (!logs.length) if (!disconnectReason && !logs.length)
return ''; return '';
return '\n' + logs.join('\n'); return '\n' + (disconnectReason ? disconnectReason + '\n' : '') + logs.join('\n');
} }
} }

View file

@ -25,7 +25,7 @@ export class PipeTransport implements ConnectionTransport {
private _pendingBuffers: Buffer[] = []; private _pendingBuffers: Buffer[] = [];
private _waitForNextTask = makeWaitForNextTask(); private _waitForNextTask = makeWaitForNextTask();
private _closed = false; private _closed = false;
private _onclose?: () => void; private _onclose?: (reason?: string) => void;
onmessage?: (message: ProtocolResponse) => void; onmessage?: (message: ProtocolResponse) => void;
@ -47,7 +47,7 @@ export class PipeTransport implements ConnectionTransport {
return this._onclose; return this._onclose;
} }
set onclose(onclose: undefined | (() => void)) { set onclose(onclose: undefined | ((reason?: string) => void)) {
this._onclose = onclose; this._onclose = onclose;
if (onclose && !this._pipeRead.readable) if (onclose && !this._pipeRead.readable)
onclose(); onclose();

View file

@ -55,7 +55,7 @@ export interface ConnectionTransport {
send(s: ProtocolRequest): void; send(s: ProtocolRequest): void;
close(): void; // Note: calling close is expected to issue onclose at some point. close(): void; // Note: calling close is expected to issue onclose at some point.
onmessage?: (message: ProtocolResponse) => void, onmessage?: (message: ProtocolResponse) => void,
onclose?: () => void, onclose?: (reason?: string) => void,
} }
export class WebSocketTransport implements ConnectionTransport { export class WebSocketTransport implements ConnectionTransport {
@ -64,7 +64,7 @@ export class WebSocketTransport implements ConnectionTransport {
private _logUrl: string; private _logUrl: string;
onmessage?: (message: ProtocolResponse) => void; onmessage?: (message: ProtocolResponse) => void;
onclose?: () => void; onclose?: (reason?: string) => void;
readonly wsEndpoint: string; readonly wsEndpoint: string;
readonly headers: HeadersArray = []; readonly headers: HeadersArray = [];
@ -175,7 +175,7 @@ export class WebSocketTransport implements ConnectionTransport {
this._ws.addEventListener('close', event => { this._ws.addEventListener('close', event => {
this._progress?.log(`<ws disconnected> ${logUrl} code=${event.code} reason=${event.reason}`); this._progress?.log(`<ws disconnected> ${logUrl} code=${event.code} reason=${event.reason}`);
if (this.onclose) if (this.onclose)
this.onclose.call(null); this.onclose.call(null, event.reason);
}); });
// Prevent Error: read ECONNRESET. // Prevent Error: read ECONNRESET.
this._ws.addEventListener('error', error => this._progress?.log(`<ws error> ${logUrl} ${error.type} ${error.message}`)); this._ws.addEventListener('error', error => this._progress?.log(`<ws error> ${logUrl} ${error.type} ${error.message}`));

View file

@ -78,11 +78,11 @@ export class WKConnection {
this.browserSession.dispatchMessage(message); this.browserSession.dispatchMessage(message);
} }
_onClose() { _onClose(reason?: string) {
this._closed = true; this._closed = true;
this._transport.onmessage = undefined; this._transport.onmessage = undefined;
this._transport.onclose = undefined; this._transport.onclose = undefined;
this._browserDisconnectedLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs()); this._browserDisconnectedLogs = helper.formatBrowserLogs(this._browserLogsCollector.recentLogs(), reason);
this.browserSession.dispose(); this.browserSession.dispose();
this._onDisconnect(); this._onDisconnect();
} }

View file

@ -4674,7 +4674,7 @@ export type JsonPipeMessageEvent = {
message: any, message: any,
}; };
export type JsonPipeClosedEvent = { export type JsonPipeClosedEvent = {
error?: SerializedError, reason?: string,
}; };
export type JsonPipeSendParams = { export type JsonPipeSendParams = {
message: any, message: any,

View file

@ -3548,4 +3548,4 @@ JsonPipe:
closed: closed:
parameters: parameters:
error: SerializedError? reason: string?

View file

@ -129,6 +129,16 @@ for (const kind of ['launchServer', 'run-server'] as const) {
expect(error.message).not.toContain('secret=MYSECRET'); expect(error.message).not.toContain('secret=MYSECRET');
}); });
test('should print custom ws close error', async ({ connect, server }) => {
server.onceWebSocketConnection((ws, request) => {
ws.on('message', message => {
ws.close(4123, 'Oh my!');
});
});
const error = await connect(`ws://localhost:${server.PORT}/ws`).catch(e => e);
expect(error.message).toContain('browserType.connect: Oh my!');
});
test('should be able to reconnect to a browser', async ({ connect, startRemoteServer, server }) => { test('should be able to reconnect to a browser', async ({ connect, startRemoteServer, server }) => {
const remoteServer = await startRemoteServer(kind); const remoteServer = await startRemoteServer(kind);
{ {
@ -325,7 +335,7 @@ for (const kind of ['launchServer', 'run-server'] as const) {
]); ]);
expect(browser.isConnected()).toBe(false); expect(browser.isConnected()).toBe(false);
const error = await page.evaluate('1 + 1').catch(e => e) as Error; const error = await page.evaluate('1 + 1').catch(e => e) as Error;
expect(error.message).toContain('has been closed'); expect(error.message).toContain('closed');
}); });
test('should throw when calling waitForNavigation after disconnect', async ({ connect, startRemoteServer }) => { test('should throw when calling waitForNavigation after disconnect', async ({ connect, startRemoteServer }) => {

View file

@ -527,3 +527,13 @@ test('setInputFiles should preserve lastModified timestamp', async ({ browserTyp
await browserServer.close(); await browserServer.close();
} }
}); });
test('should print custom ws close error', async ({ browserType, server }) => {
server.onceWebSocketConnection((ws, request) => {
ws.on('message', message => {
ws.close(4123, 'Oh my!');
});
});
const error = await browserType.connectOverCDP(`ws://localhost:${server.PORT}/ws`).catch(e => e);
expect(error.message).toContain(`Browser logs:\n\nOh my!\n`);
});