-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: INSECURE_DOCUMENT_REQUEST errors still return lhr #6608
Conversation
This comment has been minimized.
This comment has been minimized.
Bad bot. why would you close a PR |
Could we get a bit of explanation for what was happening before and why we want this instead? :) |
await GatherRunner.loadPage(driver, passContext); | ||
log.timeEnd(status); | ||
try { | ||
await GatherRunner.loadPage(driver, passContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're injecting a lot of new if/try/catch blocks into some already beefy methods, do you think there's a way we might be able to push some of these complications down a level so the busier method is still easy to parse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, if we can refactor Runner.run to easily return a (errored) LHR in its catch. Then changes to gather-runner
wouldn't be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're dropping the LHR requirement for now, so we can also drop these changes (there won't be a timing object to add this to if an error was thrown)
When there is a security issue, |
It would be much nicer if we could just throw the security error when discovered (like the PR has it in |
We should definitely do it the right way, even if it's more work, otherwise we're just going to keep paying for it until we do. It's already enough work to mentally reconstruct control flow through However, I think this gets back to #6336, which I failed to substatially bring up in the bug (#6595 (comment) :). It's still not clear to me that we want an LHR with just the single top-level error for the CLI/Extension/DevTools clients (they have varying extents of their own UI for showing fatal errors like this). I feel like we need to settle the question of how to treat runtime errors in the different clients before we start any error-handling refactors. |
OK, I removed all that complexity. Test w/ The original issue requested returning an LHR, but I believe we don't care about that anymore, since LR now correctly forms a LHR when an error is thrown. @paulirish is this correct? |
a million 👍s from me :) |
@@ -22,7 +22,7 @@ const UIStrings = { | |||
/** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */ | |||
pageLoadFailedWithDetails: '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. (Details: {errorDetails})', | |||
/** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. */ | |||
pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. ({securityMessages})', | |||
pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. {securityMessages}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for removing these? I feel like a list that might make no sense as a sentence like this belongs in parens. e.g. The URL...credentials. (Reason 1 Reason 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A realistic error message reads like this:
The URL you have provided does not have valid security credentials. This site is missing a valid, trusted certificate (net::ERR_CERT_DATE_INVALID).
Which is a valid sentence. So...ignore this maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct-o, the sentences should make sense as - is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least, in english ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add something about securityMessages
to the description for the translators. "securityMessages will be replaced with one or more strings from the browser explaining what was insecure about the page load." or whatever
lighthouse-core/lib/file-namer.js
Outdated
@@ -16,11 +16,11 @@ | |||
* Generate a filenamePrefix of hostname_YYYY-MM-DD_HH-MM-SS | |||
* Date/time uses the local timezone, however Node has unreliable ICU | |||
* support, so we must construct a YYYY-MM-DD date format manually. :/ | |||
* @param {{finalUrl: string, fetchTime: string}} lhr | |||
* @param {{requestedUrl: string, finalUrl: string, fetchTime: string}} lhr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this requestedUrl business about? Does it relate to INSECURE DOC requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the doc fails to load, the "finalUrl" is never set. so fall back to requestUrl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this requestedUrl business about?
I believe left over from the old version of the PR, should be able to remove from here now (and from github-api.js
and report-ui-features.js
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof you're right, forgot to check if that was still necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big move request :)
lighthouse-core/gather/driver.js
Outdated
} | ||
} | ||
|
||
waitForSecurityIssuesCheck() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just combine the two of these? Seems unnecessary to split (waitForSecurityIssuesCheck
doesn't really do anything in isolation and they aren't called separately)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we talked earlier about the order of the listener vs calling enable().
......maybe the order right now is OK. if you listened before calling security.enable then perhaps you would get a statechanged event for the about:blank state.
I'm not totally sure about this.
but typically we do like setting up the event listener before enable is called, because enable usually flushes all events before it results.
lighthouse-core/gather/driver.js
Outdated
await this.waitForSecurityIssuesCheck(); | ||
} finally { | ||
this.sendCommand('Security.disable'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To do this right, I think @paulirish is right and this should move into waitForFullyLoaded
. We shouldn't be mucking around looking at network traffic before our traffic monitor is allowed to start monitoring :)
Take a look at this._waitForLoadEvent
or this._waitForNetworkIdle
for the promise
/cancel
cancellable promise model used, and _waitForFullyLoaded
itself for where to put it in parallel with the timeout. It should be a pretty simple translation from waitForSecurityIssuesCheck
already in this PR.
(and forgive the state of the code in those methods...they're super old and have pretty much only grown through accretion, not really refactored, so they're a bit creaky and brittle :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the _waitForFrameNavigated
case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the
_waitForFrameNavigated
case?
that's only used for loading about:blank
, so we're ok there. We should probably make that clearer, though really if you're saying waitForNavigated: true
, you're basically forgoing all our checks already.
await GatherRunner.loadPage(driver, passContext); | ||
log.timeEnd(status); | ||
try { | ||
await GatherRunner.loadPage(driver, passContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're dropping the LHR requirement for now, so we can also drop these changes (there won't be a timing object to add this to if an error was thrown)
lighthouse-core/gather/driver.js
Outdated
|
||
// wait for Security.enable to resolve, so we skip whatever state change | ||
// events are in the pipeline | ||
await this.sendCommand('Security.enable'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really explain it, but this only works when await
ing the Security.enable
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have some tests/a test that can capture the cases that fail without this await but succeed with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also seems a bit like it has the potential to race :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test would basically be 1) go to https://expired.badssl.com/ (or load page with bad cert) 2) assert LH does not hang / exercise the full page load time out (so, within 5s??)
would that be a good candidate for a smoke test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it looks like it starts with the "neutral" security state (about: has no meaningful value for security) and then either goes to "secure" or "insecure", depending on the target page. So I can tweak the logic to listen for state changes until "not neutral" comes up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would that be a good candidate for a smoke test?
sounds great to me! who maintains badssl? do we expect it to be roughly up most of the time? oh we do, awesome yeah let's do it :D https://github.com/chromium/badssl.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just https://localhost:10200
could also work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If in offline mode, the security state is always "neutral" ...
This is me running the pwa smoke test. First two security states are the online pass (neutral -> secure), last three(?) are the offline pass (neutral -> neutral... but with explanations)
But in each case, the first state we care about is the first one with non-empty explanations. So maybe use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which makes sense... if there's no explanation for a security state, why value it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, some offline urls gives three empty, neutral states in the offline pass. Perhaps better to just disable this check for the offline pass.
lighthouse-core/gather/driver.js
Outdated
|
||
// wait for Security.enable to resolve, so we skip whatever state change | ||
// events are in the pipeline | ||
await this.sendCommand('Security.enable'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have some tests/a test that can capture the cases that fail without this await but succeed with?
lighthouse-core/gather/driver.js
Outdated
|
||
// wait for Security.enable to resolve, so we skip whatever state change | ||
// events are in the pipeline | ||
await this.sendCommand('Security.enable'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also seems a bit like it has the potential to race :/
lighthouse-core/gather/driver.js
Outdated
* @param {LH.Crdp.Security.SecurityStateChangedEvent} event | ||
*/ | ||
const securityStateChangedListener = ({securityState, explanations}) => { | ||
this.sendCommand('Security.disable'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this in a promise chain like @exterkamp's cleanup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean adding an await
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or an empty .catch
if we don't care?
lighthouse-core/gather/driver.js
Outdated
|
||
cancel = () => { | ||
this.off('Security.securityStateChanged', securityStateChangedListener); | ||
this.sendCommand('Security.disable'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here re: floating promise
lighthouse-core/gather/driver.js
Outdated
@@ -752,6 +795,7 @@ class Driver { | |||
// Wait for both load promises. Resolves on cleanup function the clears load | |||
// timeout timer. | |||
const loadPromise = Promise.all([ | |||
waitForSecurityCheck.promise, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes me a tad more nervous I feel like we need some beefier commenting on why this is OK, intuitively I wouldn't expect a securityStateChanged
to be guaranteed to fire early on every single page load, i.e. why would it fire if I go from http to http or https to https or about: to about:, etc
putting it directly in the chain of waiting for load just makes it a bit high stakes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd only work if the Security
domain is disabled before _waitForFullyLoaded
runs. If a gatherer were to enable Security
and forget to disable it, this may break?
Also, isn't this method always called from "about:" -> "target url"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, isn't this method always called from "about:" -> "target url"?
That's kinda what I'm talking about, just explaining why this is always going to be OK :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK I misunderstood. How's the comment I added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! maybe we can add a sentence about how it resolve
s too not just reject?
I'm thinking something like
// Listener that resolves or rejects on the first security state change.
// If the first change is secure, we resolve.
// If the first change is insecure, we reject.
// We can expect the security state to always change because this function
// is only used to move about:blank (neutral) -> the target url (something not neutral).
Now that I'm thinking about it maybe it belongs in the _waitForSecurityCheck
jsdoc :)
@@ -16,6 +16,7 @@ const log = require('lighthouse-logger'); | |||
|
|||
const PROTOCOL_TIMEOUT_EXIT_CODE = 67; | |||
const PAGE_HUNG_EXIT_CODE = 68; | |||
const INSECRE_DOCUMENT_REQUEST_EXIT_CODE = 69; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo fyi
lighthouse-core/gather/driver.js
Outdated
@@ -32,6 +32,8 @@ const DEFAULT_CPU_QUIET_THRESHOLD = 0; | |||
// Controls how long to wait for a response after sending a DevTools protocol command. | |||
const DEFAULT_PROTOCOL_TIMEOUT = 30000; | |||
|
|||
const SECURE_SCHEMES = ['data', 'https', 'wss', 'blob', 'chrome', 'chrome-extension', 'about']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remains to be seen if we need this here, but if we do, i'd much rather this array live in a common place than be duplicated here and in the is-on-https audit.
url-shim.js has worked well in the past. move there and import to here and the audit?
update: yup, this stuff is unnecessary now, but FYI on the feedback.
lighthouse-core/gather/driver.js
Outdated
@@ -741,6 +792,14 @@ class Driver { | |||
/** @type {NodeJS.Timer|undefined} */ | |||
let maxTimeoutHandle; | |||
|
|||
// Noop if offline or not https | |||
// https: => https | |||
const protocol = new URL(this._monitoredUrl || '').protocol.replace(/:$/, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, Brendan.
Example URL that redirects to insecure (but isn't caught currently): http://i.paul.irish
(as soon as DNS propagates, http://expiredredirect.paul.irish will do the same)
lighthouse-cli/run.js
Outdated
@@ -23,6 +23,7 @@ const opn = require('opn'); | |||
const _RUNTIME_ERROR_CODE = 1; | |||
const _PROTOCOL_TIMEOUT_EXIT_CODE = 67; | |||
const _PAGE_HUNG_EXIT_CODE = 68; | |||
const _INSECRE_DOCUMENT_REQUEST_EXIT_CODE = 69; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still INSECRE
;)
lighthouse-core/gather/driver.js
Outdated
@@ -832,9 +857,11 @@ class Driver { | |||
|
|||
// Wait for load or timeout and run the cleanup function the winner returns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit this comment to note that securityCheckPromise
will only ever reject?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OR crazy idea, the promise resolves with a cleanupFn
that throws an error :D then the three cases are a little more parallel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to your second comment, so securityCheckPromise
would be changed to resolve
instead of reject
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to your second comment, so securityCheckPromise would be changed to resolve instead of reject?
right it would go hand-in-hand with #6608 (comment) to enable to refactor
lighthouse-core/gather/driver.js
Outdated
|
||
// Promise that only rejects when an insecure security state is encountered | ||
let securityCheckCleanup = () => {}; | ||
const securityCheckPromise = new Promise((_, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a lot to unpack in this function that needs longstanding cleanup I'll take a whack at as a followup to #6944, for now WDYT about sticking this in a _waitForSecurityError
or something following the {promise, cancel}
paradigm we have currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cancelAll
and clearing the maxTimeoutHandle
complicates that a lot. It's not clear to me how to extract into a function without using multiple parameter callbacks (or is that the cleanup you had in mind?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this goes hand-in-hand with the resolve
idea. I'm not sure we could easily pull it out with it still rejecting immediately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think I see how that'd work. I'll give it a shot
lighthouse-core/gather/driver.js
Outdated
waitForLoadEvent.cancel(); | ||
waitForNetworkIdle.cancel(); | ||
waitForCPUIdle.cancel(); | ||
waitForSecurityCheck.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be easier to reason about all this canceling if each cancelable promise stores state about if it's been canceled yet and avoids doing it more than once. Then we could judiciously use cancelAll
in one place, after the Promise race.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is ripe for cleanup, but this new parallel style will make it so much easier now, thank you!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I don't think anything would be harmed today if you threw this into cancelAll
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would also have to remove the self-cancel that _waitForSecurityCheck
does, otherwise it would cancel itself twice.
I'll try the cancel state thing first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hang on before you do that I was saying I don't think it would matter if we called cancel
twice, we're just extra disabling the security domain and removing a listener that might've been removed already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, LMK if that is better or worse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks pretty good to me!
@@ -698,7 +697,6 @@ class Driver { | |||
const promise = new Promise((resolve, reject) => { | |||
checkForQuiet(this, resolve).catch(reject); | |||
cancel = () => { | |||
cancelled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like a bug, we want to just add if (cancelled) return
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, didn't meant to delete that.
can skip the guard here since reject/clearTimeout should have no effect after the first call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course, leaving out the guard looks like a mistake, so perhaps I should just add it anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can skip the guard here since reject/clearTimeout should have no effect after the first call.
of course, leaving out the guard looks like a mistake, so perhaps I should just add it anyways?
I mean, it's also unnecessary everywhere else it's be added, isn't it? All removing listeners and clearing timeouts, which have no problem being run multiple times.
I'm OK with having them since that's how we would write a cancellable promise utility instead of doing each of these separately (in which case all of the methods should have it), but I'm also OK with dropping them since they aren't necessary (except for the Security.disable
command in _waitForSecurityCheck
)
lighthouse-core/gather/driver.js
Outdated
@@ -793,6 +840,13 @@ class Driver { | |||
// CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle. | |||
let waitForCPUIdle = this._waitForNothing(); | |||
|
|||
const waitForSecurityCheck = this._waitForSecurityCheck(); | |||
const securityCheckPromise = waitForSecurityCheck.promise.then((err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: but I might have the promise resolve with the explanations and this fn create the error.
that feels a little more mainstream to resolve with data and then the type of the error is clear from within this fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -22,7 +22,10 @@ const redirectDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog. | |||
const MAX_WAIT_FOR_PROTOCOL = 20; | |||
|
|||
function createOnceStub(events) { | |||
return (eventName, cb) => { | |||
return async (eventName, cb) => { | |||
// wait a tick b/c real events never fire immediately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good. A few style things but nothing too big
@@ -666,7 +665,7 @@ class Driver { | |||
|
|||
/** @type {NodeJS.Timer|undefined} */ | |||
let lastTimeout; | |||
let cancelled = false; | |||
let canceled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boooo to one L :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer Mr. Webster's simplified spellings, but I could care more.
@@ -698,7 +697,6 @@ class Driver { | |||
const promise = new Promise((resolve, reject) => { | |||
checkForQuiet(this, resolve).catch(reject); | |||
cancel = () => { | |||
cancelled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can skip the guard here since reject/clearTimeout should have no effect after the first call.
of course, leaving out the guard looks like a mistake, so perhaps I should just add it anyways?
I mean, it's also unnecessary everywhere else it's be added, isn't it? All removing listeners and clearing timeouts, which have no problem being run multiple times.
I'm OK with having them since that's how we would write a cancellable promise utility instead of doing each of these separately (in which case all of the methods should have it), but I'm also OK with dropping them since they aren't necessary (except for the Security.disable
command in _waitForSecurityCheck
)
if (lastTimeout) clearTimeout(lastTimeout); | ||
reject(new Error('Wait for CPU idle cancelled')); | ||
reject(new Error('Wait for CPU idle canceled')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting that we reject here but not in the others
if (canceled) return; | ||
canceled = true; | ||
this.off('Security.securityStateChanged', securityStateChangedListener); | ||
this.sendCommand('Security.disable').catch(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a TODO here for whatever @patrickhulce is planning with the refactor? cancel()
should really be a promise itself to handle things like this
lighthouse-core/gather/driver.js
Outdated
* @return {{promise: Promise<LH.Crdp.Security.SecurityStateExplanation[]>, cancel: function(): void}} | ||
* @private | ||
*/ | ||
_waitForSecurityCheck() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename since it resolves only in the case of insecurity (and the check isn't singular, it just goes until it isn't needed)? _waitForInsecureState
? _monitorForInsecureState
? Something else?
waitForLoadEvent.cancel(); | ||
waitForNetworkIdle.cancel(); | ||
waitForCPUIdle.cancel(); | ||
|
||
if (await this.isPageHung()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW for future refactoring, @paulirish mentioned he doesn't think this check is necessary anymore (errors are successfully being detected elsewhere, I guess)
}); | ||
throw err; | ||
}; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, I think I disagree with @patrickhulce. The internals of SecurityStateExplanation
seem like a distraction from the actual purpose of _waitForFullyLoaded
, since either way we're using waitForSecurityCheck
to alert us of an error, the question is if we should do more work in there or out here.
If that's not convincing, maybe we could split the difference? Filter and map within _waitForSecurityCheck
and then it only needs to be
const securityCheckPromise = waitForSecurityCheck.promise.then(securityMessages => {
return function() {
throw new new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages});
}
});
(using the cleanupFn to reject the promise is a little weird but w/e :P)
@@ -830,11 +879,20 @@ class Driver { | |||
}; | |||
}); | |||
|
|||
// Wait for load or timeout and run the cleanup function the winner returns. | |||
// Wait for security issue, load or timeout and run the cleanup function the winner returns. | |||
const cleanupFn = await Promise.race([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having this cleanupFn
setup now is weird if they all can (and should) be cancelled regardless, but that can wait for the refactor too
@@ -22,7 +22,7 @@ const UIStrings = { | |||
/** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */ | |||
pageLoadFailedWithDetails: '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. (Details: {errorDetails})', | |||
/** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. */ | |||
pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. ({securityMessages})', | |||
pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. {securityMessages}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add something about securityMessages
to the description for the translators. "securityMessages will be replaced with one or more strings from the browser explaining what was insecure about the page load." or whatever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functionally, this works great.
You'll want to add this at the top of popup.js:
'INSECURE_DOCUMENT_REQUEST': 'The URL you have provided does not have valid' +
' security credentials.',
lgtm on everything else.
@@ -22,7 +22,10 @@ const redirectDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog. | |||
const MAX_WAIT_FOR_PROTOCOL = 20; | |||
|
|||
function createOnceStub(events) { | |||
return (eventName, cb) => { | |||
return async (eventName, cb) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a weird use of async/await (and depending on driver.once
calls you'll end up with an unhandled promise rejection for the throw below).
Can we just
// wait a tick b/c real events never fire immediately
setTimeout(_ => cb(events[eventName]), 0);
return;
like normal people? :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant drop the async/await in favor of the setTimeout :)
explanations: [], | ||
securityState: 'secure', | ||
}; | ||
driverStub.on = driverStub.once = createOnceStub({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this poor driverStub
was never meant to be used repeatedly :)
Can you also break the cycle and make a new Driver(connection)
within each of these tests? We can deal with having a fresh connection
later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I'm also rather fond of keeping all the mocking close to the test like I did over in manifest PR with jest.spyOn
. Given the way the past few reviews have gone I'm guessing @brendankenny feels the opposite though so take it with a grain of salt 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the way the past few reviews have gone I'm guessing @brendankenny feels the opposite
Ha, I'm not entirely sure how to interpret that. I also like maximizing locality so you can see what's actually been done to the mock you have to use. But I am generally against mocks if at all possible, if that's what that means :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like maximizing locality
phew! :)
Ha, I'm not entirely sure how to interpret that.
only a joke no worries, just I feel like I've lead @hoten chasing too many wild geese lately 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only a joke no worries
lol I was trying to read too deeply :) :)
@hoten you'll need to rerun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just the one test change and rerun yarn diff:sample-json
and I think we may be good to go!
@@ -22,7 +22,10 @@ const redirectDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog. | |||
const MAX_WAIT_FOR_PROTOCOL = 20; | |||
|
|||
function createOnceStub(events) { | |||
return (eventName, cb) => { | |||
return async (eventName, cb) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant drop the async/await in favor of the setTimeout :)
const connection = new Connection(); | ||
connection.sendCommand = sendCommandStub; | ||
driverStub = new Driver(connection); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh nice
Fixes #6595.