core(gather): handle crash if CDP target crashes#11840
Conversation
patrickhulce
left a comment
There was a problem hiding this comment.
impl sounds right to me! but we also discussed the impl a bit haha
if anyone else wants to weigh in on the fact that it's a race at the highest gather level, feel free :)
| */ | ||
| static async getGatherTerminatedPromise(driver) { | ||
| return new Promise((_, reject) => { | ||
| driver.on('Inspector.targetCrashed', _ => { |
There was a problem hiding this comment.
if iframes crash should we still consider it fatal?
There was a problem hiding this comment.
good question.. if an iframe shares a process with its parent.. it takes down both. (naturally)
but here's a iframe page crash when that iframe is isolated (oopif):
and..........
if we didn't explicitly attach to that iframe target.. we'd just see a Target.targetCrashed event, but not a Inspector.targetCrashed one.
but we DO autoattach to iframe targets. so we're attached. and thus we'll get the full fledged Inspector.targetCrashed. :)
i don't think there's an easy call here, but ... probably yes.
There was a problem hiding this comment.
nice sleuthing thanks :)
I suppose if we ever have a single gatherer that tries to evaluate something in subtarget contexts we will need to support this (framehole maybe?), so SGTM 👍 (though I would love some broader testing system at a time like this ala chat convo 😄 )
lighthouse-core/runner.js
Outdated
| const gatherPromise = GatherRunner.run(runnerOpts.config.passes, gatherOpts); | ||
| await Promise.race([ | ||
| gatherPromise, | ||
| GatherRunner.getGatherTerminatedPromise(driver), |
There was a problem hiding this comment.
Congratulations this is one of the only forked areas where we need to worry about duplication for FR 🎉 lucky you! 😃
would you be able to add this to the snapshot runner and use the shared FRTransitionalDriver type instead? :)
living in gather-runner feels a little weird at that point, but I don't have a clean new place or suggestion for it. ideas?
lighthouse-core/test/runner-test.js
Outdated
| }); | ||
|
|
||
|
|
||
| it('includes a crash runtimeError when there\'s a crash during gathering', async () => { |
There was a problem hiding this comment.
I'd love to get a smoketest of this, but it's too hard without puppeteer. Maybe with FR support one can be added to the pptr test suite there? :D
There was a problem hiding this comment.
maybe doable. looks like it requires a
driver.gotoURL('chrome://crash');you can't navigate to that URL clientside, so i think it's gotta come via protocol.
There was a problem hiding this comment.
yea this really could use a smoke test
| } | ||
|
|
||
| /** | ||
| * Reject if the gathering terminates due to the CDP target crashing, etc |
There was a problem hiding this comment.
non-review comment below
Worth noting here for lurkers that this is a good example our long-standing tradition of memory leak listeners that are never removed and promises that never die. We have a ton of these, so shouldn't need to do anything differently here, but a few folks have asked recently about why we recommend child processes and disposable contexts, this is why :)
|
@paulirish we still want this? |
|
Can you update the branch? |
|
|
This got me thinking about it a little differently. I had this rejecting out of LH extremely quickly. No LHR, nothing. But I also now remember taking the stance that we should ALWAYS create an LHR. The fact that we don't is why we (and probably others) have to do stuff like this. I moved the rejection race elsewhere and now it gets an LHR built for free. (And our smoketest runtimeError assertions are also fine. yay we have a test) also obv this depends on #15913 |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Co-authored-by: Connor Clark <cjamcl@google.com>
|
@connorjclark this is ready for another review. (sorry i forget our workflow conventions :) |
fixes #11827
page could crash (what we saw there). we can't recover from that, and we should quit.
the CDP connection could also detach.. this happens if you manually close the tab while lighthouse is gathering. Update: our pptr layer handles this part just fine. we die fatally very quickly when this happens already.