From c712eda554cc9c6afbabf273c5bdd90669853c4f Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 16 Sep 2026 16:30:28 -0700 Subject: [PATCH 1/3] File: keep the listener snapshot rooted while dispatching 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. --- Polyfills/File/Source/FileReader.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Polyfills/File/Source/FileReader.cpp b/Polyfills/File/Source/FileReader.cpp index d5c0ea04..62a41950 100644 --- a/Polyfills/File/Source/FileReader.cpp +++ b/Polyfills/File/Source/FileReader.cpp @@ -242,17 +242,21 @@ namespace Babylon::Polyfills::Internal // Snapshot the listener list so that mutations during dispatch // (e.g. a handler that calls removeEventListener) do not invalidate - // the iterator we are walking. - std::vector snapshot; + // the iterator we are walking. The snapshot holds references rather + // than bare values: a handler 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, so a bare + // function could be collected before this loop reached it. + std::vector snapshot; snapshot.reserve(it->second.size()); for (const auto& ref : it->second) { - snapshot.push_back(ref.Value()); + snapshot.push_back(Napi::Persistent(ref.Value())); } for (const auto& listener : snapshot) { - listener.Call(jsThis, {event}); + listener.Value().Call(jsThis, {event}); if (env.IsExceptionPending()) { env.GetAndClearPendingException(); From 51cebc836647df0e7ac72bc015aa3873f77897f8 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 17 Sep 2026 09:37:41 -0700 Subject: [PATCH 2/3] Test FileReader listener rooting during dispatch --- Tests/UnitTests/Scripts/tests.ts | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 23b8e4e5..dff14bf8 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -2238,6 +2238,38 @@ describe("FileReader", function () { reader.readAsText(blob); }); + it("keeps the dispatch snapshot alive when an earlier listener removes a later one", function (done) { + const reader = new FileReader(); + const blob = new Blob(["abc"]); + let laterListenerCalled = false; + let laterListener: (() => void) | null = function () { + laterListenerCalled = true; + }; + + reader.addEventListener("load", function () { + reader.removeEventListener("load", laterListener); + laterListener = null; + + // JSC_collectContinuously=1 collects during these allocations. The listener must + // remain rooted by the dispatch snapshot even after the registered reference is gone. + const pressure = []; + for (let i = 0; i < 64; ++i) { + pressure.push(new Uint8Array(16 * 1024)); + } + expect(pressure).to.have.lengthOf(64); + }); + reader.addEventListener("load", laterListener); + reader.onloadend = function () { + try { + expect(laterListenerCalled).to.equal(true); + done(); + } catch (e) { + done(e); + } + }; + reader.readAsText(blob); + }); + // -------------------------------- abort -------------------------------- it("transitions readyState to DONE after abort()", function (done) { const reader = new FileReader(); From bead2d631fbd239f43cfd75b6851c91d3274f981 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 17 Sep 2026 15:22:13 -0700 Subject: [PATCH 3/3] File: preserve listener removal state during dispatch 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. --- Polyfills/File/Source/FileReader.cpp | 31 +++++------ Polyfills/File/Source/FileReader.h | 9 +++- Tests/UnitTests/Scripts/tests.ts | 81 +++++++++++++++++++++++++--- 3 files changed, 96 insertions(+), 25 deletions(-) diff --git a/Polyfills/File/Source/FileReader.cpp b/Polyfills/File/Source/FileReader.cpp index 62a41950..d58e4f37 100644 --- a/Polyfills/File/Source/FileReader.cpp +++ b/Polyfills/File/Source/FileReader.cpp @@ -164,12 +164,12 @@ namespace Babylon::Polyfills::Internal auto& list = m_eventHandlerRefs[eventType]; for (const auto& existing : list) { - if (existing.Value() == handler) + if (existing->callback.Value() == handler) { return; } } - list.push_back(Napi::Persistent(handler)); + list.push_back(std::make_shared(Napi::Persistent(handler))); } void FileReader::RemoveEventListener(const Napi::CallbackInfo& info) @@ -191,8 +191,9 @@ namespace Babylon::Polyfills::Internal auto& list = it->second; for (auto i = list.begin(); i != list.end(); ++i) { - if (i->Value() == handler) + if ((*i)->callback.Value() == handler) { + (*i)->removed = true; list.erase(i); return; } @@ -240,23 +241,19 @@ namespace Babylon::Polyfills::Internal return; } - // Snapshot the listener list so that mutations during dispatch - // (e.g. a handler that calls removeEventListener) do not invalidate - // the iterator we are walking. The snapshot holds references rather - // than bare values: a handler 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, so a bare - // function could be collected before this loop reached it. - std::vector snapshot; - snapshot.reserve(it->second.size()); - for (const auto& ref : it->second) - { - snapshot.push_back(Napi::Persistent(ref.Value())); - } + // Share listener records: keep callbacks rooted without invalidating iteration, + // but observe removals even in nested dispatches. Re-adding a callback creates a + // new record that is not in this snapshot (DOM's invoke / inner invoke algorithms). + const auto snapshot = it->second; for (const auto& listener : snapshot) { - listener.Value().Call(jsThis, {event}); + if (listener->removed) + { + continue; + } + + listener->callback.Value().Call(jsThis, {event}); if (env.IsExceptionPending()) { env.GetAndClearPendingException(); diff --git a/Polyfills/File/Source/FileReader.h b/Polyfills/File/Source/FileReader.h index 4d3137e5..40ff8275 100644 --- a/Polyfills/File/Source/FileReader.h +++ b/Polyfills/File/Source/FileReader.h @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -21,6 +22,12 @@ namespace Babylon::Polyfills::Internal explicit FileReader(const Napi::CallbackInfo& info); private: + struct EventListener + { + Napi::FunctionReference callback; + bool removed{false}; + }; + enum class ReadMode { ArrayBuffer, @@ -65,7 +72,7 @@ namespace Babylon::Polyfills::Internal // wrapper is kept alive by an externally-held anchor (see StartRead), // so `this` is always valid when a continuation reads this field. uint64_t m_readId{0}; - std::unordered_map> m_eventHandlerRefs; + std::unordered_map>> m_eventHandlerRefs; // readonly attribute state, surfaced through the getters above. int32_t m_readyState{EMPTY}; diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index dff14bf8..04ea3f97 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -2238,30 +2238,34 @@ describe("FileReader", function () { reader.readAsText(blob); }); - it("keeps the dispatch snapshot alive when an earlier listener removes a later one", function (done) { + it("skips a listener removed during dispatch and still calls the final listener", function (done) { const reader = new FileReader(); const blob = new Blob(["abc"]); - let laterListenerCalled = false; + const calls: string[] = []; let laterListener: (() => void) | null = function () { - laterListenerCalled = true; + calls.push("removed"); }; reader.addEventListener("load", function () { + calls.push("first"); reader.removeEventListener("load", laterListener); laterListener = null; - // JSC_collectContinuously=1 collects during these allocations. The listener must - // remain rooted by the dispatch snapshot even after the registered reference is gone. + // Add allocation pressure for JSC_collectContinuously=1 runs. The assertion + // below is deterministic and does not depend on whether collection occurs. const pressure = []; for (let i = 0; i < 64; ++i) { pressure.push(new Uint8Array(16 * 1024)); } - expect(pressure).to.have.lengthOf(64); + calls.push(`allocated ${pressure.length}`); }); reader.addEventListener("load", laterListener); + reader.addEventListener("load", function () { + calls.push("last"); + }); reader.onloadend = function () { try { - expect(laterListenerCalled).to.equal(true); + expect(calls).to.deep.equal(["first", "allocated 64", "last"]); done(); } catch (e) { done(e); @@ -2270,6 +2274,69 @@ describe("FileReader", function () { reader.readAsText(blob); }); + it("defers a removed and re-added listener until the next dispatch", function () { + const reader = new FileReader(); + const calls: string[] = []; + const later = () => calls.push("later"); + const first = function () { + calls.push("first"); + reader.removeEventListener("load", first); + reader.removeEventListener("load", later); + reader.addEventListener("load", later); + }; + reader.addEventListener("load", first); + reader.addEventListener("load", later); + reader.addEventListener("load", () => calls.push("last")); + + reader.dispatchEvent({ type: "load" }); + expect(calls).to.deep.equal(["first", "last"]); + calls.length = 0; + reader.dispatchEvent({ type: "load" }); + expect(calls).to.deep.equal(["last", "later"]); + }); + + it("allows self-removal and defers newly added listeners", function () { + const reader = new FileReader(); + const calls: string[] = []; + const added = () => calls.push("added"); + const first = function () { + calls.push("first"); + reader.removeEventListener("load", first); + reader.addEventListener("load", added); + }; + reader.addEventListener("load", first); + reader.addEventListener("load", () => calls.push("last")); + + reader.dispatchEvent({ type: "load" }); + expect(calls).to.deep.equal(["first", "last"]); + calls.length = 0; + reader.dispatchEvent({ type: "load" }); + expect(calls).to.deep.equal(["last", "added"]); + }); + + it("observes removals made by a nested dispatch", function () { + const reader = new FileReader(); + const calls: string[] = []; + const first = function () { + calls.push("first"); + reader.removeEventListener("load", first); + reader.dispatchEvent({ type: "load" }); + }; + const middle = function () { + calls.push("middle"); + reader.removeEventListener("load", middle); + }; + reader.addEventListener("load", first); + reader.addEventListener("load", middle); + reader.addEventListener("load", () => calls.push("last")); + + reader.dispatchEvent({ type: "load" }); + expect(calls).to.deep.equal(["first", "middle", "last", "last"]); + calls.length = 0; + reader.dispatchEvent({ type: "load" }); + expect(calls).to.deep.equal(["last"]); + }); + // -------------------------------- abort -------------------------------- it("transitions readyState to DONE after abort()", function (done) { const reader = new FileReader();