fix(actions): do not wait for the created popups (#2219)

Since we are recommending Promise.all pattern anyway, this special
logic just adds to the possibility of timeout if something goes wrong.

For example, Firefox sometimes send Page.willOpenNewWindowAsynchronously
later than the new target arrives and input action just hangs.

Author: dgozman

src/chromium/crBrowser.ts

@@ -152,10 +152,6 @@ 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._isHeadful);
       this._crPages.set(targetInfo.targetId, crPage);
-      if (opener && opener._initializedPage) {
-        for (const signalBarrier of opener._initializedPage._frameManager._signalBarriers)
-          signalBarrier.addPopup(crPage.pageOrError());
-      }
       crPage.pageOrError().then(() => {
         context!.emit(CommonEvents.BrowserContext.Page, crPage._page);
         if (opener) {

src/firefox/ffBrowser.ts

@@ -99,10 +99,6 @@ export class FFBrowser extends BrowserBase {
     const ffPage = new FFPage(session, context, opener);
     this._ffPages.set(targetId, ffPage);
 
-    if (opener && opener._initializedPage) {
-      for (const signalBarrier of opener._initializedPage._frameManager._signalBarriers)
-        signalBarrier.addPopup(ffPage.pageOrError());
-    }
     ffPage.pageOrError().then(async () => {
       const page = ffPage._page;
       context.emit(Events.BrowserContext.Page, page);

src/firefox/ffPage.ts

@@ -62,6 +62,7 @@ export class FFPage implements PageDelegate {
     this._page = new Page(this, browserContext);
     this._networkManager = new FFNetworkManager(session, this._page);
     this._page.on(Events.Page.FrameDetached, frame => this._removeContextsForFrame(frame));
+    // TODO: remove Page.willOpenNewWindowAsynchronously from the protocol.
     this._eventListeners = [
       helper.addEventListener(this._session, 'Page.eventFired', this._onEventFired.bind(this)),
       helper.addEventListener(this._session, 'Page.frameAttached', this._onFrameAttached.bind(this)),
@@ -73,7 +74,6 @@ export class FFPage implements PageDelegate {
       helper.addEventListener(this._session, 'Runtime.executionContextCreated', this._onExecutionContextCreated.bind(this)),
       helper.addEventListener(this._session, 'Runtime.executionContextDestroyed', this._onExecutionContextDestroyed.bind(this)),
       helper.addEventListener(this._session, 'Page.linkClicked', event => this._onLinkClicked(event.phase)),
-      helper.addEventListener(this._session, 'Page.willOpenNewWindowAsynchronously', this._onWillOpenNewWindowAsynchronously.bind(this)),
       helper.addEventListener(this._session, 'Page.uncaughtError', this._onUncaughtError.bind(this)),
       helper.addEventListener(this._session, 'Runtime.console', this._onConsole.bind(this)),
       helper.addEventListener(this._session, 'Page.dialogOpened', this._onDialogOpened.bind(this)),
@@ -136,11 +136,6 @@ export class FFPage implements PageDelegate {
       this._page._frameManager.frameDidPotentiallyRequestNavigation();
   }
 
-  _onWillOpenNewWindowAsynchronously() {
-    for (const barrier of this._page._frameManager._signalBarriers)
-      barrier.expectPopup();
-  }
-
   _onNavigationStarted(params: Protocol.Page.navigationStartedPayload) {
     this._page._frameManager.frameRequestedNavigation(params.frameId, params.navigationId);
   }

src/frames.ts

@@ -954,10 +954,8 @@ function selectorToString(selector: string, state: 'attached' | 'detached' | 'vi
 }
 
 export class SignalBarrier {
-  private _frameIds = new Map<string, number>();
   private _options: types.NavigatingActionWaitOptions;
   private _protectCount = 0;
-  private _expectedPopups = 0;
   private _promise: Promise<void>;
   private _promiseCallback = () => {};
   private _deadline: number;
@@ -981,23 +979,6 @@ export class SignalBarrier {
     this.release();
   }
 
-  async expectPopup() {
-    ++this._expectedPopups;
-  }
-
-  async unexpectPopup() {
-    --this._expectedPopups;
-    this._maybeResolve();
-  }
-
-  async addPopup(pageOrError: Promise<Page | Error>) {
-    if (this._expectedPopups)
-      --this._expectedPopups;
-    this.retain();
-    await pageOrError;
-    this.release();
-  }
-
   retain() {
     ++this._protectCount;
   }
@@ -1008,7 +989,7 @@ export class SignalBarrier {
   }
 
   private async _maybeResolve() {
-    if (!this._protectCount && !this._expectedPopups && !this._frameIds.size)
+    if (!this._protectCount)
       this._promiseCallback();
   }
 }

src/webkit/wkBrowser.ts

@@ -142,10 +142,6 @@ export class WKBrowser extends BrowserBase {
     const wkPage = new WKPage(context, pageProxySession, opener || null);
     this._wkPages.set(pageProxyId, wkPage);
 
-    if (opener && opener._initializedPage) {
-      for (const signalBarrier of opener._initializedPage._frameManager._signalBarriers)
-        signalBarrier.addPopup(wkPage.pageOrError());
-    }
     wkPage.pageOrError().then(async () => {
       const page = wkPage._page;
       context!.emit(Events.BrowserContext.Page, page);

src/webkit/wkPage.ts

@@ -309,6 +309,7 @@ export class WKPage implements PageDelegate {
   }
 
   private _addSessionListeners() {
+    // TODO: remove Page.willRequestOpenWindow and Page.didRequestOpenWindow from the protocol.
     this._sessionListeners = [
       helper.addEventListener(this._session, 'Page.frameNavigated', event => this._onFrameNavigated(event.frame, false)),
       helper.addEventListener(this._session, 'Page.navigatedWithinDocument', event => this._onFrameNavigatedWithinDocument(event.frameId, event.url)),
@@ -318,8 +319,6 @@ export class WKPage implements PageDelegate {
       helper.addEventListener(this._session, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)),
       helper.addEventListener(this._session, 'Page.loadEventFired', event => this._onLifecycleEvent(event.frameId, 'load')),
       helper.addEventListener(this._session, 'Page.domContentEventFired', event => this._onLifecycleEvent(event.frameId, 'domcontentloaded')),
-      helper.addEventListener(this._session, 'Page.willRequestOpenWindow', event => this._onWillRequestOpenWindow()),
-      helper.addEventListener(this._session, 'Page.didRequestOpenWindow', event => this._onDidRequestOpenWindow(event)),
       helper.addEventListener(this._session, 'Runtime.executionContextCreated', event => this._onExecutionContextCreated(event.context)),
       helper.addEventListener(this._session, 'Console.messageAdded', event => this._onConsoleMessage(event)),
       helper.addEventListener(this._session, 'Console.messageRepeatCountUpdated', event => this._onConsoleRepeatCountUpdated(event)),
@@ -363,18 +362,6 @@ export class WKPage implements PageDelegate {
     this._page._frameManager.frameLifecycleEvent(frameId, event);
   }
 
-  private _onWillRequestOpenWindow() {
-    for (const barrier of this._page._frameManager._signalBarriers)
-      barrier.expectPopup();
-  }
-
-  private _onDidRequestOpenWindow(event: Protocol.Page.didRequestOpenWindowPayload) {
-    if (!event.opened) {
-      for (const barrier of this._page._frameManager._signalBarriers)
-        barrier.unexpectPopup();
-    }
-  }
-
   private _handleFrameTree(frameTree: Protocol.Page.FrameResourceTree) {
     this._onFrameAttached(frameTree.frame.id, frameTree.frame.parentId || null);
     this._onFrameNavigated(frameTree.frame, true);

test/autowaiting.spec.js

@@ -34,26 +34,6 @@ describe('Auto waiting', () => {
     ]);
     expect(messages.join('|')).toBe('route|navigated|click');
   });
-  it('should await popup when clicking anchor', async function({page, server}) {
-    await page.goto(server.EMPTY_PAGE);
-    await page.setContent('<a target=_blank rel=opener href="/empty.html">link</a>');
-    const messages = [];
-    await Promise.all([
-      page.waitForEvent('popup').then(() => messages.push('popup')),
-      page.click('a').then(() => messages.push('click')),
-    ]);
-    expect(messages.join('|')).toBe('popup|click');
-  });
-  it('should await popup when clicking anchor with noopener', async function({page, server}) {
-    await page.goto(server.EMPTY_PAGE);
-    await page.setContent('<a target=_blank rel=noopener href="/empty.html">link</a>');
-    const messages = [];
-    await Promise.all([
-      page.waitForEvent('popup').then(() => messages.push('popup')),
-      page.click('a').then(() => messages.push('click')),
-    ]);
-    expect(messages.join('|')).toBe('popup|click');
-  });
   it('should await cross-process navigation when clicking anchor', async({page, server}) => {
     const messages = [];
     server.setRoute('/empty.html', async (req, res) => {
@@ -150,15 +130,6 @@ describe('Auto waiting', () => {
     ]);
     expect(messages.join('|')).toBe('route|navigated|evaluate');
   });
-  it('should await new popup when evaluating', async function({page, server}) {
-    await page.goto(server.EMPTY_PAGE);
-    const messages = [];
-    await Promise.all([
-      page.waitForEvent('popup').then(() => messages.push('popup')),
-      page.evaluate(() => window._popup = window.open(window.location.href)).then(() => messages.push('evaluate')),
-    ]);
-    expect(messages.join('|')).toBe('popup|evaluate');
-  });
   it('should await navigating specified target', async({page, server}) => {
     const messages = [];
     server.setRoute('/empty.html', async (req, res) => {
@@ -254,5 +225,12 @@ describe('Auto waiting should not hang when', () => {
       popup.close();
     });
   });
+  it('opening a popup', async function({page, server}) {
+    await page.goto(server.EMPTY_PAGE);
+    await Promise.all([
+      page.waitForEvent('popup'),
+      page.evaluate(() => window._popup = window.open(window.location.href)),
+    ]);
+  });
 });