core(driver): add waitForFCP timeout (#7356)

GoogleChrome · Mar 5, 2019 · 8db87bf · 8db87bf
1 parent 9ba7199
commit 8db87bf
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 9 deletions.
diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
@@ -1208,6 +1208,7 @@ Object {
     "extraHeaders": null,
     "gatherMode": false,
     "locale": "en-US",
+    "maxWaitForFcp": 15000,
     "maxWaitForLoad": 45000,
     "onlyAudits": null,
     "onlyCategories": null,
@@ -1336,6 +1337,7 @@ Object {
     "extraHeaders": null,
     "gatherMode": false,
     "locale": "en-US",
+    "maxWaitForFcp": 15000,
     "maxWaitForLoad": 45000,
     "onlyAudits": Array [
       "metrics",

diff --git a/lighthouse-cli/test/fixtures/byte-efficiency/gzip.html b/lighthouse-cli/test/fixtures/byte-efficiency/gzip.html
@@ -13,5 +13,6 @@
 <script src="script.js"></script>
 </head>
 <body>
+  GZIP FTW!
 </body>
 </html>
diff --git a/lighthouse-core/config/constants.js b/lighthouse-core/config/constants.js
@@ -43,6 +43,7 @@ const throttling = {
 /** @type {LH.Config.Settings} */
 const defaultSettings = {
   output: 'json',
+  maxWaitForFcp: 15 * 1000,
   maxWaitForLoad: 45 * 1000,
   throttlingMethod: 'simulate',
   throttling: throttling.mobileSlow4G,

diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
@@ -562,20 +562,25 @@ class Driver {
 
   /**
    * Returns a promise that resolve when a frame has a FCP.
+   * @param {number} maxWaitForFCPMs
    * @return {{promise: Promise<void>, cancel: function(): void}}
    */
-  _waitForFCP() {
+  _waitForFCP(maxWaitForFCPMs) {
     /** @type {(() => void)} */
     let cancel = () => {
       throw new Error('_waitForFCP.cancel() called before it was defined');
     };
 
-    const promise = new Promise(resolve => {
+    const promise = new Promise((resolve, reject) => {
+      const maxWaitTimeout = setTimeout(() => {
+        reject(new LHError(LHError.errors.NO_FCP));
+      }, maxWaitForFCPMs);
+
       /** @param {LH.Crdp.Page.LifecycleEventEvent} e */
       const lifecycleListener = e => {
         if (e.name === 'firstContentfulPaint') {
-          cancel();
           resolve();
+          cancel();
         }
       };
 
@@ -586,6 +591,8 @@ class Driver {
         if (canceled) return;
         canceled = true;
         this.off('Page.lifecycleEvent', lifecycleListener);
+        maxWaitTimeout && clearTimeout(maxWaitTimeout);
+        reject(new Error('Wait for FCP canceled'));
       };
     });
 
@@ -857,17 +864,17 @@ class Driver {
    * @param {number} networkQuietThresholdMs
    * @param {number} cpuQuietThresholdMs
    * @param {number} maxWaitForLoadedMs
-   * @param {boolean} shouldWaitForFCP
+   * @param {number=} maxWaitForFCPMs
    * @return {Promise<void>}
    * @private
    */
   async _waitForFullyLoaded(pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs,
-      maxWaitForLoadedMs, shouldWaitForFCP) {
+      maxWaitForLoadedMs, maxWaitForFCPMs) {
     /** @type {NodeJS.Timer|undefined} */
     let maxTimeoutHandle;
 
     // Listener for onload. Resolves on first FCP event.
-    const waitForFCP = shouldWaitForFCP ? this._waitForFCP() : this._waitForNothing();
+    const waitForFCP = maxWaitForFCPMs ? this._waitForFCP(maxWaitForFCPMs) : this._waitForNothing();
     // Listener for onload. Resolves pauseAfterLoadMs ms after load.
     const waitForLoadEvent = this._waitForLoadEvent(pauseAfterLoadMs);
     // Network listener. Resolves when the network has been idle for networkQuietThresholdMs.
@@ -895,6 +902,11 @@ class Driver {
       return function() {
         log.verbose('Driver', 'loadEventFired and network considered idle');
       };
+    }).catch(err => {
+      // Throw the error in the cleanupFn so we still cleanup all our handlers.
+      return function() {
+        throw err;
+      };
     });
 
     // Last resort timeout. Resolves maxWaitForLoadedMs ms from now on
@@ -1038,7 +1050,7 @@ class Driver {
     await this.sendCommand('Page.enable');
     await this.sendCommand('Page.setLifecycleEventsEnabled', {enabled: true});
     await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
-    // No timeout needed for Page.navigate. See #6413.
+    // No timeout needed for Page.navigate. See https://github.com/GoogleChrome/lighthouse/pull/6413.
     const waitforPageNavigateCmd = this._innerSendCommand('Page.navigate', {url});
 
     if (waitForNavigated) {
@@ -1047,19 +1059,22 @@ class Driver {
       const passConfig = /** @type {Partial<LH.Config.Pass>} */ (passContext.passConfig || {});
       let {pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs} = passConfig;
       let maxWaitMs = passContext.settings && passContext.settings.maxWaitForLoad;
+      let maxFCPMs = passContext.settings && passContext.settings.maxWaitForFcp;
 
       /* eslint-disable max-len */
       if (typeof pauseAfterLoadMs !== 'number') pauseAfterLoadMs = DEFAULT_PAUSE_AFTER_LOAD;
       if (typeof networkQuietThresholdMs !== 'number') networkQuietThresholdMs = DEFAULT_NETWORK_QUIET_THRESHOLD;
       if (typeof cpuQuietThresholdMs !== 'number') cpuQuietThresholdMs = DEFAULT_CPU_QUIET_THRESHOLD;
       if (typeof maxWaitMs !== 'number') maxWaitMs = constants.defaultSettings.maxWaitForLoad;
+      if (typeof maxFCPMs !== 'number') maxFCPMs = constants.defaultSettings.maxWaitForFcp;
       /* eslint-enable max-len */
 
+      if (!waitForFCP) maxFCPMs = undefined;
       await this._waitForFullyLoaded(pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs,
-          maxWaitMs, waitForFCP);
+          maxWaitMs, maxFCPMs);
     }
 
-    // Bring `Page.navigate` errors back into the promise chain. See #6739.
+    // Bring `Page.navigate` errors back into the promise chain. See https://github.com/GoogleChrome/lighthouse/pull/6739.
     await waitforPageNavigateCmd;
 
     return this._endNetworkStatusMonitoring();

diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js
@@ -663,6 +663,19 @@ describe('.gotoURL', () => {
       expect(driver._waitForCPUIdle.getMockCancelFn()).toHaveBeenCalled();
     });
 
+    it('should cleanup listeners even when waits reject', async () => {
+      driver._waitForLoadEvent = createMockWaitForFn();
+
+      const loadPromise = makePromiseInspectable(driver.gotoURL(url, {waitForLoad: true}));
+
+      driver._waitForLoadEvent.mockReject();
+      await flushAllTimersAndMicrotasks();
+      expect(loadPromise).toBeDone('Did not reject load promise when load rejected');
+      await expect(loadPromise).rejects.toBeTruthy();
+      // Make sure we still cleaned up our listeners
+      expect(driver._waitForLoadEvent.getMockCancelFn()).toHaveBeenCalled();
+    });
+
     it('does not reject when page is secure', async () => {
       const secureSecurityState = {
         explanations: [],
@@ -740,6 +753,58 @@ describe('.gotoURL', () => {
   });
 });
 
+describe('._waitForFCP', () => {
+  it('should not resolve until FCP fires', async () => {
+    driver.on = driver.once = createMockOnceFn();
+
+    const waitPromise = makePromiseInspectable(driver._waitForFCP(60 * 1000).promise);
+    const listener = driver.on.findListener('Page.lifecycleEvent');
+
+    await flushAllTimersAndMicrotasks();
+    expect(waitPromise).not.toBeDone('Resolved without FCP');
+
+    listener({name: 'domContentLoaded'});
+    await flushAllTimersAndMicrotasks();
+    expect(waitPromise).not.toBeDone('Resolved on wrong event');
+
+    listener({name: 'firstContentfulPaint'});
+    await flushAllTimersAndMicrotasks();
+    expect(waitPromise).toBeDone('Did not resolve with FCP');
+    await waitPromise;
+  });
+
+  it('should timeout', async () => {
+    driver.on = driver.once = createMockOnceFn();
+
+    const waitPromise = makePromiseInspectable(driver._waitForFCP(5000).promise);
+
+    await flushAllTimersAndMicrotasks();
+    expect(waitPromise).not.toBeDone('Resolved before timeout');
+
+    jest.advanceTimersByTime(5001);
+    await flushAllTimersAndMicrotasks();
+    expect(waitPromise).toBeDone('Did not resolve after timeout');
+    await expect(waitPromise).rejects.toMatchObject({code: 'NO_FCP'});
+  });
+
+  it('should be cancellable', async () => {
+    driver.on = driver.once = createMockOnceFn();
+    driver.off = jest.fn();
+
+    const {promise: rawPromise, cancel} = driver._waitForFCP(5000);
+    const waitPromise = makePromiseInspectable(rawPromise);
+
+    await flushAllTimersAndMicrotasks();
+    expect(waitPromise).not.toBeDone('Resolved before timeout');
+
+    cancel();
+    await flushAllTimersAndMicrotasks();
+    expect(waitPromise).toBeDone('Did not cancel promise');
+    expect(driver.off).toHaveBeenCalled();
+    await expect(waitPromise).rejects.toMatchObject({message: 'Wait for FCP canceled'});
+  });
+});
+
 describe('.assertNoSameOriginServiceWorkerClients', () => {
   beforeEach(() => {
     connectionStub.sendCommand = createMockSendCommandFn()

diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
@@ -3164,6 +3164,7 @@
     "output": [
       "json"
     ],
+    "maxWaitForFcp": 15000,
     "maxWaitForLoad": 45000,
     "throttlingMethod": "devtools",
     "throttling": {

diff --git a/types/externs.d.ts b/types/externs.d.ts
@@ -93,6 +93,8 @@ declare global {
       output?: OutputMode|OutputMode[];
       /** The locale to use for the output. */
       locale?: Locale;
+      /** The maximum amount of time to wait for a page content render, in ms. If no content is rendered within this limit, the run is aborted with an error. */
+      maxWaitForFcp?: number;
       /** The maximum amount of time to wait for a page to load, in ms. */
       maxWaitForLoad?: number;
       /** List of URL patterns to block. */