diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap index 7e6e5f7b5e87..111adf17a0d3 100644 --- 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 index d6ba29c443ab..033b3020c5c8 100644 --- a/lighthouse-cli/test/fixtures/byte-efficiency/gzip.html +++ b/lighthouse-cli/test/fixtures/byte-efficiency/gzip.html @@ -13,5 +13,6 @@ + GZIP FTW! diff --git a/lighthouse-core/config/constants.js b/lighthouse-core/config/constants.js index 85c36cd3158d..2a050b12b4b4 100644 --- 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 index b3e593d29c86..e93e9d016f47 100644 --- 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, 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} * @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} */ (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 index b69ff4ddc3cc..89ec69f6193f 100644 --- 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 index fb0b83114dc0..1b41a8b4c27d 100644 --- 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 index 44810a44d5c3..5ab8e6483dad 100644 --- 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. */