From 7f1e3ca3def6d8beb9d444f805c508dc91294b2e Mon Sep 17 00:00:00 2001 From: Biru Mohanathas Date: Wed, 20 Jan 2021 21:30:08 +0200 Subject: [PATCH] fix: shutdown crash in DownloadItem callback (#27342) 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 (event_emitter_caller.h:51) gin_helper::EventEmitterMixin::Emit (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. ``` --- shell/browser/electron_browser_main_parts.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc index 8a574444d926..82f313b4e4e5 100644 --- a/shell/browser/electron_browser_main_parts.cc +++ b/shell/browser/electron_browser_main_parts.cc @@ -520,6 +520,16 @@ void ElectronBrowserMainParts::PostMainMessageLoopRun() { FreeAppDelegate(); #endif + // Shutdown the DownloadManager before destroying Node to prevent + // DownloadItem callbacks from crashing. + for (auto& iter : ElectronBrowserContext::browser_context_map()) { + auto* download_manager = + content::BrowserContext::GetDownloadManager(iter.second.get()); + if (download_manager) { + download_manager->Shutdown(); + } + } + // Make sure destruction callbacks are called before message loop is // destroyed, otherwise some objects that need to be deleted on IO thread // won't be freed.