From 4c658fb405d0b12364d1dd6c89dd8e07cf89c01e Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 19 Nov 2018 15:41:54 -0800 Subject: [PATCH 01/24] INSECURE_DOCUMENT_REQUEST errors still return lhr (#6595) --- lighthouse-core/gather/driver.js | 63 ++++++++++++------- lighthouse-core/gather/gather-runner.js | 44 ++++++------- lighthouse-core/lib/file-namer.js | 4 +- .../html/renderer/report-ui-features.js | 1 + lighthouse-core/test/gather/driver-test.js | 36 +++++++++++ lighthouse-core/test/gather/fake-driver.js | 7 +-- .../test/gather/gather-runner-test.js | 40 ------------ lighthouse-viewer/app/src/github-api.js | 1 + 8 files changed, 105 insertions(+), 91 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 7444607e62b7..0ef985619915 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -497,6 +497,35 @@ class Driver { }); } + /** + * Rejects if the security state is insecure. + * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState + * @returns {LHError|undefined} + */ + checkForSecurityIssues({securityState, explanations}) { + if (securityState === 'insecure') { + const errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; + const insecureDescriptions = explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + errorDef.message += ` ${insecureDescriptions.join(' ')}`; + return new LHError(errorDef); + } + } + + waitForSecurityIssuesCheck() { + return new Promise((resolve, reject) => { + this.once('Security.securityStateChanged', state => { + const err = this.checkForSecurityIssues(state); + if (err) { + reject(err); + } else { + resolve(); + } + }); + }); + } + /** * Returns a promise that resolve when a frame has been navigated. * Used for detecting that our about:blank reset has been completed. @@ -883,7 +912,19 @@ class Driver { this.sendCommand('Page.enable'); this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); this.setNextProtocolTimeout(30 * 1000); - this.sendCommand('Page.navigate', {url}); + this.sendCommand('Page.navigate', {url}).catch(err => { + // can timeout on security issue + if (err.code !== 'PROTOCOL_TIMEOUT') { + throw err; + } + }); + + try { + await this.sendCommand('Security.enable'); + await this.waitForSecurityIssuesCheck(); + } finally { + this.sendCommand('Security.disable'); + } if (waitForNavigated) { await this._waitForFrameNavigated(); @@ -948,26 +989,6 @@ class Driver { return result.body; } - async listenForSecurityStateChanges() { - this.on('Security.securityStateChanged', state => { - this._lastSecurityState = state; - }); - await this.sendCommand('Security.enable'); - } - - /** - * @return {LH.Crdp.Security.SecurityStateChangedEvent} - */ - getSecurityState() { - if (!this._lastSecurityState) { - // happens if 'listenForSecurityStateChanges' is not called, - // or if some assumptions about the Security domain are wrong - throw new Error('Expected a security state.'); - } - - return this._lastSecurityState; - } - /** * @param {string} name The name of API whose permission you wish to query * @return {Promise} The state of permissions, resolved in a promise. diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 50726d04313d..e6a59e53ec91 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -109,7 +109,6 @@ class GatherRunner { await driver.cacheNatives(); await driver.registerPerformanceObserver(); await driver.dismissJavaScriptDialogs(); - await driver.listenForSecurityStateChanges(); if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); log.timeEnd(status); } @@ -173,22 +172,6 @@ class GatherRunner { } } - /** - * Throws an error if the security state is insecure. - * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState - * @throws {LHError} - */ - static assertNoSecurityIssues({securityState, explanations}) { - if (securityState === 'insecure') { - const errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; - const insecureDescriptions = explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description); - errorDef.message += ` ${insecureDescriptions.join(' ')}`; - throw new LHError(errorDef); - } - } - /** * Calls beforePass() on gatherers before tracing * has started and before navigation to the target page. @@ -256,8 +239,13 @@ class GatherRunner { if (recordTrace) await driver.beginTrace(settings); // Navigate. - await GatherRunner.loadPage(driver, passContext); - log.timeEnd(status); + try { + await GatherRunner.loadPage(driver, passContext); + } catch (err) { + throw err; + } finally { + log.timeEnd(status); + } const pStatus = {msg: `Running pass methods`, id: `lh:gather:pass`}; log.time(pStatus, 'verbose'); @@ -311,8 +299,6 @@ class GatherRunner { const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); log.timeEnd(status); - this.assertNoSecurityIssues(driver.getSecurityState()); - let pageLoadError = GatherRunner.getPageLoadError(passContext.url, networkRecords); // If the driver was offline, a page load error is expected, so do not save it. if (!driver.online) pageLoadError = undefined; @@ -466,7 +452,21 @@ class GatherRunner { await GatherRunner.loadBlank(driver, passConfig.blankPage); } await GatherRunner.beforePass(passContext, gathererResults); - await GatherRunner.pass(passContext, gathererResults); + try { + await GatherRunner.pass(passContext, gathererResults); + } catch (err) { + if (err.code === LHError.errors.INSECURE_DOCUMENT_REQUEST.code) { + log.error('GatherRunner.pass', err.message); + for (const gathererResult of Object.values(gathererResults)) { + if (gathererResult) { + gathererResult.push(err); + } + } + break; + } + + throw err; + } const passData = await GatherRunner.afterPass(passContext, gathererResults); // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. diff --git a/lighthouse-core/lib/file-namer.js b/lighthouse-core/lib/file-namer.js index 6ed093a10bb9..2527a0f47da0 100644 --- a/lighthouse-core/lib/file-namer.js +++ b/lighthouse-core/lib/file-namer.js @@ -16,11 +16,11 @@ * Generate a filenamePrefix of hostname_YYYY-MM-DD_HH-MM-SS * Date/time uses the local timezone, however Node has unreliable ICU * support, so we must construct a YYYY-MM-DD date format manually. :/ - * @param {{finalUrl: string, fetchTime: string}} lhr + * @param {{requestedUrl: string, finalUrl: string, fetchTime: string}} lhr * @return {string} */ function getFilenamePrefix(lhr) { - const hostname = new (getUrlConstructor())(lhr.finalUrl).hostname; + const hostname = new (getUrlConstructor())(lhr.finalUrl || lhr.requestedUrl).hostname; const date = (lhr.fetchTime && new Date(lhr.fetchTime)) || new Date(); const timeStr = date.toLocaleTimeString('en-US', {hour12: false}); diff --git a/lighthouse-core/report/html/renderer/report-ui-features.js b/lighthouse-core/report/html/renderer/report-ui-features.js index a9d294dd08bc..0dd09ce9d0be 100644 --- a/lighthouse-core/report/html/renderer/report-ui-features.js +++ b/lighthouse-core/report/html/renderer/report-ui-features.js @@ -462,6 +462,7 @@ class ReportUIFeatures { */ _saveFile(blob) { const filename = getFilenamePrefix({ + requestedUrl: this.json.requestedUrl, finalUrl: this.json.finalUrl, fetchTime: this.json.fetchTime, }); diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 419ddd76005f..e37302d91a1f 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -219,6 +219,7 @@ describe('Browser Driver', () => { } const replayConnection = new ReplayConnection(); const driver = new Driver(replayConnection); + driver.waitForSecurityIssuesCheck = () => Promise.resolve(); // Redirect in log will go through const startUrl = 'http://en.wikipedia.org/'; @@ -507,4 +508,39 @@ describe('Multiple tab check', () => { }); }); }); + + describe('.checkForSecurityIssues', () => { + it('returns nothing when page is secure', () => { + const secureSecurityState = { + securityState: 'secure', + }; + const err = driverStub.checkForSecurityIssues(secureSecurityState); + assert.equal(err, undefined); + }); + + it('returns an error when page is insecure', () => { + const insecureSecurityState = { + explanations: [ + { + description: 'reason 1.', + securityState: 'insecure', + }, + { + description: 'blah.', + securityState: 'info', + }, + { + description: 'reason 2.', + securityState: 'insecure', + }, + ], + securityState: 'insecure', + }; + const err = driverStub.checkForSecurityIssues(insecureSecurityState); + assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); + assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST'); + /* eslint-disable-next-line max-len */ + assert.equal(err.friendlyMessage, 'The URL you have provided does not have valid security credentials. reason 1. reason 2.'); + }); + }); }); diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js index 75ce1e2f3eca..958d894fec49 100644 --- a/lighthouse-core/test/gather/fake-driver.js +++ b/lighthouse-core/test/gather/fake-driver.js @@ -81,14 +81,9 @@ const fakeDriver = { setExtraHTTPHeaders() { return Promise.resolve(); }, - listenForSecurityStateChanges() { + waitForSecurityIssuesCheck() { return Promise.resolve(); }, - getSecurityState() { - return Promise.resolve({ - securityState: 'secure', - }); - }, }; module.exports = fakeDriver; diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index c302d5db3be1..0409aeef8ade 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -322,7 +322,6 @@ describe('GatherRunner', function() { clearDataForOrigin: createCheck('calledClearStorage'), blockUrlPatterns: asyncFunc, setExtraHTTPHeaders: asyncFunc, - listenForSecurityStateChanges: asyncFunc, }; return GatherRunner.setupDriver(driver, {settings: {}}).then(_ => { @@ -382,7 +381,6 @@ describe('GatherRunner', function() { clearDataForOrigin: createCheck('calledClearStorage'), blockUrlPatterns: asyncFunc, setExtraHTTPHeaders: asyncFunc, - listenForSecurityStateChanges: asyncFunc, }; return GatherRunner.setupDriver(driver, { @@ -693,44 +691,6 @@ describe('GatherRunner', function() { }); }); - describe('#assertNoSecurityIssues', () => { - it('succeeds when page is secure', () => { - const secureSecurityState = { - securityState: 'secure', - }; - GatherRunner.assertNoSecurityIssues(secureSecurityState); - }); - - it('fails when page is insecure', () => { - const insecureSecurityState = { - explanations: [ - { - description: 'reason 1.', - securityState: 'insecure', - }, - { - description: 'blah.', - securityState: 'info', - }, - { - description: 'reason 2.', - securityState: 'insecure', - }, - ], - securityState: 'insecure', - }; - try { - GatherRunner.assertNoSecurityIssues(insecureSecurityState); - assert.fail('expected INSECURE_DOCUMENT_REQUEST LHError'); - } catch (err) { - assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); - assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST'); - /* eslint-disable-next-line max-len */ - assert.equal(err.friendlyMessage, 'The URL you have provided does not have valid security credentials. reason 1. reason 2.'); - } - }); - }); - describe('artifact collection', () => { // Make sure our gatherers never execute in parallel it('runs gatherer lifecycle methods strictly in sequence', async () => { diff --git a/lighthouse-viewer/app/src/github-api.js b/lighthouse-viewer/app/src/github-api.js index ee6f4bec47ef..adc7d5b1fe20 100644 --- a/lighthouse-viewer/app/src/github-api.js +++ b/lighthouse-viewer/app/src/github-api.js @@ -38,6 +38,7 @@ class GithubApi { return this._auth.getAccessToken() .then(accessToken => { const filename = getFilenamePrefix({ + requestedUrl: jsonFile.requestedUrl, finalUrl: jsonFile.finalUrl, fetchTime: jsonFile.fetchTime, }); From eb7f5a3d7029138dcb372b958258a4be404ecb63 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 19 Nov 2018 15:49:17 -0800 Subject: [PATCH 02/24] fix comment. remove dead code --- lighthouse-core/gather/driver.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 0ef985619915..a1a240b556ce 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -81,13 +81,6 @@ class Driver { this._eventEmitter.emit(event.method, event.params); }); - /** - * Used for monitoring network status events during gotoURL. - * @type {?LH.Crdp.Security.SecurityStateChangedEvent} - * @private - */ - this._lastSecurityState = null; - /** * @type {number} * @private @@ -498,7 +491,7 @@ class Driver { } /** - * Rejects if the security state is insecure. + * Returns LHError if the security state is insecure. * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState * @returns {LHError|undefined} */ From 3577e0cc8a1bf9c955563f299cfbbb4311519e64 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 19 Dec 2018 15:22:02 -0800 Subject: [PATCH 03/24] move security check to wait for page load --- lighthouse-core/gather/driver.js | 68 ++++++++++++------- lighthouse-core/lib/file-namer.js | 4 +- .../html/renderer/report-ui-features.js | 1 - lighthouse-core/test/gather/driver-test.js | 38 ++++++++--- lighthouse-core/test/gather/fake-driver.js | 3 - lighthouse-viewer/app/src/github-api.js | 1 - 6 files changed, 71 insertions(+), 44 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index e2d4c07c60cb..8a1d2d4cffb2 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -506,32 +506,50 @@ class Driver { } /** - * Returns LHError if the security state is insecure. - * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState - * @returns {LHError|undefined} + * Rejects if the security state is insecure. + * @return {{promise: Promise, cancel: function(): void}} + * @private */ - checkForSecurityIssues({securityState, explanations}) { - if (securityState === 'insecure') { - const insecureDescriptions = explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description); - return new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { - securityMessages: insecureDescriptions.join(' '), - }); - } - } + _waitForSecurityCheck() { + /** @type {(() => void)} */ + let cancel = () => { + throw new Error('_waitForSecurityCheck.cancel() called before it was defined'); + }; - waitForSecurityIssuesCheck() { - return new Promise((resolve, reject) => { - this.once('Security.securityStateChanged', state => { - const err = this.checkForSecurityIssues(state); - if (err) { + const promise = new Promise(async (resolve, reject) => { + /** + * @param {LH.Crdp.Security.SecurityStateChangedEvent} event + */ + const securityStateChangedListener = ({securityState, explanations}) => { + this.sendCommand('Security.disable'); + if (securityState === 'insecure') { + const insecureDescriptions = explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { + securityMessages: insecureDescriptions.join(' '), + }); reject(err); } else { resolve(); } - }); + }; + + // wait for Security.enable to resolve, so we skip whatever state change + // events are in the pipeline + await this.sendCommand('Security.enable'); + this.once('Security.securityStateChanged', securityStateChangedListener); + + cancel = () => { + this.off('Security.securityStateChanged', securityStateChangedListener); + this.sendCommand('Security.disable'); + }; }); + + return { + promise, + cancel, + }; } /** @@ -763,6 +781,9 @@ class Driver { /** @type {NodeJS.Timer|undefined} */ let maxTimeoutHandle; + + // Listener for security state change. Rejects if security issue is found. + const waitForSecurityCheck = this._waitForSecurityCheck(); // Listener for onload. Resolves pauseAfterLoadMs ms after load. const waitForLoadEvent = this._waitForLoadEvent(pauseAfterLoadMs); // Network listener. Resolves when the network has been idle for networkQuietThresholdMs. @@ -774,6 +795,7 @@ class Driver { // Wait for both load promises. Resolves on cleanup function the clears load // timeout timer. const loadPromise = Promise.all([ + waitForSecurityCheck.promise, waitForLoadEvent.promise, waitForNetworkIdle.promise, ]).then(() => { @@ -793,6 +815,7 @@ class Driver { }).then(_ => { return async () => { log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...'); + waitForSecurityCheck.cancel(); waitForLoadEvent.cancel(); waitForNetworkIdle.cancel(); waitForCPUIdle && waitForCPUIdle.cancel(); @@ -917,13 +940,6 @@ class Driver { // No timeout needed for Page.navigate. See #6413. const waitforPageNavigateCmd = this._innerSendCommand('Page.navigate', {url}); - try { - await this.sendCommand('Security.enable'); - await this.waitForSecurityIssuesCheck(); - } finally { - this.sendCommand('Security.disable'); - } - if (waitForNavigated) { await this._waitForFrameNavigated(); } else if (waitForLoad) { diff --git a/lighthouse-core/lib/file-namer.js b/lighthouse-core/lib/file-namer.js index 2527a0f47da0..6ed093a10bb9 100644 --- a/lighthouse-core/lib/file-namer.js +++ b/lighthouse-core/lib/file-namer.js @@ -16,11 +16,11 @@ * Generate a filenamePrefix of hostname_YYYY-MM-DD_HH-MM-SS * Date/time uses the local timezone, however Node has unreliable ICU * support, so we must construct a YYYY-MM-DD date format manually. :/ - * @param {{requestedUrl: string, finalUrl: string, fetchTime: string}} lhr + * @param {{finalUrl: string, fetchTime: string}} lhr * @return {string} */ function getFilenamePrefix(lhr) { - const hostname = new (getUrlConstructor())(lhr.finalUrl || lhr.requestedUrl).hostname; + const hostname = new (getUrlConstructor())(lhr.finalUrl).hostname; const date = (lhr.fetchTime && new Date(lhr.fetchTime)) || new Date(); const timeStr = date.toLocaleTimeString('en-US', {hour12: false}); diff --git a/lighthouse-core/report/html/renderer/report-ui-features.js b/lighthouse-core/report/html/renderer/report-ui-features.js index 3b76119643c7..1eef996144b5 100644 --- a/lighthouse-core/report/html/renderer/report-ui-features.js +++ b/lighthouse-core/report/html/renderer/report-ui-features.js @@ -461,7 +461,6 @@ class ReportUIFeatures { */ _saveFile(blob) { const filename = getFilenamePrefix({ - requestedUrl: this.json.requestedUrl, finalUrl: this.json.finalUrl, fetchTime: this.json.fetchTime, }); diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 63a15edbd336..41e3f7978ab4 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -99,6 +99,8 @@ connection.sendCommand = function(command, params) { case 'Tracing.start': case 'ServiceWorker.enable': case 'ServiceWorker.disable': + case 'Security.enable': + case 'Security.disable': case 'Network.setExtraHTTPHeaders': case 'Network.emulateNetworkConditions': case 'Emulation.setCPUThrottlingRate': @@ -221,7 +223,12 @@ describe('Browser Driver', () => { } const replayConnection = new ReplayConnection(); const driver = new Driver(replayConnection); - driver.waitForSecurityIssuesCheck = () => Promise.resolve(); + driver._waitForSecurityCheck = () => { + return { + promise: Promise.resolve(), + cancel: () => {}, + }; + }; // Redirect in log will go through const startUrl = 'http://en.wikipedia.org/'; @@ -537,16 +544,18 @@ describe('Multiple tab check', () => { }); }); - describe('.checkForSecurityIssues', () => { - it('returns nothing when page is secure', () => { + describe('._waitForSecurityCheck', () => { + it('returns nothing when page is secure', async () => { const secureSecurityState = { securityState: 'secure', }; - const err = driverStub.checkForSecurityIssues(secureSecurityState); - assert.equal(err, undefined); + driverStub.once = createOnceStub({ + 'Security.securityStateChanged': secureSecurityState, + }); + await driverStub._waitForSecurityCheck().promise; }); - it('returns an error when page is insecure', () => { + it('returns an error when page is insecure', async () => { const insecureSecurityState = { explanations: [ { @@ -564,11 +573,18 @@ describe('Multiple tab check', () => { ], securityState: 'insecure', }; - const err = driverStub.checkForSecurityIssues(insecureSecurityState); - assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); - assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST'); - /* eslint-disable-next-line max-len */ - expect(err.friendlyMessage).toBeDisplayString('The URL you have provided does not have valid security credentials. reason 1. reason 2.'); + driverStub.once = createOnceStub({ + 'Security.securityStateChanged': insecureSecurityState, + }); + try { + await driverStub._waitForSecurityCheck().promise; + assert.fail('_waitForSecurityCheck should have rejected'); + } catch (err) { + assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); + assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST'); + /* eslint-disable-next-line max-len */ + expect(err.friendlyMessage).toBeDisplayString('The URL you have provided does not have valid security credentials. reason 1. reason 2.'); + } }); }); }); diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js index 958d894fec49..f90ed3b0f032 100644 --- a/lighthouse-core/test/gather/fake-driver.js +++ b/lighthouse-core/test/gather/fake-driver.js @@ -81,9 +81,6 @@ const fakeDriver = { setExtraHTTPHeaders() { return Promise.resolve(); }, - waitForSecurityIssuesCheck() { - return Promise.resolve(); - }, }; module.exports = fakeDriver; diff --git a/lighthouse-viewer/app/src/github-api.js b/lighthouse-viewer/app/src/github-api.js index adc7d5b1fe20..ee6f4bec47ef 100644 --- a/lighthouse-viewer/app/src/github-api.js +++ b/lighthouse-viewer/app/src/github-api.js @@ -38,7 +38,6 @@ class GithubApi { return this._auth.getAccessToken() .then(accessToken => { const filename = getFilenamePrefix({ - requestedUrl: jsonFile.requestedUrl, finalUrl: jsonFile.finalUrl, fetchTime: jsonFile.fetchTime, }); From 2c0b25179619a54e566344b8db159dbb6efe5ec5 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 19 Dec 2018 15:23:36 -0800 Subject: [PATCH 04/24] remove finally for timing --- lighthouse-core/gather/gather-runner.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 65c53966413b..e8bf68edf1e1 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -238,11 +238,8 @@ class GatherRunner { if (recordTrace) await driver.beginTrace(settings); // Navigate. - try { - await GatherRunner.loadPage(driver, passContext); - } finally { - log.timeEnd(status); - } + await GatherRunner.loadPage(driver, passContext); + log.timeEnd(status); const pStatus = {msg: `Running pass methods`, id: `lh:gather:pass`}; log.time(pStatus, 'verbose'); From e22d9ca33a4e728e3171b219d4f7cc0cd2fc924d Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 19 Dec 2018 16:26:15 -0800 Subject: [PATCH 05/24] set cancel --- lighthouse-core/gather/driver.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 8a1d2d4cffb2..03c3827d8c9a 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -535,6 +535,9 @@ class Driver { } }; + // nothing to cancel at this point + cancel = () => {}; + // wait for Security.enable to resolve, so we skip whatever state change // events are in the pipeline await this.sendCommand('Security.enable'); From 9e54f1b5caa5a570686caedd0087979cf9c5cc2a Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 19 Dec 2018 16:58:28 -0800 Subject: [PATCH 06/24] update test name --- lighthouse-core/test/gather/driver-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 41e3f7978ab4..484ff1907a84 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -545,7 +545,7 @@ describe('Multiple tab check', () => { }); describe('._waitForSecurityCheck', () => { - it('returns nothing when page is secure', async () => { + it('does not reject when page is secure', async () => { const secureSecurityState = { securityState: 'secure', }; @@ -555,7 +555,7 @@ describe('Multiple tab check', () => { await driverStub._waitForSecurityCheck().promise; }); - it('returns an error when page is insecure', async () => { + it('rejects when page is insecure', async () => { const insecureSecurityState = { explanations: [ { From 059d95e36d489f46c462850858217d391a8cb36f Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 19 Dec 2018 17:13:40 -0800 Subject: [PATCH 07/24] tweak security logic --- lighthouse-core/gather/driver.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 03c3827d8c9a..75f7cbbd5bfa 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -521,8 +521,8 @@ class Driver { * @param {LH.Crdp.Security.SecurityStateChangedEvent} event */ const securityStateChangedListener = ({securityState, explanations}) => { - this.sendCommand('Security.disable'); if (securityState === 'insecure') { + cancel(); const insecureDescriptions = explanations .filter(exp => exp.securityState === 'insecure') .map(exp => exp.description); @@ -530,18 +530,14 @@ class Driver { securityMessages: insecureDescriptions.join(' '), }); reject(err); - } else { + } else if (securityState !== 'neutral') { + cancel(); resolve(); } }; - // nothing to cancel at this point - cancel = () => {}; - - // wait for Security.enable to resolve, so we skip whatever state change - // events are in the pipeline - await this.sendCommand('Security.enable'); - this.once('Security.securityStateChanged', securityStateChangedListener); + this.on('Security.securityStateChanged', securityStateChangedListener); + this.sendCommand('Security.enable'); cancel = () => { this.off('Security.securityStateChanged', securityStateChangedListener); From 51e4f05f144b31d1e5e5fdd2fb2fa4a4b506da50 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 19 Dec 2018 17:33:29 -0800 Subject: [PATCH 08/24] fix test --- lighthouse-core/test/gather/driver-test.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 484ff1907a84..ce88e394f93b 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -22,7 +22,10 @@ const redirectDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog. const MAX_WAIT_FOR_PROTOCOL = 20; function createOnceStub(events) { - return (eventName, cb) => { + return async (eventName, cb) => { + // wait a tick b/c real events never fire immediately + await Promise.resolve(); + if (events[eventName]) { return cb(events[eventName]); } @@ -549,7 +552,7 @@ describe('Multiple tab check', () => { const secureSecurityState = { securityState: 'secure', }; - driverStub.once = createOnceStub({ + driverStub.on = createOnceStub({ 'Security.securityStateChanged': secureSecurityState, }); await driverStub._waitForSecurityCheck().promise; @@ -573,7 +576,7 @@ describe('Multiple tab check', () => { ], securityState: 'insecure', }; - driverStub.once = createOnceStub({ + driverStub.on = createOnceStub({ 'Security.securityStateChanged': insecureSecurityState, }); try { From 1ce9144fa711edc180a0675bb900d5084af6814f Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 19 Dec 2018 19:37:22 -0800 Subject: [PATCH 09/24] add smoke test, other stuff --- lighthouse-cli/run.js | 9 +++++++++ lighthouse-cli/test/smokehouse/error-expectations.js | 6 ++++++ lighthouse-cli/test/smokehouse/smokehouse.js | 9 ++++++++- lighthouse-core/gather/driver.js | 7 +++++-- 4 files changed, 28 insertions(+), 3 deletions(-) diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js index a602edc7b4f3..ea5f72ff3347 100644 --- a/lighthouse-cli/run.js +++ b/lighthouse-cli/run.js @@ -23,6 +23,7 @@ const opn = require('opn'); const _RUNTIME_ERROR_CODE = 1; const _PROTOCOL_TIMEOUT_EXIT_CODE = 67; const _PAGE_HUNG_EXIT_CODE = 68; +const _INSECRE_DOCUMENT_REQUEST_EXIT_CODE = 69; /** * exported for testing @@ -77,6 +78,12 @@ function showPageHungError(err) { process.exit(_PAGE_HUNG_EXIT_CODE); } +/** @param {LH.LighthouseError} err */ +function showInsecureDocumentRequestError(err) { + console.error('Insecure document request:', err.friendlyMessage); + process.exit(_INSECRE_DOCUMENT_REQUEST_EXIT_CODE); +} + /** * @param {LH.LighthouseError} err */ @@ -98,6 +105,8 @@ function handleError(err) { showProtocolTimeoutError(); } else if (err.code === 'PAGE_HUNG') { showPageHungError(err); + } else if (err.code === 'INSECURE_DOCUMENT_REQUEST') { + showInsecureDocumentRequestError(err); } else { showRuntimeError(err); } diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js index 40d05780607b..8d1a59290f96 100644 --- a/lighthouse-cli/test/smokehouse/error-expectations.js +++ b/lighthouse-cli/test/smokehouse/error-expectations.js @@ -15,4 +15,10 @@ module.exports = [ errorCode: 'PAGE_HUNG', audits: {}, }, + { + requestedUrl: 'https://expired.badssl.com', + finalUrl: 'https://expired.badssl.com', + errorCode: 'INSECURE_DOCUMENT_REQUEST', + audits: {}, + }, ]; diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js index c3bf337e9535..ab6ebb27b237 100755 --- a/lighthouse-cli/test/smokehouse/smokehouse.js +++ b/lighthouse-cli/test/smokehouse/smokehouse.js @@ -16,6 +16,7 @@ const log = require('lighthouse-logger'); const PROTOCOL_TIMEOUT_EXIT_CODE = 67; const PAGE_HUNG_EXIT_CODE = 68; +const INSECRE_DOCUMENT_REQUEST_EXIT_CODE = 69; const RETRIES = 3; const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/; @@ -88,7 +89,9 @@ function runLighthouse(url, configPath, isDebug) { if (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE) { console.error(`Lighthouse debugger connection timed out ${RETRIES} times. Giving up.`); process.exit(1); - } else if (runResults.status !== 0 && runResults.status !== PAGE_HUNG_EXIT_CODE) { + } else if (runResults.status !== 0 + && runResults.status !== PAGE_HUNG_EXIT_CODE + && runResults.status !== INSECRE_DOCUMENT_REQUEST_EXIT_CODE) { console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`); console.error(runResults.stderr); process.exit(runResults.status); @@ -103,6 +106,10 @@ function runLighthouse(url, configPath, isDebug) { return {requestedUrl: url, finalUrl: url, errorCode: 'PAGE_HUNG', audits: {}}; } + if (runResults.status === INSECRE_DOCUMENT_REQUEST_EXIT_CODE) { + return {requestedUrl: url, finalUrl: url, errorCode: 'INSECURE_DOCUMENT_REQUEST', audits: {}}; + } + const lhr = fs.readFileSync(outputPath, 'utf8'); if (isDebug) { console.log('LHR output available at: ', outputPath); diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 75f7cbbd5bfa..04738a7d48a8 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -537,11 +537,11 @@ class Driver { }; this.on('Security.securityStateChanged', securityStateChangedListener); - this.sendCommand('Security.enable'); + this.sendCommand('Security.enable').catch(() => {}); cancel = () => { this.off('Security.securityStateChanged', securityStateChangedListener); - this.sendCommand('Security.disable'); + this.sendCommand('Security.disable').catch(() => {}); }; }); @@ -762,6 +762,7 @@ class Driver { /** * Returns a promise that resolves when: * - All of the following conditions have been met: + * - page has no security issues * - pauseAfterLoadMs milliseconds have passed since the load event. * - networkQuietThresholdMs milliseconds have passed since the last network request that exceeded * 2 inflight requests (network-2-quiet has been reached). @@ -782,6 +783,8 @@ class Driver { // Listener for security state change. Rejects if security issue is found. + // We can expect the security state to always change because this function + // is only used to move about:blank (neutral) -> the target url (something not neutral). const waitForSecurityCheck = this._waitForSecurityCheck(); // Listener for onload. Resolves pauseAfterLoadMs ms after load. const waitForLoadEvent = this._waitForLoadEvent(pauseAfterLoadMs); From 4bd59e48262e9bad0c8e1f72f83a2fd8860bf0dd Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 20 Dec 2018 10:50:59 -0800 Subject: [PATCH 10/24] fix offline case for security check --- lighthouse-core/gather/driver.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 04738a7d48a8..17c1c5d95e8f 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -521,6 +521,11 @@ class Driver { * @param {LH.Crdp.Security.SecurityStateChangedEvent} event */ const securityStateChangedListener = ({securityState, explanations}) => { + // ignore neutral until there is something meaningful to derive security from + if (securityState === 'neutral' && !explanations.length) { + return; + } + if (securityState === 'insecure') { cancel(); const insecureDescriptions = explanations @@ -530,7 +535,7 @@ class Driver { securityMessages: insecureDescriptions.join(' '), }); reject(err); - } else if (securityState !== 'neutral') { + } else { cancel(); resolve(); } From 53e00a7de136ee4d154a7ec633e63653c5fbf96f Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 20 Dec 2018 11:20:02 -0800 Subject: [PATCH 11/24] skip security check when offline --- lighthouse-core/gather/driver.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 17c1c5d95e8f..55c5553c4b9c 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -521,7 +521,7 @@ class Driver { * @param {LH.Crdp.Security.SecurityStateChangedEvent} event */ const securityStateChangedListener = ({securityState, explanations}) => { - // ignore neutral until there is something meaningful to derive security from + // ignore until there is something meaningful to derive security from if (securityState === 'neutral' && !explanations.length) { return; } @@ -790,7 +790,11 @@ class Driver { // Listener for security state change. Rejects if security issue is found. // We can expect the security state to always change because this function // is only used to move about:blank (neutral) -> the target url (something not neutral). - const waitForSecurityCheck = this._waitForSecurityCheck(); + // Noop if offline. + const waitForSecurityCheck = this.online ? this._waitForSecurityCheck() : { + promise: Promise.resolve(), + cancel: () => {}, + }; // Listener for onload. Resolves pauseAfterLoadMs ms after load. const waitForLoadEvent = this._waitForLoadEvent(pauseAfterLoadMs); // Network listener. Resolves when the network has been idle for networkQuietThresholdMs. From 1b6e67843e380fd10e5f1c2e4cf786dcc1f75da1 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 20 Dec 2018 11:59:44 -0800 Subject: [PATCH 12/24] skip security check if localhost --- lighthouse-core/gather/driver.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 55c5553c4b9c..ac0327b76e8f 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -506,7 +506,11 @@ class Driver { } /** - * Rejects if the security state is insecure. + * Listener that resolves or rejects on the first interesting security state change. + * If the first change is secure, we resolve. + * Otherwise, we reject. + * We can expect the security state to always change because this function + * is only used to move about:blank (neutral) -> the target url (something not neutral). * @return {{promise: Promise, cancel: function(): void}} * @private */ @@ -786,12 +790,9 @@ class Driver { /** @type {NodeJS.Timer|undefined} */ let maxTimeoutHandle; - - // Listener for security state change. Rejects if security issue is found. - // We can expect the security state to always change because this function - // is only used to move about:blank (neutral) -> the target url (something not neutral). - // Noop if offline. - const waitForSecurityCheck = this.online ? this._waitForSecurityCheck() : { + // Noop if offline or localhost + const isLocalhost = new URL(this._monitoredUrl || '').hostname !== 'localhost'; + const waitForSecurityCheck = this.online && isLocalhost ? this._waitForSecurityCheck() : { promise: Promise.resolve(), cancel: () => {}, }; From 4807cf06897ca313a5a9aa35851847c90cfbb270 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 8 Jan 2019 12:39:18 -0800 Subject: [PATCH 13/24] only do security check if online and https --- lighthouse-core/gather/driver.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index ac0327b76e8f..006f40476967 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -790,9 +790,9 @@ class Driver { /** @type {NodeJS.Timer|undefined} */ let maxTimeoutHandle; - // Noop if offline or localhost - const isLocalhost = new URL(this._monitoredUrl || '').hostname !== 'localhost'; - const waitForSecurityCheck = this.online && isLocalhost ? this._waitForSecurityCheck() : { + // Noop if offline or not https + const isHttps = new URL(this._monitoredUrl || '').protocol === 'https:'; + const waitForSecurityCheck = this.online && isHttps ? this._waitForSecurityCheck() : { promise: Promise.resolve(), cancel: () => {}, }; From 2d505e8b330322f61fc8433e609b35081ece25c1 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 8 Jan 2019 12:49:09 -0800 Subject: [PATCH 14/24] consider all secure schemes --- lighthouse-core/gather/driver.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 006f40476967..01ad648bab6e 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -32,6 +32,8 @@ const DEFAULT_CPU_QUIET_THRESHOLD = 0; // Controls how long to wait for a response after sending a DevTools protocol command. const DEFAULT_PROTOCOL_TIMEOUT = 30000; +const SECURE_SCHEMES = ['data', 'https', 'wss', 'blob', 'chrome', 'chrome-extension', 'about']; + /** * @typedef {LH.Protocol.StrictEventEmitter} CrdpEventEmitter */ @@ -791,8 +793,10 @@ class Driver { let maxTimeoutHandle; // Noop if offline or not https - const isHttps = new URL(this._monitoredUrl || '').protocol === 'https:'; - const waitForSecurityCheck = this.online && isHttps ? this._waitForSecurityCheck() : { + // https: => https + const protocol = new URL(this._monitoredUrl || '').protocol.replace(/:$/, ''); + const isSecureProtocol = SECURE_SCHEMES.includes(protocol); + const waitForSecurityCheck = this.online && isSecureProtocol ? this._waitForSecurityCheck() : { promise: Promise.resolve(), cancel: () => {}, }; From 1e1fc8afbeacabd9f10af2d2919cb910ae064c94 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 8 Jan 2019 15:16:59 -0800 Subject: [PATCH 15/24] move security check to promise race. only rejects now. --- lighthouse-cli/test/smokehouse/smokehouse.js | 6 +- lighthouse-core/gather/driver.js | 106 +++++++------------ lighthouse-core/test/gather/driver-test.js | 45 ++++++-- 3 files changed, 73 insertions(+), 84 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js index 3aca4c7923dc..b6eb8a971416 100755 --- a/lighthouse-cli/test/smokehouse/smokehouse.js +++ b/lighthouse-cli/test/smokehouse/smokehouse.js @@ -16,7 +16,7 @@ const log = require('lighthouse-logger'); const PROTOCOL_TIMEOUT_EXIT_CODE = 67; const PAGE_HUNG_EXIT_CODE = 68; -const INSECRE_DOCUMENT_REQUEST_EXIT_CODE = 69; +const INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69; const RETRIES = 3; const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/; @@ -91,7 +91,7 @@ function runLighthouse(url, configPath, isDebug) { process.exit(1); } else if (runResults.status !== 0 && runResults.status !== PAGE_HUNG_EXIT_CODE - && runResults.status !== INSECRE_DOCUMENT_REQUEST_EXIT_CODE) { + && runResults.status !== INSECURE_DOCUMENT_REQUEST_EXIT_CODE) { console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`); console.error(runResults.stderr); process.exit(runResults.status); @@ -106,7 +106,7 @@ function runLighthouse(url, configPath, isDebug) { return {requestedUrl: url, finalUrl: url, errorCode: 'PAGE_HUNG', audits: {}}; } - if (runResults.status === INSECRE_DOCUMENT_REQUEST_EXIT_CODE) { + if (runResults.status === INSECURE_DOCUMENT_REQUEST_EXIT_CODE) { return {requestedUrl: url, finalUrl: url, errorCode: 'INSECURE_DOCUMENT_REQUEST', audits: {}}; } diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 01ad648bab6e..bfbe24a36d62 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -32,8 +32,6 @@ const DEFAULT_CPU_QUIET_THRESHOLD = 0; // Controls how long to wait for a response after sending a DevTools protocol command. const DEFAULT_PROTOCOL_TIMEOUT = 30000; -const SECURE_SCHEMES = ['data', 'https', 'wss', 'blob', 'chrome', 'chrome-extension', 'about']; - /** * @typedef {LH.Protocol.StrictEventEmitter} CrdpEventEmitter */ @@ -507,61 +505,6 @@ class Driver { }); } - /** - * Listener that resolves or rejects on the first interesting security state change. - * If the first change is secure, we resolve. - * Otherwise, we reject. - * We can expect the security state to always change because this function - * is only used to move about:blank (neutral) -> the target url (something not neutral). - * @return {{promise: Promise, cancel: function(): void}} - * @private - */ - _waitForSecurityCheck() { - /** @type {(() => void)} */ - let cancel = () => { - throw new Error('_waitForSecurityCheck.cancel() called before it was defined'); - }; - - const promise = new Promise(async (resolve, reject) => { - /** - * @param {LH.Crdp.Security.SecurityStateChangedEvent} event - */ - const securityStateChangedListener = ({securityState, explanations}) => { - // ignore until there is something meaningful to derive security from - if (securityState === 'neutral' && !explanations.length) { - return; - } - - if (securityState === 'insecure') { - cancel(); - const insecureDescriptions = explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description); - const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { - securityMessages: insecureDescriptions.join(' '), - }); - reject(err); - } else { - cancel(); - resolve(); - } - }; - - this.on('Security.securityStateChanged', securityStateChangedListener); - this.sendCommand('Security.enable').catch(() => {}); - - cancel = () => { - this.off('Security.securityStateChanged', securityStateChangedListener); - this.sendCommand('Security.disable').catch(() => {}); - }; - }); - - return { - promise, - cancel, - }; - } - /** * Returns a promise that resolve when a frame has been navigated. * Used for detecting that our about:blank reset has been completed. @@ -792,14 +735,6 @@ class Driver { /** @type {NodeJS.Timer|undefined} */ let maxTimeoutHandle; - // Noop if offline or not https - // https: => https - const protocol = new URL(this._monitoredUrl || '').protocol.replace(/:$/, ''); - const isSecureProtocol = SECURE_SCHEMES.includes(protocol); - const waitForSecurityCheck = this.online && isSecureProtocol ? this._waitForSecurityCheck() : { - promise: Promise.resolve(), - cancel: () => {}, - }; // Listener for onload. Resolves pauseAfterLoadMs ms after load. const waitForLoadEvent = this._waitForLoadEvent(pauseAfterLoadMs); // Network listener. Resolves when the network has been idle for networkQuietThresholdMs. @@ -807,11 +742,43 @@ class Driver { // CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle. /** @type {{promise: Promise, cancel: function(): void}|null} */ let waitForCPUIdle = null; + const cancelAll = () => { + waitForLoadEvent.cancel(); + waitForNetworkIdle.cancel(); + waitForCPUIdle && waitForCPUIdle.cancel(); + }; + + // Promise that only rejects when ann insecure security state is encountered + let securityCheckCleanup = () => {}; + const securityCheckPromise = new Promise((_, reject) => { + /** + * @param {LH.Crdp.Security.SecurityStateChangedEvent} event + */ + const securityStateChangedListener = ({securityState, explanations}) => { + if (securityState === 'insecure') { + maxTimeoutHandle && clearTimeout(maxTimeoutHandle); + securityCheckCleanup(); + cancelAll(); + const insecureDescriptions = explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { + securityMessages: insecureDescriptions.join(' '), + }); + reject(err); + } + }; + securityCheckCleanup = () => { + this.off('Security.securityStateChanged', securityStateChangedListener); + this.sendCommand('Security.disable').catch(() => {}); + }; + this.on('Security.securityStateChanged', securityStateChangedListener); + this.sendCommand('Security.enable').catch(() => {}); + }); // Wait for both load promises. Resolves on cleanup function the clears load // timeout timer. const loadPromise = Promise.all([ - waitForSecurityCheck.promise, waitForLoadEvent.promise, waitForNetworkIdle.promise, ]).then(() => { @@ -831,10 +798,7 @@ class Driver { }).then(_ => { return async () => { log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...'); - waitForSecurityCheck.cancel(); - waitForLoadEvent.cancel(); - waitForNetworkIdle.cancel(); - waitForCPUIdle && waitForCPUIdle.cancel(); + cancelAll(); if (await this.isPageHung()) { log.warn('Driver', 'Page appears to be hung, killing JavaScript...'); @@ -847,9 +811,11 @@ class Driver { // Wait for load or timeout and run the cleanup function the winner returns. const cleanupFn = await Promise.race([ + securityCheckPromise, loadPromise, maxTimeoutPromise, ]); + securityCheckCleanup(); await cleanupFn(); } diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index ce88e394f93b..3297be45783f 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -98,6 +98,7 @@ connection.sendCommand = function(command, params) { case 'Network.getResponseBody': return new Promise(res => setTimeout(res, MAX_WAIT_FOR_PROTOCOL + 20)); case 'Page.enable': + case 'Page.navigate': case 'Network.enable': case 'Tracing.start': case 'ServiceWorker.enable': @@ -107,6 +108,7 @@ connection.sendCommand = function(command, params) { case 'Network.setExtraHTTPHeaders': case 'Network.emulateNetworkConditions': case 'Emulation.setCPUThrottlingRate': + case 'Emulation.setScriptExecutionDisabled': return Promise.resolve({}); case 'Tracing.end': return Promise.reject(new Error('tracing not started')); @@ -226,12 +228,6 @@ describe('Browser Driver', () => { } const replayConnection = new ReplayConnection(); const driver = new Driver(replayConnection); - driver._waitForSecurityCheck = () => { - return { - promise: Promise.resolve(), - cancel: () => {}, - }; - }; // Redirect in log will go through const startUrl = 'http://en.wikipedia.org/'; @@ -547,15 +543,31 @@ describe('Multiple tab check', () => { }); }); - describe('._waitForSecurityCheck', () => { + describe('Security check', () => { it('does not reject when page is secure', async () => { const secureSecurityState = { + explanations: [], securityState: 'secure', }; - driverStub.on = createOnceStub({ + driverStub.on = driverStub.once = createOnceStub({ 'Security.securityStateChanged': secureSecurityState, + 'Page.loadEventFired': {}, + 'Page.domContentEventFired': {}, }); - await driverStub._waitForSecurityCheck().promise; + + const startUrl = 'https://www.example.com'; + const loadOptions = { + waitForLoad: true, + passContext: { + passConfig: { + networkQuietThresholdMs: 1, + }, + settings: { + maxWaitForLoad: 1, + }, + }, + }; + await driverStub.gotoURL(startUrl, loadOptions); }); it('rejects when page is insecure', async () => { @@ -579,9 +591,20 @@ describe('Multiple tab check', () => { driverStub.on = createOnceStub({ 'Security.securityStateChanged': insecureSecurityState, }); + + const startUrl = 'https://www.example.com'; + const loadOptions = { + waitForLoad: true, + passContext: { + passConfig: { + networkQuietThresholdMs: 1, + }, + }, + }; + try { - await driverStub._waitForSecurityCheck().promise; - assert.fail('_waitForSecurityCheck should have rejected'); + await driverStub.gotoURL(startUrl, loadOptions); + assert.fail('security check should have rejected'); } catch (err) { assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST'); From 0a0e8e09b0cc99e73d4bb90c799863c02b4f9bcf Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 8 Jan 2019 15:19:31 -0800 Subject: [PATCH 16/24] fix typo --- lighthouse-core/gather/driver.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index bfbe24a36d62..ae9670059810 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -748,7 +748,7 @@ class Driver { waitForCPUIdle && waitForCPUIdle.cancel(); }; - // Promise that only rejects when ann insecure security state is encountered + // Promise that only rejects when an insecure security state is encountered let securityCheckCleanup = () => {}; const securityCheckPromise = new Promise((_, reject) => { /** From 3c543263a2bc121aa1ae1852b77fb5bb7d928ee5 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 9 Jan 2019 09:21:56 -0800 Subject: [PATCH 17/24] fix typo, add comment --- lighthouse-cli/run.js | 4 ++-- lighthouse-core/gather/driver.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js index ea5f72ff3347..28081d5e3c1a 100644 --- a/lighthouse-cli/run.js +++ b/lighthouse-cli/run.js @@ -23,7 +23,7 @@ const opn = require('opn'); const _RUNTIME_ERROR_CODE = 1; const _PROTOCOL_TIMEOUT_EXIT_CODE = 67; const _PAGE_HUNG_EXIT_CODE = 68; -const _INSECRE_DOCUMENT_REQUEST_EXIT_CODE = 69; +const _INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69; /** * exported for testing @@ -81,7 +81,7 @@ function showPageHungError(err) { /** @param {LH.LighthouseError} err */ function showInsecureDocumentRequestError(err) { console.error('Insecure document request:', err.friendlyMessage); - process.exit(_INSECRE_DOCUMENT_REQUEST_EXIT_CODE); + process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE); } /** diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index f56bd910d682..873f26edc14c 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -857,7 +857,7 @@ class Driver { // Wait for load or timeout and run the cleanup function the winner returns. const cleanupFn = await Promise.race([ - securityCheckPromise, + securityCheckPromise, // will only ever reject loadPromise, maxTimeoutPromise, ]); From ab0582baa8866851a6ed1a07e20c0948bf1f1ffd Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 9 Jan 2019 10:42:28 -0800 Subject: [PATCH 18/24] make security check cancelable promise again --- lighthouse-core/gather/driver.js | 80 +++++++++++++++++++++----------- 1 file changed, 52 insertions(+), 28 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 873f26edc14c..b5e5c84d6b8a 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -736,6 +736,48 @@ class Driver { }; } + /** + * Return a promise that resolves when an insecure security state is encountered + * and a method to cancel internal listeners. + * @return {{promise: Promise, cancel: function(): void}} + * @private + */ + _waitForSecurityCheck() { + /** @type {(() => void)} */ + let cancel = () => { + throw new Error('_waitForSecurityCheck.cancel() called before it was defined'); + }; + + const promise = new Promise((resolve, reject) => { + /** + * @param {LH.Crdp.Security.SecurityStateChangedEvent} event + */ + const securityStateChangedListener = ({securityState, explanations}) => { + if (securityState === 'insecure') { + cancel(); + const insecureDescriptions = explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { + securityMessages: insecureDescriptions.join(' '), + }); + resolve(err); + } + }; + cancel = () => { + this.off('Security.securityStateChanged', securityStateChangedListener); + this.sendCommand('Security.disable').catch(() => {}); + }; + this.on('Security.securityStateChanged', securityStateChangedListener); + this.sendCommand('Security.enable').catch(() => {}); + }); + + return { + promise, + cancel, + }; + } + /** * Returns whether the page appears to be hung. * @return {Promise} @@ -793,32 +835,13 @@ class Driver { waitForCPUIdle.cancel(); }; - // Promise that only rejects when an insecure security state is encountered - let securityCheckCleanup = () => {}; - const securityCheckPromise = new Promise((_, reject) => { - /** - * @param {LH.Crdp.Security.SecurityStateChangedEvent} event - */ - const securityStateChangedListener = ({securityState, explanations}) => { - if (securityState === 'insecure') { - maxTimeoutHandle && clearTimeout(maxTimeoutHandle); - securityCheckCleanup(); - cancelAll(); - const insecureDescriptions = explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description); - const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { - securityMessages: insecureDescriptions.join(' '), - }); - reject(err); - } - }; - securityCheckCleanup = () => { - this.off('Security.securityStateChanged', securityStateChangedListener); - this.sendCommand('Security.disable').catch(() => {}); + const waitForSecurityCheck = this._waitForSecurityCheck(); + const securityCheckPromise = waitForSecurityCheck.promise.then((err) => { + return function() { + maxTimeoutHandle && clearTimeout(maxTimeoutHandle); + cancelAll(); + throw err; }; - this.on('Security.securityStateChanged', securityStateChangedListener); - this.sendCommand('Security.enable').catch(() => {}); }); // Wait for both load promises. Resolves on cleanup function the clears load @@ -834,6 +857,7 @@ class Driver { return function() { log.verbose('Driver', 'loadEventFired and network considered idle'); maxTimeoutHandle && clearTimeout(maxTimeoutHandle); + waitForSecurityCheck.cancel(); }; }); @@ -844,6 +868,7 @@ class Driver { }).then(_ => { return async () => { log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...'); + waitForSecurityCheck.cancel(); cancelAll(); if (await this.isPageHung()) { @@ -855,13 +880,12 @@ class Driver { }; }); - // Wait for load or timeout and run the cleanup function the winner returns. + // Wait for security issue, load or timeout and run the cleanup function the winner returns. const cleanupFn = await Promise.race([ - securityCheckPromise, // will only ever reject + securityCheckPromise, loadPromise, maxTimeoutPromise, ]); - securityCheckCleanup(); await cleanupFn(); } From f272978c5809efb418472ce70b0c9eb581d0c2f1 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 9 Jan 2019 15:30:00 -0800 Subject: [PATCH 19/24] simplify cancels --- lighthouse-core/gather/driver.js | 38 ++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index b5e5c84d6b8a..21c58f536e92 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -539,14 +539,17 @@ class Driver { /** @param {LH.Crdp.Page.LifecycleEventEvent} e */ const lifecycleListener = e => { if (e.name === 'firstContentfulPaint') { + cancel(); resolve(); - this.off('Page.lifecycleEvent', lifecycleListener); } }; this.on('Page.lifecycleEvent', lifecycleListener); + let canceled = false; cancel = () => { + if (canceled) return; + canceled = true; this.off('Page.lifecycleEvent', lifecycleListener); }; }); @@ -627,7 +630,10 @@ class Driver { networkStatusMonitor.on('network-2-busy', logStatus); this.once('Page.domContentEventFired', domContentLoadedListener); + let canceled = false; cancel = () => { + if (canceled) return; + canceled = true; idleTimeout && clearTimeout(idleTimeout); this.off('Page.domContentEventFired', domContentLoadedListener); networkStatusMonitor.removeListener('network-2-busy', onBusy); @@ -659,7 +665,7 @@ class Driver { /** @type {NodeJS.Timer|undefined} */ let lastTimeout; - let cancelled = false; + const cancelled = false; const checkForQuietExpression = `(${pageFunctions.checkTimeSinceLastLongTaskString})()`; /** @@ -691,7 +697,6 @@ class Driver { const promise = new Promise((resolve, reject) => { checkForQuiet(this, resolve).catch(reject); cancel = () => { - cancelled = true; if (lastTimeout) clearTimeout(lastTimeout); reject(new Error('Wait for CPU idle cancelled')); }; @@ -724,7 +729,10 @@ class Driver { }; this.once('Page.loadEventFired', loadListener); + let canceled = false; cancel = () => { + if (canceled) return; + canceled = true; this.off('Page.loadEventFired', loadListener); loadTimeout && clearTimeout(loadTimeout); }; @@ -764,7 +772,10 @@ class Driver { resolve(err); } }; + let canceled = false; cancel = () => { + if (canceled) return; + canceled = true; this.off('Security.securityStateChanged', securityStateChangedListener); this.sendCommand('Security.disable').catch(() => {}); }; @@ -828,18 +839,10 @@ class Driver { const waitForNetworkIdle = this._waitForNetworkIdle(networkQuietThresholdMs); // CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle. let waitForCPUIdle = this._waitForNothing(); - const cancelAll = () => { - waitForFCP.cancel(); - waitForLoadEvent.cancel(); - waitForNetworkIdle.cancel(); - waitForCPUIdle.cancel(); - }; const waitForSecurityCheck = this._waitForSecurityCheck(); const securityCheckPromise = waitForSecurityCheck.promise.then((err) => { return function() { - maxTimeoutHandle && clearTimeout(maxTimeoutHandle); - cancelAll(); throw err; }; }); @@ -856,8 +859,6 @@ class Driver { }).then(() => { return function() { log.verbose('Driver', 'loadEventFired and network considered idle'); - maxTimeoutHandle && clearTimeout(maxTimeoutHandle); - waitForSecurityCheck.cancel(); }; }); @@ -868,9 +869,6 @@ class Driver { }).then(_ => { return async () => { log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...'); - waitForSecurityCheck.cancel(); - cancelAll(); - if (await this.isPageHung()) { log.warn('Driver', 'Page appears to be hung, killing JavaScript...'); await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: true}); @@ -886,6 +884,14 @@ class Driver { loadPromise, maxTimeoutPromise, ]); + + maxTimeoutHandle && clearTimeout(maxTimeoutHandle); + waitForFCP.cancel(); + waitForLoadEvent.cancel(); + waitForNetworkIdle.cancel(); + waitForCPUIdle.cancel(); + waitForSecurityCheck.cancel(); + await cleanupFn(); } From e6a659a6aab63c10f9407d55cc496aa4c08d39fd Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 9 Jan 2019 16:12:25 -0800 Subject: [PATCH 20/24] fix cancel bug --- lighthouse-core/gather/driver.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 21c58f536e92..fd80edb5015f 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -665,7 +665,7 @@ class Driver { /** @type {NodeJS.Timer|undefined} */ let lastTimeout; - const cancelled = false; + let canceled = false; const checkForQuietExpression = `(${pageFunctions.checkTimeSinceLastLongTaskString})()`; /** @@ -674,9 +674,9 @@ class Driver { * @return {Promise} */ async function checkForQuiet(driver, resolve) { - if (cancelled) return; + if (canceled) return; const timeSinceLongTask = await driver.evaluateAsync(checkForQuietExpression); - if (cancelled) return; + if (canceled) return; if (typeof timeSinceLongTask === 'number') { if (timeSinceLongTask >= waitForCPUQuiet) { @@ -697,8 +697,9 @@ class Driver { const promise = new Promise((resolve, reject) => { checkForQuiet(this, resolve).catch(reject); cancel = () => { + canceled = true; if (lastTimeout) clearTimeout(lastTimeout); - reject(new Error('Wait for CPU idle cancelled')); + reject(new Error('Wait for CPU idle canceled')); }; }); From 271a80a40a6d32556d1baa3e4e350f0ed72e43bf Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 9 Jan 2019 16:17:05 -0800 Subject: [PATCH 21/24] move security error creation --- lighthouse-core/gather/driver.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index fd80edb5015f..17937ff6818e 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -748,7 +748,7 @@ class Driver { /** * Return a promise that resolves when an insecure security state is encountered * and a method to cancel internal listeners. - * @return {{promise: Promise, cancel: function(): void}} + * @return {{promise: Promise, cancel: function(): void}} * @private */ _waitForSecurityCheck() { @@ -764,13 +764,7 @@ class Driver { const securityStateChangedListener = ({securityState, explanations}) => { if (securityState === 'insecure') { cancel(); - const insecureDescriptions = explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description); - const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { - securityMessages: insecureDescriptions.join(' '), - }); - resolve(err); + resolve(explanations); } }; let canceled = false; @@ -842,8 +836,14 @@ class Driver { let waitForCPUIdle = this._waitForNothing(); const waitForSecurityCheck = this._waitForSecurityCheck(); - const securityCheckPromise = waitForSecurityCheck.promise.then((err) => { + const securityCheckPromise = waitForSecurityCheck.promise.then(explanations => { return function() { + const insecureDescriptions = explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { + securityMessages: insecureDescriptions.join(' '), + }); throw err; }; }); From 8bbcbc449c9ea993fd9a35a9c5b2834dcdef030e Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 10 Jan 2019 14:35:54 -0800 Subject: [PATCH 22/24] cr changes --- clients/extension/scripts/popup.js | 2 ++ lighthouse-core/gather/driver.js | 27 +++++++++++----------- lighthouse-core/lib/lh-error.js | 2 +- lighthouse-core/test/gather/driver-test.js | 3 ++- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/clients/extension/scripts/popup.js b/clients/extension/scripts/popup.js index d49741747b07..9676a8f4a807 100644 --- a/clients/extension/scripts/popup.js +++ b/clients/extension/scripts/popup.js @@ -26,6 +26,8 @@ const NON_BUG_ERROR_MESSAGES = { 'are trying to review.', 'Cannot access contents of the page': 'Lighthouse can only audit URLs that start' + ' with http:// or https://.', + 'INSECURE_DOCUMENT_REQUEST': 'The URL you have provided does not have valid' + + ' security credentials.', 'INVALID_URL': 'Lighthouse can only audit URLs that start' + ' with http:// or https://.', }; diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 17937ff6818e..a831effb9e82 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -697,6 +697,7 @@ class Driver { const promise = new Promise((resolve, reject) => { checkForQuiet(this, resolve).catch(reject); cancel = () => { + if (canceled) return; canceled = true; if (lastTimeout) clearTimeout(lastTimeout); reject(new Error('Wait for CPU idle canceled')); @@ -748,13 +749,13 @@ class Driver { /** * Return a promise that resolves when an insecure security state is encountered * and a method to cancel internal listeners. - * @return {{promise: Promise, cancel: function(): void}} + * @return {{promise: Promise, cancel: function(): void}} * @private */ - _waitForSecurityCheck() { + _monitorForInsecureState() { /** @type {(() => void)} */ let cancel = () => { - throw new Error('_waitForSecurityCheck.cancel() called before it was defined'); + throw new Error('_monitorForInsecureState.cancel() called before it was defined'); }; const promise = new Promise((resolve, reject) => { @@ -764,7 +765,10 @@ class Driver { const securityStateChangedListener = ({securityState, explanations}) => { if (securityState === 'insecure') { cancel(); - resolve(explanations); + const insecureDescriptions = explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + resolve(insecureDescriptions.join(' ')); } }; let canceled = false; @@ -772,6 +776,7 @@ class Driver { if (canceled) return; canceled = true; this.off('Security.securityStateChanged', securityStateChangedListener); + // TODO(@patrickhulce): cancel() should really be a promise itself to handle things like this this.sendCommand('Security.disable').catch(() => {}); }; this.on('Security.securityStateChanged', securityStateChangedListener); @@ -835,16 +840,10 @@ class Driver { // CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle. let waitForCPUIdle = this._waitForNothing(); - const waitForSecurityCheck = this._waitForSecurityCheck(); - const securityCheckPromise = waitForSecurityCheck.promise.then(explanations => { + const monitorForInsecureState = this._monitorForInsecureState(); + const securityCheckPromise = monitorForInsecureState.promise.then(securityMessages => { return function() { - const insecureDescriptions = explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description); - const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, { - securityMessages: insecureDescriptions.join(' '), - }); - throw err; + throw new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages}); }; }); @@ -891,7 +890,7 @@ class Driver { waitForLoadEvent.cancel(); waitForNetworkIdle.cancel(); waitForCPUIdle.cancel(); - waitForSecurityCheck.cancel(); + monitorForInsecureState.cancel(); await cleanupFn(); } diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js index c647b885ab74..d1ef25f2c99e 100644 --- a/lighthouse-core/lib/lh-error.js +++ b/lighthouse-core/lib/lh-error.js @@ -21,7 +21,7 @@ const UIStrings = { pageLoadFailedWithStatusCode: 'Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: {statusCode})', /** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */ pageLoadFailedWithDetails: 'Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Details: {errorDetails})', - /** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. */ + /** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. securityMessages will be replaced with one or more strings from the browser explaining what was insecure about the page load. */ pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. {securityMessages}', /** Error message explaining that Chrome has encountered an error during the Lighthouse run, and that Chrome should be restarted. */ internalChromeError: 'An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.', diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 9f1159f52ce6..368e73f49a13 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -27,7 +27,8 @@ function createOnceStub(events) { await Promise.resolve(); if (events[eventName]) { - return cb(events[eventName]); + setTimeout(_ => cb(events[eventName]), 0); + return; } throw Error(`Stub not implemented: ${eventName}`); From 04b1f640230664c5a676422672b0eea9e065ac66 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 10 Jan 2019 14:52:05 -0800 Subject: [PATCH 23/24] create driver for each test --- lighthouse-core/test/gather/driver-test.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 368e73f49a13..a02f19383e71 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -15,9 +15,6 @@ const assert = require('assert'); const EventEmitter = require('events').EventEmitter; const {protocolGetVersionResponse} = require('./fake-driver'); -const connection = new Connection(); -const driverStub = new Driver(connection); - const redirectDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog.json'); const MAX_WAIT_FOR_PROTOCOL = 20; @@ -57,7 +54,7 @@ function createOnceMethodResponse(method, response) { sendCommandMockResponses.set(method, response); } -connection.sendCommand = function(command, params) { +function sendCommandStub(command, params) { sendCommandParams.push({command, params}); if (sendCommandMockResponses.has(command)) { @@ -117,10 +114,17 @@ connection.sendCommand = function(command, params) { default: throw Error(`Stub not implemented: ${command}`); } -}; +} /* eslint-env jest */ +let driverStub; +beforeEach(() => { + const connection = new Connection(); + connection.sendCommand = sendCommandStub; + driverStub = new Driver(connection); +}); + describe('Browser Driver', () => { beforeEach(() => { sendCommandParams = []; From 1da3b01150d523f67f1b51600b7d1f50e53638f4 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Fri, 11 Jan 2019 12:58:46 -0800 Subject: [PATCH 24/24] cr changes --- lighthouse-core/lib/i18n/en-US.json | 2 +- lighthouse-core/test/gather/driver-test.js | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/lib/i18n/en-US.json b/lighthouse-core/lib/i18n/en-US.json index 362ec3e596f6..3183629ed91a 100644 --- a/lighthouse-core/lib/i18n/en-US.json +++ b/lighthouse-core/lib/i18n/en-US.json @@ -993,7 +993,7 @@ }, "lighthouse-core/lib/lh-error.js | pageLoadFailedInsecure": { "message": "The URL you have provided does not have valid security credentials. {securityMessages}", - "description": "Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed." + "description": "Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. securityMessages will be replaced with one or more strings from the browser explaining what was insecure about the page load." }, "lighthouse-core/lib/lh-error.js | pageLoadFailedWithDetails": { "message": "Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Details: {errorDetails})", diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index a02f19383e71..0030b03e29a9 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -19,11 +19,9 @@ const redirectDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog. const MAX_WAIT_FOR_PROTOCOL = 20; function createOnceStub(events) { - return async (eventName, cb) => { - // wait a tick b/c real events never fire immediately - await Promise.resolve(); - + return (eventName, cb) => { if (events[eventName]) { + // wait a tick b/c real events never fire immediately setTimeout(_ => cb(events[eventName]), 0); return; }