diff --git a/Polyfills/File/Source/FileReader.cpp b/Polyfills/File/Source/FileReader.cpp index d5c0ea04..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,19 +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. - std::vector snapshot; - snapshot.reserve(it->second.size()); - for (const auto& ref : it->second) - { - snapshot.push_back(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.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 23b8e4e5..04ea3f97 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -2238,6 +2238,105 @@ describe("FileReader", function () { reader.readAsText(blob); }); + it("skips a listener removed during dispatch and still calls the final listener", function (done) { + const reader = new FileReader(); + const blob = new Blob(["abc"]); + const calls: string[] = []; + let laterListener: (() => void) | null = function () { + calls.push("removed"); + }; + + reader.addEventListener("load", function () { + calls.push("first"); + reader.removeEventListener("load", laterListener); + laterListener = null; + + // 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)); + } + calls.push(`allocated ${pressure.length}`); + }); + reader.addEventListener("load", laterListener); + reader.addEventListener("load", function () { + calls.push("last"); + }); + reader.onloadend = function () { + try { + expect(calls).to.deep.equal(["first", "allocated 64", "last"]); + done(); + } catch (e) { + done(e); + } + }; + 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();