File: keep the FileReader listener snapshot rooted while dispatching - #248
Open
matthargett wants to merge 3 commits into
Open
matthargett wants to merge 3 commits into
matthargett wants to merge 3 commits into
Conversation
FileReader::Dispatch copied the listeners into a std::vector of bare Napi::Function values before calling them. A listener that removes a later listener drops the only strong reference to it, and on JavaScriptCore nothing else roots a napi_value that lives on the C++ heap (its handle scopes are stubs; only the C stack is scanned), so the later function could be collected before the loop reached it. Snapshot FunctionReferences instead, released when dispatch returns.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The garbage-collection scenario lacks a focused automated regression test.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
Share listener records that own strong callback references so snapshots retain both roots and registration identity. Mark records removed before erasing them, and skip them even in nested dispatches; re-adding the same callback creates a new registration. Correct the earlier test's removed-listener expectation and cover removal, remove/re-add deferral, self-removal, additions, and nested dispatch. These assert DOM dispatch semantics independently of GC timing, not a deterministic collection crash.
matthargett
added a commit
to rebeckerspecialties/JsRuntimeHost
that referenced
this pull request
Sep 17, 2026
Carry the reviewed PR BabylonJS#248 shared listener records and regression coverage into the polyfill integration stack. Removed listeners are skipped, new registrations are deferred, and snapshots retain their callbacks during nested dispatch. Source: BabylonJS#248, bead2d6.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
FileReader::Dispatchoriginally copied callbacks into a heap-allocated vector of bareNapi::Functionvalues. On the JavaScriptCore backend those values are not strong roots. Removing a listener during dispatch could release its registeredFunctionReferencewhile leaving a bare value in the snapshot.There is also a semantic error in that example: a listener removed before its turn must not be invoked. The DOM invoke algorithm snapshots listener registrations while still observing their removal state. Keeping removed functions alive and calling them is not the correct contract.
Change
Each registration now has a shared record containing a strong
FunctionReferenceand a removal flag. Dispatch copies shared records, removal marks the record before erasing it, and dispatch skips removed records before retrieving their callbacks. Re-adding the same callback creates a new registration, so it cannot revive an entry in an older snapshot. Nested dispatch observes the same removal state.This supersedes the initial FunctionReference-only snapshot and corrects the initial test expectation. It belongs in this PR because rooting and removal semantics are part of the same dispatch snapshot mechanism. The diff stays within FileReader and its tests; no general EventTarget refactor or production GC hook is introduced.
Regression Coverage
The WPT listener-mutation and reentrancy cases were reviewed for contract guidance, not run unchanged in this non-DOM host.
Validation
Fresh isolated Debug JavaScriptCore build on macOS 27.0, commit
bead2d631fbd239f43cfd75b6851c91d3274f981:JSC_collectContinuously=1.JSC_collectContinuously=1.JavaScript.Allwith 231 passing and three existing V8-only skips.npm run buildandgit diff --checkpass.These are deterministic semantic regression oracles, not a deterministic GC-crash reproducer. The allocation-only test did not establish reliable pre/post collection failure; the bare-function control also did not crash in this validation. The durable rooting guarantee is provided by the shared records owning strong references. Other engines/platforms and sanitizer builds were not run in this pass.
#221 mirrors the dispatch snapshot pattern into XMLHttpRequest and should preserve both rooting and removal semantics. Fork twin: rebeckerspecialties#32.