-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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(driver): handle promise rejections from sendCommand #6739
Changes from all commits
55165e8
c5ae03e
3d64d5d
773e4f7
ab932b4
85bda02
76a4046
b1fd5b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -628,33 +628,31 @@ class Driver { | |
/** | ||
* @param {Driver} driver | ||
* @param {() => void} resolve | ||
* @return {Promise<void>} | ||
*/ | ||
function checkForQuiet(driver, resolve) { | ||
async function checkForQuiet(driver, resolve) { | ||
if (cancelled) return; | ||
const timeSinceLongTask = await driver.evaluateAsync(checkForQuietExpression); | ||
if (cancelled) return; | ||
|
||
return driver.evaluateAsync(checkForQuietExpression) | ||
.then(timeSinceLongTask => { | ||
if (cancelled) return; | ||
|
||
if (typeof timeSinceLongTask === 'number') { | ||
if (timeSinceLongTask >= waitForCPUQuiet) { | ||
log.verbose('Driver', `CPU has been idle for ${timeSinceLongTask} ms`); | ||
resolve(); | ||
} else { | ||
log.verbose('Driver', `CPU has been idle for ${timeSinceLongTask} ms`); | ||
const timeToWait = waitForCPUQuiet - timeSinceLongTask; | ||
lastTimeout = setTimeout(() => checkForQuiet(driver, resolve), timeToWait); | ||
} | ||
} | ||
}); | ||
if (typeof timeSinceLongTask === 'number') { | ||
if (timeSinceLongTask >= waitForCPUQuiet) { | ||
log.verbose('Driver', `CPU has been idle for ${timeSinceLongTask} ms`); | ||
resolve(); | ||
} else { | ||
log.verbose('Driver', `CPU has been idle for ${timeSinceLongTask} ms`); | ||
const timeToWait = waitForCPUQuiet - timeSinceLongTask; | ||
lastTimeout = setTimeout(() => checkForQuiet(driver, resolve), timeToWait); | ||
} | ||
} | ||
} | ||
|
||
/** @type {(() => void)} */ | ||
let cancel = () => { | ||
throw new Error('_waitForCPUIdle.cancel() called before it was defined'); | ||
}; | ||
const promise = new Promise((resolve, reject) => { | ||
checkForQuiet(this, resolve); | ||
checkForQuiet(this, resolve).catch(reject); | ||
cancel = () => { | ||
cancelled = true; | ||
if (lastTimeout) clearTimeout(lastTimeout); | ||
|
@@ -890,19 +888,14 @@ class Driver { | |
await this._beginNetworkStatusMonitoring(url); | ||
await this._clearIsolatedContextId(); | ||
|
||
// These can 'race' and that's OK. | ||
// We don't want to wait for Page.navigate's resolution, as it can now | ||
// happen _after_ onload: https://crbug.com/768961 | ||
this.sendCommand('Page.enable'); | ||
this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); | ||
await this.sendCommand('Page.enable'); | ||
await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); | ||
// No timeout needed for Page.navigate. See #6413. | ||
this._innerSendCommand('Page.navigate', {url}); | ||
const waitforPageNavigateCmd = this._innerSendCommand('Page.navigate', {url}); | ||
|
||
if (waitForNavigated) { | ||
await this._waitForFrameNavigated(); | ||
} | ||
|
||
if (waitForLoad) { | ||
} else if (waitForLoad) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
const passConfig = /** @type {Partial<LH.Config.Pass>} */ (passContext.passConfig || {}); | ||
let {pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs} = passConfig; | ||
let maxWaitMs = passContext.settings && passContext.settings.maxWaitForLoad; | ||
|
@@ -918,6 +911,9 @@ class Driver { | |
maxWaitMs); | ||
} | ||
|
||
// Bring `Page.navigate` errors back into the promise chain. See #6739. | ||
await waitforPageNavigateCmd; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's nothing in _endNetworkStatusMonitoring we need, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
it returns the A little too convoluted but somewhat difficult to fix since |
||
|
||
return this._endNetworkStatusMonitoring(); | ||
} | ||
|
||
|
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.
Is it okay to await these? The comments above them talk about racing like this might be purposefully not-awaited.
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.
Bug is marked as fixed now. If it truly is, can you also remove the comments?
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 big bug was definitely about not awaiting
Page.navigate
, but why are we needing toawait
these? is it just about unhandled rejections?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.
could still timeout on any protocol command. what happens in the underlying connection dies? we should avoid any possible unhandled rejection.
But
await
each command isn't the only way to avoid unhandled rejections here. wdyt of just tacking acatch
on Page.enable, or combining the two commands like this:so that it's one less event loop iteration ?
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.
right I'm onboard with that, but I'm asking because there are then other things we might try instead of serialling awaiting like
this._sendAndSilentlyIgnoreErrors
or something :)It looks like we used to be just fine waiting on these in sequence though, and the
Page.navigate
change just moved all of 'em to be together, so we don't need to go crazy avoiding theawait
#3413I'm a tad spooked by this given that it was a real nasty bug to hunt down, but I think this makes sense if the bug really is fixed. +1 to removing the comments if we're making this change
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.
_innerSendCommand
is specifically for when you don't want to timeout.I'm not sure if
_innerSendCommand
can ever reject. You could try setting a breakpoint just before, then kill Chrome, then hit play, and see what happens ... But if I read the code correctly for how the connection works, it won't ever result in a rejected promise - just a stalled one.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 I guesssendCommand
is never the real problem it's always the PROTOCOL_TIMEOUT that's the real problem?You guys beat me to the punch you're already on 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.
maybe I'm not following the conversation correctly here, but
_innerSendCommand
can reject viaconnection
:lighthouse/lighthouse-core/gather/connections/connection.js
Lines 120 to 124 in 9ebf069
(throwing inside
Promise.resolve().then(_ => {/* throw */})
rejects it, thencallback.resolve()
with a rejected promise rejects as well. We could probably make that a lot more understandable with an explicitcallback.resolve
orcallback.reject
, as well as using anasync
function for the inner part)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.
so do we want to add an empty
catch
to thethis._innerSendCommand('Page.navigate', {url});
line? Or are we assuming it's so rare that it's ok that aunhandledRejection
listener can catch 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.
Since it seems like this never happens, should we just let sleeping dragons lie?