From 8166d52ac7e38538cb47c1cca505a3023548450c Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 6 Aug 2026 16:33:30 -0700 Subject: [PATCH 1/4] XMLHttpRequest: implement the `on` handler properties `XMLHttpRequest::RaiseEvent` only dispatches to handlers stored in `m_eventHandlerRefs`, which is populated exclusively by `addEventListener`. The class exposed no accessors for the DOM `on` handler properties, so `xhr.onreadystatechange = fn` merely created an ordinary expando property on the JS wrapper that nothing ever read. The failure mode is silent and severe: the request runs to completion and `readyState`/`status` are updated correctly, but the callback never fires, so code written against the standard XMLHttpRequest API waits forever for an event that cannot arrive. There is no error and no diagnostic -- it simply hangs. Add `onreadystatechange`, `onload`, `onerror`, `onloadend` and `onabort` as instance accessors, stored in a separate map from the `addEventListener` handlers because they have assignment semantics (setting replaces the previous handler) rather than accumulating, and because they must be individually readable and clearable via `xhr.onload = null`. `RaiseEvent` now dispatches the `on` handler in addition to any `addEventListener` handlers, matching the DOM, and `Send` releases the new strong references alongside the existing ones. Also raise the `load` event on success. It was previously never raised at all, so neither `onload` nor `addEventListener("load", ...)` could fire; only `loadend` and (on failure) `error` were dispatched. Success now dispatches `load` then `loadend`, and failure continues to dispatch `error` then `loadend`, per the spec. Adds five regression tests covering handler invocation on success and on HTTP 404, get/replace/clear semantics of the property, and co-existence with `addEventListener`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 60 +++++++++++ .../XMLHttpRequest/Source/XMLHttpRequest.h | 21 ++++ Tests/UnitTests/Scripts/tests.ts | 100 ++++++++++++++++++ 3 files changed, 181 insertions(+) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index d0220d16..0714e989 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -59,9 +59,48 @@ namespace Babylon::Polyfills::Internal constexpr const char* ReadyStateChange = "readystatechange"; constexpr const char* LoadEnd = "loadend"; constexpr const char* Error = "error"; + constexpr const char* Load = "load"; + constexpr const char* Abort = "abort"; } } + const char* const XMLHttpRequest::EVENT_TYPE_NAMES[static_cast(XMLHttpRequest::EventIndex::Count)] = { + EventType::ReadyStateChange, + EventType::Load, + EventType::Error, + EventType::LoadEnd, + EventType::Abort, + }; + + template + Napi::Value XMLHttpRequest::GetEventHandler(const Napi::CallbackInfo&) + { + const auto it = m_onEventHandlerRefs.find(EVENT_TYPE_NAMES[static_cast(Index)]); + if (it == m_onEventHandlerRefs.end()) + { + return Env().Null(); + } + + return it->second.Value(); + } + + template + void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo& info, const Napi::Value& value) + { + const char* eventType = EVENT_TYPE_NAMES[static_cast(Index)]; + + // Assigning null/undefined clears the handler, matching the DOM behavior where + // `xhr.onload = null` detaches the previously assigned handler. + if (!value.IsFunction()) + { + m_onEventHandlerRefs.erase(eventType); + return; + } + + m_onEventHandlerRefs[eventType] = Napi::Persistent(value.As()); + (void)info; + } + void XMLHttpRequest::Initialize(Napi::Env env) { static constexpr auto JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME = "XMLHttpRequest"; @@ -88,6 +127,15 @@ namespace Babylon::Polyfills::Internal // to tell a DNS failure from a refused connection or a missing local asset. InstanceAccessor("errorCode", &XMLHttpRequest::GetErrorCode, nullptr), InstanceAccessor("errorDetail", &XMLHttpRequest::GetErrorDetail, nullptr), + // DOM `on` handler properties. Without these, `xhr.onreadystatechange = fn` + // silently sets an ordinary expando property that is never invoked, so code written + // against the standard XMLHttpRequest API waits forever for a callback that can + // never fire. + InstanceAccessor("onreadystatechange", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), + InstanceAccessor("onload", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), + InstanceAccessor("onerror", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), + InstanceAccessor("onloadend", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), + InstanceAccessor("onabort", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), InstanceMethod("getAllResponseHeaders", &XMLHttpRequest::GetAllResponseHeaders), InstanceMethod("getResponseHeader", &XMLHttpRequest::GetResponseHeader), InstanceMethod("setRequestHeader", &XMLHttpRequest::SetRequestHeader), @@ -322,11 +370,16 @@ namespace Babylon::Polyfills::Internal { RaiseEvent(EventType::Error); } + else + { + RaiseEvent(EventType::Load); + } RaiseEvent(EventType::LoadEnd); // Assume the XMLHttpRequest will only be used for a single request and clear the event handlers. // Single use seems to be the standard pattern, and we need to release our strong refs to event handlers. m_eventHandlerRefs.clear(); + m_onEventHandlerRefs.clear(); }); } @@ -349,6 +402,13 @@ namespace Babylon::Polyfills::Internal eventHandlerRef.Call({}); } } + + // The DOM dispatches the `on` handler alongside any addEventListener handlers. + const auto onIt = m_onEventHandlerRefs.find(eventType); + if (onIt != m_onEventHandlerRefs.end()) + { + onIt->second.Call({}); + } } } diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 74d2c3b9..2f28d95e 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -39,6 +39,23 @@ namespace Babylon::Polyfills::Internal Napi::Value GetErrorCode(const Napi::CallbackInfo& info); Napi::Value GetErrorDetail(const Napi::CallbackInfo& info); + // Indices into XMLHttpRequest::EVENT_TYPE_NAMES; used to instantiate the `on` + // property accessors below without needing a distinct method per event type. + enum class EventIndex : size_t + { + ReadyStateChange = 0, + Load = 1, + Error = 2, + LoadEnd = 3, + Abort = 4, + Count = 5, + }; + + static const char* const EVENT_TYPE_NAMES[static_cast(EventIndex::Count)]; + + template Napi::Value GetEventHandler(const Napi::CallbackInfo& info); + template void SetEventHandler(const Napi::CallbackInfo& info, const Napi::Value& value); + void AddEventListener(const Napi::CallbackInfo& info); void RemoveEventListener(const Napi::CallbackInfo& info); void Abort(const Napi::CallbackInfo& info); @@ -53,5 +70,9 @@ namespace Babylon::Polyfills::Internal JsRuntimeScheduler m_runtimeScheduler; ReadyState m_readyState{ReadyState::Unsent}; std::unordered_map> m_eventHandlerRefs; + // The DOM `on` handler properties (onreadystatechange, onload, ...). These are + // kept separate from m_eventHandlerRefs because they have assignment semantics -- setting + // one replaces the previous handler -- whereas addEventListener accumulates. + std::unordered_map m_onEventHandlerRefs; }; } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index cdc9416b..c9f08b87 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -184,6 +184,106 @@ describe("XMLHTTPRequest", function () { expect(result.readyState).to.equal(4); }); + it("should invoke the 'onreadystatechange' handler property", async function () { + // Regression test: the on handler properties were not implemented, so + // `xhr.onreadystatechange = fn` set an ordinary expando property that was never + // invoked and callers waited forever for a callback that could never fire. + this.timeout(30000); + const result = await new Promise<{ states: number[]; status: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const states: number[] = []; + const guard = setTimeout(() => reject(new Error("onreadystatechange never reached readyState 4 within 25s")), 25000); + xhr.onreadystatechange = () => { + states.push(xhr.readyState); + if (xhr.readyState === 4) { + clearTimeout(guard); + resolve({ states, status: xhr.status }); + } + }; + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.states).to.include(4); + expect(result.status).to.equal(200); + }); + + it("should invoke the 'onload' and 'onloadend' handler properties on success", async function () { + this.timeout(30000); + const result = await new Promise<{ loadFired: boolean; loadEndFired: boolean; errorFired: boolean }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let loadFired = false; + let errorFired = false; + const guard = setTimeout(() => reject(new Error("onloadend did not fire within 25s")), 25000); + xhr.onload = () => { loadFired = true; }; + xhr.onerror = () => { errorFired = true; }; + xhr.onloadend = () => { + clearTimeout(guard); + resolve({ loadFired, loadEndFired: true, errorFired }); + }; + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.loadFired).to.equal(true); + expect(result.loadEndFired).to.equal(true); + expect(result.errorFired).to.equal(false); + }); + + it("should invoke the 'onerror' handler property for HTTP 404", async function () { + this.timeout(30000); + const result = await new Promise<{ errorFired: boolean; loadFired: boolean; status: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let errorFired = false; + let loadFired = false; + const guard = setTimeout(() => reject(new Error("onloadend did not fire within 25s")), 25000); + xhr.onerror = () => { errorFired = true; }; + xhr.onload = () => { loadFired = true; }; + xhr.onloadend = () => { + clearTimeout(guard); + resolve({ errorFired, loadFired, status: xhr.status }); + }; + xhr.open("GET", "https://github.com/babylonJS/BabylonNative404"); + xhr.send(); + }); + expect(result.status).to.equal(404); + expect(result.errorFired).to.equal(true); + expect(result.loadFired).to.equal(false); + }); + + it("should let an on property be read back, replaced, and cleared", async function () { + const xhr = new XMLHttpRequest(); + expect(xhr.onload).to.equal(null); + + const first = () => { }; + xhr.onload = first; + expect(xhr.onload).to.equal(first); + + // Assignment replaces rather than accumulates, unlike addEventListener. + const second = () => { }; + xhr.onload = second; + expect(xhr.onload).to.equal(second); + + xhr.onload = null; + expect(xhr.onload).to.equal(null); + }); + + it("should invoke both an on property and addEventListener handlers", async function () { + this.timeout(30000); + const result = await new Promise<{ order: string[] }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const order: string[] = []; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.onload = () => { order.push("onload"); }; + xhr.addEventListener("load", () => { order.push("listener"); }); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ order }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.order).to.have.members(["onload", "listener"]); + }); + it("should expose errorCode/errorDetail diagnostics after a transport failure", async function () { this.timeout(30000); const xhr: any = await createRequest("GET", "http://127.0.0.1:1/"); From c870d43da28801d60910c107dd4792c5af593f1e Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 6 Aug 2026 22:10:00 -0700 Subject: [PATCH 2/4] XMLHttpRequest: dispatch 'abort', harden RaiseEvent, document coercion Addresses review feedback on the `on` handler properties: - `onabort` was exposed but no `abort` event was ever dispatched, so the handler could never fire. `Abort()` now records the caller's intent and the completion continuation reports the outcome as `abort` + `loadend` instead of `error`, matching the DOM. - `RaiseEvent` now snapshots the handler list before dispatching. A handler is free to call `addEventListener`/`removeEventListener` or reassign an `on` property, either of which would reallocate the vector or rehash the map out from under an in-flight dispatch. It also clears pending exceptions between handlers so a throwing handler neither aborts the rest of the dispatch nor escapes into the native completion continuation. This mirrors `FileReader::Dispatch`. - Documented why a non-callable assignment clears the handler rather than throwing: `EventHandler` attributes are `[LegacyTreatNonObjectAsNull]` in WebIDL, so `xhr.onload = 0` yields `null` rather than a TypeError. Adds regression tests for the abort event and the non-callable coercion. --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 55 +++++++++++++++---- .../XMLHttpRequest/Source/XMLHttpRequest.h | 2 + Tests/UnitTests/Scripts/tests.ts | 47 ++++++++++++++++ 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 0714e989..e5d481ad 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -89,8 +89,10 @@ namespace Babylon::Polyfills::Internal { const char* eventType = EVENT_TYPE_NAMES[static_cast(Index)]; - // Assigning null/undefined clears the handler, matching the DOM behavior where - // `xhr.onload = null` detaches the previously assigned handler. + // `EventHandler` attributes are declared [LegacyTreatNonObjectAsNull] in WebIDL, so a + // non-callable assignment is coerced to null rather than throwing: `xhr.onload = 0` + // leaves `xhr.onload === null`. We extend that to non-callable objects too -- storing a + // value we could never invoke would only defer the failure to dispatch time. if (!value.IsFunction()) { m_onEventHandlerRefs.erase(eventType); @@ -296,6 +298,10 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::Abort(const Napi::CallbackInfo&) { + // Record the caller's intent so the in-flight continuation reports this as an abort + // rather than a transport error. If no request is in flight this is inert, matching the + // DOM, where abort() on an unsent request produces no observable events. + m_aborted = true; m_request.Abort(); } @@ -366,7 +372,13 @@ namespace Babylon::Polyfills::Internal const bool failed = result.has_error() || statusCode < 200 || statusCode >= 300; SetReadyState(ReadyState::Done); - if (failed) + if (m_aborted) + { + // A cancelled request is not a transport failure: the DOM reports it as + // 'abort' + 'loadend' and never raises 'error'. + RaiseEvent(EventType::Abort); + } + else if (failed) { RaiseEvent(EventType::Error); } @@ -393,21 +405,44 @@ namespace Babylon::Polyfills::Internal { std::string traceName = (std::ostringstream{} << "XMLHttpRequest::RaiseEvent [" << eventType << "] [" << m_url << "]").str(); arcana::trace_region raiseEventRegion{traceName.c_str()}; + + Napi::Env env = Env(); + + // Snapshot the handlers before dispatching. A handler may call addEventListener, + // removeEventListener, or reassign an on property while it runs, which would + // otherwise reallocate the vector or rehash the map out from under this dispatch. + // (Mirrors FileReader::Dispatch.) + std::vector handlers{}; + + const auto onIt = m_onEventHandlerRefs.find(eventType); + if (onIt != m_onEventHandlerRefs.end() && !onIt->second.IsEmpty()) + { + handlers.push_back(onIt->second.Value()); + } + const auto it = m_eventHandlerRefs.find(eventType); if (it != m_eventHandlerRefs.end()) { - const auto& eventHandlerRefs = it->second; - for (const auto& eventHandlerRef : eventHandlerRefs) + handlers.reserve(handlers.size() + it->second.size()); + for (const auto& eventHandlerRef : it->second) { - eventHandlerRef.Call({}); + if (!eventHandlerRef.IsEmpty()) + { + handlers.push_back(eventHandlerRef.Value()); + } } } - // The DOM dispatches the `on` handler alongside any addEventListener handlers. - const auto onIt = m_onEventHandlerRefs.find(eventType); - if (onIt != m_onEventHandlerRefs.end()) + for (const auto& handler : handlers) { - onIt->second.Call({}); + handler.Call({}); + + // A throwing handler must not abort the remaining dispatch, and the exception must + // not escape into the native completion continuation that called us. + if (env.IsExceptionPending()) + { + env.GetAndClearPendingException(); + } } } } diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 2f28d95e..ec22818e 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -69,6 +69,8 @@ namespace Babylon::Polyfills::Internal UrlLib::UrlRequest m_request{}; JsRuntimeScheduler m_runtimeScheduler; ReadyState m_readyState{ReadyState::Unsent}; + // Set by abort(); makes the in-flight continuation report 'abort' instead of 'error'. + bool m_aborted{false}; std::unordered_map> m_eventHandlerRefs; // The DOM `on` handler properties (onreadystatechange, onload, ...). These are // kept separate from m_eventHandlerRefs because they have assignment semantics -- setting diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index c9f08b87..e2af3a53 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -266,6 +266,53 @@ describe("XMLHTTPRequest", function () { expect(xhr.onload).to.equal(null); }); + it("should coerce a non-callable on assignment to null", function () { + // EventHandler attributes are [LegacyTreatNonObjectAsNull] in WebIDL: assigning a + // non-callable value clears the handler rather than throwing a TypeError. + const xhr: any = new XMLHttpRequest(); + xhr.onload = () => { }; + expect(xhr.onload).to.not.equal(null); + + xhr.onload = 0; + expect(xhr.onload).to.equal(null); + + xhr.onload = () => { }; + xhr.onload = "not a function"; + expect(xhr.onload).to.equal(null); + + xhr.onload = () => { }; + xhr.onload = undefined; + expect(xhr.onload).to.equal(null); + }); + + it("should fire 'abort' rather than 'error' when a request is aborted", async function () { + this.timeout(30000); + const result = await new Promise<{ abortFired: boolean; errorFired: boolean; loadFired: boolean; loadEndFired: boolean }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let abortFired = false; + let errorFired = false; + let loadFired = false; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.onabort = () => { abortFired = true; }; + xhr.onerror = () => { errorFired = true; }; + xhr.onload = () => { loadFired = true; }; + xhr.onloadend = () => { + clearTimeout(guard); + resolve({ abortFired, errorFired, loadFired, loadEndFired: true }); + }; + xhr.open("GET", "https://github.com/"); + xhr.send(); + xhr.abort(); + }); + // loadend must always settle the request, whatever the outcome. + expect(result.loadEndFired).to.equal(true); + // The abort was requested before the transfer could complete, so it must be reported + // as an abort -- never as a transport error, and never as a successful load. + expect(result.abortFired).to.equal(true); + expect(result.errorFired).to.equal(false); + expect(result.loadFired).to.equal(false); + }); + it("should invoke both an on property and addEventListener handlers", async function () { this.timeout(30000); const result = await new Promise<{ order: string[] }>((resolve, reject) => { From 20b0c201a2dea7681ce01182dc19b37254b3fdf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Mon, 10 Aug 2026 20:34:35 -0700 Subject: [PATCH 3/4] XMLHttpRequest: unify on handlers with addEventListener listeners Addresses review feedback on #221. The on properties lived in a parallel map dispatched ahead of the addEventListener list, which diverged from the DOM in two ways: - Dispatch ignored registration order. addEventListener("load", a) followed by xhr.onload = b called b then a; browsers call a then b. - xhr.onload = f; xhr.addEventListener("load", f) threw, where a browser registers both and calls f twice. Both kinds of listener now share one vector per event type, tagged with isEventHandler. The setter replaces the flagged entry in place so reassignment keeps its position, matching "If eventHandler's listener is not null, then return"; it appends when absent and erases when the assigned value is not callable. The duplicate check in addEventListener and the match in removeEventListener both skip the flagged entry, since those operate on addEventListener registrations only. Also narrow the failure test so a completed HTTP transaction dispatches 'load' regardless of status: const bool failed = result.has_error() || statusCode == 0; Per spec 'error' is for network-level failure; a 404 fires 'load' and callers branch on xhr.status. UrlStatusCode::None (0) is UrlLib's "no response obtained" sentinel -- it is only ever the initial value and the reset in ResetForOpen, because every path producing a response assigns an explicit code, including non-HTTP local file reads which set Ok. So the missing-local-file-on-UWP case that this condition was widened for still reports 'error'. Tests: both 404 tests now assert load fired and error did not, plus new coverage for cross-style dispatch order, position on reassignment, a function registered both ways being called twice, and removeEventListener not removing an on handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 98 +++++++++++------ .../XMLHttpRequest/Source/XMLHttpRequest.h | 18 +++- Tests/UnitTests/Scripts/tests.ts | 100 +++++++++++++++--- 3 files changed, 165 insertions(+), 51 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index e5d481ad..aaa82848 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -75,19 +76,28 @@ namespace Babylon::Polyfills::Internal template Napi::Value XMLHttpRequest::GetEventHandler(const Napi::CallbackInfo&) { - const auto it = m_onEventHandlerRefs.find(EVENT_TYPE_NAMES[static_cast(Index)]); - if (it == m_onEventHandlerRefs.end()) + const auto it = m_listeners.find(EVENT_TYPE_NAMES[static_cast(Index)]); + if (it != m_listeners.end()) { - return Env().Null(); + for (const auto& listener : it->second) + { + if (listener.isEventHandler) + { + return listener.callback.Value(); + } + } } - return it->second.Value(); + return Env().Null(); } template - void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo& info, const Napi::Value& value) + void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo&, const Napi::Value& value) { - const char* eventType = EVENT_TYPE_NAMES[static_cast(Index)]; + auto& listeners = m_listeners[EVENT_TYPE_NAMES[static_cast(Index)]]; + const auto it = std::find_if(listeners.begin(), listeners.end(), [](const Listener& listener) { + return listener.isEventHandler; + }); // `EventHandler` attributes are declared [LegacyTreatNonObjectAsNull] in WebIDL, so a // non-callable assignment is coerced to null rather than throwing: `xhr.onload = 0` @@ -95,12 +105,24 @@ namespace Babylon::Polyfills::Internal // value we could never invoke would only defer the failure to dispatch time. if (!value.IsFunction()) { - m_onEventHandlerRefs.erase(eventType); + if (it != listeners.end()) + { + listeners.erase(it); + } + return; } - m_onEventHandlerRefs[eventType] = Napi::Persistent(value.As()); - (void)info; + if (it != listeners.end()) + { + // Replace in place so reassignment keeps this listener's position in the + // dispatch order. + it->callback = Napi::Persistent(value.As()); + } + else + { + listeners.push_back(Listener{Napi::Persistent(value.As()), true}); + } } void XMLHttpRequest::Initialize(Napi::Env env) @@ -265,31 +287,36 @@ namespace Babylon::Polyfills::Internal const std::string eventType = info[0].As().Utf8Value(); const Napi::Function eventHandler = info[1].As(); - const auto& eventHandlerRefs = m_eventHandlerRefs[eventType]; - for (auto it = eventHandlerRefs.begin(); it != eventHandlerRefs.end(); ++it) + auto& listeners = m_listeners[eventType]; + for (const auto& listener : listeners) { - if (it->Value() == eventHandler) + // Deliberately skips the `on` entry: `xhr.onload = f` followed by + // `xhr.addEventListener("load", f)` is two independent registrations, and a browser + // calls `f` twice rather than rejecting the second. + if (!listener.isEventHandler && listener.callback.Value() == eventHandler) { throw Napi::Error::New(info.Env(), "Cannot add the same event handler twice"); } } - m_eventHandlerRefs[eventType].push_back(Napi::Persistent(eventHandler)); + listeners.push_back(Listener{Napi::Persistent(eventHandler), false}); } void XMLHttpRequest::RemoveEventListener(const Napi::CallbackInfo& info) { const std::string eventType = info[0].As().Utf8Value(); const Napi::Function eventHandler = info[1].As(); - const auto itType = m_eventHandlerRefs.find(eventType); - if (itType != m_eventHandlerRefs.end()) + const auto itType = m_listeners.find(eventType); + if (itType != m_listeners.end()) { - auto& eventHandlerRefs = itType->second; - for (auto it = eventHandlerRefs.begin(); it != eventHandlerRefs.end(); ++it) + auto& listeners = itType->second; + for (auto it = listeners.begin(); it != listeners.end(); ++it) { - if (it->Value() == eventHandler) + // removeEventListener never removes an `on` handler; that is done by + // assigning null to the property. + if (!it->isEventHandler && it->callback.Value() == eventHandler) { - eventHandlerRefs.erase(it); + listeners.erase(it); break; } } @@ -369,7 +396,15 @@ namespace Babylon::Polyfills::Internal // success-only continuation here skipped readyState=Done / loadend / error and let the JS observer // hang. const auto statusCode = arcana::underlying_cast(m_request.StatusCode()); - const bool failed = result.has_error() || statusCode < 200 || statusCode >= 300; + // `error` is reserved for transport-level failure. A completed HTTP transaction + // that returned a non-2xx status (e.g. 404) is still a successful exchange, so it + // dispatches `load` and the caller branches on `xhr.status` inside the handler. + // UrlStatusCode::None (0) is UrlLib's "no response was obtained" sentinel: it is + // only ever the initial value and the reset in ResetForOpen, because every path + // that produces a response assigns an explicit code -- including the non-HTTP + // ones, where local file reads set Ok. That keeps the missing-local-file-on-UWP + // case (status left at 0) reporting `error`. + const bool failed = result.has_error() || statusCode == 0; SetReadyState(ReadyState::Done); if (m_aborted) @@ -390,8 +425,7 @@ namespace Babylon::Polyfills::Internal // Assume the XMLHttpRequest will only be used for a single request and clear the event handlers. // Single use seems to be the standard pattern, and we need to release our strong refs to event handlers. - m_eventHandlerRefs.clear(); - m_onEventHandlerRefs.clear(); + m_listeners.clear(); }); } @@ -414,21 +448,17 @@ namespace Babylon::Polyfills::Internal // (Mirrors FileReader::Dispatch.) std::vector handlers{}; - const auto onIt = m_onEventHandlerRefs.find(eventType); - if (onIt != m_onEventHandlerRefs.end() && !onIt->second.IsEmpty()) - { - handlers.push_back(onIt->second.Value()); - } - - const auto it = m_eventHandlerRefs.find(eventType); - if (it != m_eventHandlerRefs.end()) + const auto it = m_listeners.find(eventType); + if (it != m_listeners.end()) { - handlers.reserve(handlers.size() + it->second.size()); - for (const auto& eventHandlerRef : it->second) + // One pass over the single list, so handlers run in registration order regardless of + // whether they arrived via addEventListener or an `on` property. + handlers.reserve(it->second.size()); + for (const auto& listener : it->second) { - if (!eventHandlerRef.IsEmpty()) + if (!listener.callback.IsEmpty()) { - handlers.push_back(eventHandlerRef.Value()); + handlers.push_back(listener.callback.Value()); } } } diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index ec22818e..5cd3627d 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -65,16 +65,24 @@ namespace Babylon::Polyfills::Internal void SetReadyState(ReadyState readyState); void RaiseEvent(const char* eventType); + // A registered event listener. `isEventHandler` marks the single entry owned by the + // matching `on` property; every other entry came from addEventListener. Both + // kinds share one list per event type because that is what the DOM specifies: dispatch + // follows registration order, so `addEventListener("load", a)` then `xhr.onload = b` + // calls `a` then `b`, and reassigning `onload` keeps its original position rather than + // moving to the end ("If eventHandler's listener is not null, then return"). + struct Listener + { + Napi::FunctionReference callback; + bool isEventHandler; + }; + std::string m_url{}; UrlLib::UrlRequest m_request{}; JsRuntimeScheduler m_runtimeScheduler; ReadyState m_readyState{ReadyState::Unsent}; // Set by abort(); makes the in-flight continuation report 'abort' instead of 'error'. bool m_aborted{false}; - std::unordered_map> m_eventHandlerRefs; - // The DOM `on` handler properties (onreadystatechange, onload, ...). These are - // kept separate from m_eventHandlerRefs because they have assignment semantics -- setting - // one replaces the previous handler -- whereas addEventListener accumulates. - std::unordered_map m_onEventHandlerRefs; + std::unordered_map> m_listeners; }; } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index e2af3a53..cec354d6 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -159,27 +159,34 @@ describe("XMLHTTPRequest", function () { expect(notFoundXhr.statusText).to.equal("Not Found"); }); - it("should fire 'error' event for a remote URL that returns HTTP 404", async function () { + it("should fire 'load' rather than 'error' for a remote URL that returns HTTP 404", async function () { // Regression test: previously the success-only continuation in XMLHttpRequest::Send - // skipped 'error' on async failures including non-2xx HTTP responses, so onerror - // observers never ran. See https://github.com/BabylonJS/JsRuntimeHost/pull/165. + // skipped the completion events entirely on async failures, so observers never ran. + // See https://github.com/BabylonJS/JsRuntimeHost/pull/165. + // + // A 404 is a *completed* HTTP transaction, so per spec it dispatches 'load' and callers + // branch on xhr.status inside the handler; 'error' is reserved for transport-level + // failures, which report status 0. this.timeout(30000); - const result = await new Promise<{ errorFired: boolean; loadendFired: boolean; status: number; readyState: number }>((resolve, reject) => { + const result = await new Promise<{ errorFired: boolean; loadFired: boolean; loadendFired: boolean; status: number; readyState: number }>((resolve, reject) => { const xhr = new XMLHttpRequest(); let errorFired = false; + let loadFired = false; let loadendFired = false; - const guard = setTimeout(() => reject(new Error("XHR neither errored nor loadended within 25s")), 25000); + const guard = setTimeout(() => reject(new Error("XHR neither loaded nor loadended within 25s")), 25000); xhr.addEventListener("error", () => { errorFired = true; }); + xhr.addEventListener("load", () => { loadFired = true; }); xhr.addEventListener("loadend", () => { loadendFired = true; clearTimeout(guard); - resolve({ errorFired, loadendFired, status: xhr.status, readyState: xhr.readyState }); + resolve({ errorFired, loadFired, loadendFired, status: xhr.status, readyState: xhr.readyState }); }); xhr.open("GET", "https://github.com/babylonJS/BabylonNative404"); xhr.send(); }); expect(result.status).to.equal(404); - expect(result.errorFired).to.equal(true); + expect(result.loadFired).to.equal(true); + expect(result.errorFired).to.equal(false); expect(result.loadendFired).to.equal(true); expect(result.readyState).to.equal(4); }); @@ -228,7 +235,9 @@ describe("XMLHTTPRequest", function () { expect(result.errorFired).to.equal(false); }); - it("should invoke the 'onerror' handler property for HTTP 404", async function () { + it("should invoke the 'onload' handler property, not 'onerror', for HTTP 404", async function () { + // 'error' means the transfer never completed. A 404 completed and carries a status, so + // the load handler runs and inspects xhr.status. this.timeout(30000); const result = await new Promise<{ errorFired: boolean; loadFired: boolean; status: number }>((resolve, reject) => { const xhr = new XMLHttpRequest(); @@ -245,8 +254,8 @@ describe("XMLHTTPRequest", function () { xhr.send(); }); expect(result.status).to.equal(404); - expect(result.errorFired).to.equal(true); - expect(result.loadFired).to.equal(false); + expect(result.loadFired).to.equal(true); + expect(result.errorFired).to.equal(false); }); it("should let an on property be read back, replaced, and cleared", async function () { @@ -313,14 +322,81 @@ describe("XMLHTTPRequest", function () { expect(result.loadFired).to.equal(false); }); - it("should invoke both an on property and addEventListener handlers", async function () { + it("should dispatch on properties and addEventListener handlers in registration order", async function () { + // on handlers and addEventListener listeners share one list per event type, so + // dispatch follows registration order across both styles rather than running all the + // on handlers first. this.timeout(30000); const result = await new Promise<{ order: string[] }>((resolve, reject) => { const xhr = new XMLHttpRequest(); const order: string[] = []; const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.addEventListener("load", () => { order.push("first"); }); xhr.onload = () => { order.push("onload"); }; + xhr.addEventListener("load", () => { order.push("last"); }); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ order }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.order).to.deep.equal(["first", "onload", "last"]); + }); + + it("should keep an on handler's position in the dispatch order when reassigned", async function () { + // Per HTML the internal listener is registered on first set and reused thereafter ("If + // eventHandler's listener is not null, then return"), so reassigning the property must + // not move it to the end of the list. + this.timeout(30000); + const result = await new Promise<{ order: string[] }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const order: string[] = []; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.onload = () => { order.push("replaced"); }; xhr.addEventListener("load", () => { order.push("listener"); }); + xhr.onload = () => { order.push("onload"); }; + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ order }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.order).to.deep.equal(["onload", "listener"]); + }); + + it("should invoke a function registered both as an on property and via addEventListener twice", async function () { + // These are two independent registrations, so the duplicate-registration check must not + // see the on entry: a browser calls the shared function once for each. + this.timeout(30000); + const result = await new Promise<{ calls: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let calls = 0; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + const handler = () => { calls++; }; + xhr.onload = handler; + xhr.addEventListener("load", handler); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ calls }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.calls).to.equal(2); + }); + + it("should not let removeEventListener remove an on handler", async function () { + // The property is cleared by assigning null, not by removeEventListener. + this.timeout(30000); + const result = await new Promise<{ order: string[] }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const order: string[] = []; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + const handler = () => { order.push("onload"); }; + xhr.onload = handler; + xhr.removeEventListener("load", handler); xhr.addEventListener("loadend", () => { clearTimeout(guard); resolve({ order }); @@ -328,7 +404,7 @@ describe("XMLHTTPRequest", function () { xhr.open("GET", "app:///Scripts/symlink_target.js"); xhr.send(); }); - expect(result.order).to.have.members(["onload", "listener"]); + expect(result.order).to.deep.equal(["onload"]); }); it("should expose errorCode/errorDetail diagnostics after a transport failure", async function () { From 87c5c1b5c12bb3d3e47a95e1745f0c2d76cd0776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:32:10 -0700 Subject: [PATCH 4/4] XMLHttpRequest: make a duplicate addEventListener a no-op Re-adding an identical (type, callback) pair threw "Cannot add the same event handler twice". Per DOM the second add is a silent no-op, so the throw made valid browser code fail against the polyfill. The scan still skips `isEventHandler` entries, so `xhr.onload = f` followed by `xhr.addEventListener("load", f)` remains two independent registrations and still calls `f` twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48268912-5d88-4e04-93ca-0c5cd35a03ad --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 8 +++++-- Tests/UnitTests/Scripts/tests.ts | 21 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index aaa82848..399613e8 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -292,10 +292,14 @@ namespace Babylon::Polyfills::Internal { // Deliberately skips the `on` entry: `xhr.onload = f` followed by // `xhr.addEventListener("load", f)` is two independent registrations, and a browser - // calls `f` twice rather than rejecting the second. + // calls `f` twice rather than collapsing them. if (!listener.isEventHandler && listener.callback.Value() == eventHandler) { - throw Napi::Error::New(info.Env(), "Cannot add the same event handler twice"); + // Per DOM, re-adding an identical (type, callback, capture) triple is a silent + // no-op rather than an error: "If eventTarget's event listener list does not + // contain an event listener whose type is listener's type [...] then append + // listener". The listener stays registered once and is dispatched once. + return; } } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index cec354d6..c700b7fb 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -387,6 +387,27 @@ describe("XMLHTTPRequest", function () { expect(result.calls).to.equal(2); }); + it("should treat a duplicate addEventListener registration as a no-op", async function () { + // Per DOM, re-adding an identical (type, callback) pair is a silent no-op rather than an + // error, and the listener stays registered once, so it is dispatched once. + this.timeout(30000); + const result = await new Promise<{ calls: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let calls = 0; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + const handler = () => { calls++; }; + xhr.addEventListener("load", handler); + expect(() => xhr.addEventListener("load", handler)).to.not.throw(); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ calls }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.calls).to.equal(1); + }); + it("should not let removeEventListener remove an on handler", async function () { // The property is cleared by assigning null, not by removeEventListener. this.timeout(30000);