Repository: GoogleChrome/lighthouse (js)

Issue:
Incorrect main document used when retrieving page load errors
<!-- Before creating an issue please make sure you are using the latest version and have checked for duplicate issues. -->

#### Provide the steps to reproduce
1. Run LH programmatically on page that redirects a similarly matching URL (e.g. same `startsWith(request.url)`), where the redirect response does not return `text/html` as its mime type.

_More Information_

I'm not sure how common this for non-authenticated redirects, but in our case we are running LH on authenticated pages using cookie-based login (similar to [this doc](https://github.com/GoogleChrome/lighthouse/blob/master/docs/authenticated-pages.md#option-3-pass-custom-request-headers-with-lighthouse-cli), but with Node API). 

The redirect response when hitting the requestedUrl (prior to auth) does not return HTML, and so the content-type response header is empty. It then redirects to the authentication server, and finally back to the originally requested document this time with the correct mime type `text/html`.
  
<!-- If your page is only local, or is liable to change, consider uploading a repro so that we can more easily debug the problem. Some services that will help are: https://jsbin.com/, https://surge.sh/ -->

#### What is the current behavior?

LH reports error `NOT_HTML` even though document loaded in the final URL after redirecting is `text/html`. 

I don't see any documentation on this, but according @brendankenny in #11430, it should be the final matching URL (that is an html document) when loading the page.

The error occurred starting in version 6.2.0 of LH from #11042.

I have a fix locally by using the _last_ matching HTML document (as opposed to the first) in [`findMainDocument(records, finalURL)`](https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js#L443).

A naive one-line fix that works on my site in test is:

```js
// should probably search backwards using loop
const mainResource = records.slice().reverse().find(request => finalURL.startsWith(request.url) &&
        URL.equalWithExcludedFragments(request.url, finalURL));
```

As for the logic when `finalUrl` is not supplied, I think the comment on line 452 still applies. 

#### What is the expected behavior?

LH doesn't fail when valid redirect eventually responds with HTML on a matching `finalUrl`.

#### Environment Information
* Affected Channels: Node
* Lighthouse version: >=6.2.0
* Chrome version: N/A
* Node.js version: 12.9.2
* Operating System: MacOS (and Docker Linux Container)

**Related issues**
* #11042
* #11430

Pull request:
core(gather-runner): use final document when reporting non-HTML error
<!--
Thank you for submitting a pull request!
See CONTRIBUTING.MD for help in getting a change landed.
  https://github.com/GoogleChrome/lighthouse/blob/master/CONTRIBUTING.md
-->

**Summary**
<!-- What kind of change does this PR introduce? -->

<!-- Is this a bugfix, feature, refactoring, build related change, etc? -->

<!-- Describe the need for this change -->

<!-- Link any documentation or information that would help understand this change -->

Fixes a bug where non-HTML page load error would be reported for sites that redirect and resolve to a valid HTML document. The main document's mime type would be checked instead of the "final" document resolved from the redirect.

This change is needed because previously, false positive errors may be reported for valid pages that ultimately respond with HTML via redirects.

**Related Issues/PRs**
<!-- Provide any additional information we might need to understand the pull request -->
Fixes #11585
Introduced in #11042 


Patch:
diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
index 87114a72b..1c2c225c0 100644
--- a/lighthouse-core/gather/gather-runner.js
+++ b/lighthouse-core/gather/gather-runner.js
@@ -225,20 +225,21 @@ class GatherRunner {
 
   /**
    * Returns an error if we try to load a non-HTML page.
-   * @param {LH.Artifacts.NetworkRequest|undefined} mainRecord
+   * Expects a network request with all redirects resolved, otherwise the MIME type may be incorrect.
+   * @param {LH.Artifacts.NetworkRequest|undefined} finalRecord
    * @return {LH.LighthouseError|undefined}
    */
-  static getNonHtmlError(mainRecord) {
+  static getNonHtmlError(finalRecord) {
     // MIME types are case-insenstive but Chrome normalizes MIME types to be lowercase.
     const HTML_MIME_TYPE = 'text/html';
 
     // If we never requested a document, there's no doctype error, let other cases handle it.
-    if (!mainRecord) return undefined;
+    if (!finalRecord) return undefined;
 
     // 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 (HTML_MIME_TYPE !== mainRecord.mimeType) {
-      return new LHError(LHError.errors.NOT_HTML, {mimeType: mainRecord.mimeType});
+    if (HTML_MIME_TYPE !== finalRecord.mimeType) {
+      return new LHError(LHError.errors.NOT_HTML, {mimeType: finalRecord.mimeType});
     }
     return undefined;
   }
@@ -259,9 +260,15 @@ class GatherRunner {
       mainRecord = NetworkAnalyzer.findMainDocument(networkRecords, passContext.url);
     } catch (_) {}
 
+    // MIME Type is only set on the final redirected document request. Use this for the HTML check instead of root.
+    let finalRecord;
+    if (mainRecord) {
+      finalRecord = NetworkAnalyzer.resolveRedirects(mainRecord);
+    }
+
     const networkError = GatherRunner.getNetworkError(mainRecord);
     const interstitialError = GatherRunner.getInterstitialError(mainRecord, networkRecords);
-    const nonHtmlError = GatherRunner.getNonHtmlError(mainRecord);
+    const nonHtmlError = GatherRunner.getNonHtmlError(finalRecord);
 
     // Check to see if we need to ignore the page load failure.
     // e.g. When the driver is offline, the load will fail without page offline support.
diff --git a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
index 93c057983..77a91cb02 100644
--- a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
+++ b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
@@ -452,6 +452,18 @@ class NetworkAnalyzer {
     // The main document is the earliest document request, using position in networkRecords array to break ties.
     return documentRequests.reduce((min, r) => (r.startTime < min.startTime ? r : min));
   }
+
+  /**
+   * Resolves redirect chain given a main document.
+   * See: {@link NetworkAnalyzer.findMainDocument}) for how to retrieve main document.
+   *
+   * @param {LH.Artifacts.NetworkRequest} request
+   * @returns {LH.Artifacts.NetworkRequest}
+   */
+  static resolveRedirects(request) {
+    while (request.redirectDestination) request = request.redirectDestination;
+    return request;
+  }
 }
 
 module.exports = NetworkAnalyzer;


Test patch:
diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js
index 21e546659..46552822e 100644
--- a/lighthouse-core/test/gather/gather-runner-test.js
+++ b/lighthouse-core/test/gather/gather-runner-test.js
@@ -1198,6 +1198,24 @@ describe('GatherRunner', function() {
       expect(error).toBeUndefined();
     });
 
+    it('passes when the page redirects to MIME type text/html', () => {
+      const passContext = {
+        url: 'http://the-page.com',
+        passConfig: {loadFailureMode: LoadFailureMode.fatal},
+      };
+      const mainRecord = new NetworkRequest();
+      const finalRecord = new NetworkRequest();
+      const loadData = {networkRecords: [mainRecord]};
+
+      mainRecord.url = passContext.url;
+      mainRecord.redirectDestination = finalRecord;
+      finalRecord.url = 'http://the-redirected-page.com';
+      finalRecord.mimeType = 'text/html';
+
+      const error = GatherRunner.getPageLoadError(passContext, loadData, undefined);
+      expect(error).toBeUndefined();
+    });
+
     it('fails with interstitial error first', () => {
       const passContext = {
         url: 'http://the-page.com',
@@ -1275,6 +1293,23 @@ describe('GatherRunner', function() {
       const error = getAndExpectError(passContext, loadData, navigationError);
       expect(error.message).toEqual('NAVIGATION_ERROR');
     });
+
+    it('fails with non-HTML when redirect is not HTML', () => {
+      const passContext = {
+        url: 'http://the-page.com',
+        passConfig: {loadFailureMode: LoadFailureMode.fatal},
+      };
+      const mainRecord = new NetworkRequest();
+      const finalRecord = new NetworkRequest();
+      const loadData = {networkRecords: [mainRecord]};
+
+      mainRecord.url = passContext.url;
+      mainRecord.redirectDestination = finalRecord;
+      finalRecord.url = 'http://the-redirected-page.com';
+
+      const error = getAndExpectError(passContext, loadData, navigationError);
+      expect(error.message).toEqual('NOT_HTML');
+    });
   });
 
   describe('artifact collection', () => {
diff --git a/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js b/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js
index 6e65a275d..cce5b8b8d 100644
--- a/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js
+++ b/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js
@@ -10,6 +10,9 @@ const assert = require('assert').strict;
 const NetworkAnalyzer = require('../../../../lib/dependency-graph/simulator/network-analyzer.js');
 const NetworkRecords = require('../../../../computed/network-records.js');
 const devtoolsLog = require('../../../fixtures/traces/progressive-app-m60.devtools.log.json');
+const devtoolsLogWithRedirect = require(
+  '../../../fixtures/traces/site-with-redirect.devtools.log.json'
+);
 
 /* eslint-env jest */
 describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
@@ -385,4 +388,26 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
       assert.equal(mainDocument.url, 'https://www.example.com');
     });
   });
+
+  describe('#resolveRedirects', () => {
+    it('should resolve to the same document when no redirect', async () => {
+      const records = await NetworkRecords.request(devtoolsLog, {computedCache: new Map()});
+
+      const mainDocument = NetworkAnalyzer.findMainDocument(records, 'https://pwa.rocks/');
+      const finalDocument = NetworkAnalyzer.resolveRedirects(mainDocument);
+      assert.equal(mainDocument.url, finalDocument.url);
+      assert.equal(finalDocument.url, 'https://pwa.rocks/');
+    });
+
+    it('should resolve to the final document with redirects', async () => {
+      const records = await NetworkRecords.request(devtoolsLogWithRedirect, {
+        computedCache: new Map(),
+      });
+
+      const mainDocument = NetworkAnalyzer.findMainDocument(records, 'http://www.vkontakte.ru/');
+      const finalDocument = NetworkAnalyzer.resolveRedirects(mainDocument);
+      assert.notEqual(mainDocument.url, finalDocument.url);
+      assert.equal(finalDocument.url, 'https://m.vk.com/');
+    });
+  });
 });