core: throw fatally on page hang #6497

patrickhulce · 2018-11-07T03:48:15Z

Summary
I've spent way too much time waffling back and forth on how to handle this, so I thought I'd just put it up for discussion. This PR throws a fatal error if, after our maxWaitForLoad is reached, the page doesn't seem to respond.

Right now this basically gets us earlier exit than trying to do all the gathering and not much else. I agree with @brendankenny that it's a bit dangerous to try to kill JavaScript and continue. If we do anything else I think it might be alternative 1, but still feels risky.

Alternatives:

Wait for maxWaitForLoad, kill JavaScript, wait and see if page is still hung, then either fatally throw or try to continue
Wait for maxWaitForLoad, kill JavaScript, and try to continue right away
Continuously check if page is hung, kill JavaScript as soon as we think it's hung, and do A or B

brendankenny · 2018-11-08T00:21:28Z

lighthouse-cli/test/fixtures/infinite-loop.html

+  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 end.


🤣🤣 nicely done

(never ends :)

brendankenny · 2018-11-08T00:21:47Z

lighthouse-cli/test/smokehouse/error-config.js

@@ -0,0 +1,19 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


brendankenny · 2018-11-08T00:22:23Z

lighthouse-cli/test/fixtures/infinite-loop.html

@@ -0,0 +1,16 @@
+<html>


brendankenny · 2018-11-08T00:23:10Z

lighthouse-cli/test/smokehouse/error-expectations.js

+'use strict';
+
+/**
+ * Expected Lighthouse audit values for sites with various errors.


brendankenny · 2018-11-08T00:25:51Z

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -98,10 +99,12 @@ function runLighthouse(url, configPath, isDebug) {
    console.error(`STDERR: ${runResults.stderr}`);
  }

-  const lhr = fs.readFileSync(outputPath, 'utf8');
+  const lhr = runResults.status === PAGE_HUNG_EXIT_CODE ?


since we don't really do anything else with it below (e.g. no path to debug output), could this be just an early return of the PAGE_HUNG LHR?

yeah good point, done!

brendankenny · 2018-11-08T00:43:42Z

lighthouse-core/gather/driver.js

-    this._nextProtocolTimeout = DEFAULT_PROTOCOL_TIMEOUT;
+    const timeout = typeof this._nextProtocolTimeout === 'undefined' ?
+      DEFAULT_PROTOCOL_TIMEOUT : this._nextProtocolTimeout;
+    this._nextProtocolTimeout = undefined;


hmm, I don't follow what this is doing. It seems the default state goes to undefined, which now gets interpreted as "use the default", but why was that needed for an extra signal?

removed it all 👍

brendankenny · 2018-11-08T00:45:44Z

re: alternatives, I personally vote for what's here and then ramping up in the future if we think we can get away with them

paulirish · 2018-11-07T11:23:10Z

lighthouse-cli/test/fixtures/infinite-loop.html

+  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,


paulirish · 2018-11-07T11:23:39Z

lighthouse-cli/test/smokehouse/error-config.js

@@ -0,0 +1,19 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


its like finding a pokemon! got 'em!

paulirish · 2018-11-08T10:42:30Z

lighthouse-core/gather/driver.js

+
+        if (await this.isPageHung()) {
+          log.warn('Driver', 'Page appears to be hung, killing JavaScript...');
+          await this.sendCommand('Runtime.terminateExecution');


i think we should also run setScriptDisabled({disabled: true}) at this point.

paulirish · 2018-11-08T10:45:14Z

lighthouse-core/gather/driver.js

+  async isPageHung() {
+    try {
+      this.setNextProtocolTimeout(5000);
+      await this.evaluateAsync('"ping"');


to simplify we could do a pure sendCommand('Runtime.evalute') here. we don't really need all that promise wrapping stuff with something so simple.

but i guess it also means we could probably avoid some of the nextTimeout juggling we have since _evaluateInContext has its own timeout.

double bonus?! :)

paulirish · 2018-11-08T10:48:14Z

lighthouse-core/gather/driver.js

+   */
+  async isPageHung() {
+    try {
+      this.setNextProtocolTimeout(5000);


5s feels generous. why not 1s

well for turning a run fatal it seemed like we wanted to be pretty sure it's hung and not just hitting a long task during a slow page load, but if you think 1s is enough and 45s pages be damned, I'm on board :)

paulirish · 2018-11-08T10:58:51Z

lighthouse-core/gather/driver.js

@@ -749,7 +773,7 @@ class Driver {
      loadPromise,
      maxTimeoutPromise,
    ]);
-    cleanupFn();
+    await cleanupFn();


now necessary since we have the awaits inside of maxTimeoutPromise, right?

well the awaits are inside the function that it returns too, so we want to wait to find out if the page is hung before moving on

order of execution is

wait for earlier of load + quiet and maxWaitForLoad

If we reached maxWaitForLoad then check if we're hung

we don't want to be doing our killing and hang check while we're trying to see if the page is quiet.

paulirish · 2018-11-08T10:59:12Z

lighthouse-core/lib/lh-error.js

+  /* Used when the page stopped responding and did not finish loading. */
+  PAGE_HUNG: {
+    code: 'PAGE_HUNG',
+    message: strings.pageLoadFailed,


can we do a new string?

sure, the one I have is largely the same as pageLoadFailed though, so if you have particular hopes of what you want to convey, input welcome :)

paulirish · 2018-11-08T11:00:35Z

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: {


this'll need to go into the proto. :)

paulirish

yes I think we should do this.

I will point out, I think it's possible for the hang to happen during mid-gathering, in which case we wouldn't catch this.

After landing, I think next step is to drop protocol timeout to 10-ish seconds. And then run isPageHung() between gatherers. Unfortch, this overkill is only necessary because of LR, but... yah.

paulirish · 2018-11-16T08:19:02Z

@patrickhulce some of the smokehouse tests are broke. can you take a look

patrickhulce · 2018-11-16T15:15:47Z

some of the smokehouse tests are broke

sneaky, it was an extra empty object definition in the smoketests {} 😈

core: throw fatally on page hang

0fa7840

patrickhulce requested review from brendankenny and paulirish as code owners November 7, 2018 03:48

brendankenny reviewed Nov 8, 2018

View reviewed changes

paulirish requested changes Nov 8, 2018

View reviewed changes

feedback

18ef012

patrickhulce requested a review from exterkamp as a code owner November 8, 2018 14:55

patrickhulce added 2 commits November 8, 2018 09:30

error config description

60dea14

lint

10ba80c

paulirish approved these changes Nov 16, 2018

View reviewed changes

remove empty smoketest

fb6c86a

paulirish merged commit 4457ed2 into master Nov 16, 2018

paulirish deleted the fatal_error_on_hang branch November 16, 2018 16:32

This was referenced Nov 16, 2018

core(driver): assert page not hung after each gatherer #6585

Closed

DevTools Error: The asynchronous expression exceeded the allotted time of 60 #6604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: throw fatally on page hang #6497

core: throw fatally on page hang #6497

patrickhulce commented Nov 7, 2018

brendankenny Nov 8, 2018

brendankenny Nov 8, 2018

brendankenny Nov 8, 2018

patrickhulce Nov 8, 2018

brendankenny Nov 8, 2018

patrickhulce Nov 8, 2018

brendankenny Nov 8, 2018

brendankenny Nov 8, 2018

patrickhulce Nov 8, 2018

brendankenny Nov 8, 2018

patrickhulce Nov 8, 2018

brendankenny commented Nov 8, 2018

paulirish Nov 7, 2018

paulirish Nov 7, 2018

paulirish Nov 8, 2018

patrickhulce Nov 8, 2018

paulirish Nov 8, 2018

patrickhulce Nov 8, 2018

paulirish Nov 8, 2018

patrickhulce Nov 8, 2018

paulirish Nov 8, 2018

patrickhulce Nov 8, 2018

patrickhulce Nov 8, 2018

paulirish Nov 8, 2018

patrickhulce Nov 8, 2018

paulirish Nov 8, 2018

patrickhulce Nov 8, 2018

paulirish left a comment

paulirish commented Nov 16, 2018

patrickhulce commented Nov 16, 2018

		@@ -0,0 +1,19 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

core: throw fatally on page hang #6497

core: throw fatally on page hang #6497

Conversation

patrickhulce commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

paulirish commented Nov 16, 2018

patrickhulce commented Nov 16, 2018