From ba582089bcc81ab48f1aa022eedb168a062b13c5 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 19 Aug 2019 11:21:42 -0500 Subject: [PATCH 1/4] core(gather-runner): only fail for mainRecord interstitials --- lighthouse-core/gather/gather-runner.js | 47 +++++++++++++------------ 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 71b13a002513..292cb0743cde 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -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'); @@ -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} 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) { @@ -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} 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); } @@ -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; From 0af35abb529e078aca981c6434b25426717d9ef8 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 19 Aug 2019 15:25:37 -0500 Subject: [PATCH 2/4] tests --- .../test/fixtures/badssl-iframe.html | 7 +++ .../test/smokehouse/error-expectations.js | 19 +++++++ lighthouse-core/gather/gather-runner.js | 1 - .../test/gather/gather-runner-test.js | 57 +++++++++++++------ 4 files changed, 66 insertions(+), 18 deletions(-) create mode 100644 lighthouse-cli/test/fixtures/badssl-iframe.html diff --git a/lighthouse-cli/test/fixtures/badssl-iframe.html b/lighthouse-cli/test/fixtures/badssl-iframe.html new file mode 100644 index 000000000000..d42c34f96211 --- /dev/null +++ b/lighthouse-cli/test/fixtures/badssl-iframe.html @@ -0,0 +1,7 @@ + + + +

I have a bad iframe!

+ + + diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js index 7030f03f02a2..4b1fb708dd7b 100644 --- a/lighthouse-cli/test/smokehouse/error-expectations.js +++ b/lighthouse-cli/test/smokehouse/error-expectations.js @@ -59,4 +59,23 @@ module.exports = [ }, }, }, + { + lhr: { + requestedUrl: 'http://localhost:10200/badssl-iframe.html', + finalUrl: 'http://localhost:10200/badssl-iframe.html', + audits: { + 'first-contentful-paint': { + scoreDisplayMode: 'numeric', + }, + }, + }, + artifacts: { + devtoolsLogs: { + defaultPass: NONEMPTY_ARRAY, + }, + traces: { + defaultPass: {traceEvents: NONEMPTY_ARRAY}, + }, + }, + }, ]; diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 292cb0743cde..c6fe6ce46034 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -9,7 +9,6 @@ const log = require('lighthouse-logger'); 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'); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 8d8077df0be5..3b2a0a1eac51 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -426,12 +426,15 @@ describe('GatherRunner', function() { it('returns a pageLoadError and no artifacts when there is a network error', async () => { const requestedUrl = 'https://example.com'; - // This page load error should be overriden by NO_DOCUMENT_REQUEST (for being - // more specific) since requestedUrl does not match any network request in fixture. + // This page load error should be overriden by ERRORED_DOCUMENT_REQUEST (for being + // more specific) since the main document network request failed with a 500. const navigationError = new LHError(LHError.errors.NO_FCP); const driver = Object.assign({}, fakeDriver, { online: true, gotoURL: url => url.includes('blank') ? null : Promise.reject(navigationError), + endDevtoolsLog() { + return networkRecordsToDevtoolsLog([{url: requestedUrl, statusCode: 500}]); + }, }); const config = new Config({ @@ -450,7 +453,7 @@ describe('GatherRunner', function() { const artifacts = await GatherRunner.run(config.passes, options); expect(artifacts.LighthouseRunWarnings).toHaveLength(1); expect(artifacts.PageLoadError).toBeInstanceOf(Error); - expect(artifacts.PageLoadError.code).toEqual('NO_DOCUMENT_REQUEST'); + expect(artifacts.PageLoadError.code).toEqual('ERRORED_DOCUMENT_REQUEST'); expect(artifacts.TestGatherer).toBeUndefined(); }); @@ -789,6 +792,7 @@ describe('GatherRunner', function() { // resolved URL here does not match any request in the network records, causing a runtime error. gotoURL: async _ => requestedUrl, online: true, + endDevtoolsLog: () => [], }); const config = new Config({ @@ -871,14 +875,14 @@ describe('GatherRunner', function() { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; - assert.ok(!GatherRunner.getNetworkError(url, [mainRecord])); + assert.ok(!GatherRunner.getNetworkError(mainRecord)); }); it('passes when the page is loaded, ignoring any fragment', () => { const url = 'http://example.com/#/page/list'; const mainRecord = new NetworkRequest(); mainRecord.url = 'http://example.com'; - assert.ok(!GatherRunner.getNetworkError(url, [mainRecord])); + assert.ok(!GatherRunner.getNetworkError(mainRecord)); }); it('fails when page fails to load', () => { @@ -887,7 +891,7 @@ describe('GatherRunner', function() { mainRecord.url = url; mainRecord.failed = true; mainRecord.localizedFailDescription = 'foobar'; - const error = GatherRunner.getNetworkError(url, [mainRecord]); + const error = GatherRunner.getNetworkError(mainRecord); assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST'); assert.equal(error.code, 'FAILED_DOCUMENT_REQUEST'); expect(error.friendlyMessage) @@ -895,9 +899,7 @@ describe('GatherRunner', function() { }); it('fails when page times out', () => { - const url = 'http://the-page.com'; - const records = []; - const error = GatherRunner.getNetworkError(url, records); + const error = GatherRunner.getNetworkError(undefined); assert.equal(error.message, 'NO_DOCUMENT_REQUEST'); assert.equal(error.code, 'NO_DOCUMENT_REQUEST'); expect(error.friendlyMessage).toBeDisplayString(/^Lighthouse was unable to reliably load/); @@ -908,7 +910,7 @@ describe('GatherRunner', function() { const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 404; - const error = GatherRunner.getNetworkError(url, [mainRecord]); + const error = GatherRunner.getNetworkError(mainRecord); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.equal(error.code, 'ERRORED_DOCUMENT_REQUEST'); expect(error.friendlyMessage) @@ -920,7 +922,7 @@ describe('GatherRunner', function() { const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 500; - const error = GatherRunner.getNetworkError(url, [mainRecord]); + const error = GatherRunner.getNetworkError(mainRecord); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.equal(error.code, 'ERRORED_DOCUMENT_REQUEST'); expect(error.friendlyMessage) @@ -933,7 +935,7 @@ describe('GatherRunner', function() { mainRecord.url = url; mainRecord.failed = true; mainRecord.localizedFailDescription = 'net::ERR_NAME_NOT_RESOLVED'; - const error = GatherRunner.getNetworkError(url, [mainRecord]); + const error = GatherRunner.getNetworkError(mainRecord); assert.equal(error.message, 'DNS_FAILURE'); assert.equal(error.code, 'DNS_FAILURE'); expect(error.friendlyMessage).toBeDisplayString(/^DNS servers could not resolve/); @@ -941,11 +943,15 @@ describe('GatherRunner', function() { }); describe('#getInterstitialError', () => { + it('passes when the page was not requested', () => { + expect(GatherRunner.getInterstitialError(undefined, [])).toBeUndefined(); + }); + 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(); + expect(GatherRunner.getInterstitialError(mainRecord, [mainRecord])).toBeUndefined(); }); it('passes when page fails to load normally', () => { @@ -954,7 +960,7 @@ describe('GatherRunner', function() { mainRecord.url = url; mainRecord.failed = true; mainRecord.localizedFailDescription = 'foobar'; - expect(GatherRunner.getInterstitialError([mainRecord])).toBeUndefined(); + expect(GatherRunner.getInterstitialError(mainRecord, [mainRecord])).toBeUndefined(); }); it('passes when page gets a generic interstitial but somehow also loads everything', () => { @@ -966,7 +972,7 @@ describe('GatherRunner', function() { interstitialRecord.url = 'data:text/html;base64,abcdef'; interstitialRecord.documentURL = 'chrome-error://chromewebdata/'; const records = [mainRecord, interstitialRecord]; - expect(GatherRunner.getInterstitialError(records)).toBeUndefined(); + expect(GatherRunner.getInterstitialError(mainRecord, records)).toBeUndefined(); }); it('fails when page gets a generic interstitial', () => { @@ -979,7 +985,7 @@ describe('GatherRunner', function() { interstitialRecord.url = 'data:text/html;base64,abcdef'; interstitialRecord.documentURL = 'chrome-error://chromewebdata/'; const records = [mainRecord, interstitialRecord]; - const error = GatherRunner.getInterstitialError(records); + const error = GatherRunner.getInterstitialError(mainRecord, records); expect(error.message).toEqual('CHROME_INTERSTITIAL_ERROR'); expect(error.code).toEqual('CHROME_INTERSTITIAL_ERROR'); expect(error.friendlyMessage).toBeDisplayString(/^Chrome prevented/); @@ -995,12 +1001,29 @@ describe('GatherRunner', function() { interstitialRecord.url = 'data:text/html;base64,abcdef'; interstitialRecord.documentURL = 'chrome-error://chromewebdata/'; const records = [mainRecord, interstitialRecord]; - const error = GatherRunner.getInterstitialError(records); + const error = GatherRunner.getInterstitialError(mainRecord, 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/); }); + + it('passes when page iframe gets a generic interstitial', () => { + const url = 'http://the-page.com'; + const mainRecord = new NetworkRequest(); + mainRecord.url = url; + mainRecord.failed = false; + const iframeRecord = new NetworkRequest(); + iframeRecord.failed = true; + iframeRecord.url = 'https://the-ad.com'; + iframeRecord.documentURL = 'https://the-ad.com'; + const interstitialRecord = new NetworkRequest(); + interstitialRecord.url = 'data:text/html;base64,abcdef'; + interstitialRecord.documentURL = 'chrome-error://chromewebdata/'; + const records = [mainRecord, iframeRecord, interstitialRecord]; + const error = GatherRunner.getInterstitialError(mainRecord, records); + expect(error).toBeUndefined(); + }); }); describe('#getPageLoadError', () => { From fd7a910831a641931d9bf3978fa66845b4eabbdd Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 19 Aug 2019 15:28:33 -0500 Subject: [PATCH 3/4] lint --- .../test/gather/gather-runner-test.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 3b2a0a1eac51..a05e49d4ecd8 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -878,13 +878,6 @@ describe('GatherRunner', function() { assert.ok(!GatherRunner.getNetworkError(mainRecord)); }); - it('passes when the page is loaded, ignoring any fragment', () => { - const url = 'http://example.com/#/page/list'; - const mainRecord = new NetworkRequest(); - mainRecord.url = 'http://example.com'; - assert.ok(!GatherRunner.getNetworkError(mainRecord)); - }); - it('fails when page fails to load', () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); @@ -1042,6 +1035,16 @@ describe('GatherRunner', function() { expect(error).toBeUndefined(); }); + it('passes when the page is loaded, ignoring any fragment', () => { + const url = 'http://example.com/#/page/list'; + const mainRecord = new NetworkRequest(); + const passContext = {url, driver: {online: true}}; + const loadData = {networkRecords: [mainRecord]}; + mainRecord.url = 'http://example.com'; + 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(); From 3fdcf2ecffc47969d24a53b71d29125cc00ae40c Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 20 Aug 2019 10:47:54 -0500 Subject: [PATCH 4/4] add explanatory comment --- lighthouse-cli/test/smokehouse/error-expectations.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js index 4b1fb708dd7b..a77071d2f140 100644 --- a/lighthouse-cli/test/smokehouse/error-expectations.js +++ b/lighthouse-cli/test/smokehouse/error-expectations.js @@ -61,6 +61,9 @@ module.exports = [ }, { lhr: { + // Our interstitial error handling used to be quite aggressive, so we'll test a page + // that has a bad iframe to make sure LH audits successfully. + // https://github.com/GoogleChrome/lighthouse/issues/9562 requestedUrl: 'http://localhost:10200/badssl-iframe.html', finalUrl: 'http://localhost:10200/badssl-iframe.html', audits: {