From 461818dfd2325669bd18f99d5360dfc43d9ad7f8 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Thu, 1 Aug 2024 13:24:46 +0200 Subject: [PATCH] refactor(test runner): use closeReason from server Currently, when the server closes a BrowerContext/Page (e.g. because the page crashed), there's no way for the server to pass back a _closeReason. The client will always show the fallback error message 'Target page, context or browser has been closed'. This PR makes the server send back the close reason as part of a new `reason` parameter on the `close` event, allowing both client- and server-originating closes to have a specific reason. --- packages/playwright-core/src/client/browserContext.ts | 6 +++--- packages/playwright-core/src/client/page.ts | 8 ++++---- packages/playwright-core/src/protocol/validator.ts | 8 ++++++-- packages/playwright-core/src/server/browserContext.ts | 2 +- .../src/server/dispatchers/browserContextDispatcher.ts | 4 ++-- .../src/server/dispatchers/pageDispatcher.ts | 4 ++-- packages/playwright-core/src/server/page.ts | 4 ++-- packages/protocol/src/channels.ts | 8 ++++++-- packages/protocol/src/protocol.yml | 4 ++++ 9 files changed, 30 insertions(+), 18 deletions(-) diff --git a/packages/playwright-core/src/client/browserContext.ts b/packages/playwright-core/src/client/browserContext.ts index 3eb8214ca5..e97cda6e28 100644 --- a/packages/playwright-core/src/client/browserContext.ts +++ b/packages/playwright-core/src/client/browserContext.ts @@ -88,7 +88,7 @@ export class BrowserContext extends ChannelOwner this.clock = new Clock(this); this._channel.on('bindingCall', ({ binding }) => this._onBinding(BindingCall.from(binding))); - this._channel.on('close', () => this._onClose()); + this._channel.on('close', ({ reason }) => this._onClose(reason)); this._channel.on('page', ({ page }) => this._onPage(Page.from(page))); this._channel.on('route', ({ route }) => this._onRoute(network.Route.from(route))); this._channel.on('backgroundPage', ({ page }) => { @@ -431,7 +431,8 @@ export class BrowserContext extends ChannelOwner return CDPSession.from(result.session); } - _onClose() { + _onClose(reason?: string) { + this._closeReason = reason; if (this._browser) this._browser._contexts.delete(this); this._browserType?._contexts?.delete(this); @@ -447,7 +448,6 @@ export class BrowserContext extends ChannelOwner async close(options: { reason?: string } = {}): Promise { if (this._closeWasCalled) return; - this._closeReason = options.reason; this._closeWasCalled = true; await this._wrapApiCall(async () => { await this.request.dispose(options); diff --git a/packages/playwright-core/src/client/page.ts b/packages/playwright-core/src/client/page.ts index 84848e433d..75f4b77fd2 100644 --- a/packages/playwright-core/src/client/page.ts +++ b/packages/playwright-core/src/client/page.ts @@ -129,7 +129,7 @@ export class Page extends ChannelOwner implements api.Page this._opener = Page.fromNullable(initializer.opener); this._channel.on('bindingCall', ({ binding }) => this._onBinding(BindingCall.from(binding))); - this._channel.on('close', () => this._onClose()); + this._channel.on('close', ({ reason }) => this._onClose(reason)); this._channel.on('crash', () => this._onCrash()); this._channel.on('download', ({ url, suggestedFilename, artifact }) => { const artifactObject = Artifact.from(artifact); @@ -218,8 +218,9 @@ export class Page extends ChannelOwner implements api.Page this.emit(Events.Page.Worker, worker); } - _onClose() { + _onClose(reason?: string) { this._closed = true; + this._closeReason = reason; this._browserContext._pages.delete(this); this._browserContext._backgroundPages.delete(this); this._disposeHarRouters(); @@ -598,11 +599,10 @@ export class Page extends ChannelOwner implements api.Page } async close(options: { runBeforeUnload?: boolean, reason?: string } = {}) { - this._closeReason = options.reason; this._closeWasCalled = true; try { if (this._ownedContext) - await this._ownedContext.close(); + await this._ownedContext.close(options); else await this._channel.close(options); } catch (e) { diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index 81755c79bc..f899812622 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -815,7 +815,9 @@ scheme.BrowserContextConsoleEvent = tObject({ }), page: tChannel(['Page']), }); -scheme.BrowserContextCloseEvent = tOptional(tObject({})); +scheme.BrowserContextCloseEvent = tObject({ + reason: tOptional(tString), +}); scheme.BrowserContextDialogEvent = tObject({ dialog: tChannel(['Dialog']), }); @@ -1045,7 +1047,9 @@ scheme.PageInitializer = tObject({ scheme.PageBindingCallEvent = tObject({ binding: tChannel(['BindingCall']), }); -scheme.PageCloseEvent = tOptional(tObject({})); +scheme.PageCloseEvent = tObject({ + reason: tOptional(tString), +}); scheme.PageCrashEvent = tOptional(tObject({})); scheme.PageDownloadEvent = tObject({ url: tString, diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index da21dff708..631552e258 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -252,7 +252,7 @@ export abstract class BrowserContext extends SdkObject { if (this._isPersistentContext) this.onClosePersistent(); this._closePromiseFulfill!(new Error('Context closed')); - this.emit(BrowserContext.Events.Close); + this.emit(BrowserContext.Events.Close, { reason: this._closeReason }); } // BrowserContext methods. diff --git a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts index 5654950360..e7c8f49ae4 100644 --- a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts @@ -82,8 +82,8 @@ export class BrowserContextDispatcher extends Dispatcher { this._dispatchEvent('page', { page: PageDispatcher.from(this, page) }); }); - this.addObjectListener(BrowserContext.Events.Close, () => { - this._dispatchEvent('close'); + this.addObjectListener(BrowserContext.Events.Close, params => { + this._dispatchEvent('close', params); this._dispose(); }); this.addObjectListener(BrowserContext.Events.PageError, (error: Error, page: Page) => { diff --git a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts index 3101cd051d..755bd91bbf 100644 --- a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts @@ -70,8 +70,8 @@ export class PageDispatcher extends Dispatcher { - this._dispatchEvent('close'); + this.addObjectListener(Page.Events.Close, params => { + this._dispatchEvent('close', params); this._dispose(); }); this.addObjectListener(Page.Events.Crash, () => this._dispatchEvent('crash')); diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index 6206681048..b2d11c4122 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -221,7 +221,7 @@ export class Page extends SdkObject { // in that case we fire another Close event to ensure that each reported Page will have // corresponding Close event after it is reported on the context. if (this.isClosed()) - this.emit(Page.Events.Close); + this.emit(Page.Events.Close, { reason: this._closeReason }); else this.instrumentation.onPageOpen(this); } @@ -280,7 +280,7 @@ export class Page extends SdkObject { this._frameThrottler.dispose(); assert(this._closedState !== 'closed', 'Page closed twice'); this._closedState = 'closed'; - this.emit(Page.Events.Close); + this.emit(Page.Events.Close, { reason: this._closeReason }); this._closedPromise.resolve(); this.instrumentation.onPageClose(this); this.openScope.close(new TargetClosedError()); diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index f3e0a2c35a..8a5a481ddf 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -1545,7 +1545,9 @@ export type BrowserContextConsoleEvent = { }, page: PageChannel, }; -export type BrowserContextCloseEvent = {}; +export type BrowserContextCloseEvent = { + reason?: string, +}; export type BrowserContextDialogEvent = { dialog: DialogChannel, }; @@ -1963,7 +1965,9 @@ export interface PageChannel extends PageEventTarget, EventTargetChannel { export type PageBindingCallEvent = { binding: BindingCallChannel, }; -export type PageCloseEvent = {}; +export type PageCloseEvent = { + reason?: string, +}; export type PageCrashEvent = {}; export type PageDownloadEvent = { url: string, diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index 4c25212c57..d2811cfb19 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -1285,6 +1285,8 @@ BrowserContext: page: Page close: + parameters: + reason: string? dialog: parameters: @@ -1737,6 +1739,8 @@ Page: binding: BindingCall close: + parameters: + reason: string? crash: