Browse Source

fix: Shutdown crash in DownloadItem callback (#27418)

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.
```

Co-authored-by: Biru Mohanathas <[email protected]>
trop[bot] 4 years ago
parent
commit
6bfccca157
1 changed files with 10 additions and 0 deletions
  1. 10 0
      shell/browser/electron_browser_main_parts.cc

+ 10 - 0
shell/browser/electron_browser_main_parts.cc

@@ -517,6 +517,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.