Running with headless Chrome crashes when provided with non-existent url.

[jburger424]

Provide the steps to reproduce

1. lighthouse https://not-a-url.com --chrome-flags=--headless

What is the current behavior?

events.js:167
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at TCP.onStreamRead (internal/stream_base_commons.js:111:27)
Emitted 'error' event at:
    at Socket.socketErrorListener (_http_client.js:391:9)
    at Socket.emit (events.js:182:13)
    at emitErrorNT (internal/streams/destroy.js:82:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
    at process._tickCallback (internal/process/next_tick.js:63:19)

No report is saved.

What is the expected behavior?

Runner:warn ... encountered an error: DNS_FAILURE

Report should save with following error:

There were issues affecting this run of Lighthouse:
  DNS servers could not resolve the provided domain.

Environment Information

• Affected Channels: CLI
• Lighthouse version: 4.2.0
• Node.js version: 6.9.0
• Operating System: Debian GNU/Linux

Related issues

[patrickhulce]

FWIW this seems linux specific as I cannot reproduce on Mac. Resident linux folks can take a look? :)

If Chrome is crashing, then it's probably worth filing a crbug instead of just LH bug.

[brendankenny]

I'm able to get the same thing on my Mac.

We should be catching this, so I assume (based on that and the error) it's happening in an event and so outside our normal execution flow.

[jburger424]

It looks like this is a new issue in 4.2.0, unable to reproduce on 4.1.0.

[brendankenny]

hmm, at first glance it appears to be from running this line with an already killed Chrome instance, which is supposed to not do this :)

lighthouse/lighthouse-core/gather/driver.js

Lines 821 to 822 in 9efe177

	// TODO(@patrickhulce): cancel() should really be a promise itself to handle things like this
	this.sendCommand('Security.disable').catch(() => {});

[brendankenny]

sorry, I take that back. That line happily runs and returns, it just happens to occur while other things are going wrong.

Looks like it's an error off the GET /json/close/... request in CriConnection._runJsonCommand as we driver.disconnect() and we don't have an error event handler on it, so it throws all the way to the top.

We can just add an error handler in there and reject in that case (just have to handle interaction with the CONNECT_TIMEOUT case's error handler).

Not sure why this would have changed in 4.2, though. Something in run.js after #7339? It's not clear how this would have been in the promise chain before, though.

[brendankenny]

bisected to #7356

[patrickhulce]

Woah my bad, I was on branch split a long time ago 😆

We should definitely fix the unhelpful/misleading stack, but I'm not sure we actually want to change the end behavior. I was under the impression everyone else was on board with converting NO_FCP to a loud, fatal error.

[brendankenny]

We should definitely fix the unhelpful/misleading stack, but I'm not sure we actually want to change the end behavior. I was under the impression everyone else was on board with converting NO_FCP to a loud, fatal error.

I see now. There's no FCP event fired, and since that's checked before page load errors and we want it to fail loudly, it's the resulting error instead of DNS_FAILURE.

It does seem like DNS_FAILURE is the more helpful error to tell what went wrong in this case. I don't know the best way to sequence this, though.

Should NO_FCP not be thrown from Driver._waitForFullyLoaded and instead it would resolve early so we aren't left waiting? GatherRunner.getPageLoadError would then run and catch this case, and if not a page load error, then (eventually) NO_FCP would come up from the audits of the trace (or could even be added as another check in getPageLoadError). That does seem to break the pattern of _waitForFullyLoaded though, where if we know we're in an error case it rejects.

[patrickhulce]

Yeah for sure, I'm all on board for ensuring the most helpful message is sent to the user. Just wanted to make sure we're on the same page that the fix won't involve giving a report and exiting with 0 👍

I think it makes sense to check for other failures (security/dns/offline/etc) before choosing NO_FCP as the fatal error.

[benschwarz]

👋 We experienced this error and have some details to share.

We upgraded from Lighthouse 4.1.0 to 4.3.0. Following the upgrade we observed some Lighthouse runs were failing when the pages being tested had a DNS related issue.

This is a problem, because Lighthouse behaves differently based on if Chrome is in headless mode.

To observe this behaviour, test with any of the following URLs:

• https://expired.badssl.com
• https://untrusted-root.badssl.com
• https://does-not-exist.calibreapp.com

Lighthouse 4.1.0

• ✅With Chrome --headless LH waits until timeout, generates a lighthouse report, which contains a DNS failure warning.
• ✅In headful, the run does not timeout, generates a lighthouse report, which contains a DNS failure warning.

Lighthouse since 4.2.0+

• 💥With Chrome --headless, LH runs, then exits with an exception/error:

  ChromeLauncher Killing Chrome instance 42360 +6ms
events.js:173
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at TCP.onStreamRead (internal/stream_base_commons.js:162:27)
Emitted 'error' event at:
    at Socket.socketErrorListener (_http_client.js:399:9)
    at Socket.emit (events.js:188:13)
    at emitErrorNT (internal/streams/destroy.js:82:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
    at process.internalTickCallback (internal/process/next_tick.js:72:19)

• 💥With headful, LH runs and generates a lighthouse report, which contains a DNS failure warning.

I noticed that Pagespeed Insights is still running LH 4.1.0. It's likely that if/when that gets updated to 4.2.0 or 4.3.0 that error rates will climb. 😱

Tested and verified using:

• Mac, Linux
• Node 10, 11
• Lighthouse CLI
• Lighthouse Node API

Let me know if there's any other info I can share 🕵️

[patrickhulce]

a fix is up at #8340 if y'all want to check out the details of what happens in each environment now.

[benschwarz]

Thanks @patrickhulce, we'll check it out and report back. 🥇