From bae8b619ec9c0e18e2eb627130f81f207be268d5 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 6 Aug 2026 16:33:30 -0700 Subject: [PATCH 01/16] 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 ++++ .../Source/Scripts/tests.xmlHttpRequest.ts | 100 ++++++++++++++++++ 3 files changed, 181 insertions(+) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index bac1f90f..f1c7a60b 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), @@ -328,11 +376,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(); }); } @@ -355,6 +408,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/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index ab50e950..829a7172 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -87,6 +87,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:///Assets/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:///Assets/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:///Assets/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 7b21713fa7acc71383349ce6ec38c53a17e9e4ed Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 6 Aug 2026 22:10:00 -0700 Subject: [PATCH 02/16] 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 + .../Source/Scripts/tests.xmlHttpRequest.ts | 47 ++++++++++++++++ 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index f1c7a60b..dc7f075c 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); @@ -302,6 +304,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(); } @@ -372,7 +378,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); } @@ -399,21 +411,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/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index 829a7172..948731f4 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -169,6 +169,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 1799e8090cfb0ec1f6a421f12df3e943232fdad1 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 03/16] 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 ++- .../Source/Scripts/tests.xmlHttpRequest.ts | 104 +++++++++++++++--- 3 files changed, 167 insertions(+), 53 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index dc7f075c..6e5ab47f 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) @@ -271,31 +293,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; } } @@ -375,7 +402,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) @@ -396,8 +431,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(); }); } @@ -420,21 +454,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/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index 948731f4..94de538d 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -62,27 +62,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); }); @@ -131,7 +138,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(); @@ -148,8 +157,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 () { @@ -216,22 +225,89 @@ 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:///Assets/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:///Assets/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:///Assets/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 }); + }); xhr.open("GET", "app:///Assets/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 () { @@ -349,4 +425,4 @@ describe("XMLHTTPRequest", function () { expect(xhr.responseText.length).to.equal(16); expect(xhr.response).to.equal(xhr.responseText); }); -}); +} From 3dd69ad1fe7f8074a46a6e00a515df87977b06a6 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 04/16] 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 +++++-- .../Source/Scripts/tests.xmlHttpRequest.ts | 21 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 6e5ab47f..5c0788d0 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -298,10 +298,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/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index 94de538d..83a96cdc 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -290,6 +290,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:///Assets/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); From 7f0f615c6d2672dbe5a31ecc6c3bdec4e08e8738 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Tue, 22 Sep 2026 13:09:37 -0700 Subject: [PATCH 05/16] Enforce WebIDL event-handler conversion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp | 13 ++++++++----- .../Source/Scripts/tests.xmlHttpRequest.ts | 9 +++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 5c0788d0..f1ff3a6d 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -99,11 +99,10 @@ namespace Babylon::Polyfills::Internal 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` - // 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()) + // `EventHandler` attributes are declared [LegacyTreatNonObjectAsNull] in WebIDL: + // primitive assignments clear the handler, while a non-callable object fails callback + // conversion with a TypeError. + if (!value.IsObject()) { if (it != listeners.end()) { @@ -112,6 +111,10 @@ namespace Babylon::Polyfills::Internal return; } + if (!value.IsFunction()) + { + throw Napi::TypeError::New(Env(), "Event handler must be callable"); + } if (it != listeners.end()) { diff --git a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index 83a96cdc..06fb5f2c 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -197,6 +197,15 @@ describe("XMLHTTPRequest", function () { expect(xhr.onload).to.equal(null); }); + it("should reject a non-callable object assigned to an on property", function () { + const xhr: any = new XMLHttpRequest(); + const handler = () => { }; + xhr.onload = handler; + + expect(() => { xhr.onload = {}; }).to.throw(TypeError); + expect(xhr.onload).to.equal(handler); + }); + 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) => { From f23fae8990eb8d120a71f52de613f0857a54796a Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Tue, 22 Sep 2026 15:05:18 -0700 Subject: [PATCH 06/16] Complete XMLHttpRequest event semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Polyfills/XMLHttpRequest/Readme.md | 9 +- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 185 +++++++++++------- .../XMLHttpRequest/Source/XMLHttpRequest.h | 13 +- .../Source/Scripts/tests.xmlHttpRequest.ts | 118 ++++++++++- 4 files changed, 247 insertions(+), 78 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Readme.md b/Polyfills/XMLHttpRequest/Readme.md index ea00d006..8480bc78 100644 --- a/Polyfills/XMLHttpRequest/Readme.md +++ b/Polyfills/XMLHttpRequest/Readme.md @@ -2,10 +2,15 @@ Minimal implementation of XMLHttpRequest required to support the Babylon.js RequestFile method. Under the hood, XMLHttpRequest is implemented using various platform-specific APIs in the UrlLib dependency. ## Event listening -We do not support `onload`-style event listeners. Instead, you should listen to events using `addEventListener`. At the moment, we only support the following events: +Events can be observed with `addEventListener` or the corresponding +`onreadystatechange`, `onload`, `onerror`, `onloadend`, and `onabort` properties. Handlers receive +an event whose `target` and `currentTarget` are the request, and run with the request as `this`. +At the moment, we only support the following events: * `loadend` * `readystatechange` -* `error` (fired on a transport failure or a non-`2xx` HTTP response, before `loadend`) +* `load` (fired after any completed HTTP response, including non-`2xx` responses) +* `error` (fired on a transport failure, before `loadend`) +* `abort` (fired when an active request is aborted, before `loadend`) ## Local files Unlike the web, XMLHttpRequest supports loading local files using two schemes: diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index f1ff3a6d..49c4b298 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -81,9 +81,9 @@ namespace Babylon::Polyfills::Internal { for (const auto& listener : it->second) { - if (listener.isEventHandler) + if (listener->active && listener->isEventHandler) { - return listener.callback.Value(); + return listener->callback.Value(); } } } @@ -95,36 +95,32 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo&, const Napi::Value& value) { 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; + const auto it = std::find_if(listeners.begin(), listeners.end(), [](const std::shared_ptr& listener) { + return listener->isEventHandler; }); - // `EventHandler` attributes are declared [LegacyTreatNonObjectAsNull] in WebIDL: - // primitive assignments clear the handler, while a non-callable object fails callback - // conversion with a TypeError. + // Event handler attributes treat primitive values as null. Object values are retained + // verbatim for the getter; non-callable objects are simply skipped during dispatch. if (!value.IsObject()) { if (it != listeners.end()) { + (*it)->active = false; listeners.erase(it); } return; } - if (!value.IsFunction()) - { - throw Napi::TypeError::New(Env(), "Event handler must be callable"); - } if (it != listeners.end()) { // Replace in place so reassignment keeps this listener's position in the // dispatch order. - it->callback = Napi::Persistent(value.As()); + (*it)->callback = Napi::Persistent(value.As()); } else { - listeners.push_back(Listener{Napi::Persistent(value.As()), true}); + listeners.push_back(std::make_shared(Listener{Napi::Persistent(value.As()), true})); } } @@ -194,14 +190,14 @@ namespace Babylon::Polyfills::Internal Napi::Value XMLHttpRequest::GetResponse(const Napi::CallbackInfo&) { - if (m_request.ResponseType() == UrlLib::UrlResponseType::String) + if (m_request->ResponseType() == UrlLib::UrlResponseType::String) { - const std::string_view responseString{m_request.ResponseString()}; + const std::string_view responseString{m_request->ResponseString()}; return Napi::String::New(Env(), responseString.data(), responseString.size()); } else { - gsl::span responseBuffer{m_request.ResponseBuffer()}; + gsl::span responseBuffer{m_request->ResponseBuffer()}; auto arrayBuffer{Napi::ArrayBuffer::New(Env(), responseBuffer.size())}; std::memcpy(arrayBuffer.Data(), responseBuffer.data(), arrayBuffer.ByteLength()); return arrayBuffer; @@ -214,66 +210,66 @@ namespace Babylon::Polyfills::Internal // example, inlines the .wasm payload as a JavaScript string literal. Passing .data() // alone would hand a const char* to Napi and truncate at the first null, so the length // has to be supplied explicitly. - const std::string_view responseString{m_request.ResponseString()}; + const std::string_view responseString{m_request->ResponseString()}; return Napi::String::New(Env(), responseString.data(), responseString.size()); } Napi::Value XMLHttpRequest::GetResponseType(const Napi::CallbackInfo&) { - return Napi::Value::From(Env(), ResponseType::EnumToString(m_request.ResponseType())); + return Napi::Value::From(Env(), ResponseType::EnumToString(m_request->ResponseType())); } void XMLHttpRequest::SetResponseType(const Napi::CallbackInfo&, const Napi::Value& value) { - m_request.ResponseType(ResponseType::StringToEnum(value.As().Utf8Value())); + m_request->ResponseType(ResponseType::StringToEnum(value.As().Utf8Value())); } Napi::Value XMLHttpRequest::GetResponseURL(const Napi::CallbackInfo&) { - return Napi::Value::From(Env(), m_request.ResponseUrl().data()); + return Napi::Value::From(Env(), m_request->ResponseUrl().data()); } Napi::Value XMLHttpRequest::GetStatus(const Napi::CallbackInfo&) { - return Napi::Value::From(Env(), arcana::underlying_cast(m_request.StatusCode())); + return Napi::Value::From(Env(), arcana::underlying_cast(m_request->StatusCode())); } Napi::Value XMLHttpRequest::GetStatusText(const Napi::CallbackInfo&) { // Per the XHR spec, statusText is the empty string until a response is available // (status 0 means UNSENT/OPENED or a network error). - if (arcana::underlying_cast(m_request.StatusCode()) == 0) + if (arcana::underlying_cast(m_request->StatusCode()) == 0) { return Napi::String::New(Env(), ""); } - return Napi::String::New(Env(), std::string{m_request.StatusText()}); + return Napi::String::New(Env(), std::string{m_request->StatusText()}); } Napi::Value XMLHttpRequest::GetErrorCode(const Napi::CallbackInfo&) { // Stable symbolic token for a transport failure (e.g. "CURLE_COULDNT_CONNECT", // "NSURLErrorTimedOut", "AppResourceNotFound"); empty when there was no transport failure. - return Napi::String::New(Env(), std::string{m_request.ErrorSymbol()}); + return Napi::String::New(Env(), std::string{m_request->ErrorSymbol()}); } Napi::Value XMLHttpRequest::GetErrorDetail(const Napi::CallbackInfo&) { // Full normalized ":(): " string; empty when there was no // transport failure. - return Napi::String::New(Env(), std::string{m_request.ErrorString()}); + return Napi::String::New(Env(), std::string{m_request->ErrorString()}); } Napi::Value XMLHttpRequest::GetResponseHeader(const Napi::CallbackInfo& info) { const auto headerName = info[0].As().Utf8Value(); - const auto header = m_request.GetResponseHeader(headerName); + const auto header = m_request->GetResponseHeader(headerName); return header ? Napi::Value::From(Env(), header.value()) : info.Env().Null(); } Napi::Value XMLHttpRequest::GetAllResponseHeaders(const Napi::CallbackInfo&) { - auto responseHeaders = m_request.GetAllResponseHeaders(); + auto responseHeaders = m_request->GetAllResponseHeaders(); Napi::Object responseHeadersObject = Napi::Object::New(Env()); for (auto& iter : responseHeaders) @@ -288,7 +284,7 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::SetRequestHeader(const Napi::CallbackInfo& info) { - m_request.SetRequestHeader(info[0].As().Utf8Value(), info[1].As().Utf8Value()); + m_request->SetRequestHeader(info[0].As().Utf8Value(), info[1].As().Utf8Value()); } void XMLHttpRequest::AddEventListener(const Napi::CallbackInfo& info) @@ -302,7 +298,7 @@ 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 collapsing them. - if (!listener.isEventHandler && listener.callback.Value() == eventHandler) + if (listener->active && !listener->isEventHandler && listener->callback.Value() == eventHandler) { // 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 @@ -312,7 +308,7 @@ namespace Babylon::Polyfills::Internal } } - listeners.push_back(Listener{Napi::Persistent(eventHandler), false}); + listeners.push_back(std::make_shared(Listener{Napi::Persistent(eventHandler.As()), false})); } void XMLHttpRequest::RemoveEventListener(const Napi::CallbackInfo& info) @@ -327,8 +323,9 @@ namespace Babylon::Polyfills::Internal { // removeEventListener never removes an `on` handler; that is done by // assigning null to the property. - if (!it->isEventHandler && it->callback.Value() == eventHandler) + if ((*it)->active && !(*it)->isEventHandler && (*it)->callback.Value() == eventHandler) { + (*it)->active = false; listeners.erase(it); break; } @@ -338,11 +335,25 @@ 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(); + if (m_readyState == ReadyState::Done) + { + m_readyState = ReadyState::Unsent; + return; + } + + if (!m_sendActive) + { + return; + } + + m_sendActive = false; + ++m_sendId; + m_request->Abort(); + + SetReadyState(ReadyState::Done); + RaiseEvent(EventType::Abort); + RaiseEvent(EventType::LoadEnd); + m_readyState = ReadyState::Unsent; } void XMLHttpRequest::Open(const Napi::CallbackInfo& info) @@ -351,7 +362,14 @@ namespace Babylon::Polyfills::Internal try { - m_request.Open(MethodType::StringToEnum(info[0].As().Utf8Value()), m_url); + if (m_sendActive) + { + m_request->Abort(); + m_sendActive = false; + } + ++m_sendId; + m_request = std::make_shared(); + m_request->Open(MethodType::StringToEnum(info[0].As().Utf8Value()), m_url); } catch (const std::exception& e) { @@ -381,7 +399,7 @@ namespace Babylon::Polyfills::Internal if (info[0].IsString()) { - m_request.SetRequestBody(info[0].As().Utf8Value()); + m_request->SetRequestBody(info[0].As().Utf8Value()); } } @@ -398,17 +416,26 @@ namespace Babylon::Polyfills::Internal // automatically once the request settles and the lambda is destroyed -- // no member self-reference to clear. (Mirrors FileReader's anchor.) auto anchor = std::make_shared(Napi::Persistent(info.This().As())); + const auto request = m_request; + const auto sendId = ++m_sendId; + m_sendActive = true; - m_request.SendAsync() + request->SendAsync() .then(arcana::inline_scheduler, arcana::cancellation::none(), [sendRegion{std::move(sendRegion)}]() mutable { sendRegion.reset(); }) - .then(m_runtimeScheduler, arcana::cancellation::none(), [this, anchor{std::move(anchor)}](const arcana::expected& result) { + .then(m_runtimeScheduler, arcana::cancellation::none(), [this, anchor{std::move(anchor)}, request, sendId](const arcana::expected& result) { + if (sendId != m_sendId) + { + return; + } + + m_sendActive = false; // Run on every outcome -- transport exception OR underlying request succeeded but ended in a non-2xx // status (e.g. a missing local file on UWP, where UrlLib silently retains status 0). The previous // 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 auto statusCode = arcana::underlying_cast(request->StatusCode()); // `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. @@ -420,13 +447,11 @@ namespace Babylon::Polyfills::Internal const bool failed = result.has_error() || statusCode == 0; SetReadyState(ReadyState::Done); - if (m_aborted) + if (sendId != m_sendId) { - // A cancelled request is not a transport failure: the DOM reports it as - // 'abort' + 'loadend' and never raises 'error'. - RaiseEvent(EventType::Abort); + return; } - else if (failed) + if (failed) { RaiseEvent(EventType::Error); } @@ -435,10 +460,6 @@ namespace Babylon::Polyfills::Internal 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_listeners.clear(); }); } @@ -455,38 +476,66 @@ namespace Babylon::Polyfills::Internal Napi::Env env = Env(); - // Snapshot the handlers before dispatching. A handler may call addEventListener, + // Snapshot stable listener records 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{}; + std::vector> listeners{}; const auto it = m_listeners.find(eventType); if (it != m_listeners.end()) { - // 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 (!listener.callback.IsEmpty()) - { - handlers.push_back(listener.callback.Value()); - } - } + listeners = it->second; + } + + const auto jsThis = Value(); + auto event = Napi::Object::New(env); + event.Set("type", eventType); + event.Set("target", jsThis); + event.Set("currentTarget", jsThis); + if (std::strcmp(eventType, EventType::ReadyStateChange) != 0) + { + event.Set("lengthComputable", false); + event.Set("loaded", 0); + event.Set("total", 0); } - for (const auto& handler : handlers) + std::vector> unhandledErrors{}; + for (const auto& listener : listeners) { - handler.Call({}); + if (!listener->active) + { + continue; + } + + const auto callback = listener->callback.Value(); + if (!callback.IsFunction()) + { + continue; + } + + try + { + callback.As().Call(jsThis, {event}); + } + catch (const Napi::Error& error) + { + unhandledErrors.push_back(std::make_shared(Napi::Persistent(error.Value()))); + continue; + } - // 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(); + auto error = env.GetAndClearPendingException(); + unhandledErrors.push_back(std::make_shared(Napi::Persistent(error.Value()))); } } + + for (const auto& error : unhandledErrors) + { + m_runtimeScheduler([env, error]() { + Napi::Error{env, error->Value()}.ThrowAsJavaScriptException(); + }); + } } } diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 5cd3627d..e33b6f94 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -5,6 +5,8 @@ #include #include +#include +#include #include #include @@ -73,16 +75,17 @@ namespace Babylon::Polyfills::Internal // moving to the end ("If eventHandler's listener is not null, then return"). struct Listener { - Napi::FunctionReference callback; + Napi::ObjectReference callback; bool isEventHandler; + bool active{true}; }; std::string m_url{}; - UrlLib::UrlRequest m_request{}; + std::shared_ptr m_request{std::make_shared()}; 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_listeners; + uint64_t m_sendId{}; + bool m_sendActive{false}; + std::unordered_map>> m_listeners; }; } diff --git a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index 06fb5f2c..2ba40ea7 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -197,15 +197,43 @@ describe("XMLHTTPRequest", function () { expect(xhr.onload).to.equal(null); }); - it("should reject a non-callable object assigned to an on property", function () { + it("should retain but not invoke a non-callable object assigned to an on property", function () { const xhr: any = new XMLHttpRequest(); - const handler = () => { }; + const handler = {}; xhr.onload = handler; - expect(() => { xhr.onload = {}; }).to.throw(TypeError); expect(xhr.onload).to.equal(handler); }); + it("should preserve handlers when the request completes and is reused", async function () { + this.timeout(30000); + const xhr = new XMLHttpRequest(); + let loadCalls = 0; + let loadEndCalls = 0; + const onload = () => { loadCalls++; }; + xhr.onload = onload; + + await new Promise((resolve, reject) => { + const guard = setTimeout(() => reject(new Error("reused XHR did not complete twice within 25s")), 25000); + xhr.onloadend = () => { + ++loadEndCalls; + expect(xhr.onload).to.equal(onload); + if (loadEndCalls === 1) { + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + } else { + clearTimeout(guard); + resolve(); + } + }; + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + + expect(loadCalls).to.equal(2); + expect(loadEndCalls).to.equal(2); + }); + 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) => { @@ -234,6 +262,39 @@ describe("XMLHTTPRequest", function () { expect(result.loadFired).to.equal(false); }); + it("should make abort synchronous, inert before send, and reusable", async function () { + this.timeout(30000); + const xhr = new XMLHttpRequest(); + const order: string[] = []; + xhr.onreadystatechange = () => { + if (xhr.readyState === XMLHttpRequest.DONE) { + order.push("readystatechange"); + } + }; + xhr.onabort = () => { order.push("abort"); }; + xhr.onloadend = () => { order.push("loadend"); }; + + xhr.open("GET", "https://github.com/"); + xhr.abort(); + expect(xhr.readyState).to.equal(XMLHttpRequest.OPENED); + expect(order).to.deep.equal([]); + + xhr.send(); + xhr.abort(); + expect(order).to.deep.equal(["readystatechange", "abort", "loadend"]); + expect(xhr.readyState).to.equal(XMLHttpRequest.UNSENT); + + await new Promise((resolve, reject) => { + const guard = setTimeout(() => reject(new Error("XHR did not complete after abort reuse within 25s")), 25000); + xhr.onload = () => { + clearTimeout(guard); + resolve(); + }; + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + }); + 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 @@ -278,6 +339,57 @@ describe("XMLHTTPRequest", function () { expect(result.order).to.deep.equal(["onload", "listener"]); }); + it("should observe listener removal and on reassignment during dispatch", async function () { + this.timeout(30000); + const result = await new Promise<{ order: string[] }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const order: string[] = []; + const removed = () => { order.push("removed"); }; + const replacement = () => { order.push("replacement"); }; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.addEventListener("load", () => { + order.push("first"); + xhr.removeEventListener("load", removed); + xhr.onload = replacement; + }); + xhr.addEventListener("load", removed); + xhr.onload = () => { order.push("original"); }; + xhr.onloadend = () => { + clearTimeout(guard); + resolve({ order }); + }; + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + expect(result.order).to.deep.equal(["first", "replacement"]); + }); + + it("should invoke handlers with the XHR as this and a populated event", async function () { + this.timeout(30000); + const xhr = new XMLHttpRequest(); + await new Promise((resolve, reject) => { + const guard = setTimeout(() => reject(new Error("load did not fire within 25s")), 25000); + xhr.onload = function (event: any) { + try { + expect(this).to.equal(xhr); + expect(event.type).to.equal("load"); + expect(event.target).to.equal(xhr); + expect(event.currentTarget).to.equal(xhr); + expect(event.lengthComputable).to.equal(false); + expect(event.loaded).to.equal(0); + expect(event.total).to.equal(0); + clearTimeout(guard); + resolve(); + } catch (error) { + clearTimeout(guard); + reject(error); + } + }; + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + }); + 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. From 47f5ab0750af4ef651eda74f34f301cb9a168531 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Tue, 22 Sep 2026 15:42:44 -0700 Subject: [PATCH 07/16] Preserve reentrant XHR requests and dispatch typed events Guard abort and completion event sequences against replacement sends; construct Event/ProgressEvent instances with dispatch lifecycle and stop-immediate-propagation semantics. Cover reentrant callbacks and event prototypes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Polyfills/XMLHttpRequest/Readme.md | 3 + .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 130 ++++++++++++++++-- .../XMLHttpRequest/Source/XMLHttpRequest.h | 1 + .../Source/Scripts/tests.xmlHttpRequest.ts | 122 ++++++++++++++++ 4 files changed, 242 insertions(+), 14 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Readme.md b/Polyfills/XMLHttpRequest/Readme.md index 8480bc78..bbc30971 100644 --- a/Polyfills/XMLHttpRequest/Readme.md +++ b/Polyfills/XMLHttpRequest/Readme.md @@ -5,6 +5,9 @@ Minimal implementation of XMLHttpRequest required to support the Babylon.js Requ Events can be observed with `addEventListener` or the corresponding `onreadystatechange`, `onload`, `onerror`, `onloadend`, and `onabort` properties. Handlers receive an event whose `target` and `currentTarget` are the request, and run with the request as `this`. +`readystatechange` dispatches an `Event`; `load`, `error`, `abort`, and `loadend` dispatch +`ProgressEvent` instances (`lengthComputable === false`, `loaded === total === 0`). +Missing constructors are installed without replacing host-provided ones. At the moment, we only support the following events: * `loadend` * `readystatechange` diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 49c4b298..25cbe803 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -3,8 +3,8 @@ #include #include #include -#include #include +#include namespace Babylon::Polyfills::Internal { @@ -63,6 +63,86 @@ namespace Babylon::Polyfills::Internal constexpr const char* Load = "load"; constexpr const char* Abort = "abort"; } + + constexpr const char* EVENT_FACTORY_NAME = "__jsRuntimeHostMakeXHREvent"; + constexpr const char* EVENT_FACTORY_SOURCE = R"JS( + (function (global) { + if (typeof global.Event !== "function") { + function Event(type, init) { + init = init || {}; + this.type = String(type); + this.bubbles = !!init.bubbles; + this.cancelable = !!init.cancelable; + this.composed = !!init.composed; + this.defaultPrevented = false; + this.target = null; + this.currentTarget = null; + this.eventPhase = 0; + this.timeStamp = Date.now(); + this.isTrusted = false; + this.cancelBubble = false; + } + Event.prototype.preventDefault = function () { + if (this.cancelable) this.defaultPrevented = true; + }; + Event.prototype.stopPropagation = function () { this.cancelBubble = true; }; + Event.prototype.stopImmediatePropagation = function () { this.cancelBubble = true; }; + Event.prototype.composedPath = function () { + return this.currentTarget === null ? [] : [this.target]; + }; + Event.NONE = Event.prototype.NONE = 0; + Event.CAPTURING_PHASE = Event.prototype.CAPTURING_PHASE = 1; + Event.AT_TARGET = Event.prototype.AT_TARGET = 2; + Event.BUBBLING_PHASE = Event.prototype.BUBBLING_PHASE = 3; + global.Event = Event; + } + + if (typeof global.ProgressEvent !== "function") { + function ProgressEvent(type, init) { + init = init || {}; + var event = new global.Event(type, init); + Object.setPrototypeOf(event, ProgressEvent.prototype); + event.lengthComputable = !!init.lengthComputable; + event.loaded = Number(init.loaded || 0); + event.total = Number(init.total || 0); + return event; + } + ProgressEvent.prototype = Object.create(global.Event.prototype); + ProgressEvent.prototype.constructor = ProgressEvent; + global.ProgressEvent = ProgressEvent; + } + + return function (type, target, progress) { + var event = progress + ? new global.ProgressEvent(type, {lengthComputable: false, loaded: 0, total: 0}) + : new global.Event(type); + var currentTarget = target; + var eventPhase = 2; + Object.defineProperties(event, { + target: {value: target, configurable: true}, + currentTarget: {get: function () { return currentTarget; }, configurable: true}, + eventPhase: {get: function () { return eventPhase; }, configurable: true} + }); + var stopped = false; + var stop = event.stopImmediatePropagation; + Object.defineProperty(event, "stopImmediatePropagation", { + configurable: true, + value: function () { + stopped = true; + return stop.call(this); + } + }); + return { + value: event, + isStopped: function () { return stopped; }, + end: function () { + currentTarget = null; + eventPhase = 0; + } + }; + }; + })(typeof globalThis === "object" ? globalThis : this) + )JS"; } const char* const XMLHttpRequest::EVENT_TYPE_NAMES[static_cast(XMLHttpRequest::EventIndex::Count)] = { @@ -169,6 +249,12 @@ namespace Babylon::Polyfills::Internal InstanceMethod("send", &XMLHttpRequest::Send), }); + auto eventFactory = env.RunScript(EVENT_FACTORY_SOURCE, "XMLHttpRequestEvents.js"); + auto descriptor = Napi::Object::New(env); + descriptor.Set("value", eventFactory); + auto object = env.Global().Get("Object").As(); + object.Get("defineProperty").As().Call(object, {func, Napi::String::New(env, EVENT_FACTORY_NAME), descriptor}); + if (env.Global().Get(JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME).IsUndefined()) { env.Global().Set(JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME, func); @@ -180,6 +266,7 @@ namespace Babylon::Polyfills::Internal XMLHttpRequest::XMLHttpRequest(const Napi::CallbackInfo& info) : Napi::ObjectWrap{info} , m_runtimeScheduler{JsRuntime::GetFromJavaScript(info.Env())} + , m_makeEvent{Napi::Persistent(info.NewTarget().As().Get(EVENT_FACTORY_NAME).As())} { } @@ -347,13 +434,24 @@ namespace Babylon::Polyfills::Internal } m_sendActive = false; - ++m_sendId; + const auto abortedSendId = ++m_sendId; m_request->Abort(); SetReadyState(ReadyState::Done); + if (m_sendId != abortedSendId) + { + return; + } RaiseEvent(EventType::Abort); + if (m_sendId != abortedSendId) + { + return; + } RaiseEvent(EventType::LoadEnd); - m_readyState = ReadyState::Unsent; + if (m_sendId == abortedSendId) + { + m_readyState = ReadyState::Unsent; + } } void XMLHttpRequest::Open(const Napi::CallbackInfo& info) @@ -459,7 +557,10 @@ namespace Babylon::Polyfills::Internal { RaiseEvent(EventType::Load); } - RaiseEvent(EventType::LoadEnd); + if (sendId == m_sendId) + { + RaiseEvent(EventType::LoadEnd); + } }); } @@ -488,16 +589,12 @@ namespace Babylon::Polyfills::Internal } const auto jsThis = Value(); - auto event = Napi::Object::New(env); - event.Set("type", eventType); - event.Set("target", jsThis); - event.Set("currentTarget", jsThis); - if (std::strcmp(eventType, EventType::ReadyStateChange) != 0) - { - event.Set("lengthComputable", false); - event.Set("loaded", 0); - event.Set("total", 0); - } + auto dispatch = m_makeEvent.Value().Call({ + Napi::String::New(env, eventType), + jsThis, + Napi::Boolean::New(env, std::string_view{eventType} != EventType::ReadyStateChange), + }).As(); + auto event = dispatch.Get("value").As(); std::vector> unhandledErrors{}; for (const auto& listener : listeners) @@ -528,8 +625,13 @@ namespace Babylon::Polyfills::Internal auto error = env.GetAndClearPendingException(); unhandledErrors.push_back(std::make_shared(Napi::Persistent(error.Value()))); } + if (dispatch.Get("isStopped").As().Call(dispatch, {}).ToBoolean().Value()) + { + break; + } } + dispatch.Get("end").As().Call(dispatch, {}); for (const auto& error : unhandledErrors) { m_runtimeScheduler([env, error]() { diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index e33b6f94..0796c3fb 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -83,6 +83,7 @@ namespace Babylon::Polyfills::Internal std::string m_url{}; std::shared_ptr m_request{std::make_shared()}; JsRuntimeScheduler m_runtimeScheduler; + Napi::FunctionReference m_makeEvent; ReadyState m_readyState{ReadyState::Unsent}; uint64_t m_sendId{}; bool m_sendActive{false}; diff --git a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index 2ba40ea7..f315a7d4 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -295,6 +295,128 @@ describe("XMLHTTPRequest", function () { }); }); + it("should preserve a replacement request started during each synchronous abort event", async function () { + this.timeout(30000); + for (const phase of ["readystatechange", "abort", "loadend"]) { + const xhr = new XMLHttpRequest(); + let restarted = false; + let loads = 0; + let aborts = 0; + const completed = new Promise((resolve, reject) => { + const guard = setTimeout(() => reject(new Error(`XHR replacement after ${phase} did not complete`)), 25000); + xhr.addEventListener(phase, () => { + if (restarted || (phase === "readystatechange" && xhr.readyState !== XMLHttpRequest.DONE)) { + return; + } + restarted = true; + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + xhr.addEventListener("abort", () => { aborts++; }); + xhr.addEventListener("load", () => { loads++; }); + xhr.addEventListener("loadend", () => { + if (xhr.status === 200 && restarted) { + clearTimeout(guard); + resolve(); + } + }); + }); + xhr.open("GET", "https://github.com/"); + xhr.send(); + xhr.abort(); + expect(restarted).to.equal(true); + expect(xhr.readyState).to.equal(XMLHttpRequest.OPENED); + await completed; + expect(xhr.readyState).to.equal(XMLHttpRequest.DONE); + expect(loads).to.equal(1); + expect(aborts).to.equal(phase === "readystatechange" ? 0 : 1); + } + }); + + it("should not deliver an old loadend to a request started in onload", async function () { + this.timeout(30000); + const xhr = new XMLHttpRequest(); + let loads = 0; + const loadEndStatuses: number[] = []; + await new Promise((resolve, reject) => { + const guard = setTimeout(() => reject(new Error("XHR replacement from onload did not complete")), 25000); + xhr.onload = () => { + if (++loads === 1) { + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + } + }; + xhr.onloadend = () => { + loadEndStatuses.push(xhr.status); + if (loads === 2) { + clearTimeout(guard); + resolve(); + } + }; + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + expect(loads).to.equal(2); + expect(loadEndStatuses).to.deep.equal([200]); + }); + + it("should dispatch Event and ProgressEvent instances with working event methods", async function () { + this.timeout(30000); + const xhr = new XMLHttpRequest(); + let readyEvent: Event | undefined; + let loadEvent: ProgressEvent | undefined; + let skipped = false; + await new Promise((resolve, reject) => { + const guard = setTimeout(() => reject(new Error("XHR events did not complete")), 25000); + xhr.onreadystatechange = (event: Event) => { + if (xhr.readyState === XMLHttpRequest.DONE) { + readyEvent = event; + } + }; + xhr.addEventListener("load", (event: ProgressEvent) => { + try { + expect(event).to.be.instanceOf(Event); + expect(event).to.be.instanceOf(ProgressEvent); + expect(event.type).to.equal("load"); + expect(event.target).to.equal(xhr); + expect(event.currentTarget).to.equal(xhr); + expect(event.eventPhase).to.equal(Event.AT_TARGET); + expect(event.lengthComputable).to.equal(false); + expect(event.loaded).to.equal(0); + expect(event.total).to.equal(0); + expect(event.cancelable).to.equal(false); + event.preventDefault(); + expect(event.defaultPrevented).to.equal(false); + event.stopImmediatePropagation(); + Object.freeze(event); + loadEvent = event; + } catch (error) { + clearTimeout(guard); + reject(error); + } + }); + xhr.addEventListener("load", () => { skipped = true; }); + xhr.onloadend = (event: ProgressEvent) => { + try { + expect(event).to.be.instanceOf(ProgressEvent); + expect(readyEvent).to.be.instanceOf(Event); + expect(readyEvent).to.not.be.instanceOf(ProgressEvent); + expect(readyEvent!.type).to.equal("readystatechange"); + clearTimeout(guard); + resolve(); + } catch (error) { + clearTimeout(guard); + reject(error); + } + }; + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + expect(skipped).to.equal(false); + expect(loadEvent!.currentTarget).to.equal(null); + expect(loadEvent!.eventPhase).to.equal(Event.NONE); + }); + 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 From 39ef5f775dc3f2978081d42789f80f2fe893ef3d Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 23 Sep 2026 09:18:59 -0700 Subject: [PATCH 08/16] Keep XHR event dispatch compatible with JSI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 39 ++++++++++--------- .../XMLHttpRequest/Source/XMLHttpRequest.h | 4 +- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 25cbe803..245bf5c8 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -249,7 +249,7 @@ namespace Babylon::Polyfills::Internal InstanceMethod("send", &XMLHttpRequest::Send), }); - auto eventFactory = env.RunScript(EVENT_FACTORY_SOURCE, "XMLHttpRequestEvents.js"); + auto eventFactory = Napi::Eval(env, EVENT_FACTORY_SOURCE, "XMLHttpRequestEvents.js"); auto descriptor = Napi::Object::New(env); descriptor.Set("value", eventFactory); auto object = env.Global().Get("Object").As(); @@ -420,7 +420,7 @@ namespace Babylon::Polyfills::Internal } } - void XMLHttpRequest::Abort(const Napi::CallbackInfo&) + void XMLHttpRequest::Abort(const Napi::CallbackInfo& info) { if (m_readyState == ReadyState::Done) { @@ -437,17 +437,18 @@ namespace Babylon::Polyfills::Internal const auto abortedSendId = ++m_sendId; m_request->Abort(); - SetReadyState(ReadyState::Done); + auto jsThis = info.This().As(); + SetReadyState(ReadyState::Done, jsThis); if (m_sendId != abortedSendId) { return; } - RaiseEvent(EventType::Abort); + RaiseEvent(EventType::Abort, jsThis); if (m_sendId != abortedSendId) { return; } - RaiseEvent(EventType::LoadEnd); + RaiseEvent(EventType::LoadEnd, jsThis); if (m_sendId == abortedSendId) { m_readyState = ReadyState::Unsent; @@ -478,7 +479,7 @@ namespace Babylon::Polyfills::Internal throw Napi::Error::New(info.Env(), "Unknown error opening URL"); } - SetReadyState(ReadyState::Opened); + SetReadyState(ReadyState::Opened, info.This().As()); } void XMLHttpRequest::Send(const Napi::CallbackInfo& info) @@ -544,33 +545,34 @@ namespace Babylon::Polyfills::Internal // case (status left at 0) reporting `error`. const bool failed = result.has_error() || statusCode == 0; - SetReadyState(ReadyState::Done); + auto jsThis = anchor->Value(); + SetReadyState(ReadyState::Done, jsThis); if (sendId != m_sendId) { return; } if (failed) { - RaiseEvent(EventType::Error); + RaiseEvent(EventType::Error, jsThis); } else { - RaiseEvent(EventType::Load); + RaiseEvent(EventType::Load, jsThis); } if (sendId == m_sendId) { - RaiseEvent(EventType::LoadEnd); + RaiseEvent(EventType::LoadEnd, jsThis); } }); } - void XMLHttpRequest::SetReadyState(ReadyState readyState) + void XMLHttpRequest::SetReadyState(ReadyState readyState, const Napi::Object& jsThis) { m_readyState = readyState; - RaiseEvent(EventType::ReadyStateChange); + RaiseEvent(EventType::ReadyStateChange, jsThis); } - void XMLHttpRequest::RaiseEvent(const char* eventType) + void XMLHttpRequest::RaiseEvent(const char* eventType, const Napi::Object& jsThis) { std::string traceName = (std::ostringstream{} << "XMLHttpRequest::RaiseEvent [" << eventType << "] [" << m_url << "]").str(); arcana::trace_region raiseEventRegion{traceName.c_str()}; @@ -588,7 +590,6 @@ namespace Babylon::Polyfills::Internal listeners = it->second; } - const auto jsThis = Value(); auto dispatch = m_makeEvent.Value().Call({ Napi::String::New(env, eventType), jsThis, @@ -596,7 +597,7 @@ namespace Babylon::Polyfills::Internal }).As(); auto event = dispatch.Get("value").As(); - std::vector> unhandledErrors{}; + std::vector> unhandledErrors{}; for (const auto& listener : listeners) { if (!listener->active) @@ -616,14 +617,14 @@ namespace Babylon::Polyfills::Internal } catch (const Napi::Error& error) { - unhandledErrors.push_back(std::make_shared(Napi::Persistent(error.Value()))); + unhandledErrors.push_back(std::make_shared(error)); continue; } if (env.IsExceptionPending()) { auto error = env.GetAndClearPendingException(); - unhandledErrors.push_back(std::make_shared(Napi::Persistent(error.Value()))); + unhandledErrors.push_back(std::make_shared(std::move(error))); } if (dispatch.Get("isStopped").As().Call(dispatch, {}).ToBoolean().Value()) { @@ -634,8 +635,8 @@ namespace Babylon::Polyfills::Internal dispatch.Get("end").As().Call(dispatch, {}); for (const auto& error : unhandledErrors) { - m_runtimeScheduler([env, error]() { - Napi::Error{env, error->Value()}.ThrowAsJavaScriptException(); + m_runtimeScheduler([error]() { + error->ThrowAsJavaScriptException(); }); } } diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 0796c3fb..63da2636 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -64,8 +64,8 @@ namespace Babylon::Polyfills::Internal void Open(const Napi::CallbackInfo& info); void Send(const Napi::CallbackInfo& info); - void SetReadyState(ReadyState readyState); - void RaiseEvent(const char* eventType); + void SetReadyState(ReadyState readyState, const Napi::Object& jsThis); + void RaiseEvent(const char* eventType, const Napi::Object& jsThis); // A registered event listener. `isEventHandler` marks the single entry owned by the // matching `on` property; every other entry came from addEventListener. Both From 0a7cec68f2c347a1abe3066aab243f8c46bc12d9 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 23 Sep 2026 10:01:33 -0700 Subject: [PATCH 09/16] Snapshot XHR status on the JavaScript thread Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 15 +++++++------- .../XMLHttpRequest/Source/XMLHttpRequest.h | 3 +++ .../Source/Scripts/tests.xmlHttpRequest.ts | 20 +++++++++++++++++-- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 245bf5c8..107b00c6 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -318,19 +318,14 @@ namespace Babylon::Polyfills::Internal Napi::Value XMLHttpRequest::GetStatus(const Napi::CallbackInfo&) { - return Napi::Value::From(Env(), arcana::underlying_cast(m_request->StatusCode())); + return Napi::Value::From(Env(), m_statusCode); } Napi::Value XMLHttpRequest::GetStatusText(const Napi::CallbackInfo&) { // Per the XHR spec, statusText is the empty string until a response is available // (status 0 means UNSENT/OPENED or a network error). - if (arcana::underlying_cast(m_request->StatusCode()) == 0) - { - return Napi::String::New(Env(), ""); - } - - return Napi::String::New(Env(), std::string{m_request->StatusText()}); + return Napi::String::New(Env(), m_statusText); } Napi::Value XMLHttpRequest::GetErrorCode(const Napi::CallbackInfo&) @@ -435,6 +430,8 @@ namespace Babylon::Polyfills::Internal m_sendActive = false; const auto abortedSendId = ++m_sendId; + m_statusCode = 0; + m_statusText.clear(); m_request->Abort(); auto jsThis = info.This().As(); @@ -469,6 +466,8 @@ namespace Babylon::Polyfills::Internal ++m_sendId; m_request = std::make_shared(); m_request->Open(MethodType::StringToEnum(info[0].As().Utf8Value()), m_url); + m_statusCode = 0; + m_statusText.clear(); } catch (const std::exception& e) { @@ -544,6 +543,8 @@ namespace Babylon::Polyfills::Internal // 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; + m_statusCode = statusCode; + m_statusText = statusCode == 0 ? "" : std::string{request->StatusText()}; auto jsThis = anchor->Value(); SetReadyState(ReadyState::Done, jsThis); diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 63da2636..2993ef25 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -85,6 +85,9 @@ namespace Babylon::Polyfills::Internal JsRuntimeScheduler m_runtimeScheduler; Napi::FunctionReference m_makeEvent; ReadyState m_readyState{ReadyState::Unsent}; + // UrlLib writes its status on a worker thread; event handlers read these JS-thread snapshots. + uint32_t m_statusCode{}; + std::string m_statusText{}; uint64_t m_sendId{}; bool m_sendActive{false}; std::unordered_map>> m_listeners; diff --git a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index f315a7d4..a77a4d7b 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -266,13 +266,25 @@ describe("XMLHTTPRequest", function () { this.timeout(30000); const xhr = new XMLHttpRequest(); const order: string[] = []; + const statuses: number[] = []; + const statusTexts: string[] = []; xhr.onreadystatechange = () => { if (xhr.readyState === XMLHttpRequest.DONE) { order.push("readystatechange"); + statuses.push(xhr.status); + statusTexts.push(xhr.statusText); } }; - xhr.onabort = () => { order.push("abort"); }; - xhr.onloadend = () => { order.push("loadend"); }; + xhr.onabort = () => { + order.push("abort"); + statuses.push(xhr.status); + statusTexts.push(xhr.statusText); + }; + xhr.onloadend = () => { + order.push("loadend"); + statuses.push(xhr.status); + statusTexts.push(xhr.statusText); + }; xhr.open("GET", "https://github.com/"); xhr.abort(); @@ -282,6 +294,8 @@ describe("XMLHTTPRequest", function () { xhr.send(); xhr.abort(); expect(order).to.deep.equal(["readystatechange", "abort", "loadend"]); + expect(statuses).to.deep.equal([0, 0, 0]); + expect(statusTexts).to.deep.equal(["", "", ""]); expect(xhr.readyState).to.equal(XMLHttpRequest.UNSENT); await new Promise((resolve, reject) => { @@ -293,6 +307,8 @@ describe("XMLHTTPRequest", function () { xhr.open("GET", "app:///Assets/symlink_target.js"); xhr.send(); }); + expect(xhr.status).to.equal(200); + expect(xhr.statusText).to.equal("OK"); }); it("should preserve a replacement request started during each synchronous abort event", async function () { From d9dcd649781774b613892ac6b9d4371992623962 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 09:08:29 -0700 Subject: [PATCH 10/16] Correct XHR event handler conversion and completed abort state Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 24 ++++++-- Tests/UnitTests/CMakeLists.txt | 3 +- .../Android/app/src/main/cpp/CMakeLists.txt | 3 +- .../Source/Scripts/tests.xmlHttpRequest.ts | 47 +++++++++++++-- .../UnitTests/Source/Tests.XMLHttpRequest.cpp | 57 +++++++++++++++++++ 5 files changed, 124 insertions(+), 10 deletions(-) create mode 100644 Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 107b00c6..8cfc358b 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -174,13 +174,17 @@ namespace Babylon::Polyfills::Internal template void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo&, const Napi::Value& value) { + if (value.IsObject() && !value.IsFunction()) + { + throw Napi::TypeError::New(Env(), "XMLHttpRequest event handler must be callable"); + } + auto& listeners = m_listeners[EVENT_TYPE_NAMES[static_cast(Index)]]; const auto it = std::find_if(listeners.begin(), listeners.end(), [](const std::shared_ptr& listener) { return listener->isEventHandler; }); - // Event handler attributes treat primitive values as null. Object values are retained - // verbatim for the getter; non-callable objects are simply skipped during dispatch. + // [LegacyTreatNonObjectAsNull] clears the handler for primitive values. if (!value.IsObject()) { if (it != listeners.end()) @@ -284,6 +288,11 @@ namespace Babylon::Polyfills::Internal } else { + if (m_readyState != ReadyState::Done) + { + return Env().Null(); + } + gsl::span responseBuffer{m_request->ResponseBuffer()}; auto arrayBuffer{Napi::ArrayBuffer::New(Env(), responseBuffer.size())}; std::memcpy(arrayBuffer.Data(), responseBuffer.data(), arrayBuffer.ByteLength()); @@ -419,6 +428,12 @@ namespace Babylon::Polyfills::Internal { if (m_readyState == ReadyState::Done) { + const auto responseType = m_request->ResponseType(); + auto request = std::make_shared(); + request->ResponseType(responseType); + m_request = std::move(request); + m_statusCode = 0; + m_statusText.clear(); m_readyState = ReadyState::Unsent; return; } @@ -612,6 +627,7 @@ namespace Babylon::Polyfills::Internal continue; } + bool caughtException = false; try { callback.As().Call(jsThis, {event}); @@ -619,10 +635,10 @@ namespace Babylon::Polyfills::Internal catch (const Napi::Error& error) { unhandledErrors.push_back(std::make_shared(error)); - continue; + caughtException = true; } - if (env.IsExceptionPending()) + if (!caughtException && env.IsExceptionPending()) { auto error = env.GetAndClearPendingException(); unhandledErrors.push_back(std::make_shared(std::move(error))); diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index 69cc6cac..42fd3b3b 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -30,7 +30,8 @@ set(SOURCES "Source/Tests.NodeApi.cpp" "Source/Tests.Scheduling.cpp" "Source/Tests.StandardStreamLogger.cpp" - "Source/Tests.TimeoutDispatcher.cpp") + "Source/Tests.TimeoutDispatcher.cpp" + "Source/Tests.XMLHttpRequest.cpp") if(APPLE) if(IOS) diff --git a/Tests/UnitTests/Source/Android/app/src/main/cpp/CMakeLists.txt b/Tests/UnitTests/Source/Android/app/src/main/cpp/CMakeLists.txt index aa933dba..cef5155a 100644 --- a/Tests/UnitTests/Source/Android/app/src/main/cpp/CMakeLists.txt +++ b/Tests/UnitTests/Source/Android/app/src/main/cpp/CMakeLists.txt @@ -27,7 +27,8 @@ add_library(UnitTestsJNI SHARED ${UNIT_TESTS_DIR}/Source/Tests.NodeApi.cpp ${UNIT_TESTS_DIR}/Source/Tests.Scheduling.cpp ${UNIT_TESTS_DIR}/Source/Tests.StandardStreamLogger.cpp - ${UNIT_TESTS_DIR}/Source/Tests.TimeoutDispatcher.cpp) + ${UNIT_TESTS_DIR}/Source/Tests.TimeoutDispatcher.cpp + ${UNIT_TESTS_DIR}/Source/Tests.XMLHttpRequest.cpp) if(NAPI_JAVASCRIPT_ENGINE STREQUAL "V8") target_sources(UnitTestsJNI PRIVATE ${UNIT_TESTS_DIR}/Source/Tests.V8ForegroundTaskRunner.cpp) diff --git a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index a77a4d7b..9a20042e 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -179,8 +179,7 @@ describe("XMLHTTPRequest", function () { }); 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. + // [LegacyTreatNonObjectAsNull] applies to primitives, not objects. const xhr: any = new XMLHttpRequest(); xhr.onload = () => { }; expect(xhr.onload).to.not.equal(null); @@ -197,12 +196,19 @@ describe("XMLHTTPRequest", function () { expect(xhr.onload).to.equal(null); }); - it("should retain but not invoke a non-callable object assigned to an on property", function () { + it("should reject a non-callable object without replacing the existing on handler", function () { const xhr: any = new XMLHttpRequest(); - const handler = {}; + const handler = () => { }; xhr.onload = handler; + expect(() => { xhr.onload = {}; }).to.throw(TypeError); expect(xhr.onload).to.equal(handler); + expect(() => { xhr.onload = new Number(1); }).to.throw(TypeError); + expect(xhr.onload).to.equal(handler); + + const fresh: any = new XMLHttpRequest(); + expect(() => { fresh.onload = {}; }).to.throw(TypeError); + expect(fresh.onload).to.equal(null); }); it("should preserve handlers when the request completes and is reused", async function () { @@ -311,6 +317,39 @@ describe("XMLHTTPRequest", function () { expect(xhr.statusText).to.equal("OK"); }); + it("should clear a completed response on abort without discarding responseType", async function () { + const xhr: any = await createRequest("GET", "app:///Assets/symlink_target.js"); + expect(xhr.status).to.equal(200); + expect(xhr.responseText).to.not.equal(""); + + xhr.abort(); + expect(xhr.readyState).to.equal(XMLHttpRequest.UNSENT); + expect(xhr.status).to.equal(0); + expect(xhr.statusText).to.equal(""); + expect(xhr.response).to.equal(""); + expect(xhr.responseText).to.equal(""); + expect(xhr.responseURL).to.equal(""); + expect(Object.keys(xhr.getAllResponseHeaders())).to.deep.equal([]); + expect(xhr.errorCode).to.equal(""); + expect(xhr.errorDetail).to.equal(""); + + const binary: any = new XMLHttpRequest(); + binary.open("GET", "app:///Assets/symlink_target.js"); + binary.responseType = "arraybuffer"; + await new Promise((resolve, reject) => { + const guard = setTimeout(() => reject(new Error("binary XHR did not complete")), 25000); + binary.onloadend = () => { + clearTimeout(guard); + resolve(); + }; + binary.send(); + }); + expect(binary.response).to.be.instanceOf(ArrayBuffer); + binary.abort(); + expect(binary.responseType).to.equal("arraybuffer"); + expect(binary.response).to.equal(null); + }); + it("should preserve a replacement request started during each synchronous abort event", async function () { this.timeout(30000); for (const phase of ["readystatechange", "abort", "loadend"]) { diff --git a/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp b/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp new file mode 100644 index 00000000..5404ef95 --- /dev/null +++ b/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp @@ -0,0 +1,57 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include + +TEST(XMLHttpRequest, ThrowingStoppedListenerSkipsRemainingListeners) +{ + auto dispatched = std::make_shared>(); + auto reportedError = std::make_shared>(); + auto errorCount = std::make_shared>(0); + auto dispatchedFuture = dispatched->get_future(); + auto errorFuture = reportedError->get_future(); + + Babylon::AppRuntime::Options options{}; + options.UnhandledExceptionHandler = [reportedError, errorCount](const Napi::Error&) { + if (errorCount->fetch_add(1) == 0) + { + reportedError->set_value(); + } + }; + + Babylon::AppRuntime runtime{options}; + runtime.Dispatch([dispatched](Napi::Env env) { + Babylon::Polyfills::XMLHttpRequest::Initialize(env); + env.Global().Set("reportAbortedListeners", Napi::Function::New(env, [dispatched](const Napi::CallbackInfo& info) { + dispatched->set_value(info[0].As().Utf8Value()); + })); + }); + + Babylon::ScriptLoader loader{runtime}; + loader.Eval(R"( + var xhr = new XMLHttpRequest(); + var invoked = []; + xhr.addEventListener("abort", function (event) { + invoked.push("first"); + event.stopImmediatePropagation(); + throw new Error("stopped listener failed"); + }); + xhr.addEventListener("abort", function () { invoked.push("second"); }); + xhr.addEventListener("loadend", function () { + reportAbortedListeners(invoked.join(",")); + }); + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + xhr.abort(); + )", ""); + + ASSERT_EQ(dispatchedFuture.wait_for(std::chrono::seconds(10)), std::future_status::ready); + EXPECT_EQ(dispatchedFuture.get(), "first"); + ASSERT_EQ(errorFuture.wait_for(std::chrono::seconds(10)), std::future_status::ready); + EXPECT_EQ(errorCount->load(), 1); +} From b46ee7dc14ada6da35466c419198e9f8bd721259 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 09:27:06 -0700 Subject: [PATCH 11/16] Preserve JavaScript errors thrown by JSI instance setters Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Core/Node-API-JSI/Include/napi/napi-inl.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Core/Node-API-JSI/Include/napi/napi-inl.h b/Core/Node-API-JSI/Include/napi/napi-inl.h index 14fb745c..936dc1d8 100644 --- a/Core/Node-API-JSI/Include/napi/napi-inl.h +++ b/Core/Node-API-JSI/Include/napi/napi-inl.h @@ -2368,7 +2368,11 @@ ObjectWrap::DefineClass(napi_env env, descriptor.setProperty(rt, "set", jsi::Function::createFromHostFunction(rt, name, 0, [env, setter{p.instanceSetter}, data{p.data}](jsi::Runtime& rt, const jsi::Value& thisVal, const jsi::Value* args, size_t count) -> jsi::Value { T* nativeObject{Unwrap(env, thisVal.getObject(rt))}; - (nativeObject->*setter)({env, thisVal, args, count, nullptr, data}, {env, {rt, args[0]}}); + try { + (nativeObject->*setter)({env, thisVal, args, count, nullptr, data}, {env, {rt, args[0]}}); + } catch (const Error& error) { + throw jsi::JSError(rt, jsi::Value{rt, static_cast(error.Value())}); + } return {}; })); } From 5d9ccaea9c9ed2f4c2a8bd954043b9a3986d7d04 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 24 Sep 2026 10:05:15 -0700 Subject: [PATCH 12/16] Finish XHR request before testing throwing listener cleanup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp b/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp index 5404ef95..aaada324 100644 --- a/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp +++ b/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp @@ -27,7 +27,7 @@ TEST(XMLHttpRequest, ThrowingStoppedListenerSkipsRemainingListeners) Babylon::AppRuntime runtime{options}; runtime.Dispatch([dispatched](Napi::Env env) { Babylon::Polyfills::XMLHttpRequest::Initialize(env); - env.Global().Set("reportAbortedListeners", Napi::Function::New(env, [dispatched](const Napi::CallbackInfo& info) { + env.Global().Set("reportStoppedListeners", Napi::Function::New(env, [dispatched](const Napi::CallbackInfo& info) { dispatched->set_value(info[0].As().Utf8Value()); })); }); @@ -36,22 +36,26 @@ TEST(XMLHttpRequest, ThrowingStoppedListenerSkipsRemainingListeners) loader.Eval(R"( var xhr = new XMLHttpRequest(); var invoked = []; - xhr.addEventListener("abort", function (event) { + xhr.addEventListener("load", function (event) { invoked.push("first"); event.stopImmediatePropagation(); throw new Error("stopped listener failed"); }); - xhr.addEventListener("abort", function () { invoked.push("second"); }); + xhr.addEventListener("load", function () { invoked.push("second"); }); xhr.addEventListener("loadend", function () { - reportAbortedListeners(invoked.join(",")); + reportStoppedListeners(invoked.join(",")); }); xhr.open("GET", "app:///Assets/symlink_target.js"); xhr.send(); - xhr.abort(); )", ""); ASSERT_EQ(dispatchedFuture.wait_for(std::chrono::seconds(10)), std::future_status::ready); EXPECT_EQ(dispatchedFuture.get(), "first"); ASSERT_EQ(errorFuture.wait_for(std::chrono::seconds(10)), std::future_status::ready); EXPECT_EQ(errorCount->load(), 1); + + auto drained = std::make_shared>(); + auto drainedFuture = drained->get_future(); + runtime.Dispatch([drained](Napi::Env) { drained->set_value(); }); + ASSERT_EQ(drainedFuture.wait_for(std::chrono::seconds(10)), std::future_status::ready); } From ad60f55baf348411422b0f8de35f2f8b345a303c 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, 28 Sep 2026 10:03:57 -0700 Subject: [PATCH 13/16] Fix XMLHttpRequest test suite closing delimiter The describe() call was missing its closing `);` after the #257 test-file split, which broke the webpack/babel build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index 9a20042e..bc59fcd6 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -744,4 +744,4 @@ describe("XMLHTTPRequest", function () { expect(xhr.responseText.length).to.equal(16); expect(xhr.response).to.equal(xhr.responseText); }); -} +}); From dd8f60e9c2ada1eae6a4c91c2826d34e35a953a4 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Mon, 5 Oct 2026 16:44:28 -0700 Subject: [PATCH 14/16] Address XHR reuse, abort response safety, and listener ownership review Prepare replacement requests before invalidating active sends and preserve responseType. Detach canceled transport state before synchronous callbacks and guard response access while the transport is active. Keep callbacks in a JavaScript WeakMap keyed by their XHR, with stable native listener records and short-lived persistent invocation references. Retain non-callable handler objects by identity without invoking them. Pin the Android response-read fix from BabylonJS/UrlLib#38. Add reuse, failed-open, object-identity, self-capturing GC, streaming-abort, and truncated HTTP response regressions, and update the polyfill documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CMakeLists.txt | 5 +- Polyfills/XMLHttpRequest/Readme.md | 25 +- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 141 +++++++--- .../XMLHttpRequest/Source/XMLHttpRequest.h | 6 +- Tests/UnitTests/CMakeLists.txt | 3 + .../Android/app/src/main/AndroidManifest.xml | 2 +- .../main/res/xml/network_security_config.xml | 6 + .../Source/Scripts/tests.xmlHttpRequest.ts | 136 +++++++++- .../UnitTests/Source/Tests.XMLHttpRequest.cpp | 252 ++++++++++++++++++ 9 files changed, 518 insertions(+), 58 deletions(-) create mode 100644 Tests/UnitTests/Source/Android/app/src/main/res/xml/network_security_config.xml diff --git a/CMakeLists.txt b/CMakeLists.txt index 21882d3e..7e87faee 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -48,8 +48,9 @@ FetchContent_Declare(llhttp URL "https://github.com/nodejs/llhttp/archive/refs/tags/release/v8.1.0.tar.gz" EXCLUDE_FROM_ALL) FetchContent_Declare(UrlLib - GIT_REPOSITORY https://github.com/BabylonJS/UrlLib.git - GIT_TAG 0c991337a1160ba7a2d062bf8e342d0a66f48dc9 + # Repoint upstream after BabylonJS/UrlLib#38 merges. + GIT_REPOSITORY https://github.com/bkaradzic-microsoft/UrlLib.git + GIT_TAG 89efc235e2a69df4582aaa11855b45c4adbee7f8 EXCLUDE_FROM_ALL) FetchContent_Declare(quickjs-ng GIT_REPOSITORY https://github.com/quickjs-ng/quickjs.git diff --git a/Polyfills/XMLHttpRequest/Readme.md b/Polyfills/XMLHttpRequest/Readme.md index bbc30971..4c5f872c 100644 --- a/Polyfills/XMLHttpRequest/Readme.md +++ b/Polyfills/XMLHttpRequest/Readme.md @@ -8,6 +8,13 @@ an event whose `target` and `currentTarget` are the request, and run with the re `readystatechange` dispatches an `Event`; `load`, `error`, `abort`, and `loadend` dispatch `ProgressEvent` instances (`lengthComputable === false`, `loaded === total === 0`). Missing constructors are installed without replacing host-provided ones. +Both registration styles share registration order, retain handlers across requests, and +observe removals or property reassignment before a listener's turn. Duplicate +`addEventListener` registrations are ignored. Primitive property assignments clear the +handler; non-callable objects are retained by identity but are not invoked. +Callbacks are owned by JavaScript so a handler capturing its own XHR does not keep an +otherwise unreachable request alive. Exceptions are reported to the runtime's +unhandled-exception handler without preventing later listeners from running. At the moment, we only support the following events: * `loadend` * `readystatechange` @@ -15,13 +22,25 @@ At the moment, we only support the following events: * `error` (fired on a transport failure, before `loadend`) * `abort` (fired when an active request is aborted, before `loadend`) +Active `abort()` dispatches `readystatechange`, `abort`, and `loadend` synchronously, +with status 0 and an empty response, then returns to `UNSENT`. A request opened from +one of those callbacks is preserved. Abort before send is inert; abort after completion +clears the response without events. Unlike the previous implementation, cancellation +raises `abort`, not `error`; completed non-`2xx` responses raise `load`, not `error`. + +Reopening preserves `responseType`. If validating or opening a replacement fails, the +previous response or active request remains intact. Response bodies and headers are +only exposed after successful transport completion; they are not read from an active +worker. Changing `responseType`, changing request headers, or sending again while a +send is active throws. + ## Local files Unlike the web, XMLHttpRequest supports loading local files using two schemes: * `file:///` allows you to load from an absolute path * `app:///` allows you to load from a relative path, either the current program or package depending on platform ## Other things to be aware of: -* Only `GET` requests are currently supported +* Only `GET` requests are supported on all platforms; `POST` support depends on the UrlLib backend * For `readyState`, we only support `UNSENT`, `OPENED`, and `DONE` ## Transport-error diagnostics (non-standard) @@ -34,6 +53,6 @@ tell a DNS failure from a refused connection or a missing local asset: * `errorDetail` -- the full normalized `":(): "` string Both are empty strings unless the request failed at the transport layer, and are populated only -on backends that expose the detail (Apple, Linux) -- empty on Windows/Android until those -backends populate `UrlLib`'s accessors. Browsers do not expose these properties, so +on backends that expose the detail (Apple, Linux, Android) -- empty on Windows until that +backend populates `UrlLib`'s accessors. Browsers do not expose these properties, so spec-conformant code is unaffected. diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 8cfc358b..54c813b5 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -112,7 +112,8 @@ namespace Babylon::Polyfills::Internal global.ProgressEvent = ProgressEvent; } - return function (type, target, progress) { + var callbacks = new WeakMap(); + var makeEvent = function (type, target, progress) { var event = progress ? new global.ProgressEvent(type, {lengthComputable: false, loaded: 0, total: 0}) : new global.Event(type); @@ -141,6 +142,18 @@ namespace Babylon::Polyfills::Internal } }; }; + makeEvent.getCallbacks = function (target) { + var values = callbacks.get(target); + if (!values) { + values = Object.create(null); + callbacks.set(target, values); + } + return values; + }; + makeEvent.removeCallback = function (target, key) { + delete callbacks.get(target)[key]; + }; + return makeEvent; })(typeof globalThis === "object" ? globalThis : this) )JS"; } @@ -154,7 +167,7 @@ namespace Babylon::Polyfills::Internal }; template - Napi::Value XMLHttpRequest::GetEventHandler(const Napi::CallbackInfo&) + Napi::Value XMLHttpRequest::GetEventHandler(const Napi::CallbackInfo& info) { const auto it = m_listeners.find(EVENT_TYPE_NAMES[static_cast(Index)]); if (it != m_listeners.end()) @@ -163,7 +176,7 @@ namespace Babylon::Polyfills::Internal { if (listener->active && listener->isEventHandler) { - return listener->callback.Value(); + return GetCallbacks(info.This().As()).Get(listener->callbackKey); } } } @@ -172,13 +185,9 @@ namespace Babylon::Polyfills::Internal } template - void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo&, const Napi::Value& value) + void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo& info, const Napi::Value& value) { - if (value.IsObject() && !value.IsFunction()) - { - throw Napi::TypeError::New(Env(), "XMLHttpRequest event handler must be callable"); - } - + auto callbacks = GetCallbacks(info.This().As()); auto& listeners = m_listeners[EVENT_TYPE_NAMES[static_cast(Index)]]; const auto it = std::find_if(listeners.begin(), listeners.end(), [](const std::shared_ptr& listener) { return listener->isEventHandler; @@ -190,6 +199,7 @@ namespace Babylon::Polyfills::Internal if (it != listeners.end()) { (*it)->active = false; + m_removeCallback.Call({info.This(), Napi::String::New(Env(), (*it)->callbackKey)}); listeners.erase(it); } @@ -200,11 +210,13 @@ namespace Babylon::Polyfills::Internal { // Replace in place so reassignment keeps this listener's position in the // dispatch order. - (*it)->callback = Napi::Persistent(value.As()); + callbacks.Set((*it)->callbackKey, value); } else { - listeners.push_back(std::make_shared(Listener{Napi::Persistent(value.As()), true})); + const auto key = std::to_string(++m_nextListenerId); + callbacks.Set(key, value); + listeners.push_back(std::make_shared(Listener{key, true})); } } @@ -271,6 +283,8 @@ namespace Babylon::Polyfills::Internal : Napi::ObjectWrap{info} , m_runtimeScheduler{JsRuntime::GetFromJavaScript(info.Env())} , m_makeEvent{Napi::Persistent(info.NewTarget().As().Get(EVENT_FACTORY_NAME).As())} + , m_getCallbacks{Napi::Persistent(m_makeEvent.Value().Get("getCallbacks").As())} + , m_removeCallback{Napi::Persistent(m_makeEvent.Value().Get("removeCallback").As())} { } @@ -283,12 +297,17 @@ namespace Babylon::Polyfills::Internal { if (m_request->ResponseType() == UrlLib::UrlResponseType::String) { + if (m_readyState != ReadyState::Done || m_statusCode == 0) + { + return Napi::String::New(Env(), ""); + } + const std::string_view responseString{m_request->ResponseString()}; return Napi::String::New(Env(), responseString.data(), responseString.size()); } else { - if (m_readyState != ReadyState::Done) + if (m_readyState != ReadyState::Done || m_statusCode == 0) { return Env().Null(); } @@ -302,6 +321,11 @@ namespace Babylon::Polyfills::Internal Napi::Value XMLHttpRequest::GetResponseText(const Napi::CallbackInfo&) { + if (m_readyState != ReadyState::Done || m_statusCode == 0) + { + return Napi::String::New(Env(), ""); + } + // The body may legitimately contain embedded nulls: Emscripten's EXPORT_ES6 output, for // example, inlines the .wasm payload as a JavaScript string literal. Passing .data() // alone would hand a const char* to Napi and truncate at the first null, so the length @@ -317,11 +341,19 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::SetResponseType(const Napi::CallbackInfo&, const Napi::Value& value) { + if (m_sendActive) + { + throw Napi::Error::New(Env(), "Cannot change responseType while XMLHttpRequest is sending"); + } m_request->ResponseType(ResponseType::StringToEnum(value.As().Utf8Value())); } Napi::Value XMLHttpRequest::GetResponseURL(const Napi::CallbackInfo&) { + if (m_readyState != ReadyState::Done || m_statusCode == 0) + { + return Napi::String::New(Env(), ""); + } return Napi::Value::From(Env(), m_request->ResponseUrl().data()); } @@ -341,27 +373,35 @@ namespace Babylon::Polyfills::Internal { // Stable symbolic token for a transport failure (e.g. "CURLE_COULDNT_CONNECT", // "NSURLErrorTimedOut", "AppResourceNotFound"); empty when there was no transport failure. - return Napi::String::New(Env(), std::string{m_request->ErrorSymbol()}); + return Napi::String::New(Env(), m_sendActive ? "" : std::string{m_request->ErrorSymbol()}); } Napi::Value XMLHttpRequest::GetErrorDetail(const Napi::CallbackInfo&) { // Full normalized ":(): " string; empty when there was no // transport failure. - return Napi::String::New(Env(), std::string{m_request->ErrorString()}); + return Napi::String::New(Env(), m_sendActive ? "" : std::string{m_request->ErrorString()}); } Napi::Value XMLHttpRequest::GetResponseHeader(const Napi::CallbackInfo& info) { const auto headerName = info[0].As().Utf8Value(); + if (m_readyState != ReadyState::Done || m_statusCode == 0) + { + return info.Env().Null(); + } const auto header = m_request->GetResponseHeader(headerName); return header ? Napi::Value::From(Env(), header.value()) : info.Env().Null(); } Napi::Value XMLHttpRequest::GetAllResponseHeaders(const Napi::CallbackInfo&) { - auto responseHeaders = m_request->GetAllResponseHeaders(); Napi::Object responseHeadersObject = Napi::Object::New(Env()); + if (m_readyState != ReadyState::Done || m_statusCode == 0) + { + return responseHeadersObject; + } + auto responseHeaders = m_request->GetAllResponseHeaders(); for (auto& iter : responseHeaders) { @@ -375,6 +415,10 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::SetRequestHeader(const Napi::CallbackInfo& info) { + if (m_sendActive) + { + throw Napi::Error::New(Env(), "Cannot change request headers while XMLHttpRequest is sending"); + } m_request->SetRequestHeader(info[0].As().Utf8Value(), info[1].As().Utf8Value()); } @@ -382,6 +426,7 @@ namespace Babylon::Polyfills::Internal { const std::string eventType = info[0].As().Utf8Value(); const Napi::Function eventHandler = info[1].As(); + auto callbacks = GetCallbacks(info.This().As()); auto& listeners = m_listeners[eventType]; for (const auto& listener : listeners) @@ -389,7 +434,7 @@ 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 collapsing them. - if (listener->active && !listener->isEventHandler && listener->callback.Value() == eventHandler) + if (listener->active && !listener->isEventHandler && callbacks.Get(listener->callbackKey) == eventHandler) { // 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 @@ -399,13 +444,16 @@ namespace Babylon::Polyfills::Internal } } - listeners.push_back(std::make_shared(Listener{Napi::Persistent(eventHandler.As()), false})); + const auto key = std::to_string(++m_nextListenerId); + callbacks.Set(key, eventHandler); + listeners.push_back(std::make_shared(Listener{key, false})); } void XMLHttpRequest::RemoveEventListener(const Napi::CallbackInfo& info) { const std::string eventType = info[0].As().Utf8Value(); const Napi::Function eventHandler = info[1].As(); + auto callbacks = GetCallbacks(info.This().As()); const auto itType = m_listeners.find(eventType); if (itType != m_listeners.end()) { @@ -414,9 +462,10 @@ namespace Babylon::Polyfills::Internal { // removeEventListener never removes an `on` handler; that is done by // assigning null to the property. - if ((*it)->active && !(*it)->isEventHandler && (*it)->callback.Value() == eventHandler) + if ((*it)->active && !(*it)->isEventHandler && callbacks.Get((*it)->callbackKey) == eventHandler) { (*it)->active = false; + m_removeCallback.Call({info.This(), Napi::String::New(Env(), (*it)->callbackKey)}); listeners.erase(it); break; } @@ -426,28 +475,29 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::Abort(const Napi::CallbackInfo& info) { - if (m_readyState == ReadyState::Done) + if (m_readyState != ReadyState::Done && !m_sendActive) { - const auto responseType = m_request->ResponseType(); - auto request = std::make_shared(); - request->ResponseType(responseType); - m_request = std::move(request); - m_statusCode = 0; - m_statusText.clear(); - m_readyState = ReadyState::Unsent; return; } - if (!m_sendActive) + auto request = std::make_shared(); + request->ResponseType(m_request->ResponseType()); + const auto abortedSendId = ++m_sendId; + m_statusCode = 0; + m_statusText.clear(); + + if (m_readyState == ReadyState::Done) { + m_request = std::move(request); + m_readyState = ReadyState::Unsent; return; } m_sendActive = false; - const auto abortedSendId = ++m_sendId; - m_statusCode = 0; - m_statusText.clear(); m_request->Abort(); + // Cancellation does not join the worker. Its continuation owns the old request; + // synchronous abort handlers must only see an empty, worker-independent response. + m_request = std::move(request); auto jsThis = info.This().As(); SetReadyState(ReadyState::Done, jsThis); @@ -469,18 +519,22 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::Open(const Napi::CallbackInfo& info) { - m_url = info[1].As(); - try { + auto url = info[1].As().Utf8Value(); + const auto method = MethodType::StringToEnum(info[0].As().Utf8Value()); + auto request = std::make_shared(); + request->ResponseType(m_request->ResponseType()); + request->Open(method, url); + if (m_sendActive) { m_request->Abort(); m_sendActive = false; } ++m_sendId; - m_request = std::make_shared(); - m_request->Open(MethodType::StringToEnum(info[0].As().Utf8Value()), m_url); + m_request = std::move(request); + m_url = std::move(url); m_statusCode = 0; m_statusText.clear(); } @@ -498,7 +552,7 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::Send(const Napi::CallbackInfo& info) { - if (m_readyState != ReadyState::Opened) + if (m_readyState != ReadyState::Opened || m_sendActive) { throw Napi::Error::New(info.Env(), "XMLHttpRequest must be opened before it can be sent"); } @@ -558,8 +612,8 @@ namespace Babylon::Polyfills::Internal // 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; - m_statusCode = statusCode; - m_statusText = statusCode == 0 ? "" : std::string{request->StatusText()}; + m_statusCode = failed ? 0 : statusCode; + m_statusText = failed ? "" : std::string{request->StatusText()}; auto jsThis = anchor->Value(); SetReadyState(ReadyState::Done, jsThis); @@ -588,6 +642,13 @@ namespace Babylon::Polyfills::Internal RaiseEvent(EventType::ReadyStateChange, jsThis); } + Napi::Object XMLHttpRequest::GetCallbacks(const Napi::Object& jsThis) + { + // The WeakMap makes self-capturing callbacks a collectable JS cycle, even on + // backends whose Napi::Weak implementation retains objects strongly. + return m_getCallbacks.Value().Call({jsThis}).As(); + } + void XMLHttpRequest::RaiseEvent(const char* eventType, const Napi::Object& jsThis) { std::string traceName = (std::ostringstream{} << "XMLHttpRequest::RaiseEvent [" << eventType << "] [" << m_url << "]").str(); @@ -612,6 +673,7 @@ namespace Babylon::Polyfills::Internal Napi::Boolean::New(env, std::string_view{eventType} != EventType::ReadyStateChange), }).As(); auto event = dispatch.Get("value").As(); + auto callbacks = GetCallbacks(jsThis); std::vector> unhandledErrors{}; for (const auto& listener : listeners) @@ -621,16 +683,17 @@ namespace Babylon::Polyfills::Internal continue; } - const auto callback = listener->callback.Value(); + const auto callback = callbacks.Get(listener->callbackKey); if (!callback.IsFunction()) { continue; } bool caughtException = false; + auto handler = Napi::Persistent(callback.As()); try { - callback.As().Call(jsThis, {event}); + handler.Call(jsThis, {event}); } catch (const Napi::Error& error) { diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 2993ef25..6d920ce3 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -66,6 +66,7 @@ namespace Babylon::Polyfills::Internal void SetReadyState(ReadyState readyState, const Napi::Object& jsThis); void RaiseEvent(const char* eventType, const Napi::Object& jsThis); + Napi::Object GetCallbacks(const Napi::Object& jsThis); // A registered event listener. `isEventHandler` marks the single entry owned by the // matching `on` property; every other entry came from addEventListener. Both @@ -75,7 +76,7 @@ namespace Babylon::Polyfills::Internal // moving to the end ("If eventHandler's listener is not null, then return"). struct Listener { - Napi::ObjectReference callback; + std::string callbackKey; bool isEventHandler; bool active{true}; }; @@ -84,11 +85,14 @@ namespace Babylon::Polyfills::Internal std::shared_ptr m_request{std::make_shared()}; JsRuntimeScheduler m_runtimeScheduler; Napi::FunctionReference m_makeEvent; + Napi::FunctionReference m_getCallbacks; + Napi::FunctionReference m_removeCallback; ReadyState m_readyState{ReadyState::Unsent}; // UrlLib writes its status on a worker thread; event handlers read these JS-thread snapshots. uint32_t m_statusCode{}; std::string m_statusText{}; uint64_t m_sendId{}; + uint64_t m_nextListenerId{}; bool m_sendActive{false}; std::unordered_map>> m_listeners; }; diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index 42fd3b3b..0c6bc215 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -71,6 +71,9 @@ if(NAPI_JAVASCRIPT_ENGINE STREQUAL "V8") endif() target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") target_compile_definitions(UnitTests PRIVATE NAPI_JAVASCRIPT_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "Chakra") + target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_CHAKRA) +endif() # The V8JSI Node-API shim does not implement napi_create_dataview, so the # CreateDataViewRejectsOverflowingRange test is compiled out on that backend. diff --git a/Tests/UnitTests/Source/Android/app/src/main/AndroidManifest.xml b/Tests/UnitTests/Source/Android/app/src/main/AndroidManifest.xml index c3537fae..95ce977c 100644 --- a/Tests/UnitTests/Source/Android/app/src/main/AndroidManifest.xml +++ b/Tests/UnitTests/Source/Android/app/src/main/AndroidManifest.xml @@ -1,7 +1,7 @@ - + diff --git a/Tests/UnitTests/Source/Android/app/src/main/res/xml/network_security_config.xml b/Tests/UnitTests/Source/Android/app/src/main/res/xml/network_security_config.xml new file mode 100644 index 00000000..5a742e19 --- /dev/null +++ b/Tests/UnitTests/Source/Android/app/src/main/res/xml/network_security_config.xml @@ -0,0 +1,6 @@ + + + + 127.0.0.1 + + diff --git a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index bc59fcd6..bf41b92b 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -178,7 +178,7 @@ describe("XMLHTTPRequest", function () { expect(xhr.onload).to.equal(null); }); - it("should coerce a non-callable on assignment to null", function () { + it("should coerce a primitive on assignment to null", function () { // [LegacyTreatNonObjectAsNull] applies to primitives, not objects. const xhr: any = new XMLHttpRequest(); xhr.onload = () => { }; @@ -196,19 +196,131 @@ describe("XMLHTTPRequest", function () { expect(xhr.onload).to.equal(null); }); - it("should reject a non-callable object without replacing the existing on handler", function () { - const xhr: any = new XMLHttpRequest(); - const handler = () => { }; - xhr.onload = handler; + it("should retain non-callable on objects by identity without invoking them", async function () { + this.timeout(30000); + const xhr = new XMLHttpRequest(); + const object = { handleEvent: () => { throw new Error("object must not be invoked"); } }; + for (const name of ["onload", "onerror", "onabort", "onloadend", "onreadystatechange"]) { + Reflect.set(xhr, name, () => { throw new Error("replaced handler must not run"); }); + Reflect.set(xhr, name, object); + expect(Reflect.get(xhr, name)).to.equal(object); + } + const boxed = new Number(1); + Reflect.set(xhr, "onload", boxed); + expect(xhr.onload).to.equal(boxed); + await new Promise((resolve, reject) => { + const guard = setTimeout(() => reject(new Error("object handler XHR did not complete")), 25000); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve(); + }); + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + expect(xhr.onload).to.equal(boxed); + xhr.onload = null; + expect(xhr.onload).to.equal(null); + }); - expect(() => { xhr.onload = {}; }).to.throw(TypeError); - expect(xhr.onload).to.equal(handler); - expect(() => { xhr.onload = new Number(1); }).to.throw(TypeError); - expect(xhr.onload).to.equal(handler); + it("should preserve arraybuffer responseType when reopened", async function () { + this.timeout(30000); + const xhr = await createRequest("GET", "app:///Assets/symlink_target.js", undefined, "arraybuffer"); + const first = new Uint8Array(xhr.response); + expect(first.length).to.be.greaterThan(0); + xhr.open("GET", "app:///Assets/symlink_target.js"); + expect(xhr.responseType).to.equal("arraybuffer"); + expect(xhr.response).to.equal(null); + await new Promise((resolve) => { + xhr.onloadend = () => resolve(); + xhr.send(); + }); + expect(xhr.response).to.be.instanceOf(ArrayBuffer); + expect(Array.from(new Uint8Array(xhr.response))).to.deep.equal(Array.from(first)); + }); - const fresh: any = new XMLHttpRequest(); - expect(() => { fresh.onload = {}; }).to.throw(TypeError); - expect(fresh.onload).to.equal(null); + it("should preserve a completed response when open fails", async function () { + this.timeout(30000); + const xhr = await createRequest("GET", "app:///Assets/symlink_target.js"); + const response = xhr.responseText; + const url = xhr.responseURL; + const headers = xhr.getAllResponseHeaders(); + for (const [method, invalidUrl] of [["INVALID", url], ["GET", "not a URL"]]) { + expect(() => xhr.open(method, invalidUrl)).to.throw(); + expect(xhr.readyState).to.equal(XMLHttpRequest.DONE); + expect(xhr.status).to.equal(200); + expect(xhr.statusText).to.equal("OK"); + expect(xhr.responseText).to.equal(response); + expect(xhr.response).to.equal(response); + expect(xhr.responseURL).to.equal(url); + expect(xhr.getAllResponseHeaders()).to.deep.equal(headers); + } + }); + + it("should let an active request finish when a replacement open fails", async function () { + this.timeout(30000); + const xhr = new XMLHttpRequest(); + let loads = 0; + let aborts = 0; + const completed = new Promise((resolve) => { + xhr.onload = () => { loads++; }; + xhr.onabort = () => { aborts++; }; + xhr.onloadend = () => resolve(); + }); + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + expect(() => xhr.open("INVALID", "app:///Assets/symlink_target.js")).to.throw(); + expect(() => xhr.open("GET", "not a URL")).to.throw(); + expect(xhr.readyState).to.equal(XMLHttpRequest.OPENED); + await completed; + expect(loads).to.equal(1); + expect(aborts).to.equal(0); + expect(xhr.status).to.equal(200); + expect(xhr.responseText).to.not.equal(""); + }); + + it("should expose an empty response during and after synchronous abort", function () { + for (const responseType of ["text", "arraybuffer"] as const) { + const xhr = new XMLHttpRequest(); + const responses: unknown[] = []; + const texts: string[] = []; + const urls: string[] = []; + const headers: unknown[] = []; + const read = () => { + responses.push(xhr.response); + texts.push(xhr.responseText); + urls.push(xhr.responseURL); + headers.push(xhr.getAllResponseHeaders()); + expect(xhr.getResponseHeader("content-type")).to.equal(null); + }; + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.responseType = responseType; + xhr.send(); + read(); + xhr.onreadystatechange = read; + xhr.onabort = read; + xhr.onloadend = read; + xhr.abort(); + read(); + expect(responses).to.deep.equal(Array(5).fill(responseType === "text" ? "" : null)); + expect(texts).to.deep.equal(Array(5).fill("")); + expect(urls).to.deep.equal(Array(5).fill("")); + expect(headers).to.deep.equal(Array(5).fill({})); + expect(xhr.responseType).to.equal(responseType); + } + }); + + it("should reject transport mutation while send is active", async function () { + this.timeout(30000); + const xhr = new XMLHttpRequest(); + const completed = new Promise((resolve) => { xhr.onloadend = () => resolve(); }); + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + expect(() => { xhr.responseType = "arraybuffer"; }).to.throw(); + expect(() => xhr.setRequestHeader("Accept", "*/*")).to.throw(); + expect(() => xhr.send()).to.throw(); + await completed; + expect(xhr.status).to.equal(200); + expect(xhr.responseType).to.equal("text"); }); it("should preserve handlers when the request completes and is reused", async function () { diff --git a/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp b/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp index aaada324..6b90ca66 100644 --- a/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp +++ b/Tests/UnitTests/Source/Tests.XMLHttpRequest.cpp @@ -7,6 +7,258 @@ #include #include #include +#include + +#if !defined(_WIN32) +#include +#include +#include + +namespace +{ + class ResponseServer + { + public: + ResponseServer(int status, bool truncated, bool streaming = false) + { + m_listener = socket(AF_INET, SOCK_STREAM, 0); + if (m_listener == -1) + { + throw std::runtime_error{"Cannot create test HTTP socket"}; + } + sockaddr_in address{}; + address.sin_family = AF_INET; + address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + socklen_t length = sizeof(address); + if (bind(m_listener, reinterpret_cast(&address), sizeof(address)) != 0 || + getsockname(m_listener, reinterpret_cast(&address), &length) != 0 || + listen(m_listener, 1) != 0) + { + close(m_listener); + throw std::runtime_error{"Cannot bind test HTTP server"}; + } + m_port = ntohs(address.sin_port); + m_worker = std::thread{[this, status, truncated, streaming]() { + const int connection = accept(m_listener, nullptr, nullptr); + if (connection == -1) + { + return; + } + timeval timeout{2, 0}; + setsockopt(connection, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)); + setsockopt(connection, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout)); +#if defined(__APPLE__) + int noSignal = 1; + setsockopt(connection, SOL_SOCKET, SO_NOSIGPIPE, &noSignal, sizeof(noSignal)); +#endif + char request[4096]; + if (recv(connection, request, sizeof(request), 0) > 0) + { + const std::string headers = "HTTP/1.1 " + std::to_string(status) + + (status == 404 ? " Not Found\r\n" : " OK\r\n") + + "Content-Length: " + (streaming ? "100000000" : truncated ? "100" : "5") + + "\r\nContent-Type: text/plain\r\nConnection: close\r\n\r\nhello"; + const auto sendAll = [connection](const std::string& text) { + size_t offset{}; + while (offset < text.size()) + { +#if defined(MSG_NOSIGNAL) + constexpr int flags = MSG_NOSIGNAL; +#else + constexpr int flags = 0; +#endif + const auto sent = send(connection, text.data() + offset, text.size() - offset, flags); + if (sent <= 0) + { + return false; + } + offset += static_cast(sent); + } + return true; + }; + if (sendAll(headers)) + { + m_started.set_value(); + const std::string chunk(4096, 'x'); + while (streaming && !m_stop && sendAll(chunk)) + { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + } + } + shutdown(connection, SHUT_RDWR); + close(connection); + }}; + } + + ~ResponseServer() + { + m_stop = true; + shutdown(m_listener, SHUT_RDWR); + m_worker.join(); + close(m_listener); + } + + std::string Url() const + { + return "http://127.0.0.1:" + std::to_string(m_port) + "/"; + } + + bool WaitForBody() + { + return m_started.get_future().wait_for(std::chrono::seconds(10)) == std::future_status::ready; + } + + private: + int m_listener{}; + uint16_t m_port{}; + std::atomic m_stop{}; + std::promise m_started; + std::thread m_worker; + }; +} + +TEST(XMLHttpRequest, CompletedAndTruncatedHttpBodies) +{ + for (const int status : {200, 404}) + { + for (const bool truncated : {false, true}) + { + SCOPED_TRACE(std::to_string(status) + (truncated ? " truncated" : " complete")); + Babylon::AppRuntime runtime{}; + ResponseServer server{status, truncated}; + auto completed = std::make_shared>(); + auto result = completed->get_future(); + runtime.Dispatch([completed, url = server.Url()](Napi::Env env) { + Babylon::Polyfills::XMLHttpRequest::Initialize(env); + env.Global().Set("testUrl", url); + env.Global().Set("report", Napi::Function::New(env, [completed](const Napi::CallbackInfo& info) { + completed->set_value(info[0].As().Utf8Value()); + })); + Napi::Eval(env, R"( + var xhr = new XMLHttpRequest(); + var events = []; + xhr.onload = function () { events.push("load"); }; + xhr.onerror = function () { events.push("error"); }; + xhr.onloadend = function () { + events.push("loadend"); + report(events.join(",") + ":" + xhr.status + ":" + xhr.responseText); + }; + xhr.open("GET", testUrl); + xhr.send(); + )", ""); + }); + ASSERT_EQ(result.wait_for(std::chrono::seconds(10)), std::future_status::ready); + EXPECT_EQ(result.get(), truncated ? "error,loadend:0:" : "load,loadend:" + std::to_string(status) + ":hello"); + } + } +} + +TEST(XMLHttpRequest, AbortResponseReadsWhileBodyIsStreaming) +{ + for (const char* responseType : {"text", "arraybuffer"}) + { + SCOPED_TRACE(responseType); + Babylon::AppRuntime runtime{}; + ResponseServer server{200, false, true}; + runtime.Dispatch([url = server.Url(), responseType](Napi::Env env) { + Babylon::Polyfills::XMLHttpRequest::Initialize(env); + env.Global().Set("testUrl", url); + env.Global().Set("testResponseType", responseType); + Napi::Eval(env, R"( + var xhr = new XMLHttpRequest(); + xhr.open("GET", testUrl); + xhr.responseType = testResponseType; + xhr.send(); + )", ""); + }); + ASSERT_TRUE(server.WaitForBody()); + auto checked = std::make_shared>(); + runtime.Dispatch([checked](Napi::Env env) { + checked->set_value(Napi::Eval(env, R"( + (function () { + var valid = true; + var calls = 0; + var read = function () { + ++calls; + valid = valid && xhr.response === (testResponseType === "text" ? "" : null) && + xhr.responseText === "" && xhr.responseURL === "" && + xhr.status === 0 && xhr.statusText === "" && + xhr.errorCode === "" && xhr.errorDetail === "" && + xhr.getResponseHeader("content-type") === null && + Object.keys(xhr.getAllResponseHeaders()).length === 0; + }; + for (var i = 0; i < 1000; ++i) read(); + xhr.onreadystatechange = read; + xhr.onabort = read; + xhr.onloadend = read; + xhr.abort(); + read(); + return valid && calls === 1004 && xhr.readyState === XMLHttpRequest.UNSENT; + })(); + )", "").As().Value()); + }); + auto result = checked->get_future(); + ASSERT_EQ(result.wait_for(std::chrono::seconds(10)), std::future_status::ready); + EXPECT_TRUE(result.get()); + } +} +#endif + +#if defined(JSRUNTIMEHOST_NAPI_ENGINE_CHAKRA) +#define USE_EDGEMODE_JSRT +#include + +TEST(XMLHttpRequest, CollectsCompletedRequestWithSelfCapturingHandlers) +{ + auto finalized = std::make_shared>(0); + auto completed = std::make_shared>(); + auto completedFuture = completed->get_future(); + Babylon::AppRuntime runtime{}; + runtime.Dispatch([finalized, completed](Napi::Env env) { + Babylon::Polyfills::XMLHttpRequest::Initialize(env); + env.Global().Set("observeXHR", Napi::Function::New(env, [finalized](const Napi::CallbackInfo& info) { + auto marker = Napi::External::New(info.Env(), new int{}, [finalized](Napi::Env, int* value) { + delete value; + ++*finalized; + }); + info[0].As().Set("marker", marker); + })); + env.Global().Set("completedXHR", Napi::Function::New(env, [completed](const Napi::CallbackInfo&) { + completed->set_value(); + })); + Napi::Eval(env, R"( + (function () { + var xhr = new XMLHttpRequest(); + observeXHR(xhr); + xhr.onload = function () { xhr.loaded = true; }; + xhr.addEventListener("loadend", function () { + if (xhr.loaded) completedXHR(); + }); + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + })(); + )", ""); + }); + ASSERT_EQ(completedFuture.wait_for(std::chrono::seconds(10)), std::future_status::ready); + + // Collect on later dispatches, after the completion continuation releases its anchor. + for (int attempt = 0; attempt < 5 && finalized->load() == 0; ++attempt) + { + std::promise collected; + runtime.Dispatch([&collected](Napi::Env) { + JsContextRef context{}; + JsRuntimeHandle engine{}; + EXPECT_EQ(JsGetCurrentContext(&context), JsNoError); + EXPECT_EQ(JsGetRuntime(context, &engine), JsNoError); + EXPECT_EQ(JsCollectGarbage(engine), JsNoError); + collected.set_value(); + }); + collected.get_future().get(); + } + EXPECT_EQ(finalized->load(), 1); +} +#endif TEST(XMLHttpRequest, ThrowingStoppedListenerSkipsRemainingListeners) { From 972edff235925aabb57f94517c9f47304f44beb2 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Mon, 5 Oct 2026 18:16:59 -0700 Subject: [PATCH 15/16] Pin UrlLib Apple truncated-response fix for macOS CI The new loopback regression exposed NSURLSession completing a truncated 404 body without an NSError. Pick up explicit fixed-length identity-body validation from the companion UrlLib PR, retaining the original XHR regression expectations on all POSIX platforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2b2272ba-79f8-41d0-aae1-f514a0f712ad --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7e87faee..666025e2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -50,7 +50,7 @@ FetchContent_Declare(llhttp FetchContent_Declare(UrlLib # Repoint upstream after BabylonJS/UrlLib#38 merges. GIT_REPOSITORY https://github.com/bkaradzic-microsoft/UrlLib.git - GIT_TAG 89efc235e2a69df4582aaa11855b45c4adbee7f8 + GIT_TAG 85e8f026d8cb54fd6ed298b6c0be9bdadb67dcc8 EXCLUDE_FROM_ALL) FetchContent_Declare(quickjs-ng GIT_REPOSITORY https://github.com/quickjs-ng/quickjs.git From c1a94332f15ae861b76d05103bb80d4ccf8ac4bb Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Tue, 6 Oct 2026 12:57:48 -0700 Subject: [PATCH 16/16] Fix XHR event propagation and constructor forwarding Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305 --- Polyfills/XMLHttpRequest/Readme.md | 2 + .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 16 ++++---- .../Source/Scripts/tests.xmlHttpRequest.ts | 40 +++++++++++++++++++ 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Readme.md b/Polyfills/XMLHttpRequest/Readme.md index 4c5f872c..867ad6e7 100644 --- a/Polyfills/XMLHttpRequest/Readme.md +++ b/Polyfills/XMLHttpRequest/Readme.md @@ -15,6 +15,8 @@ handler; non-callable objects are retained by identity but are not invoked. Callbacks are owned by JavaScript so a handler capturing its own XHR does not keep an otherwise unreachable request alive. Exceptions are reported to the runtime's unhandled-exception handler without preventing later listeners from running. +`stopImmediatePropagation()` skips later listeners on the same request, including +when called through `Event.prototype`; `stopPropagation()` does not. At the moment, we only support the following events: * `loadend` * `readystatechange` diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 54c813b5..7e919e6d 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -64,9 +64,11 @@ namespace Babylon::Polyfills::Internal constexpr const char* Abort = "abort"; } + constexpr const char* JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME = "XMLHttpRequest"; constexpr const char* EVENT_FACTORY_NAME = "__jsRuntimeHostMakeXHREvent"; constexpr const char* EVENT_FACTORY_SOURCE = R"JS( (function (global) { + var immediateStops = new WeakSet(); if (typeof global.Event !== "function") { function Event(type, init) { init = init || {}; @@ -86,7 +88,10 @@ namespace Babylon::Polyfills::Internal if (this.cancelable) this.defaultPrevented = true; }; Event.prototype.stopPropagation = function () { this.cancelBubble = true; }; - Event.prototype.stopImmediatePropagation = function () { this.cancelBubble = true; }; + Event.prototype.stopImmediatePropagation = function () { + immediateStops.add(this); + this.cancelBubble = true; + }; Event.prototype.composedPath = function () { return this.currentTarget === null ? [] : [this.target]; }; @@ -124,18 +129,17 @@ namespace Babylon::Polyfills::Internal currentTarget: {get: function () { return currentTarget; }, configurable: true}, eventPhase: {get: function () { return eventPhase; }, configurable: true} }); - var stopped = false; var stop = event.stopImmediatePropagation; Object.defineProperty(event, "stopImmediatePropagation", { configurable: true, value: function () { - stopped = true; + immediateStops.add(this); return stop.call(this); } }); return { value: event, - isStopped: function () { return stopped; }, + isStopped: function () { return immediateStops.has(event); }, end: function () { currentTarget = null; eventPhase = 0; @@ -222,8 +226,6 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::Initialize(Napi::Env env) { - static constexpr auto JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME = "XMLHttpRequest"; - Napi::Function func = DefineClass( env, JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME, @@ -282,7 +284,7 @@ namespace Babylon::Polyfills::Internal XMLHttpRequest::XMLHttpRequest(const Napi::CallbackInfo& info) : Napi::ObjectWrap{info} , m_runtimeScheduler{JsRuntime::GetFromJavaScript(info.Env())} - , m_makeEvent{Napi::Persistent(info.NewTarget().As().Get(EVENT_FACTORY_NAME).As())} + , m_makeEvent{Napi::Persistent(JsRuntime::NativeObject::GetFromJavaScript(info.Env()).Get(JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME).As().Get(EVENT_FACTORY_NAME).As())} , m_getCallbacks{Napi::Persistent(m_makeEvent.Value().Get("getCallbacks").As())} , m_removeCallback{Napi::Persistent(m_makeEvent.Value().Get("removeCallback").As())} { diff --git a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts index bf41b92b..b54397d5 100644 --- a/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts +++ b/Tests/UnitTests/Source/Scripts/tests.xmlHttpRequest.ts @@ -25,6 +25,19 @@ describe("XMLHTTPRequest", function () { this.timeout(0); + it("should support a forwarding constructor with a different new target", function () { + if (typeof Reflect !== "object" || typeof Reflect.construct !== "function") { + this.skip(); + } + + function ForwardedXHR() {} + ForwardedXHR.prototype = XMLHttpRequest.prototype; + const xhr = Reflect.construct(XMLHttpRequest, [], ForwardedXHR) as XMLHttpRequest; + expect(xhr.readyState).to.equal(XMLHttpRequest.UNSENT); + xhr.open("GET", "app:///Assets/symlink_target.js"); + expect(xhr.readyState).to.equal(XMLHttpRequest.OPENED); + }); + it("should have readyState=4 when load ends", async function () { const xhr = await createRequest("GET", "https://github.com/"); expect(xhr.readyState).to.equal(4); @@ -584,6 +597,33 @@ describe("XMLHTTPRequest", function () { expect(loadEvent!.eventPhase).to.equal(Event.NONE); }); + it("should honor prototype immediate stop without skipping listeners after stopPropagation", async function () { + this.timeout(30000); + for (const immediate of [false, true]) { + const calls: string[] = []; + await new Promise((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const guard = setTimeout(() => reject(new Error("XHR loadend did not fire")), 25000); + xhr.addEventListener("load", (event: Event) => { + calls.push("first"); + if (immediate) { + Event.prototype.stopImmediatePropagation.call(event); + } else { + Event.prototype.stopPropagation.call(event); + } + }); + xhr.addEventListener("load", () => { calls.push("second"); }); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve(); + }); + xhr.open("GET", "app:///Assets/symlink_target.js"); + xhr.send(); + }); + expect(calls).to.deep.equal(immediate ? ["first"] : ["first", "second"]); + } + }); + 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