GoogleChrome · brendankenny · Jun 21, 2019 · Jun 12, 2019 · Jun 20, 2019 · Jun 20, 2019
diff --git a/lighthouse-cli/test/smokehouse/error-config.js b/lighthouse-cli/test/smokehouse/error-config.js
@@ -14,8 +14,6 @@ module.exports = {
     maxWaitForLoad: 5000,
     onlyAudits: [
       'first-contentful-paint',
-      // TODO: this audit collects a non-base artifact, allowing the runtimeError to be collected.
-      'content-width',
     ],
   },
 };
diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js
@@ -13,24 +13,44 @@ module.exports = [
     lhr: {
       requestedUrl: 'http://localhost:10200/infinite-loop.html',
       finalUrl: 'http://localhost:10200/infinite-loop.html',
-      audits: {},
-      // TODO: runtimeError only exists because of selection of audits.
       runtimeError: {code: 'PAGE_HUNG'},
+      audits: {
+        'first-contentful-paint': {
+          scoreDisplayMode: 'error',
+          errorMessage: 'Required traces gatherer did not run.',
+        },
+      },
     },
     artifacts: {
-      ViewportDimensions: {code: 'PAGE_HUNG'},
+      PageLoadError: {code: 'PAGE_HUNG'},
+      devtoolsLogs: {
+        'pageLoadError-defaultPass': [/* ... */],
+      },
+      traces: {
+        'pageLoadError-defaultPass': {traceEvents: [/* ... */]},
+      },
     },
   },
   {
     lhr: {
       requestedUrl: 'https://expired.badssl.com',
       finalUrl: 'https://expired.badssl.com/',
-      audits: {},
-      // TODO: runtimeError only exists because of selection of audits.
       runtimeError: {code: 'INSECURE_DOCUMENT_REQUEST'},
+      audits: {
+        'first-contentful-paint': {
+          scoreDisplayMode: 'error',
+          errorMessage: 'Required traces gatherer did not run.',
+        },
+      },
     },
     artifacts: {
-      ViewportDimensions: {code: 'INSECURE_DOCUMENT_REQUEST'},
+      PageLoadError: {code: 'INSECURE_DOCUMENT_REQUEST'},
+      devtoolsLogs: {
+        'pageLoadError-defaultPass': [/* ... */],
+      },
+      traces: {
+        'pageLoadError-defaultPass': {traceEvents: [/* ... */]},
+      },
     },
   },
 ];
diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
@@ -408,28 +408,6 @@ class GatherRunner {
     log.timeEnd(apStatus);
   }
 
-  /**
-   * Generate a set of artfiacts for the given pass as if all the gatherers
-   * failed with the given pageLoadError.
-   * @param {LH.Gatherer.PassContext} passContext
-   * @param {LHError} pageLoadError
-   * @return {{pageLoadError: LHError, artifacts: Partial<LH.GathererArtifacts>}}
-   */
-  static generatePageLoadErrorArtifacts(passContext, pageLoadError) {
-    /** @type {Partial<Record<keyof LH.GathererArtifacts, LHError>>} */
-    const errorArtifacts = {};
-    for (const gathererDefn of passContext.passConfig.gatherers) {
-      const gatherer = gathererDefn.instance;
-      errorArtifacts[gatherer.name] = pageLoadError;
-    }
-
-    return {
-      pageLoadError,
-      // @ts-ignore - TODO(bckenny): figure out how to usefully type errored artifacts.
-      artifacts: errorArtifacts,
-    };
-  }
-
   /**
    * Takes the results of each gatherer phase for each gatherer and uses the
    * last produced value (that's not undefined) as the artifact for that
@@ -495,6 +473,7 @@ class GatherRunner {
       settings: options.settings,
       URL: {requestedUrl: options.requestedUrl, finalUrl: options.requestedUrl},
       Timing: [],
+      PageLoadError: null,
     };
   }
 
@@ -594,7 +573,10 @@ class GatherRunner {
         Object.assign(artifacts, passResults.artifacts);
 
         // If we encountered a pageLoadError, don't try to keep loading the page in future passes.
-        if (passResults.pageLoadError) break;
+        if (passResults.pageLoadError) {
+          baseArtifacts.PageLoadError = passResults.pageLoadError;
+          break;
+        }
 
         if (isFirstPass) {
           await GatherRunner.populateBaseArtifacts(passContext);
@@ -606,8 +588,9 @@ class GatherRunner {
       GatherRunner.finalizeBaseArtifacts(baseArtifacts);
       return /** @type {LH.Artifacts} */ ({...baseArtifacts, ...artifacts}); // Cast to drop Partial<>.
     } catch (err) {
-      // cleanup on error
+      // Clean up on error. Don't await so that the root error, not a disposal error, is shown.
       GatherRunner.disposeDriver(driver, options);
+
       throw err;
     }
   }
@@ -660,14 +643,15 @@ class GatherRunner {
     // Disable throttling so the afterPass analysis isn't throttled
     await driver.setThrottling(passContext.settings, {useThrottling: false});
 
-    // If there were any load errors, treat all gatherers as if they errored.
+    // In case of load error, save log and trace with an error prefix, return no artifacts for this pass.
     const pageLoadError = GatherRunner.getPageLoadError(passContext, loadData, possibleNavError);
     if (pageLoadError) {
       log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url);
       passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage);
       GatherRunner._addLoadDataToBaseArtifacts(passContext, loadData,
           `pageLoadError-${passConfig.passName}`);
-      return GatherRunner.generatePageLoadErrorArtifacts(passContext, pageLoadError);
+
+      return {artifacts: {}, pageLoadError};
     }
 
     // If no error, save devtoolsLog and trace.

diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js
@@ -352,12 +352,18 @@ class Runner {
   }
 
   /**
-   * Returns first runtimeError found in artifacts.
+   * Searches a pass's artifacts for any `lhrRuntimeError` error artifacts.
+   * Returns the first one found or `null` if none found.
    * @param {LH.Artifacts} artifacts
    * @return {LH.Result['runtimeError']|undefined}
    */
   static getArtifactRuntimeError(artifacts) {
-    for (const possibleErrorArtifact of Object.values(artifacts)) {
+    const possibleErrorArtifacts = [
+      artifacts.PageLoadError, // Preferentially use `PageLoadError`, if it exists.
+      ...Object.values(artifacts), // Otherwise check amongst all artifacts.
+    ];
+
+    for (const possibleErrorArtifact of possibleErrorArtifacts) {
       if (possibleErrorArtifact instanceof LHError && possibleErrorArtifact.lhrRuntimeError) {
         const errorMessage = possibleErrorArtifact.friendlyMessage || possibleErrorArtifact.message;
 

diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js
@@ -424,9 +424,10 @@ describe('GatherRunner', function() {
     assert.equal(tests.calledCleanBrowserCaches, true);
   });
 
-  it('fails artifacts with network errors', async () => {
+  it('returns a pageLoadError and no artifacts with network errors', async () => {
     const requestedUrl = 'https://example.com';
-    // This page load error should be overriden by NO_DOCUMENT_REQUEST for being more specific
+    // 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.
     const navigationError = new LHError(LHError.errors.NO_FCP);
     const driver = Object.assign({}, fakeDriver, {
       online: true,
@@ -450,13 +451,14 @@ describe('GatherRunner', function() {
       baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}),
     };
 
-    const {artifacts} = await GatherRunner.runPass(passContext);
+    const {artifacts, pageLoadError} = await GatherRunner.runPass(passContext);
     expect(passContext.LighthouseRunWarnings).toHaveLength(1);
-    expect(artifacts.TestGatherer).toBeInstanceOf(Error);
-    expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST');
+    expect(pageLoadError).toBeInstanceOf(Error);
+    expect(pageLoadError.code).toEqual('NO_DOCUMENT_REQUEST');
+    expect(artifacts).toEqual({});
   });
 
-  it('fails artifacts with navigation errors', async () => {
+  it('returns a pageLoadError and no artifacts with navigation errors', async () => {
     const requestedUrl = 'https://example.com';
     // This time, NO_FCP should win because it's the only error left.
     const navigationError = new LHError(LHError.errors.NO_FCP);
@@ -485,10 +487,11 @@ describe('GatherRunner', function() {
       baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}),
     };
 
-    const {artifacts} = await GatherRunner.runPass(passContext);
+    const {artifacts, pageLoadError} = await GatherRunner.runPass(passContext);
     expect(passContext.LighthouseRunWarnings).toHaveLength(1);
-    expect(artifacts.TestGatherer).toBeInstanceOf(Error);
-    expect(artifacts.TestGatherer.code).toEqual('NO_FCP');
+    expect(pageLoadError).toBeInstanceOf(Error);
+    expect(pageLoadError.code).toEqual('NO_FCP');
+    expect(artifacts).toEqual({});
   });
 
   it('does not clear origin storage with flag --disable-storage-reset', () => {
@@ -806,7 +809,8 @@ describe('GatherRunner', function() {
     const options = {driver, requestedUrl, settings: config.settings};
     const artifacts = await GatherRunner.run(config.passes, options);
 
-    expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST');
+    expect(artifacts.PageLoadError.code).toEqual('NO_DOCUMENT_REQUEST');
+    expect(artifacts.TestGatherer).toBeUndefined();
 
     // The only loadData available should be prefixed with `pageLoadError-`.
     expect(Object.keys(artifacts.traces)).toEqual(['pageLoadError-firstPass']);
@@ -856,12 +860,15 @@ describe('GatherRunner', function() {
     expect(t2.called).toBe(true);
     expect(t3.called).toBe(false);
 
-    // But only t1 has a valid artifact, t2 has an error artifact, and t3 never ran.
+    // But only t1 has a valid artifact; t2 and t3 aren't defined.
     expect(artifacts.Test1).toBe('MyArtifact');
-    expect(artifacts.Test2).toBeInstanceOf(LHError);
-    expect(artifacts.Test2.code).toEqual('NO_FCP');
+    expect(artifacts.Test2).toBeUndefined();
     expect(artifacts.Test3).toBeUndefined();
 
+    // PageLoadError artifact has the error.
+    expect(artifacts.PageLoadError).toBeInstanceOf(LHError);
+    expect(artifacts.PageLoadError.code).toEqual('NO_FCP');
+
     // firstPass has a saved trace and devtoolsLog, secondPass has an error trace and log.
     expect(Object.keys(artifacts.traces)).toEqual(['firstPass', 'pageLoadError-secondPass']);
     expect(Object.keys(artifacts.devtoolsLogs)).toEqual(['firstPass', 'pageLoadError-secondPass']);

diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js
@@ -631,13 +631,18 @@ describe('Runner', () => {
     });
   });
 
-  it('includes a top-level runtimeError when a gatherer throws one', async () => {
+  describe('lhr.runtimeError', () => {
     const NO_FCP = LHError.errors.NO_FCP;
     class RuntimeErrorGatherer extends Gatherer {
       afterPass() {
         throw new LHError(NO_FCP);
       }
     }
+    class RuntimeError2Gatherer extends Gatherer {
+      afterPass() {
+        throw new LHError(LHError.errors.NO_SCREENSHOTS);
+      }
+    }
     class WarningAudit extends Audit {
       static get meta() {
         return {
@@ -652,19 +657,55 @@ describe('Runner', () => {
       }
     }
 
-    const config = new Config({
-      passes: [{gatherers: [RuntimeErrorGatherer]}],
+    const configJson = {
+      passes: [
+        {gatherers: [RuntimeErrorGatherer]},
+        {gatherers: [RuntimeError2Gatherer], passName: 'second'},
+      ],
       audits: [WarningAudit],
-    });
-    const {lhr} = await Runner.run(null, {url: 'https://example.com/', config, driverMock});
+    };
 
-    // Audit error included the runtimeError
-    assert.strictEqual(lhr.audits['test-audit'].scoreDisplayMode, 'error');
-    assert.ok(lhr.audits['test-audit'].errorMessage.includes(NO_FCP.code));
-    // And it bubbled up to the runtimeError.
-    assert.strictEqual(lhr.runtimeError.code, NO_FCP.code);
-    expect(lhr.runtimeError.message)
-      .toBeDisplayString(/Something .*\(NO_FCP\)/);
+    it('includes a top-level runtimeError when a gatherer throws one', async () => {
+      const config = new Config(configJson);
+      const {lhr} = await Runner.run(null, {url: 'https://example.com/', config, driverMock});
+
+      // Audit error included the runtimeError
+      expect(lhr.audits['test-audit'].scoreDisplayMode).toEqual('error');
+      expect(lhr.audits['test-audit'].errorMessage).toEqual(expect.stringContaining(NO_FCP.code));
+
+      // And it bubbled up to the runtimeError.
+      expect(lhr.runtimeError.code).toEqual(NO_FCP.code);
+      expect(lhr.runtimeError.message).toBeDisplayString(/Something .*\(NO_FCP\)/);
+    });
+
+    it('includes a pageLoadError runtimeError over any gatherer runtimeErrors', async () => {
+      const url = 'https://www.reddit.com/r/nba';
+      let firstLoad = true;
+      const errorDriverMock = Object.assign({}, driverMock, {
+        online: true,
+        // Loads the page successfully in the first pass, fails with PAGE_HUNG in the second.
+        async gotoURL(url) {
+          if (url.includes('blank')) return null;
+          if (firstLoad) {
+            firstLoad = false;
+            return url;
+          } else {
+            throw new LHError(LHError.errors.PAGE_HUNG);
+          }
+        },
+      });
+
+      const config = new Config(configJson);
+      const {lhr} = await Runner.run(null, {url, config, driverMock: errorDriverMock});
+
+      // Audit error still includes the gatherer runtimeError.
+      expect(lhr.audits['test-audit'].scoreDisplayMode).toEqual('error');
+      expect(lhr.audits['test-audit'].errorMessage).toEqual(expect.stringContaining(NO_FCP.code));
+
+      // But top-level runtimeError is the pageLoadError.
+      expect(lhr.runtimeError.code).toEqual(LHError.errors.PAGE_HUNG.code);
+      expect(lhr.runtimeError.message).toBeDisplayString(/because the page stopped responding/);
+    });
   });
 
   it('localized errors thrown from driver', async () => {

diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts
@@ -49,6 +49,8 @@ declare global {
       URL: {requestedUrl: string, finalUrl: string};
       /** The timing instrumentation of the gather portion of a run. */
       Timing: Artifacts.MeasureEntry[];
+      /** If loading the page failed, value is the error that caused it. Otherwise null. */
+      PageLoadError: LighthouseError | null;
     }
 
     /**