fix(popup): do not report frameless pages (#2910)

Author: pavelfeldman

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);
           });
         }
       });

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;

src/page.ts

@@ -490,6 +490,11 @@ export class Page extends EventEmitter {
       await this._ownedContext.close();
   }
 
+  _setIsError() {
+    if (!this._frameManager.mainFrame())
+      this._frameManager.frameAttached('<dummy>', null);
+  }
+
   isClosed(): boolean {
     return this._closedState === 'closed';
   }

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 };

src/rpc/client/page.ts

@@ -80,6 +80,7 @@ export class Page extends ChannelOwner<PageChannel, PageInitializer> {
     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());

src/rpc/server/pageDispatcher.ts

@@ -39,7 +39,8 @@ export class PageDispatcher extends Dispatcher<Page, PageInitializer> 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'));

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;

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(`<a href="${server.EMPTY_PAGE}" target="_blank">Click me</a>`);
+    });
+    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() {

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: [

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) {