From daa531a95e2489db84120cc223eefe87c74f57c5 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 4 Jun 2019 14:50:29 -0500 Subject: [PATCH 1/4] core(gather-runner): convert PAGE_HUNG to non-fatal runtimeError --- clients/extension/scripts/popup.js | 7 ------ lighthouse-cli/run.js | 18 --------------- .../test/smokehouse/error-expectations.js | 4 ++-- lighthouse-cli/test/smokehouse/smokehouse.js | 13 ++--------- lighthouse-core/gather/driver.js | 22 ++++++++++++++----- lighthouse-core/gather/gather-runner.js | 7 ++++-- lighthouse-core/lib/lh-error.js | 8 ------- proto/lighthouse-result.proto | 3 ++- 8 files changed, 28 insertions(+), 54 deletions(-) diff --git a/clients/extension/scripts/popup.js b/clients/extension/scripts/popup.js index bf33fe7c26eb..382e2d635207 100644 --- a/clients/extension/scripts/popup.js +++ b/clients/extension/scripts/popup.js @@ -13,14 +13,7 @@ * Lighthouse itself, mapped to more useful strings to report to the user. */ const NON_BUG_ERROR_MESSAGES = { - // The user tries to review an error page or has network issues - 'ERRORED_DOCUMENT_REQUEST': 'Unable to load the page. Please verify the url you ' + - 'are trying to review.', - 'FAILED_DOCUMENT_REQUEST': 'Unable to load the page. Please verify the url you ' + - 'are trying to review.', 'DNS_FAILURE': 'DNS servers could not resolve the provided domain.', - 'INSECURE_DOCUMENT_REQUEST': 'The URL you have provided does not have a valid' + - ' SSL certificate.', 'INVALID_URL': 'Lighthouse can only audit URLs that start' + ' with http:// or https://.', diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js index 88c6a4ba848d..cc8c717f1995 100644 --- a/lighthouse-cli/run.js +++ b/lighthouse-cli/run.js @@ -24,8 +24,6 @@ const opn = require('opn'); const _RUNTIME_ERROR_CODE = 1; const _PROTOCOL_TIMEOUT_EXIT_CODE = 67; -const _PAGE_HUNG_EXIT_CODE = 68; -const _INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69; /** * exported for testing @@ -76,18 +74,6 @@ function printProtocolTimeoutErrorAndExit() { return process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE); } -/** @param {LighthouseError} err @return {never} */ -function printPageHungErrorAndExit(err) { - console.error('Page hung:', err.friendlyMessage); - return process.exit(_PAGE_HUNG_EXIT_CODE); -} - -/** @param {LighthouseError} err @return {never} */ -function printInsecureDocumentRequestErrorAndExit(err) { - console.error('Insecure document request:', err.friendlyMessage); - return process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE); -} - /** * @param {LighthouseError} err * @return {never} @@ -109,10 +95,6 @@ function printErrorAndExit(err) { return printConnectionErrorAndExit(); } else if (err.code === 'CRI_TIMEOUT') { return printProtocolTimeoutErrorAndExit(); - } else if (err.code === 'PAGE_HUNG') { - return printPageHungErrorAndExit(err); - } else if (err.code === 'INSECURE_DOCUMENT_REQUEST') { - return printInsecureDocumentRequestErrorAndExit(err); } else { return printRuntimeErrorAndExit(err); } diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js index 55afc640f430..c7d43683349f 100644 --- a/lighthouse-cli/test/smokehouse/error-expectations.js +++ b/lighthouse-cli/test/smokehouse/error-expectations.js @@ -10,19 +10,19 @@ */ module.exports = [ { - errorCode: 'PAGE_HUNG', lhr: { requestedUrl: 'http://localhost:10200/infinite-loop.html', finalUrl: 'http://localhost:10200/infinite-loop.html', audits: {}, + runtimeError: {code: 'PAGE_HUNG'}, }, }, { - errorCode: undefined, lhr: { requestedUrl: 'https://expired.badssl.com', finalUrl: 'https://expired.badssl.com/', audits: {}, + runtimeError: {code: 'FAILED_DOCUMENT_REQUEST'}, }, }, ]; diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js index 680de8884578..6d7291681634 100755 --- a/lighthouse-cli/test/smokehouse/smokehouse.js +++ b/lighthouse-cli/test/smokehouse/smokehouse.js @@ -16,8 +16,6 @@ const log = require('lighthouse-logger'); const {collateResults, report} = require('./smokehouse-report.js'); const PROTOCOL_TIMEOUT_EXIT_CODE = 67; -const PAGE_HUNG_EXIT_CODE = 68; -const INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69; const RETRIES = 3; /** @@ -45,9 +43,6 @@ function resolveLocalOrCwd(payloadPath) { */ function isUnexpectedFatalResult(exitCode, outputPath) { return exitCode !== 0 - // These runtime errors are currently fatal but "expected" runtime errors we are asserting against. - && exitCode !== PAGE_HUNG_EXIT_CODE - && exitCode !== INSECURE_DOCUMENT_REQUEST_EXIT_CODE // On runtime errors we exit with a error status code, but still output a report. // If the report exists, it wasn't a fatal LH error we need to abort on, it's one we're asserting :) && !fs.existsSync(outputPath); @@ -116,15 +111,11 @@ function runLighthouse(url, configPath, isDebug) { let errorCode; let lhr = {requestedUrl: url, finalUrl: url, audits: {}}; - if (runResults.status === PAGE_HUNG_EXIT_CODE) { - errorCode = 'PAGE_HUNG'; - } else if (runResults.status === INSECURE_DOCUMENT_REQUEST_EXIT_CODE) { - errorCode = 'INSECURE_DOCUMENT_REQUEST'; - } else { + if (fs.existsSync(outputPath)) { lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8')); if (isDebug) { console.log('LHR output available at: ', outputPath); - } else if (fs.existsSync(outputPath)) { + } else { fs.unlinkSync(outputPath); } } diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index a07c4d6d6295..be8ece98648a 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -1452,7 +1452,7 @@ class Driver { * @param {string} url * @return {Promise} */ - clearDataForOrigin(url) { + async clearDataForOrigin(url) { const origin = new URL(url).origin; // Clear all types of storage except cookies, so the user isn't logged out. @@ -1469,10 +1469,22 @@ class Driver { 'cache_storage', ].join(','); - return this.sendCommand('Storage.clearDataForOrigin', { - origin: origin, - storageTypes: typesToClear, - }); + // `Storage.clearDataForOrigin` is one of our PROTOCOL_TIMEOUT culprits and this command is also + // run in the context of PAGE_HUNG to cleanup. We'll keep the timeout low and just warn if it fails. + this.setNextProtocolTimeout(5000); + + try { + await this.sendCommand('Storage.clearDataForOrigin', { + origin: origin, + storageTypes: typesToClear, + }); + } catch (err) { + if (/** @type {LighthouseError} */(err).code === 'PROTOCOL_TIMEOUT') { + log.warn('Driver', 'clearDataForOrigin timed out'); + } else { + throw err; + } + } } /** diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 24f3baafb578..99fc7b6ffd5e 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -71,7 +71,7 @@ class GatherRunner { passContext.url = finalUrl; } catch (err) { // If it's one of our loading-based LHErrors, we'll treat it as a page load error. - if (err.code === 'NO_FCP') { + if (err.code === 'NO_FCP' || err.code === 'PAGE_HUNG') { return {navigationError: err}; } @@ -452,7 +452,7 @@ class GatherRunner { traces: {}, devtoolsLogs: {}, settings: options.settings, - URL: {requestedUrl: options.requestedUrl, finalUrl: ''}, + URL: {requestedUrl: options.requestedUrl, finalUrl: options.requestedUrl}, Timing: [], }; } @@ -552,6 +552,9 @@ class GatherRunner { const passResults = await GatherRunner.runPass(passContext); Object.assign(artifacts, passResults.artifacts); + // If we encountered a pageLoadError, don't try to keep loading the page in future passes. + if (passResults.pageLoadError) break; + if (isFirstPass) { await GatherRunner.populateBaseArtifacts(passContext); isFirstPass = false; diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js index fdbf06d7abfa..8738e6fbcdf3 100644 --- a/lighthouse-core/lib/lh-error.js +++ b/lighthouse-core/lib/lh-error.js @@ -172,14 +172,6 @@ const ERRORS = { message: UIStrings.pageLoadFailedWithStatusCode, lhrRuntimeError: true, }, - /* Used when security error prevents page load. - * Requires an additional `securityMessages` field for translation. - */ - INSECURE_DOCUMENT_REQUEST: { - code: 'INSECURE_DOCUMENT_REQUEST', - message: UIStrings.pageLoadFailedInsecure, - lhrRuntimeError: true, - }, /* Used when the page stopped responding and did not finish loading. */ PAGE_HUNG: { code: 'PAGE_HUNG', diff --git a/proto/lighthouse-result.proto b/proto/lighthouse-result.proto index d3d485782d2e..ba2530899ce2 100644 --- a/proto/lighthouse-result.proto +++ b/proto/lighthouse-result.proto @@ -43,7 +43,8 @@ enum LighthouseError { PARSING_PROBLEM = 14; // The trace data failed to stream over the protocol. READ_FAILED = 15; - // Used when security error prevents page load. + // Used when security error prevents page load in versions <5.2.0. + // Unused in versions >=5.2.0. INSECURE_DOCUMENT_REQUEST = 16; // Used when protocol command times out. PROTOCOL_TIMEOUT = 17; From 81387a9ae3c9ff137cc4b5e7d0f69b52adc6250e Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 4 Jun 2019 21:18:11 -0500 Subject: [PATCH 2/4] errorCode cleanup --- .../test/smokehouse/smokehouse-report.js | 6 ---- lighthouse-cli/test/smokehouse/smokehouse.js | 31 +++++-------------- lighthouse-core/lib/lh-error.js | 8 +++++ proto/lighthouse-result.proto | 3 +- types/smokehouse.d.ts | 1 - 5 files changed, 16 insertions(+), 33 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/smokehouse-report.js b/lighthouse-cli/test/smokehouse/smokehouse-report.js index 656561f32152..f3f4f04b0af2 100644 --- a/lighthouse-cli/test/smokehouse/smokehouse-report.js +++ b/lighthouse-cli/test/smokehouse/smokehouse-report.js @@ -155,12 +155,6 @@ function collateResults(actual, expected) { }); return [ - { - name: 'error code', - actual: actual.errorCode, - expected: expected.errorCode, - equal: actual.errorCode === expected.errorCode, - }, { name: 'final url', actual: actual.lhr.finalUrl, diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js index 6d7291681634..aea3a8e6b55d 100755 --- a/lighthouse-cli/test/smokehouse/smokehouse.js +++ b/lighthouse-cli/test/smokehouse/smokehouse.js @@ -36,18 +36,6 @@ function resolveLocalOrCwd(payloadPath) { return resolved; } -/** - * Determines if the Lighthouse run ended in an unexpected fatal result. - * @param {number} exitCode - * @param {string} outputPath - */ -function isUnexpectedFatalResult(exitCode, outputPath) { - return exitCode !== 0 - // On runtime errors we exit with a error status code, but still output a report. - // If the report exists, it wasn't a fatal LH error we need to abort on, it's one we're asserting :) - && !fs.existsSync(outputPath); -} - /** * Launch Chrome and do a full Lighthouse run. * @param {string} url @@ -103,21 +91,17 @@ 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 (isUnexpectedFatalResult(runResults.status, outputPath)) { - console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`); + } else if (!fs.existsSync(outputPath)) { + console.error(`Lighthouse run failed to produce a report and exited with ${runResults.status}. stderr to follow:`); // eslint-disable-line max-len console.error(runResults.stderr); process.exit(runResults.status); } - let errorCode; - let lhr = {requestedUrl: url, finalUrl: url, audits: {}}; - if (fs.existsSync(outputPath)) { - lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8')); - if (isDebug) { - console.log('LHR output available at: ', outputPath); - } else { - fs.unlinkSync(outputPath); - } + let lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8')); + if (isDebug) { + console.log('LHR output available at: ', outputPath); + } else { + fs.unlinkSync(outputPath); } // Artifacts are undefined if they weren't written to disk (e.g. if there was an error). @@ -127,7 +111,6 @@ function runLighthouse(url, configPath, isDebug) { } catch (e) {} return { - errorCode, lhr, artifacts, }; diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js index 8738e6fbcdf3..fdbf06d7abfa 100644 --- a/lighthouse-core/lib/lh-error.js +++ b/lighthouse-core/lib/lh-error.js @@ -172,6 +172,14 @@ const ERRORS = { message: UIStrings.pageLoadFailedWithStatusCode, lhrRuntimeError: true, }, + /* Used when security error prevents page load. + * Requires an additional `securityMessages` field for translation. + */ + INSECURE_DOCUMENT_REQUEST: { + code: 'INSECURE_DOCUMENT_REQUEST', + message: UIStrings.pageLoadFailedInsecure, + lhrRuntimeError: true, + }, /* Used when the page stopped responding and did not finish loading. */ PAGE_HUNG: { code: 'PAGE_HUNG', diff --git a/proto/lighthouse-result.proto b/proto/lighthouse-result.proto index ba2530899ce2..d3d485782d2e 100644 --- a/proto/lighthouse-result.proto +++ b/proto/lighthouse-result.proto @@ -43,8 +43,7 @@ enum LighthouseError { PARSING_PROBLEM = 14; // The trace data failed to stream over the protocol. READ_FAILED = 15; - // Used when security error prevents page load in versions <5.2.0. - // Unused in versions >=5.2.0. + // Used when security error prevents page load. INSECURE_DOCUMENT_REQUEST = 16; // Used when protocol command times out. PROTOCOL_TIMEOUT = 17; diff --git a/types/smokehouse.d.ts b/types/smokehouse.d.ts index 5dc28921a256..246e9cbc25b8 100644 --- a/types/smokehouse.d.ts +++ b/types/smokehouse.d.ts @@ -22,7 +22,6 @@ export type ExpectedLHR = Pick export type ExpectedRunnerResult = { - errorCode?: string; lhr: ExpectedLHR, artifacts?: Partial } From 4c375f69c63b41f7f883a4b963c8ef70c8301216 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 4 Jun 2019 21:30:42 -0500 Subject: [PATCH 3/4] let -> const --- lighthouse-cli/test/smokehouse/smokehouse.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js index aea3a8e6b55d..0d3f5ff3f869 100755 --- a/lighthouse-cli/test/smokehouse/smokehouse.js +++ b/lighthouse-cli/test/smokehouse/smokehouse.js @@ -97,7 +97,7 @@ function runLighthouse(url, configPath, isDebug) { process.exit(runResults.status); } - let lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8')); + const lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8')); if (isDebug) { console.log('LHR output available at: ', outputPath); } else { From 3f8500e30db25958436ff34311443a93230c4ce4 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 4 Jun 2019 21:51:47 -0500 Subject: [PATCH 4/4] tsc --- 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 be8ece98648a..a5a8ee8a6ee3 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -1479,7 +1479,7 @@ class Driver { storageTypes: typesToClear, }); } catch (err) { - if (/** @type {LighthouseError} */(err).code === 'PROTOCOL_TIMEOUT') { + if (/** @type {LH.LighthouseError} */(err).code === 'PROTOCOL_TIMEOUT') { log.warn('Driver', 'clearDataForOrigin timed out'); } else { throw err;