Skip to content

Commit

Permalink
core(gather-runner): convert PAGE_HUNG to non-fatal runtimeError (#9121)
Browse files Browse the repository at this point in the history
  • Loading branch information
patrickhulce authored and brendankenny committed Jun 5, 2019
1 parent ccc5a92 commit 9ebae67
Show file tree
Hide file tree
Showing 8 changed files with 30 additions and 73 deletions.
7 changes: 0 additions & 7 deletions clients/extension/scripts/popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -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://.',

Expand Down
18 changes: 0 additions & 18 deletions lighthouse-cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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}
Expand All @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-cli/test/smokehouse/error-expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'},
},
},
];
6 changes: 0 additions & 6 deletions lighthouse-cli/test/smokehouse/smokehouse-report.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
38 changes: 6 additions & 32 deletions lighthouse-cli/test/smokehouse/smokehouse.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -38,21 +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
// 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);
}

/**
* Launch Chrome and do a full Lighthouse run.
* @param {string} url
Expand Down Expand Up @@ -108,25 +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 (runResults.status === PAGE_HUNG_EXIT_CODE) {
errorCode = 'PAGE_HUNG';
} else if (runResults.status === INSECURE_DOCUMENT_REQUEST_EXIT_CODE) {
errorCode = 'INSECURE_DOCUMENT_REQUEST';
const lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8'));
if (isDebug) {
console.log('LHR output available at: ', outputPath);
} else {
lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8'));
if (isDebug) {
console.log('LHR output available at: ', outputPath);
} else if (fs.existsSync(outputPath)) {
fs.unlinkSync(outputPath);
}
fs.unlinkSync(outputPath);
}

// Artifacts are undefined if they weren't written to disk (e.g. if there was an error).
Expand All @@ -136,7 +111,6 @@ function runLighthouse(url, configPath, isDebug) {
} catch (e) {}

return {
errorCode,
lhr,
artifacts,
};
Expand Down
22 changes: 17 additions & 5 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -1452,7 +1452,7 @@ class Driver {
* @param {string} url
* @return {Promise<void>}
*/
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.
Expand All @@ -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 {LH.LighthouseError} */(err).code === 'PROTOCOL_TIMEOUT') {
log.warn('Driver', 'clearDataForOrigin timed out');
} else {
throw err;
}
}
}

/**
Expand Down
7 changes: 5 additions & 2 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -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};
}

Expand Down Expand Up @@ -452,7 +452,7 @@ class GatherRunner {
traces: {},
devtoolsLogs: {},
settings: options.settings,
URL: {requestedUrl: options.requestedUrl, finalUrl: ''},
URL: {requestedUrl: options.requestedUrl, finalUrl: options.requestedUrl},
Timing: [],
};
}
Expand Down Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion types/smokehouse.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
export type ExpectedLHR = Pick<LH.Result, 'audits' | 'finalUrl' | 'requestedUrl'>

export type ExpectedRunnerResult = {
errorCode?: string;
lhr: ExpectedLHR,
artifacts?: Partial<LH.Artifacts>
}
Expand Down

0 comments on commit 9ebae67

Please sign in to comment.