Repository: GoogleChrome/lighthouse
Pull request #6578 (2018-11-16 08:04:12)

core(driver): assert securityIssues before getPageLoadError
I _think_ we want to throw on security issues before we consider pageLoadError situations. wdyt?


Patch:
diff --git a/clients/extension/manifest.json b/clients/extension/manifest.json
index 21bc64040..8093ed4b8 100644
--- a/clients/extension/manifest.json
+++ b/clients/extension/manifest.json
@@ -1,6 +1,6 @@
 {
   "name": "__MSG_appName__",
-  "version": "3.3.0.4000",
+  "version": "3.3.0.4001",
   "minimum_chrome_version": "66",
   "manifest_version": 2,
   "description": "__MSG_appDescription__",
diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js
index 6a55fe5f5..52661c3dd 100644
--- a/lighthouse-cli/run.js
+++ b/lighthouse-cli/run.js
@@ -22,6 +22,7 @@ const opn = require('opn');
 
 const _RUNTIME_ERROR_CODE = 1;
 const _PROTOCOL_TIMEOUT_EXIT_CODE = 67;
+const _PAGE_HUNG_EXIT_CODE = 68;
 
 /**
  * exported for testing
@@ -70,6 +71,12 @@ function showProtocolTimeoutError() {
   process.exit(_PROTOCOL_TIMEOUT_EXIT_CODE);
 }
 
+/** @param {LH.LighthouseError} err */
+function showPageHungError(err) {
+  console.error('Page hung:', err.friendlyMessage);
+  process.exit(_PAGE_HUNG_EXIT_CODE);
+}
+
 /**
  * @param {LH.LighthouseError} err
  */
@@ -89,6 +96,8 @@ function handleError(err) {
     showConnectionError();
   } else if (err.code === 'CRI_TIMEOUT') {
     showProtocolTimeoutError();
+  } else if (err.code === 'PAGE_HUNG') {
+    showPageHungError(err);
   } else {
     showRuntimeError(err);
   }
diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
index 01decbb6a..7444607e6 100644
--- a/lighthouse-core/gather/driver.js
+++ b/lighthouse-core/gather/driver.js
@@ -369,6 +369,7 @@ class Driver {
       includeCommandLineAPI: true,
       awaitPromise: true,
       returnByValue: true,
+      timeout: 60000,
       contextId,
     };
 
@@ -687,6 +688,25 @@ class Driver {
     };
   }
 
+  /**
+   * Returns whether the page appears to be hung.
+   * @return {Promise<boolean>}
+   */
+  async isPageHung() {
+    try {
+      this.setNextProtocolTimeout(1000);
+      await this.sendCommand('Runtime.evaluate', {
+        expression: '"ping"',
+        returnByValue: true,
+        timeout: 1000,
+      });
+
+      return false;
+    } catch (err) {
+      return true;
+    }
+  }
+
   /**
    * Returns a promise that resolves when:
    * - All of the following conditions have been met:
@@ -736,11 +756,18 @@ class Driver {
     const maxTimeoutPromise = new Promise((resolve, reject) => {
       maxTimeoutHandle = setTimeout(resolve, maxWaitForLoadedMs);
     }).then(_ => {
-      return function() {
-        log.warn('Driver', 'Timed out waiting for page load. Moving on...');
+      return async () => {
+        log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...');
         waitForLoadEvent.cancel();
         waitForNetworkIdle.cancel();
         waitForCPUIdle && waitForCPUIdle.cancel();
+
+        if (await this.isPageHung()) {
+          log.warn('Driver', 'Page appears to be hung, killing JavaScript...');
+          await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: true});
+          await this.sendCommand('Runtime.terminateExecution');
+          throw new LHError(LHError.errors.PAGE_HUNG);
+        }
       };
     });
 
@@ -749,7 +776,7 @@ class Driver {
       loadPromise,
       maxTimeoutPromise,
     ]);
-    cleanupFn();
+    await cleanupFn();
   }
 
   /**
diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
index 6eaeb4b07..7c65e840b 100644
--- a/lighthouse-core/gather/gather-runner.js
+++ b/lighthouse-core/gather/gather-runner.js
@@ -300,6 +300,8 @@ class GatherRunner {
     const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);
     log.timeEnd(status);
 
+    this.assertNoSecurityIssues(driver.getSecurityState());
+
     let pageLoadError = GatherRunner.getPageLoadError(passContext.url, networkRecords);
     // If the driver was offline, a page load error is expected, so do not save it.
     if (!driver.online) pageLoadError = undefined;
@@ -309,8 +311,6 @@ class GatherRunner {
       passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage);
     }
 
-    this.assertNoSecurityIssues(driver.getSecurityState());
-
     // Expose devtoolsLog, networkRecords, and trace (if present) to gatherers
     /** @type {LH.Gatherer.LoadData} */
     const passData = {
diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js
index 2a09ba497..9b4049cf2 100644
--- a/lighthouse-core/lib/lh-error.js
+++ b/lighthouse-core/lib/lh-error.js
@@ -148,6 +148,12 @@ const ERRORS = {
     message: strings.pageLoadFailedInsecure,
     lhrRuntimeError: true,
   },
+  /* Used when the page stopped responding and did not finish loading. */
+  PAGE_HUNG: {
+    code: 'PAGE_HUNG',
+    message: strings.pageLoadFailedHung,
+    lhrRuntimeError: true,
+  },
 
   // Protocol internal failures
   TRACING_ALREADY_STARTED: {
diff --git a/lighthouse-core/lib/strings.js b/lighthouse-core/lib/strings.js
index 377a4117e..ead92c977 100644
--- a/lighthouse-core/lib/strings.js
+++ b/lighthouse-core/lib/strings.js
@@ -11,6 +11,7 @@ module.exports = {
   badTraceRecording: `Something went wrong with recording the trace over your page load. Please run Lighthouse again.`,
   pageLoadTookTooLong: `Your page took too long to load. Please follow the opportunities in the report to reduce your page load time, and then try re-running Lighthouse.`,
   pageLoadFailed: `Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests.`,
+  pageLoadFailedHung: `Lighthouse was unable to reliably load the URL you requested because the page stopped responding.`,
   pageLoadFailedInsecure: `The URL you have provided does not have valid security credentials.`,
   internalChromeError: `An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.`,
   requestContentTimeout: 'Fetching resource content has exceeded the allotted time',
diff --git a/proto/lighthouse-result.proto b/proto/lighthouse-result.proto
index befb32303..a34ce7abb 100644
--- a/proto/lighthouse-result.proto
+++ b/proto/lighthouse-result.proto
@@ -47,6 +47,8 @@ enum LighthouseError {
   INSECURE_DOCUMENT_REQUEST = 16;
   // Used when protocol command times out.
   PROTOCOL_TIMEOUT = 17;
+  // Used when the page is not responding after maxWaitForLoad.
+  PAGE_HUNG = 18;
 }
 
 // The overarching Lighthouse Response object (LHR)


Test patch:
diff --git a/lighthouse-cli/test/fixtures/infinite-loop.html b/lighthouse-cli/test/fixtures/infinite-loop.html
new file mode 100644
index 000000000..2b77d7f1c
--- /dev/null
+++ b/lighthouse-cli/test/fixtures/infinite-loop.html
@@ -0,0 +1,22 @@
+<!doctype html>
+<!--
+ * Copyright 2018 Google Inc. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+-->
+<html>
+<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
+<body>
+  This is the function that never ends
+  Yes, it just goes on and on my friends.
+  Some people started computing it, not knowing what it was.
+  And they'll continue computing it forever just because,
+  This is the function that never ends.
+
+  <script>
+    while (true) {
+      for (let i = 0; i < 1000000; i++) ;
+    }
+  </script>
+</body>
+</html>
diff --git a/lighthouse-cli/test/smokehouse/error-config.js b/lighthouse-cli/test/smokehouse/error-config.js
new file mode 100644
index 000000000..771bdf604
--- /dev/null
+++ b/lighthouse-cli/test/smokehouse/error-config.js
@@ -0,0 +1,19 @@
+/**
+ * @license Copyright 2018 Google Inc. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+'use strict';
+
+/**
+ * Config file for sites with various errors, just fail out quickly.
+ */
+module.exports = {
+  extends: 'lighthouse:default',
+  settings: {
+    maxWaitForLoad: 5000,
+    onlyAudits: [
+      'first-contentful-paint',
+    ],
+  },
+};
diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js
new file mode 100644
index 000000000..40d057806
--- /dev/null
+++ b/lighthouse-cli/test/smokehouse/error-expectations.js
@@ -0,0 +1,18 @@
+/**
+ * @license Copyright 2018 Google Inc. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+'use strict';
+
+/**
+ * Expected Lighthouse audit values for sites with various errors.
+ */
+module.exports = [
+  {
+    requestedUrl: 'http://localhost:10200/infinite-loop.html',
+    finalUrl: 'http://localhost:10200/infinite-loop.html',
+    errorCode: 'PAGE_HUNG',
+    audits: {},
+  },
+];
diff --git a/lighthouse-cli/test/smokehouse/run-smoke.js b/lighthouse-cli/test/smokehouse/run-smoke.js
index 93e71efae..12f2cb86d 100644
--- a/lighthouse-cli/test/smokehouse/run-smoke.js
+++ b/lighthouse-cli/test/smokehouse/run-smoke.js
@@ -29,6 +29,11 @@ const SMOKETESTS = [{
   config: smokehouseDir + 'a11y/a11y-config.js',
   expectations: 'a11y/expectations.js',
   batch: 'parallel-first',
+}, {
+  id: 'errors',
+  expectations: smokehouseDir + 'error-expectations.js',
+  config: smokehouseDir + 'error-config.js',
+  batch: 'errors',
 }, {
   id: 'pwa',
   expectations: smokehouseDir + 'pwa-expectations.js',
diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js
index 8510b4c0b..c3bf337e9 100755
--- a/lighthouse-cli/test/smokehouse/smokehouse.js
+++ b/lighthouse-cli/test/smokehouse/smokehouse.js
@@ -15,6 +15,7 @@ const yargs = require('yargs');
 const log = require('lighthouse-logger');
 
 const PROTOCOL_TIMEOUT_EXIT_CODE = 67;
+const PAGE_HUNG_EXIT_CODE = 68;
 const RETRIES = 3;
 const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/;
 
@@ -87,7 +88,7 @@ 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 (runResults.status !== 0) {
+  } else if (runResults.status !== 0 && runResults.status !== PAGE_HUNG_EXIT_CODE) {
     console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`);
     console.error(runResults.stderr);
     process.exit(runResults.status);
@@ -98,10 +99,14 @@ function runLighthouse(url, configPath, isDebug) {
     console.error(`STDERR: ${runResults.stderr}`);
   }
 
+  if (runResults.status === PAGE_HUNG_EXIT_CODE) {
+    return {requestedUrl: url, finalUrl: url, errorCode: 'PAGE_HUNG', audits: {}};
+  }
+
   const lhr = fs.readFileSync(outputPath, 'utf8');
   if (isDebug) {
     console.log('LHR output available at: ', outputPath);
-  } else {
+  } else if (fs.existsSync(outputPath)) {
     fs.unlinkSync(outputPath);
   }