core(gather-runner): treat NO_FCP as a pageLoadError (#8340)

lighthouse-cli/run.js

@@ -65,25 +65,25 @@ function getDebuggableChrome(flags) {
 }
 
 /** @return {never} */
-function showConnectionError() {
+function printConnectionErrorAndExit() {
   console.error('Unable to connect to Chrome');
   return process.exit(_RUNTIME_ERROR_CODE);
 }
 
 /** @return {never} */
-function showProtocolTimeoutError() {
+function printProtocolTimeoutErrorAndExit() {
   console.error('Debugger protocol timed out while connecting to Chrome.');
   return process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE);
 }
 
 /** @param {LighthouseError} err @return {never} */
-function showPageHungError(err) {
+function printPageHungErrorAndExit(err) {
   console.error('Page hung:', err.friendlyMessage);
   return process.exit(_PAGE_HUNG_EXIT_CODE);
 }
 
 /** @param {LighthouseError} err @return {never} */
-function showInsecureDocumentRequestError(err) {
+function printInsecureDocumentRequestErrorAndExit(err) {
   console.error('Insecure document request:', err.friendlyMessage);
   return process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE);
 }
@@ -92,7 +92,7 @@ function showInsecureDocumentRequestError(err) {
  * @param {LighthouseError} err
  * @return {never}
  */
-function showRuntimeError(err) {
+function printRuntimeErrorAndExit(err) {
   console.error('Runtime error encountered:', err.friendlyMessage || err.message);
   if (err.stack) {
     console.error(err.stack);
@@ -104,17 +104,17 @@ function showRuntimeError(err) {
  * @param {LighthouseError} err
  * @return {never}
  */
-function handleError(err) {
+function printErrorAndExit(err) {
   if (err.code === 'ECONNREFUSED') {
-    return showConnectionError();
+    return printConnectionErrorAndExit();
   } else if (err.code === 'CRI_TIMEOUT') {
-    return showProtocolTimeoutError();
+    return printProtocolTimeoutErrorAndExit();
   } else if (err.code === 'PAGE_HUNG') {
-    return showPageHungError(err);
+    return printPageHungErrorAndExit(err);
   } else if (err.code === 'INSECURE_DOCUMENT_REQUEST') {
-    return showInsecureDocumentRequestError(err);
+    return printInsecureDocumentRequestErrorAndExit(err);
   } else {
-    return showRuntimeError(err);
+    return printRuntimeErrorAndExit(err);
   }
 }
 
@@ -218,10 +218,24 @@ async function runLighthouse(url, flags, config) {
     await potentiallyKillChrome(launchedChrome);
     process.removeListener('unhandledRejection', handleTheUnhandled);
 
+    // Runtime errors indicate something was *very* wrong with the page result.
+    // We don't want the user to have to parse the report to figure it out, so we'll still exit
+    // with an error code after we saved the results.
+    if (runnerResult && runnerResult.lhr.runtimeError) {
+      const {runtimeError} = runnerResult.lhr;
+      return printErrorAndExit({
+        name: 'LHError',
+        friendlyMessage: runtimeError.message,
+        lhrRuntimeError: true,
+        code: runtimeError.code,
+        message: runtimeError.message,
+      });
+    }
+
     return runnerResult;
   } catch (err) {
     await potentiallyKillChrome(launchedChrome).catch(() => {});
-    handleError(err);
+    return printErrorAndExit(err);
   }
 }
 

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -38,6 +38,21 @@ function resolveLocalOrCwd(payloadPath) {
   return resolved;
 }
 
+/**
+ * Determines if the Lighthouse run ended in an unexpected fatal result.
+ * @param {number} exitCode
+ * @param {string} outputPath
+ */
+function isUnexpectedFatalResult(exitCode, outputPath) {
+  return exitCode !== 0
+    // These runtime errors are currently fatal but "expected" runtime errors we are asserting against.
+    && exitCode !== PAGE_HUNG_EXIT_CODE
+    && exitCode !== INSECURE_DOCUMENT_REQUEST_EXIT_CODE
+    // On runtime errors we exit with a error status code, but still output a report.
+    // If the report exists, it wasn't a fatal LH error we need to abort on, it's one we're asserting :)
+    && !fs.existsSync(outputPath);
+}
+
 /**
  * Launch Chrome and do a full Lighthouse run.
  * @param {string} url
@@ -85,22 +100,20 @@ function runLighthouse(url, configPath, isDebug) {
     runResults = spawnSync(command, args, {encoding: 'utf8', stdio: ['pipe', 'pipe', 'inherit']});
   } while (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE && runCount <= RETRIES);
 
+  if (isDebug) {
+    console.log(`STDOUT: ${runResults.stdout}`);
+    console.error(`STDERR: ${runResults.stderr}`);
+  }
+
   if (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE) {
     console.error(`Lighthouse debugger connection timed out ${RETRIES} times. Giving up.`);
     process.exit(1);
-  } else if (runResults.status !== 0
-     && runResults.status !== PAGE_HUNG_EXIT_CODE
-     && runResults.status !== INSECURE_DOCUMENT_REQUEST_EXIT_CODE) {
+  } else if (isUnexpectedFatalResult(runResults.status, outputPath)) {
     console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`);
     console.error(runResults.stderr);
     process.exit(runResults.status);
   }
 
-  if (isDebug) {
-    console.log(`STDOUT: ${runResults.stdout}`);
-    console.error(`STDERR: ${runResults.stderr}`);
-  }
-
   let errorCode;
   let lhr = {requestedUrl: url, finalUrl: url, audits: {}};
   if (runResults.status === PAGE_HUNG_EXIT_CODE) {

lighthouse-core/gather/gather-runner.js

@@ -48,10 +48,11 @@ class GatherRunner {
    * Loads options.url with specified options. If the main document URL
    * redirects, options.url will be updated accordingly. As such, options.url
    * will always represent the post-redirected URL. options.requestedUrl is the
-   * pre-redirect starting URL.
+   * pre-redirect starting URL. If the navigation errors with "expected" errors such as
+   * NO_FCP, a `navigationError` is returned.
    * @param {Driver} driver
    * @param {LH.Gatherer.PassContext} passContext
-   * @return {Promise<void>}
+   * @return {Promise<{navigationError?: LH.LighthouseError}>}
    */
   static async loadPage(driver, passContext) {
     const gatherers = passContext.passConfig.gatherers;
@@ -61,15 +62,25 @@ class GatherRunner {
       args: [gatherers.map(g => g.instance.name).join(', ')],
     };
     log.time(status);
+    try {
+      const finalUrl = await driver.gotoURL(passContext.url, {
+        waitForFCP: passContext.passConfig.recordTrace,
+        waitForLoad: true,
+        passContext,
+      });
+      passContext.url = finalUrl;
+    } catch (err) {
+      // If it's one of our loading-based LHErrors, we'll treat it as a page load error.
+      if (err.code === 'NO_FCP') {
+        return {navigationError: err};
+      }
 
-    const finalUrl = await driver.gotoURL(passContext.url, {
-      waitForFCP: passContext.passConfig.recordTrace,
-      waitForLoad: true,
-      passContext,
-    });
-    passContext.url = finalUrl;
+      throw err;
+    } finally {
+      log.timeEnd(status);
+    }
 
-    log.timeEnd(status);
+    return {};
   }
 
   /**
@@ -122,7 +133,7 @@ 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
-   * @return {LHError|undefined}
+   * @return {LH.LighthouseError|undefined}
    */
   static getNetworkError(url, networkRecords) {
     const mainRecord = networkRecords.find(record => {
@@ -161,14 +172,24 @@ class GatherRunner {
    * main document request failure, a security issue, etc.
    * @param {LH.Gatherer.PassContext} passContext
    * @param {LH.Gatherer.LoadData} loadData
+   * @param {LighthouseError|undefined} navigationError
+   * @return {LighthouseError|undefined}
    */
-  static getPageLoadError(passContext, loadData) {
+  static getPageLoadError(passContext, loadData, navigationError) {
     const networkError = GatherRunner.getNetworkError(passContext.url, loadData.networkRecords);
 
     //  If the driver was offline, the load will fail without offline support. Ignore this case.
     if (!passContext.driver.online) return;
 
-    return networkError;
+    // Network errors are usually the most specific and provide the best reason for why the page failed to load.
+    // Prefer networkError over navigationError.
+    // Example: `DNS_FAILURE` is better than `NO_FCP`.
+    if (networkError) return networkError;
+
+    // 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.
+    return navigationError;
   }
 
   /**
@@ -576,7 +597,7 @@ class GatherRunner {
 
     // Navigate, start recording, and run `pass()` on gatherers.
     await GatherRunner.beginRecording(passContext);
-    await GatherRunner.loadPage(driver, passContext);
+    const {navigationError: possibleNavError} = await GatherRunner.loadPage(driver, passContext);
     await GatherRunner.pass(passContext, gathererResults);
     const loadData = await GatherRunner.endRecording(passContext);
 
@@ -589,7 +610,7 @@ class GatherRunner {
     if (loadData.trace) baseArtifacts.traces[passConfig.passName] = loadData.trace;
 
     // If there were any load errors, treat all gatherers as if they errored.
-    const pageLoadError = GatherRunner.getPageLoadError(passContext, loadData);
+    const pageLoadError = GatherRunner.getPageLoadError(passContext, loadData, possibleNavError);
     if (pageLoadError) {
       log.error('GatherRunner', pageLoadError.friendlyMessage, passContext.url);
       passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage);

lighthouse-core/test/gather/gather-runner-test.js

@@ -13,6 +13,8 @@ const assert = require('assert');
 const Config = require('../../config/config.js');
 const unresolvedPerfLog = require('./../fixtures/unresolved-perflog.json');
 const NetworkRequest = require('../../lib/network-request.js');
+const LHError = require('../../lib/lh-error.js');
+const networkRecordsToDevtoolsLog = require('../network-records-to-devtools-log.js');
 
 jest.mock('../../lib/stack-collector.js', () => () => Promise.resolve([]));
 
@@ -100,7 +102,7 @@ describe('GatherRunner', function() {
     };
 
     const passContext = {
-      requestedUrl: url1,
+      url: url1,
       settings: {},
       passConfig: {
         gatherers: [],
@@ -112,6 +114,26 @@ describe('GatherRunner', function() {
     });
   });
 
+  it('loads a page and returns a pageLoadError', async () => {
+    const url = 'https://example.com';
+    const error = new LHError(LHError.errors.NO_FCP);
+    const driver = {
+      gotoURL() {
+        return Promise.reject(error);
+      },
+    };
+
+    const passContext = {
+      url,
+      settings: {},
+      passConfig: {gatherers: []},
+    };
+
+    const {navigationError} = await GatherRunner.loadPage(driver, passContext);
+    expect(navigationError).toEqual(error);
+    expect(passContext.url).toEqual(url);
+  });
+
   it('collects benchmark as an artifact', async () => {
     const url = 'https://example.com';
     const driver = fakeDriver;
@@ -396,6 +418,73 @@ describe('GatherRunner', function() {
     assert.equal(tests.calledCleanBrowserCaches, true);
   });
 
+  it('fails 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
+    const navigationError = new LHError(LHError.errors.NO_FCP);
+    const driver = Object.assign({}, fakeDriver, {
+      online: true,
+      gotoURL: url => url.includes('blank') ? null : Promise.reject(navigationError),
+    });
+
+    const passConfig = {
+      gatherers: [
+        {instance: new TestGatherer()},
+      ],
+    };
+
+    const settings = {};
+
+    const passContext = {
+      url: requestedUrl,
+      driver,
+      passConfig,
+      settings,
+      LighthouseRunWarnings: [],
+      baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}),
+    };
+
+    const {artifacts} = await GatherRunner.runPass(passContext);
+    expect(passContext.LighthouseRunWarnings).toHaveLength(1);
+    expect(artifacts.TestGatherer).toBeInstanceOf(Error);
+    expect(artifacts.TestGatherer.code).toEqual('NO_DOCUMENT_REQUEST');
+  });
+
+  it('fails 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);
+    const driver = Object.assign({}, fakeDriver, {
+      online: true,
+      gotoURL: url => url.includes('blank') ? null : Promise.reject(navigationError),
+      endDevtoolsLog() {
+        return networkRecordsToDevtoolsLog([{url: requestedUrl}]);
+      },
+    });
+
+    const passConfig = {
+      gatherers: [
+        {instance: new TestGatherer()},
+      ],
+    };
+
+    const settings = {};
+
+    const passContext = {
+      url: requestedUrl,
+      driver,
+      passConfig,
+      settings,
+      LighthouseRunWarnings: [],
+      baseArtifacts: await GatherRunner.initializeBaseArtifacts({driver, settings, requestedUrl}),
+    };
+
+    const {artifacts} = await GatherRunner.runPass(passContext);
+    expect(passContext.LighthouseRunWarnings).toHaveLength(1);
+    expect(artifacts.TestGatherer).toBeInstanceOf(Error);
+    expect(artifacts.TestGatherer.code).toEqual('NO_FCP');
+  });
+
   it('does not clear origin storage with flag --disable-storage-reset', () => {
     const asyncFunc = () => Promise.resolve();
     const tests = {

types/lhr.d.ts

@@ -8,6 +8,8 @@ import LHError = require('../lighthouse-core/lib/lh-error.js');
 
 declare global {
   module LH {
+    export type LighthouseError = LHError;
+
     export type I18NMessageEntry = string | {path: string, values: any};
 
     export interface I18NMessages {