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: do not emit NOT_HTML error if record not ok #15271

Merged
merged 1 commit into from
Jul 17, 2023
Merged

core: do not emit NOT_HTML error if record not ok #15271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions core/lib/navigation-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ function getNonHtmlError(finalRecord) {
// If we never requested a document, there's no doctype error, let other cases handle it.
if (!finalRecord) return undefined;

// If the document request error'd, we should not complain about a bad mimeType.
if (!finalRecord.mimeType || finalRecord.statusCode === -1) return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we emit a different error in this case instead of returning no error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So ideally when this occurs, the networkError would be what getPageLoadError returns.

For some reason in LR that wasn't happening. The mainRecord (redirected) failed, yet we didn't emit a network error type.

The fallback in this case is a generic navigationError. So in isolation, this function returning nothing when there is no mimeType-specific error is consistent with the error pattern used here.


// mimeType is determined by the browser, we assume Chrome is determining mimeType correctly,
// independently of 'Content-Type' response headers, and always sending mimeType if well-formed.
if (finalRecord.mimeType !== HTML_MIME_TYPE && finalRecord.mimeType !== XHTML_MIME_TYPE) {
Expand Down Expand Up @@ -163,6 +166,8 @@ function getPageLoadError(navigationError, context) {
// Navigation errors are rather generic and express some failure of the page to render properly.
// Use `navigationError` as the last resort.
// Example: `NO_FCP`, the page never painted content for some unknown reason.
// Note that the caller possibly gave this to us as undefined, in which case we have determined
// there to be no error with this page load - but there was perhaps a warning.
return navigationError;
}

Expand Down
86 changes: 53 additions & 33 deletions core/test/lib/navigation-error-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ const LoadFailureMode = /** @type {const} */ ({
warn: 'warn',
});

/**
* Unless the test specifies otherwise, all status codes will be 200.
* @return {NetworkRequest}
*/
function makeNetworkRequest() {
const record = new NetworkRequest();
record.statusCode = 200;
return record;
}

describe('#getNetworkError', () => {
/**
* @param {NetworkRequest=} mainRecord
Expand All @@ -30,14 +40,14 @@ describe('#getNetworkError', () => {

it('passes when the page is loaded', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
expect(getNetworkError(mainRecord)).toBeUndefined();
});

it('fails when page fails to load', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'foobar';
Expand All @@ -58,7 +68,7 @@ describe('#getNetworkError', () => {

it('fails when page returns with a 404', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 404;
const error = getAndExpectError(mainRecord);
Expand All @@ -69,7 +79,7 @@ describe('#getNetworkError', () => {

it('fails when page returns with a 500', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 500;
const error = getAndExpectError(mainRecord);
Expand All @@ -80,7 +90,7 @@ describe('#getNetworkError', () => {

it('fails when page domain doesn\'t resolve', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'net::ERR_NAME_NOT_RESOLVED';
Expand Down Expand Up @@ -108,14 +118,14 @@ describe('#getInterstitialError', () => {

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

it('passes when page fails to load normally', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'foobar';
Expand All @@ -125,9 +135,9 @@ describe('#getInterstitialError', () => {
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();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
const interstitialRecord = new NetworkRequest();
const interstitialRecord = makeNetworkRequest();
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
Expand All @@ -136,11 +146,11 @@ describe('#getInterstitialError', () => {

it('fails when page gets a generic interstitial', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'ERR_CONNECTION_RESET';
const interstitialRecord = new NetworkRequest();
const interstitialRecord = makeNetworkRequest();
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
Expand All @@ -152,11 +162,11 @@ describe('#getInterstitialError', () => {

it('fails when page gets a security interstitial', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'net::ERR_CERT_COMMON_NAME_INVALID';
const interstitialRecord = new NetworkRequest();
const interstitialRecord = makeNetworkRequest();
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
Expand All @@ -169,14 +179,14 @@ describe('#getInterstitialError', () => {

it('passes when page iframe gets a generic interstitial', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.failed = false;
const iframeRecord = new NetworkRequest();
const iframeRecord = makeNetworkRequest();
iframeRecord.failed = true;
iframeRecord.url = 'https://the-ad.com';
iframeRecord.documentURL = 'https://the-ad.com';
const interstitialRecord = new NetworkRequest();
const interstitialRecord = makeNetworkRequest();
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, iframeRecord, interstitialRecord];
Expand All @@ -201,7 +211,7 @@ describe('#getNonHtmlError', () => {

it('passes when the page is of MIME type text/html', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const mimeType = 'text/html';
mainRecord.url = url;
mainRecord.mimeType = mimeType;
Expand All @@ -210,17 +220,26 @@ describe('#getNonHtmlError', () => {

it('passes when the page is of MIME type application/xhtml+xml', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const mimeType = 'application/xhtml+xml';
mainRecord.url = url;
mainRecord.mimeType = mimeType;
expect(getNonHtmlError(mainRecord)).toBeUndefined();
});

it('passes when the page document did not load', () => {
const url = 'http://the-page.com';
const mainRecord = makeNetworkRequest();
mainRecord.statusCode = -1;
mainRecord.url = url;
mainRecord.mimeType = '';
expect(getNonHtmlError(mainRecord)).toBeUndefined();
});

it('fails when the page is not of MIME type text/html', () => {
const url = 'http://the-page.com';
const mimeType = 'application/xml';
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
mainRecord.url = url;
mainRecord.mimeType = mimeType;
const error = getAndExpectError(mainRecord);
Expand Down Expand Up @@ -249,7 +268,7 @@ describe('#getPageLoadError', () => {
});

it('passes when the page is loaded', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
Expand All @@ -263,7 +282,7 @@ describe('#getPageLoadError', () => {
});

it('passes when the page is loaded, ignoring any fragment', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://example.com/#/page/list',
networkRecords: [mainRecord],
Expand All @@ -277,7 +296,7 @@ describe('#getPageLoadError', () => {
});

it('passes when the page is expected to fail', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
Expand All @@ -292,14 +311,14 @@ describe('#getPageLoadError', () => {
});

it('passes when the page redirects to MIME type text/html', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
loadFailureMode: LoadFailureMode.fatal,
warnings: [],
};
const finalRecord = new NetworkRequest();
const finalRecord = makeNetworkRequest();

mainRecord.url = context.url;
mainRecord.redirectDestination = finalRecord;
Expand All @@ -311,8 +330,8 @@ describe('#getPageLoadError', () => {
});

it('fails with interstitial error first', () => {
const mainRecord = new NetworkRequest();
const interstitialRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const interstitialRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord, interstitialRecord],
Expand All @@ -330,7 +349,7 @@ describe('#getPageLoadError', () => {
});

it('fails with network error second', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
Expand All @@ -346,7 +365,7 @@ describe('#getPageLoadError', () => {
});

it('fails with non-HTML error third', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
Expand All @@ -362,7 +381,7 @@ describe('#getPageLoadError', () => {
});

it('warns with XHTML type', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
Expand All @@ -380,7 +399,7 @@ describe('#getPageLoadError', () => {
});

it('fails with nav error last', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
Expand All @@ -396,7 +415,7 @@ describe('#getPageLoadError', () => {
});

it('fails when loadFailureMode is warn', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
Expand All @@ -412,18 +431,19 @@ describe('#getPageLoadError', () => {
});

it('fails with non-HTML when redirect is not HTML', () => {
const mainRecord = new NetworkRequest();
const mainRecord = makeNetworkRequest();
const context = {
url: 'http://the-page.com',
networkRecords: [mainRecord],
loadFailureMode: LoadFailureMode.fatal,
warnings: [],
};
const finalRecord = new NetworkRequest();
const finalRecord = makeNetworkRequest();

mainRecord.url = context.url;
mainRecord.redirectDestination = finalRecord;
finalRecord.url = 'http://the-redirected-page.com';
finalRecord.mimeType = 'text/todo';

const error = getAndExpectError(navigationError, context);
expect(error.message).toEqual('NOT_HTML');
Expand Down
Loading