From c89c30e3336266e9188ebf4321c03c3e5eb4d48f Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Fri, 10 Jul 2020 13:15:39 -0700 Subject: [PATCH] fix(popup): do not report frameless pages (#2910) --- src/chromium/crBrowser.ts | 9 ++++++--- src/firefox/ffBrowser.ts | 4 +++- src/page.ts | 5 +++++ src/rpc/channels.ts | 3 ++- src/rpc/client/page.ts | 1 + src/rpc/server/pageDispatcher.ts | 3 ++- src/webkit/wkBrowser.ts | 4 +++- test/browsercontext.spec.js | 17 +++++++++++++++++ test/channels.spec.js | 2 +- test/page.spec.js | 2 +- 10 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 32b431e535..6eb07b7ff0 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -151,12 +151,15 @@ export class CRBrowser extends BrowserBase { const opener = targetInfo.openerId ? this._crPages.get(targetInfo.openerId) || null : null; const crPage = new CRPage(session, targetInfo.targetId, context, opener, !!this._options.headful); this._crPages.set(targetInfo.targetId, crPage); - crPage.pageOrError().then(() => { - context!.emit(CommonEvents.BrowserContext.Page, crPage._page); + crPage.pageOrError().then(pageOrError => { + const page = crPage._page; + if (pageOrError instanceof Error) + page._setIsError(); + context!.emit(CommonEvents.BrowserContext.Page, page); if (opener) { opener.pageOrError().then(openerPage => { if (openerPage instanceof Page && !openerPage.isClosed()) - openerPage.emit(CommonEvents.Page.Popup, crPage._page); + openerPage.emit(CommonEvents.Page.Popup, page); }); } }); diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index 4e6e1f3103..23dfd40f26 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -97,8 +97,10 @@ export class FFBrowser extends BrowserBase { const ffPage = new FFPage(session, context, opener); this._ffPages.set(targetId, ffPage); - ffPage.pageOrError().then(async () => { + ffPage.pageOrError().then(async pageOrError => { const page = ffPage._page; + if (pageOrError instanceof Error) + page._setIsError(); context.emit(Events.BrowserContext.Page, page); if (!opener) return; diff --git a/src/page.ts b/src/page.ts index 1c043e7ea9..b1ac1e0cc1 100644 --- a/src/page.ts +++ b/src/page.ts @@ -490,6 +490,11 @@ export class Page extends EventEmitter { await this._ownedContext.close(); } + _setIsError() { + if (!this._frameManager.mainFrame()) + this._frameManager.frameAttached('', null); + } + isClosed(): boolean { return this._closedState === 'closed'; } diff --git a/src/rpc/channels.ts b/src/rpc/channels.ts index 8899a2d533..d9e94d8947 100644 --- a/src/rpc/channels.ts +++ b/src/rpc/channels.ts @@ -167,7 +167,8 @@ export interface PageChannel extends Channel { export type PageInitializer = { mainFrame: FrameChannel, - viewportSize: types.Size | null + viewportSize: types.Size | null, + isClosed: boolean }; export type PageAttribution = { isPage?: boolean }; diff --git a/src/rpc/client/page.ts b/src/rpc/client/page.ts index a1ec0def94..6e9ea6fb71 100644 --- a/src/rpc/client/page.ts +++ b/src/rpc/client/page.ts @@ -80,6 +80,7 @@ export class Page extends ChannelOwner { this._mainFrame._page = this; this._frames.add(this._mainFrame); this._viewportSize = initializer.viewportSize; + this._closed = initializer.isClosed; this._channel.on('bindingCall', bindingCall => this._onBinding(BindingCall.from(bindingCall))); this._channel.on('close', () => this._onClose()); diff --git a/src/rpc/server/pageDispatcher.ts b/src/rpc/server/pageDispatcher.ts index 256f53fab2..61fde182b4 100644 --- a/src/rpc/server/pageDispatcher.ts +++ b/src/rpc/server/pageDispatcher.ts @@ -39,7 +39,8 @@ export class PageDispatcher extends Dispatcher implements constructor(scope: DispatcherScope, page: Page) { super(scope, page, 'page', { mainFrame: FrameDispatcher.from(scope, page.mainFrame()), - viewportSize: page.viewportSize() + viewportSize: page.viewportSize(), + isClosed: page.isClosed() }); this._page = page; page.on(Events.Page.Close, () => this._dispatchEvent('close')); diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 2846ed0f2e..95f93543e5 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -140,8 +140,10 @@ export class WKBrowser extends BrowserBase { const wkPage = new WKPage(context, pageProxySession, opener || null); this._wkPages.set(pageProxyId, wkPage); - wkPage.pageOrError().then(async () => { + wkPage.pageOrError().then(async pageOrError => { const page = wkPage._page; + if (pageOrError instanceof Error) + page._setIsError(); context!.emit(Events.BrowserContext.Page, page); if (!opener) return; diff --git a/test/browsercontext.spec.js b/test/browsercontext.spec.js index b8b648f7bd..df90a2acf4 100644 --- a/test/browsercontext.spec.js +++ b/test/browsercontext.spec.js @@ -129,6 +129,23 @@ describe('BrowserContext', function() { ]); await context.close(); }); + it('should not report frameless pages on error', async({browser, server}) => { + const context = await browser.newContext(); + page = await context.newPage(); + server.setRoute('/empty.html', (req, res) => { + res.end(`Click me`); + }); + let popup; + context.on('page', p => popup = p); + await page.goto(server.EMPTY_PAGE); + await page.click('"Click me"'); + await context.close(); + if (popup) { + // This races on Firefox :/ + expect(popup.isClosed()).toBeTruthy(); + expect(popup.mainFrame()).toBeTruthy(); + } + }) }); describe('BrowserContext({userAgent})', function() { diff --git a/test/channels.spec.js b/test/channels.spec.js index 25a284cd36..347df572d8 100644 --- a/test/channels.spec.js +++ b/test/channels.spec.js @@ -48,7 +48,7 @@ describe.skip(!CHANNEL)('Channels', function() { await expectScopeState(browser, GOLDEN_PRECONDITION); }); - it('should scope CDPSession handles', async({browserType, browser, server}) => { + it.skip(!CHROMIUM)('should scope CDPSession handles', async({browserType, browser, server}) => { const GOLDEN_PRECONDITION = { objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ], scopes: [ diff --git a/test/page.spec.js b/test/page.spec.js index 26def08106..6657811a66 100644 --- a/test/page.spec.js +++ b/test/page.spec.js @@ -113,7 +113,7 @@ describe('Async stacks', () => { }); }); -describe.fail(FFOX && WIN)('Page.Events.Crash', function() { +describe.fail(FFOX && WIN).skip(CHANNEL)('Page.Events.Crash', function() { // Firefox Win: it just doesn't crash sometimes. function crash(page) {