Skip to content

Commit

Permalink
core(gather-runner): treat Chrome interstitials as pageLoadErrors (#9176
Browse files Browse the repository at this point in the history
)
  • Loading branch information
patrickhulce authored and brendankenny committed Jun 12, 2019
1 parent c9996f4 commit 1e19418
Show file tree
Hide file tree
Showing 5 changed files with 180 additions and 3 deletions.
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 @@ -27,10 +27,10 @@ module.exports = [
finalUrl: 'https://expired.badssl.com/',
audits: {},
// TODO: runtimeError only exists because of selection of audits.
runtimeError: {code: 'FAILED_DOCUMENT_REQUEST'},
runtimeError: {code: 'INSECURE_DOCUMENT_REQUEST'},
},
artifacts: {
ViewportDimensions: {code: 'FAILED_DOCUMENT_REQUEST'},
ViewportDimensions: {code: 'INSECURE_DOCUMENT_REQUEST'},
},
},
];
37 changes: 36 additions & 1 deletion lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,37 @@ class GatherRunner {
}
}

/**
* Returns an error if we ended up on the `chrome-error` page and all other requests failed.
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
* @return {LH.LighthouseError|undefined}
*/
static getInterstitialError(networkRecords) {
const interstitialRequest = networkRecords
.find(record => record.documentURL.startsWith('chrome-error://'));
// If the page didn't end up on a chrome interstitial, there's no error here.
if (!interstitialRequest) return undefined;

const pageNetworkRecords = networkRecords
.filter(record => !URL.NON_NETWORK_PROTOCOLS.includes(record.protocol) &&
!record.documentURL.startsWith('chrome-error://'));
// If none of the requests failed, there's no error here.
// We don't expect that this case could ever occur, but better safe than sorry.
// Note also that in cases of redirects, the initial requests could succeed and we still end up
// on the error interstitial page.
if (!pageNetworkRecords.some(record => record.failed)) return undefined;

// If a request failed with the `net::ERR_CERT_*` collection of errors, then it's a security issue.
const insecureRequest = pageNetworkRecords.find(record =>
record.failed && record.localizedFailDescription.startsWith('net::ERR_CERT'));
if (insecureRequest) {
return new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages:
insecureRequest.localizedFailDescription});
}

return new LHError(LHError.errors.CHROME_INTERSTITIAL_ERROR);
}

/**
* Returns an error if the page load should be considered failed, e.g. from a
* main document request failure, a security issue, etc.
Expand All @@ -177,10 +208,14 @@ class GatherRunner {
*/
static getPageLoadError(passContext, loadData, navigationError) {
const networkError = GatherRunner.getNetworkError(passContext.url, loadData.networkRecords);
const interstitialError = GatherRunner.getInterstitialError(loadData.networkRecords);

// If the driver was offline, the load will fail without offline support. Ignore this case.
// If the driver was offline, the load will fail without offline support. Ignore this case.
if (!passContext.driver.online) return;

// We want to special-case the interstitial beyond FAILED_DOCUMENT_REQUEST. See https://github.com/GoogleChrome/lighthouse/pull/8865#issuecomment-497507618
if (interstitialError) return interstitialError;

// Network errors are usually the most specific and provide the best reason for why the page failed to load.
// Prefer networkError over navigationError.
// Example: `DNS_FAILURE` is better than `NO_FCP`.
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-core/lib/i18n/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -1363,6 +1363,10 @@
"message": "The URL you have provided does not have a valid security certificate. {securityMessages}",
"description": "Error message explaining that the security certificate of the page Lighthouse observed was 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 | pageLoadFailedInterstitial": {
"message": "Chrome prevented page load with an interstitial. Make sure you are testing the correct URL and that the server is properly responding to all requests.",
"description": "Error message explaining that Chrome prevented the page from loading and displayed an interstitial screen instead, so the URL cannot be accessed."
},
"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})",
"description": "Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability."
Expand Down
9 changes: 9 additions & 0 deletions lighthouse-core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ const UIStrings = {
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 security certificate of the page Lighthouse observed was 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 a valid security certificate. {securityMessages}',
/** Error message explaining that Chrome prevented the page from loading and displayed an interstitial screen instead, so the URL cannot be accessed. */
pageLoadFailedInterstitial: 'Chrome prevented page load with an interstitial. Make sure you are testing the correct URL and that the server is properly responding to all requests.',
/** 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.',
/** Error message explaining that fetching the resources of the webpage has taken longer than the maximum time. */
Expand Down Expand Up @@ -180,6 +182,13 @@ const ERRORS = {
message: UIStrings.pageLoadFailedInsecure,
lhrRuntimeError: true,
},
/* Used when any Chrome interstitial error prevents page load.
*/
CHROME_INTERSTITIAL_ERROR: {
code: 'CHROME_INTERSTITIAL_ERROR',
message: UIStrings.pageLoadFailedInterstitial,
lhrRuntimeError: true,
},
/* Used when the page stopped responding and did not finish loading. */
PAGE_HUNG: {
code: 'PAGE_HUNG',
Expand Down
129 changes: 129 additions & 0 deletions lighthouse-core/test/gather/gather-runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -850,6 +850,135 @@ describe('GatherRunner', function() {
});
});

describe('#getInterstitialError', () => {
it('passes when the page is loaded', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
expect(GatherRunner.getInterstitialError([mainRecord])).toBeUndefined();
});

it('passes when page fails to load normally', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'foobar';
expect(GatherRunner.getInterstitialError([mainRecord])).toBeUndefined();
});

it('passes when page gets a generic interstitial but somehow also loads everything', () => {
// This case, AFAIK, is impossible, but we'll err on the side of not tanking the run.
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
const interstitialRecord = new NetworkRequest();
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
expect(GatherRunner.getInterstitialError(records)).toBeUndefined();
});

it('fails when page gets a generic interstitial', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'ERR_CONNECTION_RESET';
const interstitialRecord = new NetworkRequest();
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
const error = GatherRunner.getInterstitialError(records);
expect(error.message).toEqual('CHROME_INTERSTITIAL_ERROR');
expect(error.code).toEqual('CHROME_INTERSTITIAL_ERROR');
expect(error.friendlyMessage).toBeDisplayString(/^Chrome prevented/);
});

it('fails when page gets a security interstitial', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'net::ERR_CERT_COMMON_NAME_INVALID';
const interstitialRecord = new NetworkRequest();
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
const error = GatherRunner.getInterstitialError(records);
expect(error.message).toEqual('INSECURE_DOCUMENT_REQUEST');
expect(error.code).toEqual('INSECURE_DOCUMENT_REQUEST');
expect(error.friendlyMessage).toBeDisplayString(/valid security certificate/);
expect(error.friendlyMessage).toBeDisplayString(/net::ERR_CERT_COMMON_NAME_INVALID/);
});
});

describe('#getPageLoadError', () => {
let navigationError;

beforeEach(() => {
navigationError = new Error('NAVIGATION_ERROR');
});

it('passes when the page is loaded', () => {
const passContext = {url: 'http://the-page.com', driver: {online: true}};
const mainRecord = new NetworkRequest();
const loadData = {networkRecords: [mainRecord]};
mainRecord.url = passContext.url;
const error = GatherRunner.getPageLoadError(passContext, loadData, undefined);
expect(error).toBeUndefined();
});

it('passes when the page is offline', () => {
const passContext = {url: 'http://the-page.com', driver: {online: false}};
const mainRecord = new NetworkRequest();
const loadData = {networkRecords: [mainRecord]};
mainRecord.url = passContext.url;
mainRecord.failed = true;

const error = GatherRunner.getPageLoadError(passContext, loadData, undefined);
expect(error).toBeUndefined();
});

it('fails with interstitial error first', () => {
const passContext = {url: 'http://the-page.com', driver: {online: true}};
const mainRecord = new NetworkRequest();
const interstitialRecord = new NetworkRequest();
const loadData = {networkRecords: [mainRecord, interstitialRecord]};

mainRecord.url = passContext.url;
mainRecord.failed = true;
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';

const error = GatherRunner.getPageLoadError(passContext, loadData, navigationError);
expect(error.message).toEqual('CHROME_INTERSTITIAL_ERROR');
});

it('fails with network error next', () => {
const passContext = {url: 'http://the-page.com', driver: {online: true}};
const mainRecord = new NetworkRequest();
const loadData = {networkRecords: [mainRecord]};

mainRecord.url = passContext.url;
mainRecord.failed = true;

const error = GatherRunner.getPageLoadError(passContext, loadData, navigationError);
expect(error.message).toEqual('FAILED_DOCUMENT_REQUEST');
});

it('fails with nav error last', () => {
const passContext = {url: 'http://the-page.com', driver: {online: true}};
const mainRecord = new NetworkRequest();
const loadData = {networkRecords: [mainRecord]};

mainRecord.url = passContext.url;

const error = GatherRunner.getPageLoadError(passContext, loadData, navigationError);
expect(error.message).toEqual('NAVIGATION_ERROR');
});
});

describe('artifact collection', () => {
// Make sure our gatherers never execute in parallel
it('runs gatherer lifecycle methods strictly in sequence', async () => {
Expand Down

0 comments on commit 1e19418

Please sign in to comment.