From d117d0bb93374ae0eaaf2a6049dbd68a0b9677eb Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 2 Nov 2020 13:06:54 -0800 Subject: [PATCH] feat(scopes): make page a scope (#4300) --- src/dispatchers/dispatcher.ts | 20 ++++++++------------ src/dispatchers/pageDispatcher.ts | 9 ++++++--- src/server/frames.ts | 3 ++- src/utils/errors.ts | 2 +- test/channels.spec.ts | 7 ++++--- test/chromium/session.spec.ts | 2 +- test/page-close.spec.ts | 12 +++++++++++- 7 files changed, 33 insertions(+), 22 deletions(-) diff --git a/src/dispatchers/dispatcher.ts b/src/dispatchers/dispatcher.ts index e155472fcde01..709ec5fffce62 100644 --- a/src/dispatchers/dispatcher.ts +++ b/src/dispatchers/dispatcher.ts @@ -73,7 +73,7 @@ export class Dispatcher extends EventEmitter implements chann (object as any)[dispatcherSymbol] = this; if (this._parent) - this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid }, !!isScope); + this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid }); } _dispatchEvent(method: string, params: Dispatcher | any = {}) { @@ -126,9 +126,8 @@ export class DispatcherConnection { private _validateParams: (type: string, method: string, params: any) => any; private _validateMetadata: (metadata: any) => any; - sendMessageToClient(guid: string, method: string, params: any, disallowDispatchers?: boolean) { - const allowDispatchers = !disallowDispatchers; - this.onmessage({ guid, method, params: this._replaceDispatchersWithGuids(params, allowDispatchers) }); + sendMessageToClient(guid: string, method: string, params: any) { + this.onmessage({ guid, method, params: this._replaceDispatchersWithGuids(params) }); } constructor() { @@ -178,26 +177,23 @@ export class DispatcherConnection { try { const validated = this._validateParams(dispatcher._type, method, params); const result = await (dispatcher as any)[method](validated, this._validateMetadata(metadata)); - this.onmessage({ id, result: this._replaceDispatchersWithGuids(result, true) }); + this.onmessage({ id, result: this._replaceDispatchersWithGuids(result) }); } catch (e) { this.onmessage({ id, error: serializeError(e) }); } } - private _replaceDispatchersWithGuids(payload: any, allowDispatchers: boolean): any { + private _replaceDispatchersWithGuids(payload: any): any { if (!payload) return payload; - if (payload instanceof Dispatcher) { - if (!allowDispatchers) - throw new Error(`Channels are not allowed in the scope's initialzier`); + if (payload instanceof Dispatcher) return { guid: payload._guid }; - } if (Array.isArray(payload)) - return payload.map(p => this._replaceDispatchersWithGuids(p, allowDispatchers)); + return payload.map(p => this._replaceDispatchersWithGuids(p)); if (typeof payload === 'object') { const result: any = {}; for (const key of Object.keys(payload)) - result[key] = this._replaceDispatchersWithGuids(payload[key], allowDispatchers); + result[key] = this._replaceDispatchersWithGuids(payload[key]); return result; } return payload; diff --git a/src/dispatchers/pageDispatcher.ts b/src/dispatchers/pageDispatcher.ts index 9b402bc298ced..1bd863ba5c980 100644 --- a/src/dispatchers/pageDispatcher.ts +++ b/src/dispatchers/pageDispatcher.ts @@ -43,14 +43,17 @@ export class PageDispatcher extends Dispatcher i videoRelativePath: page._video ? page._video._relativePath : undefined, viewportSize: page.viewportSize() || undefined, isClosed: page.isClosed() - }); + }, true); this._page = page; - page.on(Page.Events.Close, () => this._dispatchEvent('close')); + page.on(Page.Events.Close, () => { + this._dispatchEvent('close'); + this._dispose(); + }); page.on(Page.Events.Console, message => this._dispatchEvent('console', { message: new ConsoleMessageDispatcher(this._scope, message) })); page.on(Page.Events.Crash, () => this._dispatchEvent('crash')); page.on(Page.Events.DOMContentLoaded, () => this._dispatchEvent('domcontentloaded')); page.on(Page.Events.Dialog, dialog => this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this._scope, dialog) })); - page.on(Page.Events.Download, download => this._dispatchEvent('download', { download: new DownloadDispatcher(this._scope, download) })); + page.on(Page.Events.Download, download => this._dispatchEvent('download', { download: new DownloadDispatcher(scope, download) })); this._page.on(Page.Events.FileChooser, (fileChooser: FileChooser) => this._dispatchEvent('fileChooser', { element: new ElementHandleDispatcher(this._scope, fileChooser.element()), isMultiple: fileChooser.isMultiple() diff --git a/src/server/frames.ts b/src/server/frames.ts index de54ff172b57b..cf99a1bff55a0 100644 --- a/src/server/frames.ts +++ b/src/server/frames.ts @@ -296,7 +296,8 @@ export class FrameManager { this.removeChildFramesRecursively(frame); frame._onDetached(); this._frames.delete(frame._id); - this._page.emit(Page.Events.FrameDetached, frame); + if (!this._page.isClosed()) + this._page.emit(Page.Events.FrameDetached, frame); } private _inflightRequestFinished(request: network.Request) { diff --git a/src/utils/errors.ts b/src/utils/errors.ts index 47dcc0d056ade..d064129795b63 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -26,7 +26,7 @@ class CustomError extends Error { export class TimeoutError extends CustomError {} export const kBrowserClosedError = 'Browser has been closed'; -export const kBrowserOrContextClosedError = 'Target browser or context has been closed'; +export const kBrowserOrContextClosedError = 'Target page, context or browser has been closed'; export function isSafeCloseError(error: Error) { return error.message.endsWith(kBrowserClosedError) || error.message.endsWith(kBrowserOrContextClosedError); diff --git a/test/channels.spec.ts b/test/channels.spec.ts index 60ff074ec176d..507bb03ab87e2 100644 --- a/test/channels.spec.ts +++ b/test/channels.spec.ts @@ -63,9 +63,10 @@ it('should scope context handles', async ({browser, server}) => { { _guid: 'Browser', objects: [ { _guid: 'BrowserContext', objects: [ { _guid: 'Frame', objects: [] }, - { _guid: 'Page', objects: [] }, - { _guid: 'Request', objects: [] }, - { _guid: 'Response', objects: [] }, + { _guid: 'Page', objects: [ + { _guid: 'Request', objects: [] }, + { _guid: 'Response', objects: [] }, + ]}, ]}, ] }, ] }, diff --git a/test/chromium/session.spec.ts b/test/chromium/session.spec.ts index 4402a878bcc1b..1fa0b909849a3 100644 --- a/test/chromium/session.spec.ts +++ b/test/chromium/session.spec.ts @@ -75,7 +75,7 @@ describe('session', (suite, { browserName }) => { } catch (e) { error = e; } - expect(error.message).toContain('Target browser or context has been closed'); + expect(error.message).toContain('Target page, context or browser has been closed'); }); it('should throw nice errors', async function({page}) { diff --git a/test/page-close.spec.ts b/test/page-close.spec.ts index f7cae6af29369..8b5f1e2d05280 100644 --- a/test/page-close.spec.ts +++ b/test/page-close.spec.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { it } from './fixtures'; +import { it, expect } from './fixtures'; it('should close page with active dialog', (test, { browserName, platform }) => { test.fixme(browserName === 'webkit' && platform === 'darwin', 'WebKit hangs on a Mac'); @@ -43,3 +43,13 @@ it('should access page after beforeunload', (test, { browserName }) => { await dialog.dismiss(); await page.evaluate(() => document.title); }); + +it('should not accept after close', (test, { browserName, platform }) => { + test.fixme(browserName === 'webkit' && platform === 'darwin', 'WebKit hangs on a Mac'); +}, async ({page}) => { + page.evaluate(() => alert()).catch(() => {}); + const dialog = await page.waitForEvent('dialog'); + await page.close(); + const e = await dialog.dismiss().catch(e => e); + expect(e.message).toContain('Target page, context or browser has been closed'); +});