8000 fix: Shutdown crash in DownloadItem callback by poiru · Pull Request #27342 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: Shutdown crash in DownloadItem callback #27342

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 20, 2021
Merged

fix: Shutdown crash in DownloadItem callback #27342

merged 1 commit into from
Jan 20, 2021

Conversation

poiru
Copy link
Contributor
@poiru poiru commented Jan 17, 2021 •
edited by miniak
Loading

The call stack for one of our top crashes looks like this:

node::Abort (node_errors.cc:241)
node::Assert (node_errors.cc:256)
node::MakeCallback (callback.cc:226)
gin_helper::internal::CallMethodWithArgs (event_emitter_caller.cc:23)
gin_helper::EmitEvent<T> (event_emitter_caller.h:51)
gin_helper::EventEmitterMixin<T>::Emit<T> (event_emitter_mixin.h:81)
electron::api::DownloadItem::OnDownloadUpdated (electron_api_download_item.cc:115)
download::DownloadItemImpl::UpdateObservers (download_item_impl.cc:482)
content::DownloadManagerImpl::Shutdown (download_manager_impl.cc:508)
content::BrowserContext::~BrowserContext (browser_context.cc:476)

Full stack here: https://sentry.io/share/issue/9b030a0601b547188181b543c16ecda2/

During browser shutdown, the DownloadManager was being cleaned up
after the Node environment had already been destroyed. This caused the
DownloadItem::OnDownloadUpdated callback to crash when trying to emit
the JS done event.

To prevent this, we now manually shut down the DownloadManager
earlier. This is also mentioned in the comment on
DownloadManager::Shutdown:

// Shutdown the download manager. Content calls this when BrowserContext is
// being destructed. If the embedder needs this to be called earlier, it can
// call it. In that case, the delegate's Shutdown() method will only be called
// once.

Notes: Fixed shutdown crash when quitting with in-progress downloads

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jan 17, 2021
@poiru
fix: Shutdown crash in DownloadItem callback
a0480de 
The call stack for one of our top crashes looks like this:

```
node::Abort (node_errors.cc:241)
node::Assert (node_errors.cc:256)
node::MakeCallback (callback.cc:226)
gin_helper::internal::CallMethodWithArgs (event_emitter_caller.cc:23)
gin_helper::EmitEvent<T> (event_emitter_caller.h:51)
gin_helper::EventEmitterMixin<T>::Emit<T> (event_emitter_mixin.h:81)
electron::api::DownloadItem::OnDownloadUpdated (electron_api_download_item.cc:115)
download::DownloadItemImpl::UpdateObservers (download_item_impl.cc:482)
content::DownloadManagerImpl::Shutdown (download_manager_impl.cc:508)
content::BrowserContext::~BrowserContext (browser_context.cc:476)
```

Full stack here: https://sentry.io/share/issue/9b030a0601b547188181b543c16ecda2/

During browser shutdown, the `DownloadManager` was being cleaned up
*after* the Node environment had already been destroyed. This caused the
`DownloadItem::OnDownloadUpdated` callback to crash when trying to emit
the JS `done` event.

To prevent this, we now manually shut down the `DownloadManager`
earlier. This is also mentioned in the comment on
`DownloadManager::Shutdown`:

```
// Shutdown the download manager. Content calls this when BrowserContext is
// being destructed. If the embedder needs this to be called earlier, it can
// call it. In that case, the delegate's Shutdown() method will only be called
// once.
```
@poiru poiru force-pushed the fix-shutdown-downloaditem-crash branch from 81152e7 to a0480de Compare January 17, 2021 14:52
@codebytere codebytere added the semver/patch backwards-compatible bug fixes label Jan 19, 2021
@electron-cation electron-cation bot added new-pr 🌱 PR opened recently and removed new-pr 🌱 PR opened recently labels Jan 19, 2021
@codebytere
Copy link
Member

@poiru what versions does this affect?

@poiru
Copy link
Contributor Author
poiru commented Jan 19, 2021

@codebytere We've gotten crash reports from Electron 10.x so at least that, maybe we can backport to 10.x and above?

@codebytere
Copy link
Member

session.serviceWorkers getFromVersionID() should report the correct script url and scope

Failure is known and unrelated.

@codebytere codebytere merged commit 7f1e3ca into electron:master Jan 20, 2021
@release-clerk
Copy link
release-clerk bot commented Jan 20, 2021

Release Notes Persisted

Fixed shutdown crash when quitting with in-progress downloads

This was referenced Jan 20, 2021
Merged
Merged
@trop
Copy link
Contributor
trop bot commented Jan 20, 2021

I have automatically backported this PR to "10-x-y", please check out #27417

@trop
Copy link
Contributor
trop bot commented Jan 20, 2021

I have automatically backported this PR to "12-x-y", please check out #27418

@trop trop bot mentioned this pull request Jan 20, 2021
Merged
@trop
Copy link
Contributor
trop bot commented Jan 20, 2021

I have automatically backported this PR to "11-x-y", please check out #27419

@poiru poiru deleted the fix-shutdown-downloaditem-crash branch January 20, 2021 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniak miniak miniak approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@poiru @codebytere @miniak
0