ckerr

#54961: fix: use-after-free in DownloadItem wrappers

Merged
Created: Oct 14, 2026, 7:35:13 PM
Merged: Oct 15, 2026, 3:19:03 AM
6 comments
Target: main

Description of Change

Fix a use-after-free issue introduced in #51890. Discovered by AddressSanitizer. with the stack

electron::api::DownloadItem
UpdateObservers()
DownloadItemImpl::Remove()
DownloadManagerImpl::RemoveDownloadsByURLAndTime()
BrowsingDataRemoverImpl` (`DATA_TYPE_DOWNLOADS`)
Session::ClearData()

api::DownloadItem is a cppgc wrapper. It added itself as an observer of its native download::DownloadItem in its constructor and removed itself only in its destructor. A cppgc destructor is a finalizer that runs during sweeping, which can come well after the garbage collection that found the wrapper unreachable. In between, the native item could still notify the dead wrapper. A completed download stays in the session's DownloadManager after its wrapper becomes collectable, so clearData() in that window made a virtual call into the dead, not-yet-swept wrapper. Without ASan, the dead wrapper then emitted done through a stale JS handle, which fails a V8 debug check on DCHECK builds. The destructor also called Remove() on the native item, which ran DownloadManager code and other observers from inside a finalizer.

This PR moves the observer registration and the Remove() call onto a NativePeer<DownloadItem>, following the pattern from #54410:

  • The peer forwards OnDownloadUpdated and OnDownloadDestroyed only through wrapper(), while it is_active(). wrapper() is a WeakPersistent, cleared during marking, so a dead but unswept wrapper is never called.
  • The peer removes the native item in TearDownNative(), which runs from a posted task after the wrapper is finalized, or at shutdown. It still clears its pointer before Remove() deletes the item (#54513).
  • The wrapper reads the native item through the peer and its destructor is now = default. Events, keep_alive_, and the FromOrCreate / UserDataLink lookup are unchanged.

To test this, the PR adds a DCHECK-only testing binding, collectGarbageBeforeFinalizationForTesting(beforeCollection, beforeFinalization). It runs both callbacks and a full collection in one fresh task, with sweeping left incomplete (V8's CppHeap::SetForceIncrementalSweepingForTesting()). A new spec in cpp-heap.spec.ts completes a download and holds its DownloadItem until that task. It then drops the item, checks with a WeakRef that the collection found it unreachable, and calls ses.clearData({ dataTypes: ['downloads'] }) before the wrapper is finalized. Under ASan the old code fails this spec with the same use-after-poison, because cppgc poisons unreachable objects when sweeping starts. On other DCHECK builds the old code hits the V8 debug check above when the dead wrapper emits done; this was reproduced by making the old call on the dead wrapper under gdb.

This PR needs manual backports to 45, 44, 43. The bug was discovered in 43, but they have different levels of cppgcification so trop won't be able to do the work. (Those branches have the updated DownloadItem but not NativePeer from #54410 and #53759, which are both no-backport).

Validation

On a Linux x64 Testing build of this change: the new spec passed 4/4, and 30/30 when repeated in one run. The session download and clearData() specs passed 24/24, and the asar download specs 2/2. cpp-heap.spec.ts and api-session.spec.ts in full: 200 passed, 10 skipped.

Checklist

🤖 Generated with Claude Code

Release Notes

Notes: Fixed a crash in ses.clearData() after a DownloadItem was garbage collected.

Backports

43-x-y
Pending
Waiting for a manual backport
44-x-y
Pending
Waiting for a manual backport
45-x-y
Pending
Waiting for a manual backport

Semver Impact

Major
Breaking changes
Minor
New features
Patch
Bug fixes
None
Docs, tests, etc.

Semantic Versioning helps users understand the impact of updates:

  • Major (X.y.z): Breaking changes that may require code modifications
  • Minor (x.Y.z): New features that maintain backward compatibility
  • Patch (x.y.Z): Bug fixes that don't change the API
  • None: Changes that don't affect using facing parts of Electron