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.

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

core(gather-runner): only fail for mainRecord interstitials #9576

Merged
merged 4 commits into from
Aug 22, 2019
Merged
Changes from 1 commit
Commits
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
47 changes: 24 additions & 23 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const manifestParser = require('../lib/manifest-parser.js');
const stacksGatherer = require('../lib/stack-collector.js');
const LHError = require('../lib/lh-error.js');
const URL = require('../lib/url-shim.js');
const NetworkAnalyzer = require('../lib/dependency-graph/simulator/network-analyzer.js');
const NetworkRecorder = require('../lib/network-recorder.js');
const constants = require('../config/constants.js');
const i18n = require('../lib/i18n/i18n.js');
Expand Down Expand Up @@ -132,16 +133,10 @@ class GatherRunner {

/**
* Returns an error if the original network request failed or wasn't found.
* @param {string} url The URL of the original requested page.
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
* @param {LH.Artifacts.NetworkRequest|undefined} mainRecord
* @return {LH.LighthouseError|undefined}
*/
static getNetworkError(url, networkRecords) {
const mainRecord = networkRecords.find(record => {
// record.url is actual request url, so needs to be compared without any URL fragment.
return URL.equalWithExcludedFragments(record.url, url);
});

static getNetworkError(mainRecord) {
if (!mainRecord) {
return new LHError(LHError.errors.NO_DOCUMENT_REQUEST);
} else if (mainRecord.failed) {
Expand Down Expand Up @@ -170,32 +165,31 @@ class GatherRunner {

/**
* Returns an error if we ended up on the `chrome-error` page and all other requests failed.
* @param {LH.Artifacts.NetworkRequest|undefined} mainRecord
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
* @return {LH.LighthouseError|undefined}
*/
static getInterstitialError(networkRecords) {
static getInterstitialError(mainRecord, networkRecords) {
// If we never requested a document, there's no interstitial error, let other cases handle it.
if (!mainRecord) return undefined;

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 the main document didn't fail, we didn't end up on an interstitial.
// FIXME: This doesn't handle client-side redirects.
// None of our error-handling deals with this case either because passContext.url doesn't handle non-network redirects.
if (!mainRecord.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) {
if (mainRecord.localizedFailDescription.startsWith('net::ERR_CERT')) {
return new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages:
insecureRequest.localizedFailDescription});
mainRecord.localizedFailDescription});
}

// If we made it this far, it's a generic Chrome interstitial error.
return new LHError(LHError.errors.CHROME_INTERSTITIAL_ERROR);
}

Expand All @@ -208,8 +202,15 @@ class GatherRunner {
* @return {LighthouseError|undefined}
*/
static getPageLoadError(passContext, loadData, navigationError) {
const networkError = GatherRunner.getNetworkError(passContext.url, loadData.networkRecords);
const interstitialError = GatherRunner.getInterstitialError(loadData.networkRecords);
const {networkRecords} = loadData;
/** @type {LH.Artifacts.NetworkRequest|undefined} */
let mainRecord;
try {
mainRecord = NetworkAnalyzer.findMainDocument(networkRecords, passContext.url);
} catch (_) {}

const networkError = GatherRunner.getNetworkError(mainRecord);
const interstitialError = GatherRunner.getInterstitialError(mainRecord, networkRecords);

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