core(gather-runner): convert PAGE_HUNG to non-fatal runtimeError (#9121)

GoogleChrome · Jun 5, 2019 · 9ebae67 · 9ebae67
1 parent ccc5a92
commit 9ebae67
Show file tree

Hide file tree

Showing 8 changed files with 30 additions and 73 deletions.
diff --git a/clients/extension/scripts/popup.js b/clients/extension/scripts/popup.js
@@ -13,14 +13,7 @@
  * Lighthouse itself, mapped to more useful strings to report to the user.
  */
 const NON_BUG_ERROR_MESSAGES = {
-  // The user tries to review an error page or has network issues
-  'ERRORED_DOCUMENT_REQUEST': 'Unable to load the page. Please verify the url you ' +
-      'are trying to review.',
-  'FAILED_DOCUMENT_REQUEST': 'Unable to load the page. Please verify the url you ' +
-      'are trying to review.',
   'DNS_FAILURE': 'DNS servers could not resolve the provided domain.',
-  'INSECURE_DOCUMENT_REQUEST': 'The URL you have provided does not have a valid' +
-      ' SSL certificate.',
   'INVALID_URL': 'Lighthouse can only audit URLs that start' +
       ' with http:// or https://.',
 

diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js
@@ -24,8 +24,6 @@ const opn = require('opn');
 
 const _RUNTIME_ERROR_CODE = 1;
 const _PROTOCOL_TIMEOUT_EXIT_CODE = 67;
-const _PAGE_HUNG_EXIT_CODE = 68;
-const _INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69;
 
 /**
  * exported for testing
@@ -76,18 +74,6 @@ function printProtocolTimeoutErrorAndExit() {
   return process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE);
 }
 
-/** @param {LighthouseError} err @return {never} */
-function printPageHungErrorAndExit(err) {
-  console.error('Page hung:', err.friendlyMessage);
-  return process.exit(_PAGE_HUNG_EXIT_CODE);
-}
-
-/** @param {LighthouseError} err @return {never} */
-function printInsecureDocumentRequestErrorAndExit(err) {
-  console.error('Insecure document request:', err.friendlyMessage);
-  return process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE);
-}
-
 /**
  * @param {LighthouseError} err
  * @return {never}
@@ -109,10 +95,6 @@ function printErrorAndExit(err) {
     return printConnectionErrorAndExit();
   } else if (err.code === 'CRI_TIMEOUT') {
     return printProtocolTimeoutErrorAndExit();
-  } else if (err.code === 'PAGE_HUNG') {
-    return printPageHungErrorAndExit(err);
-  } else if (err.code === 'INSECURE_DOCUMENT_REQUEST') {
-    return printInsecureDocumentRequestErrorAndExit(err);
   } else {
     return printRuntimeErrorAndExit(err);
   }

diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js
@@ -10,19 +10,19 @@
  */
 module.exports = [
   {
-    errorCode: 'PAGE_HUNG',
     lhr: {
       requestedUrl: 'http://localhost:10200/infinite-loop.html',
       finalUrl: 'http://localhost:10200/infinite-loop.html',
       audits: {},
+      runtimeError: {code: 'PAGE_HUNG'},
     },
   },
   {
-    errorCode: undefined,
     lhr: {
       requestedUrl: 'https://expired.badssl.com',
       finalUrl: 'https://expired.badssl.com/',
       audits: {},
+      runtimeError: {code: 'FAILED_DOCUMENT_REQUEST'},
     },
   },
 ];
diff --git a/lighthouse-cli/test/smokehouse/smokehouse-report.js b/lighthouse-cli/test/smokehouse/smokehouse-report.js
@@ -155,12 +155,6 @@ function collateResults(actual, expected) {
   });
 
   return [
-    {
-      name: 'error code',
-      actual: actual.errorCode,
-      expected: expected.errorCode,
-      equal: actual.errorCode === expected.errorCode,
-    },
     {
       name: 'final url',
       actual: actual.lhr.finalUrl,

diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js
@@ -16,8 +16,6 @@ const log = require('lighthouse-logger');
 const {collateResults, report} = require('./smokehouse-report.js');
 
 const PROTOCOL_TIMEOUT_EXIT_CODE = 67;
-const PAGE_HUNG_EXIT_CODE = 68;
-const INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69;
 const RETRIES = 3;
 
 /**
@@ -38,21 +36,6 @@ 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
@@ -108,25 +91,17 @@ function runLighthouse(url, configPath, isDebug) {
   if (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE) {
     console.error(`Lighthouse debugger connection timed out ${RETRIES} times. Giving up.`);
     process.exit(1);
-  } else if (isUnexpectedFatalResult(runResults.status, outputPath)) {
-    console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`);
+  } else if (!fs.existsSync(outputPath)) {
+    console.error(`Lighthouse run failed to produce a report and exited with ${runResults.status}. stderr to follow:`); // eslint-disable-line max-len
     console.error(runResults.stderr);
     process.exit(runResults.status);
   }
 
-  let errorCode;
-  let lhr = {requestedUrl: url, finalUrl: url, audits: {}};
-  if (runResults.status === PAGE_HUNG_EXIT_CODE) {
-    errorCode = 'PAGE_HUNG';
-  } else if (runResults.status === INSECURE_DOCUMENT_REQUEST_EXIT_CODE) {
-    errorCode = 'INSECURE_DOCUMENT_REQUEST';
+  const lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8'));
+  if (isDebug) {
+    console.log('LHR output available at: ', outputPath);
   } else {
-    lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8'));
-    if (isDebug) {
-      console.log('LHR output available at: ', outputPath);
-    } else if (fs.existsSync(outputPath)) {
-      fs.unlinkSync(outputPath);
-    }
+    fs.unlinkSync(outputPath);
   }
 
   // Artifacts are undefined if they weren't written to disk (e.g. if there was an error).
@@ -136,7 +111,6 @@ function runLighthouse(url, configPath, isDebug) {
   } catch (e) {}
 
   return {
-    errorCode,
     lhr,
     artifacts,
   };

diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
@@ -1452,7 +1452,7 @@ class Driver {
    * @param {string} url
    * @return {Promise<void>}
    */
-  clearDataForOrigin(url) {
+  async clearDataForOrigin(url) {
     const origin = new URL(url).origin;
 
     // Clear all types of storage except cookies, so the user isn't logged out.
@@ -1469,10 +1469,22 @@ class Driver {
       'cache_storage',
     ].join(',');
 
-    return this.sendCommand('Storage.clearDataForOrigin', {
-      origin: origin,
-      storageTypes: typesToClear,
-    });
+    // `Storage.clearDataForOrigin` is one of our PROTOCOL_TIMEOUT culprits and this command is also
+    // run in the context of PAGE_HUNG to cleanup. We'll keep the timeout low and just warn if it fails.
+    this.setNextProtocolTimeout(5000);
+
+    try {
+      await this.sendCommand('Storage.clearDataForOrigin', {
+        origin: origin,
+        storageTypes: typesToClear,
+      });
+    } catch (err) {
+      if (/** @type {LH.LighthouseError} */(err).code === 'PROTOCOL_TIMEOUT') {
+        log.warn('Driver', 'clearDataForOrigin timed out');
+      } else {
+        throw err;
+      }
+    }
   }
 
   /**

diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
@@ -71,7 +71,7 @@ class GatherRunner {
       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') {
+      if (err.code === 'NO_FCP' || err.code === 'PAGE_HUNG') {
         return {navigationError: err};
       }
 
@@ -452,7 +452,7 @@ class GatherRunner {
       traces: {},
       devtoolsLogs: {},
       settings: options.settings,
-      URL: {requestedUrl: options.requestedUrl, finalUrl: ''},
+      URL: {requestedUrl: options.requestedUrl, finalUrl: options.requestedUrl},
       Timing: [],
     };
   }
@@ -552,6 +552,9 @@ class GatherRunner {
         const passResults = await GatherRunner.runPass(passContext);
         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 (isFirstPass) {
           await GatherRunner.populateBaseArtifacts(passContext);
           isFirstPass = false;

diff --git a/types/smokehouse.d.ts b/types/smokehouse.d.ts
@@ -22,7 +22,6 @@
   export type ExpectedLHR = Pick<LH.Result, 'audits' | 'finalUrl' | 'requestedUrl'>
 
   export type ExpectedRunnerResult = {
-    errorCode?: string;
     lhr: ExpectedLHR,
     artifacts?: Partial<LH.Artifacts>
   }