Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(gather-runner): convert PAGE_HUNG to non-fatal runtimeError #9121

Merged
merged 4 commits into from
Jun 5, 2019
Merged

core(gather-runner): convert PAGE_HUNG to non-fatal runtimeError #9121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
daa531a
core(gather-runner): convert PAGE_HUNG to non-fatal runtimeError
patrickhulce Jun 4, 2019
81387a9
errorCode cleanup
patrickhulce Jun 5, 2019
4c375f6
let -> const
patrickhulce Jun 5, 2019
3f8500e
tsc
patrickhulce Jun 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions clients/extension/scripts/popup.js
View file
Open in desktop
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 ' +
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
'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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gets a bit weird because we don't populate but still return them. All the other artifacts are either undefined, an Error, or the artifact, but we haven't until now done half-finished artifacts.

Seems ok for now, though


if (isFirstPass) {
await GatherRunner.populateBaseArtifacts(passContext);
isFirstPass = false;
Expand Down
1 change: 0 additions & 1 deletion types/smokehouse.d.ts
View file
Open in desktop
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