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
fix: destroy WebContents synchronously on shutdown #15541
Conversation
9f28853
to
7416c2e
Compare
This PR has accidentally revealed a crash in other place:
|
d5b5a65
to
2ed1a11
Compare
The crash turned out to be a mistake when refactoring. All tests are passing now. |
@zcbenz The |
Normally this means we send invalid annotations, so let's just send a failure without them. E.g. electron/electron#15541
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 👍
app.on('ready', function () { | ||
new BrowserView({}) // eslint-disable-line | ||
|
||
process.nextTick(() => app.quit()) |
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 this has to be on the next tick ?
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'm trying to emulate the original crash case, having process.nextTick
or not does not really affect the test.
#64) Normally this means we send invalid annotations, so let's just send a failure without them. E.g. electron/electron#15541
@zcbenz just remembered, we had disabled a spec in 69 due to the same crash https://github.com/orgs/electron/projects/24#card-13687849, can you try enabling it in this PR ? Thanks! |
The test is timeout on my machine, after quitting the app directly I'm seeing the |
2ed1a11
to
d6c0b35
Compare
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 sense to me and looks good. 👏 for the documentation in code, explains what's going on well!
Release Notes Persisted
|
/trop run backport-to 3-1-x |
The backport process for this PR has been manually initiated, |
I was unable to backport this PR to "3-1-x" cleanly; |
Description of Change
Destroying WebContents asynchronously on shutdown may delay the destroying task after the shutdown task, resulting in an assertion in
ResourceDispatcherHostImpl::OnShutdown
.This PR destroys WebContents synchronously on shutdown, so we can fix #15133 without breaking #8930.
I have also added notes for
DestroyWebContents
on how to use it correctly.Checklist
npm test
passesRelease Notes
Notes: Fix crash on exit when using
BrowserView
.