File: keep the FileReader listener snapshot rooted while dispatching - #248
Open
matthargett wants to merge 1 commit into
Open
matthargett wants to merge 1 commit into
matthargett wants to merge 1 commit 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
| for (const auto& ref : it->second) | ||
| { | ||
| snapshot.push_back(ref.Value()); | ||
| snapshot.push_back(Napi::Persistent(ref.Value())); |
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::Dispatchsnapshots the listener list asstd::vector<Napi::Function>— barenapi_values — before calling each listener. If a listener removes a later listener (removeEventListenerinside a handler), that later function's only strong reference (itsFunctionReferenceinm_eventHandlerRefs) goes away mid-dispatch. On JavaScriptCore nothing else roots a value that lives only on the C++ heap — the backend's handle scopes are stubs and it scans just the C stack — so a collection triggered by the running handler can reclaim the function before the loop reaches it. Same class of bug as theCompressionStreamoutput-chunk corruption on #211 (reproducible there withJSC_collectContinuously=1).Change
Snapshot
Napi::FunctionReferences (Napi::Persistent) instead of values; they are released when dispatch returns. Behaviour is otherwise unchanged.#221 mirrors the same snapshot pattern into
XMLHttpRequest("Mirrors FileReader::Dispatch"); I left a note there so it can take the same shape.Verified on macOS (JavaScriptCore): full
UnitTestsgreen, also underJSC_collectContinuously=1. Fork twin: rebeckerspecialties#32.