feat(click): start wire auto-waiting click in firefox (#1233)

package.json

@@ -9,7 +9,7 @@
   "main": "index.js",
   "playwright": {
     "chromium_revision": "747023",
-    "firefox_revision": "1032",
+    "firefox_revision": "1035",
     "webkit_revision": "1168"
   },
   "scripts": {

src/firefox/ffPage.ts

@@ -60,10 +60,11 @@ export class FFPage implements PageDelegate {
       helper.addEventListener(this._session, 'Page.frameDetached', this._onFrameDetached.bind(this)),
       helper.addEventListener(this._session, 'Page.navigationAborted', this._onNavigationAborted.bind(this)),
       helper.addEventListener(this._session, 'Page.navigationCommitted', this._onNavigationCommitted.bind(this)),
-      helper.addEventListener(this._session, 'Page.navigationStarted', this._onNavigationStarted.bind(this)),
+      helper.addEventListener(this._session, 'Page.navigationStarted', event => this._onNavigationStarted(event.frameId)),
       helper.addEventListener(this._session, 'Page.sameDocumentNavigation', this._onSameDocumentNavigation.bind(this)),
       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.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)),
@@ -116,7 +117,15 @@ export class FFPage implements PageDelegate {
     }
   }
 
-  _onNavigationStarted() {
+  _onLinkClicked(phase: 'before' | 'after') {
+    if (phase === 'before')
+      this._page._frameManager.frameWillPotentiallyRequestNavigation();
+    else
+      this._page._frameManager.frameDidPotentiallyRequestNavigation();
+  }
+
+  _onNavigationStarted(frameId: string) {
+    this._page._frameManager.frameRequestedNavigation(frameId);
   }
 
   _onNavigationAborted(params: Protocol.Page.navigationAbortedPayload) {

src/frames.ts

@@ -59,7 +59,7 @@ export class FrameManager {
   private _mainFrame: Frame;
   readonly _lifecycleWatchers = new Set<() => void>();
   readonly _consoleMessageTags = new Map<string, ConsoleTagHandler>();
-  private _navigationRequestCollectors = new Set<Set<string>>();
+  private _pendingNavigationBarriers = new Set<PendingNavigationBarrier>();
 
   constructor(page: Page) {
     this._page = page;
@@ -108,34 +108,39 @@ export class FrameManager {
     }
   }
 
-  async waitForNavigationsCreatedBy<T>(action: () => Promise<T>): Promise<T> {
+  async waitForNavigationsCreatedBy<T>(action: () => Promise<T>, options?: WaitForNavigationOptions): Promise<T> {
-    const frameIds = new Set<string>();
+    const barrier = new PendingNavigationBarrier(options);
-    this._navigationRequestCollectors.add(frameIds);
+    this._pendingNavigationBarriers.add(barrier);
     try {
       const result = await action();
-      if (!frameIds.size)
+      await barrier.waitFor();
-        return result;
+      // Resolve in the next task, after all waitForNavigations.
-      const frames = Array.from(frameIds.values()).map(frameId => this._frames.get(frameId));
-      await Promise.all(frames.map(frame => frame!.waitForNavigation({ waitUntil: []}))).catch(e => {});
       await new Promise(platform.makeWaitForNextTask());
       return result;
     } finally {
-      this._navigationRequestCollectors.delete(frameIds);
+      this._pendingNavigationBarriers.delete(barrier);
     }
   }
 
-  frameRequestedNavigation(frameId: string) {
+  frameWillPotentiallyRequestNavigation() {
-    for (const frameIds of this._navigationRequestCollectors)
+    for (const barrier of this._pendingNavigationBarriers)
-      frameIds.add(frameId);
+      barrier.retain();
   }
 
-  _cancelFrameRequestedNavigation(frameId: string) {
+  frameDidPotentiallyRequestNavigation() {
-    for (const frameIds of this._navigationRequestCollectors)
+    for (const barrier of this._pendingNavigationBarriers)
-      frameIds.delete(frameId);
+      barrier.release();
+  }
+
+  frameRequestedNavigation(frameId: string) {
+    const frame = this._frames.get(frameId);
+    if (!frame)
+      return;
+    for (const barrier of this._pendingNavigationBarriers)
+      barrier.addFrame(frame);
   }
 
   frameCommittedNewDocumentNavigation(frameId: string, url: string, name: string, documentId: string, initial: boolean) {
-    this._cancelFrameRequestedNavigation(frameId);
     const frame = this._frames.get(frameId)!;
     for (const child of frame.childFrames())
       this._removeFramesRecursively(child);
@@ -150,7 +155,6 @@ export class FrameManager {
   }
 
   frameCommittedSameDocumentNavigation(frameId: string, url: string) {
-    this._cancelFrameRequestedNavigation(frameId);
     const frame = this._frames.get(frameId);
     if (!frame)
       return;
@@ -235,7 +239,6 @@ export class FrameManager {
     if (request._documentId && frame) {
       const isCurrentDocument = frame._lastDocumentId === request._documentId;
       if (!isCurrentDocument) {
-        this._cancelFrameRequestedNavigation(frame._id);
         let errorText = request.failure()!.errorText;
         if (canceled)
           errorText += '; maybe frame was detached?';
@@ -248,13 +251,11 @@ export class FrameManager {
   }
 
   provisionalLoadFailed(frame: Frame, documentId: string, error: string) {
-    this._cancelFrameRequestedNavigation(frame._id);
     for (const watcher of frame._documentWatchers)
       watcher(documentId, new Error(error));
   }
 
   private _removeFramesRecursively(frame: Frame) {
-    this._cancelFrameRequestedNavigation(frame._id);
     for (const child of frame.childFrames())
       this._removeFramesRecursively(child);
     frame._onDetached();
@@ -1105,3 +1106,42 @@ function selectorToString(selector: string, visibility: types.Visibility): strin
   }
   return `${label}${selector}`;
 }
+
+class PendingNavigationBarrier {
+  private _frameIds = new Map<string, number>();
+  private _waitOptions: WaitForNavigationOptions | undefined;
+  private _protectCount = 0;
+  private _promise: Promise<void>;
+  private _promiseCallback = () => {};
+
+  constructor(options?: WaitForNavigationOptions) {
+    this._waitOptions = options;
+    this._promise = new Promise(f => this._promiseCallback = f);
+    this.retain();
+  }
+
+  waitFor(): Promise<void> {
+    this.release();
+    return this._promise;
+  }
+
+  async addFrame(frame: Frame) {
+    this.retain();
+    await frame.waitForNavigation(this._waitOptions).catch(e => {});
+    this.release();
+  }
+
+  retain() {
+    ++this._protectCount;
+  }
+
+  release() {
+    --this._protectCount;
+    this._maybeResolve();
+  }
+
+  private async _maybeResolve() {
+    if (!this._protectCount && !this._frameIds.size)
+      this._promiseCallback();
+  }
+}

src/platform.ts

@@ -277,7 +277,9 @@ export function fetchUrl(url: string): Promise<string> {
 
 // See https://joel.tools/microtasks/
 export function makeWaitForNextTask() {
-  assert(isNode, 'Waitng for the next task is only supported in nodejs');
+  if (!isNode)
+    return (func: () => void) => setTimeout(func, 0);
+
   if (parseInt(process.versions.node, 10) >= 11)
     return setImmediate;
 

test/browsercontext.spec.js

@@ -344,7 +344,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF
       expect(error.message).toBe('Function "baz" has been already registered in one of the pages');
       await context.close();
     });
-    it.skip(FFOX)('should be callable from-inside addInitScript', async({browser, server}) => {
+    it.fail(FFOX)('should be callable from-inside addInitScript', async({browser, server}) => {
       const context = await browser.newContext();
       let args = [];
       await context.exposeFunction('woof', function(arg) {

test/navigation.spec.js

@@ -561,7 +561,7 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
           return page.setContent(`<script src='networkidle.js'></script>`, { waitUntil: 'networkidle2' });
         }, true);
       });
-      it.skip(FFOX)('should wait for networkidle0 in setContent with request from previous navigation', async({page, server}) => {
+      it.fail(FFOX)('should wait for networkidle0 in setContent with request from previous navigation', async({page, server}) => {
         // TODO: in Firefox window.stop() does not cancel outstanding requests, and we also lack 'init' lifecycle,
         // therefore we don't clear inflight requests at the right time.
         await page.goto(server.EMPTY_PAGE);
@@ -571,7 +571,7 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
           return page.setContent(`<script src='networkidle.js'></script>`, { waitUntil: 'networkidle0' });
         }, true);
       });
-      it.skip(FFOX)('should wait for networkidle2 in setContent with request from previous navigation', async({page, server}) => {
+      it.fail(FFOX)('should wait for networkidle2 in setContent with request from previous navigation', async({page, server}) => {
         // TODO: in Firefox window.stop() does not cancel outstanding requests, and we also lack 'init' lifecycle,
         // therefore we don't clear inflight requests at the right time.
         await page.goto(server.EMPTY_PAGE);
@@ -800,7 +800,7 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
     });
   });
 
-  describe.fail(FFOX)('Page.automaticWaiting', () => {
+  describe('Page.automaticWaiting', () => {
     it('clicking anchor should await navigation', async({page, server}) => {
       const messages = [];
       server.setRoute('/empty.html', async (req, res) => {
@@ -869,7 +869,7 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
       ]);
       expect(messages.join('|')).toBe('route|waitForNavigation|click');
     });
-    it('assigning to location should await navigation', async({page, server}) => {
+    it('assigning location should await navigation', async({page, server}) => {
       const messages = [];
       server.setRoute('/empty.html', async (req, res) => {
         messages.push('route');
@@ -881,12 +881,32 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
       ]);
       expect(messages.join('|')).toBe('route|waitForNavigation|evaluate');
     });
-    it('should await navigating specified target', async({page, server, httpsServer}) => {
+    it.fail(CHROMIUM)('assigning location twice should await navigation', async({page, server}) => {
       const messages = [];
-      server.setRoute('/empty.html', async (req, res) => {
+      server.setRoute('/empty.html?cancel', async (req, res) => { messages.push('routecancel'); res.end('done'); });
-        messages.push('route');
+      server.setRoute('/empty.html?override', async (req, res) => { messages.push('routeoverride'); res.end('done'); });
-        res.end('done');
+      await Promise.all([
-      });
+        page.evaluate(`
+          window.location.href = "${server.EMPTY_PAGE}?cancel";
+          window.location.href = "${server.EMPTY_PAGE}?override";
+        `).then(() => messages.push('evaluate')),
+      ]);
+      expect(messages.join('|')).toBe('routeoverride|evaluate');
+    });
+    it('evaluating reload should await navigation', async({page, server}) => {
+      const messages = [];
+      await page.goto(server.EMPTY_PAGE);
+      server.setRoute('/empty.html', async (req, res) => { messages.push('route'); res.end('done'); });
+
+      await Promise.all([
+        page.evaluate(`window.location.reload()`).then(() => messages.push('evaluate')),
+        page.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation')),
+      ]);
+      expect(messages.join('|')).toBe('route|waitForNavigation|evaluate');
+    });
+    it('should await navigating specified target', async({page, server}) => {
+      const messages = [];
+      server.setRoute('/empty.html', async (req, res) => { messages.push('route'); res.end('done'); });
 
       await page.setContent(`
         <a href="${server.EMPTY_PAGE}" target=target>empty.html</a>
@@ -902,25 +922,39 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
     });
   });
 
-  describe('Page.automaticWaiting should not hang', () => {
+  describe('Page.automaticWaiting should not hang when', () => {
-    it('should not throw when clicking on links which do not commit navigation', async({page, server, httpsServer}) => {
+    it('clicking on links which do not commit navigation', async({page, server, httpsServer}) => {
       await page.goto(server.EMPTY_PAGE);
       await page.setContent(`<a href='${httpsServer.EMPTY_PAGE}'>foobar</a>`);
       await page.click('a');
     });
-    it('should not throw when clicking on download link', async({page, server, httpsServer}) => {
+    it('clicking on download link', async({page, server, httpsServer}) => {
       await page.setContent(`<a href="${server.PREFIX}/wasm/table2.wasm" download=true>table2.wasm</a>`);
       await page.click('a');
     });
-    it('should not hang on window.stop', async({page, server, httpsServer}) => {
+    it('window.stop async', async({page, server, httpsServer}) => {
       server.setRoute('/empty.html', async (req, res) => {});
-      await Promise.all([
+      await page.evaluate((url) => {
-        page.waitForNavigation().catch(e => {}),
-        page.evaluate((url) => {
           window.location.href = url;
           setTimeout(() => window.stop(), 100);
-        }, server.EMPTY_PAGE)
+        }, server.EMPTY_PAGE);
-      ]);
+    });
+    it.fail(WEBKIT)('window.stop sync', async({page, server, httpsServer}) => {
+      server.setRoute('/empty.html', async (req, res) => {});
+      await page.evaluate((url) => {
+          window.location.href = url;
+          window.stop();
+        }, server.EMPTY_PAGE);
+    });
+    it('assigning location to about:blank', async({page, server}) => {
+      await page.goto(server.EMPTY_PAGE);
+      await page.evaluate(`window.location.href = "about:blank";`);
+    });
+    it('assigning location to about:blank after non-about:blank', async({page, server}) => {
+      server.setRoute('/empty.html', async (req, res) => {});
+      await page.evaluate(`
+          window.location.href = "${server.EMPTY_PAGE}";
+          window.location.href = "about:blank";`);
     });
   });