From ac5f9f408ed9ab746e7038b7548b3e33860e6b32 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 1 Mar 2019 11:20:48 -0600 Subject: [PATCH 1/6] core(driver): add waitForFCP timeout --- lighthouse-core/config/constants.js | 1 + lighthouse-core/gather/driver.js | 33 +++++++++---- lighthouse-core/test/gather/driver-test.js | 52 +++++++++++++++++++++ lighthouse-core/test/results/sample_v2.json | 3 +- types/externs.d.ts | 2 + 5 files changed, 81 insertions(+), 10 deletions(-) diff --git a/lighthouse-core/config/constants.js b/lighthouse-core/config/constants.js index 3c4f21fb28e1..43d8f568086b 100644 --- a/lighthouse-core/config/constants.js +++ b/lighthouse-core/config/constants.js @@ -42,6 +42,7 @@ const throttling = { /** @type {LH.Config.Settings} */ const defaultSettings = { output: 'json', + maxWaitForFcp: 30 * 1000, maxWaitForLoad: 45 * 1000, throttlingMethod: 'simulate', throttling: throttling.mobileSlow4G, diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index bcb475ced925..8ae280c8dbd8 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -548,20 +548,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(); } }; @@ -572,6 +577,8 @@ class Driver { if (canceled) return; canceled = true; this.off('Page.lifecycleEvent', lifecycleListener); + maxWaitTimeout && clearTimeout(maxWaitTimeout); + reject(new Error('Wait for FCP canceled')); }; }); @@ -843,17 +850,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. @@ -881,6 +888,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 @@ -1018,7 +1030,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) { @@ -1027,19 +1039,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 5befd18dd8b7..62dcc8478df3 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -739,6 +739,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 166f4359a401..67fc7cfefb00 100644 --- a/lighthouse-core/test/results/sample_v2.json +++ b/lighthouse-core/test/results/sample_v2.json @@ -3158,6 +3158,7 @@ "output": [ "json" ], + "maxWaitForFcp": 30000, "maxWaitForLoad": 45000, "throttlingMethod": "devtools", "throttling": { @@ -5615,4 +5616,4 @@ ] } } -} \ No newline at end of file +} diff --git a/types/externs.d.ts b/types/externs.d.ts index 455587626b90..3f2a32885ea4 100644 --- a/types/externs.d.ts +++ b/types/externs.d.ts @@ -93,6 +93,8 @@ declare global { output?: OutputMode|OutputMode[]; // The locale setting locale?: Locale; + // The maximum amount of time to wait for a page render content 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 From 7905fe5f39680ace2dd011904cbaf6f7359f7a5d Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 1 Mar 2019 11:26:20 -0600 Subject: [PATCH 2/6] extra test for fix --- lighthouse-core/test/gather/driver-test.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 62dcc8478df3..5a4297fd9a7a 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -662,6 +662,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: [], From a571bf035e3e2ef917a2f60ac41eb25c5d958ae0 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 1 Mar 2019 11:28:23 -0600 Subject: [PATCH 3/6] snapshots --- lighthouse-cli/test/cli/__snapshots__/index-test.js.snap | 2 ++ lighthouse-core/gather/driver.js | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap index 7e6e5f7b5e87..1314cb97e171 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": 30000, "maxWaitForLoad": 45000, "onlyAudits": null, "onlyCategories": null, @@ -1336,6 +1337,7 @@ Object { "extraHeaders": null, "gatherMode": false, "locale": "en-US", + "maxWaitForFcp": 30000, "maxWaitForLoad": 45000, "onlyAudits": Array [ "metrics", diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 8ae280c8dbd8..1f624a5c7ec2 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -890,7 +890,7 @@ class Driver { }; }).catch(err => { // Throw the error in the cleanupFn so we still cleanup all our handlers. - return function () { + return function() { throw err; }; }); From e69e3f1a0d1bccef6e2c8dee3b28f564832ab18c Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Sat, 2 Mar 2019 10:36:53 -0600 Subject: [PATCH 4/6] Update types/externs.d.ts Co-Authored-By: patrickhulce --- types/externs.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/externs.d.ts b/types/externs.d.ts index 3f2a32885ea4..80aa2cdff40e 100644 --- a/types/externs.d.ts +++ b/types/externs.d.ts @@ -93,7 +93,7 @@ declare global { output?: OutputMode|OutputMode[]; // The locale setting locale?: Locale; - // The maximum amount of time to wait for a page render content in ms, if no content is rendered within this limit the run is aborted with an error + // 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; From 83c043e96a655c3ad8971e802205d96b4b268468 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Sat, 2 Mar 2019 10:38:36 -0600 Subject: [PATCH 5/6] lower to 15s --- lighthouse-core/config/constants.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/config/constants.js b/lighthouse-core/config/constants.js index 43d8f568086b..27fef2c8233b 100644 --- a/lighthouse-core/config/constants.js +++ b/lighthouse-core/config/constants.js @@ -42,7 +42,7 @@ const throttling = { /** @type {LH.Config.Settings} */ const defaultSettings = { output: 'json', - maxWaitForFcp: 30 * 1000, + maxWaitForFcp: 15 * 1000, maxWaitForLoad: 45 * 1000, throttlingMethod: 'simulate', throttling: throttling.mobileSlow4G, From 9c708e41ffdc247be368ed67e0f3680bf231c003 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Sat, 2 Mar 2019 11:22:18 -0600 Subject: [PATCH 6/6] gzip fix --- lighthouse-cli/test/cli/__snapshots__/index-test.js.snap | 4 ++-- lighthouse-cli/test/fixtures/byte-efficiency/gzip.html | 1 + lighthouse-core/test/results/sample_v2.json | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap index 1314cb97e171..111adf17a0d3 100644 --- a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap +++ b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap @@ -1208,7 +1208,7 @@ Object { "extraHeaders": null, "gatherMode": false, "locale": "en-US", - "maxWaitForFcp": 30000, + "maxWaitForFcp": 15000, "maxWaitForLoad": 45000, "onlyAudits": null, "onlyCategories": null, @@ -1337,7 +1337,7 @@ Object { "extraHeaders": null, "gatherMode": false, "locale": "en-US", - "maxWaitForFcp": 30000, + "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/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json index 67fc7cfefb00..43b7b927915b 100644 --- a/lighthouse-core/test/results/sample_v2.json +++ b/lighthouse-core/test/results/sample_v2.json @@ -3158,7 +3158,7 @@ "output": [ "json" ], - "maxWaitForFcp": 30000, + "maxWaitForFcp": 15000, "maxWaitForLoad": 45000, "throttlingMethod": "devtools", "throttling": { @@ -5616,4 +5616,4 @@ ] } } -} +} \ No newline at end of file