From 8166d52ac7e38538cb47c1cca505a3023548450c Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 6 Aug 2026 16:33:30 -0700 Subject: [PATCH 1/2] XMLHttpRequest: implement the `on` handler properties `XMLHttpRequest::RaiseEvent` only dispatches to handlers stored in `m_eventHandlerRefs`, which is populated exclusively by `addEventListener`. The class exposed no accessors for the DOM `on` handler properties, so `xhr.onreadystatechange = fn` merely created an ordinary expando property on the JS wrapper that nothing ever read. The failure mode is silent and severe: the request runs to completion and `readyState`/`status` are updated correctly, but the callback never fires, so code written against the standard XMLHttpRequest API waits forever for an event that cannot arrive. There is no error and no diagnostic -- it simply hangs. Add `onreadystatechange`, `onload`, `onerror`, `onloadend` and `onabort` as instance accessors, stored in a separate map from the `addEventListener` handlers because they have assignment semantics (setting replaces the previous handler) rather than accumulating, and because they must be individually readable and clearable via `xhr.onload = null`. `RaiseEvent` now dispatches the `on` handler in addition to any `addEventListener` handlers, matching the DOM, and `Send` releases the new strong references alongside the existing ones. Also raise the `load` event on success. It was previously never raised at all, so neither `onload` nor `addEventListener("load", ...)` could fire; only `loadend` and (on failure) `error` were dispatched. Success now dispatches `load` then `loadend`, and failure continues to dispatch `error` then `loadend`, per the spec. Adds five regression tests covering handler invocation on success and on HTTP 404, get/replace/clear semantics of the property, and co-existence with `addEventListener`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09 --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 60 +++++++++++ .../XMLHttpRequest/Source/XMLHttpRequest.h | 21 ++++ Tests/UnitTests/Scripts/tests.ts | 100 ++++++++++++++++++ 3 files changed, 181 insertions(+) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index d0220d16..0714e989 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -59,9 +59,48 @@ namespace Babylon::Polyfills::Internal constexpr const char* ReadyStateChange = "readystatechange"; constexpr const char* LoadEnd = "loadend"; constexpr const char* Error = "error"; + constexpr const char* Load = "load"; + constexpr const char* Abort = "abort"; } } + const char* const XMLHttpRequest::EVENT_TYPE_NAMES[static_cast(XMLHttpRequest::EventIndex::Count)] = { + EventType::ReadyStateChange, + EventType::Load, + EventType::Error, + EventType::LoadEnd, + EventType::Abort, + }; + + template + Napi::Value XMLHttpRequest::GetEventHandler(const Napi::CallbackInfo&) + { + const auto it = m_onEventHandlerRefs.find(EVENT_TYPE_NAMES[static_cast(Index)]); + if (it == m_onEventHandlerRefs.end()) + { + return Env().Null(); + } + + return it->second.Value(); + } + + template + void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo& info, const Napi::Value& value) + { + const char* eventType = EVENT_TYPE_NAMES[static_cast(Index)]; + + // Assigning null/undefined clears the handler, matching the DOM behavior where + // `xhr.onload = null` detaches the previously assigned handler. + if (!value.IsFunction()) + { + m_onEventHandlerRefs.erase(eventType); + return; + } + + m_onEventHandlerRefs[eventType] = Napi::Persistent(value.As()); + (void)info; + } + void XMLHttpRequest::Initialize(Napi::Env env) { static constexpr auto JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME = "XMLHttpRequest"; @@ -88,6 +127,15 @@ namespace Babylon::Polyfills::Internal // to tell a DNS failure from a refused connection or a missing local asset. InstanceAccessor("errorCode", &XMLHttpRequest::GetErrorCode, nullptr), InstanceAccessor("errorDetail", &XMLHttpRequest::GetErrorDetail, nullptr), + // DOM `on` handler properties. Without these, `xhr.onreadystatechange = fn` + // silently sets an ordinary expando property that is never invoked, so code written + // against the standard XMLHttpRequest API waits forever for a callback that can + // never fire. + InstanceAccessor("onreadystatechange", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), + InstanceAccessor("onload", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), + InstanceAccessor("onerror", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), + InstanceAccessor("onloadend", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), + InstanceAccessor("onabort", &XMLHttpRequest::GetEventHandler, &XMLHttpRequest::SetEventHandler), InstanceMethod("getAllResponseHeaders", &XMLHttpRequest::GetAllResponseHeaders), InstanceMethod("getResponseHeader", &XMLHttpRequest::GetResponseHeader), InstanceMethod("setRequestHeader", &XMLHttpRequest::SetRequestHeader), @@ -322,11 +370,16 @@ namespace Babylon::Polyfills::Internal { RaiseEvent(EventType::Error); } + else + { + RaiseEvent(EventType::Load); + } RaiseEvent(EventType::LoadEnd); // Assume the XMLHttpRequest will only be used for a single request and clear the event handlers. // Single use seems to be the standard pattern, and we need to release our strong refs to event handlers. m_eventHandlerRefs.clear(); + m_onEventHandlerRefs.clear(); }); } @@ -349,6 +402,13 @@ namespace Babylon::Polyfills::Internal eventHandlerRef.Call({}); } } + + // The DOM dispatches the `on` handler alongside any addEventListener handlers. + const auto onIt = m_onEventHandlerRefs.find(eventType); + if (onIt != m_onEventHandlerRefs.end()) + { + onIt->second.Call({}); + } } } diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 74d2c3b9..2f28d95e 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -39,6 +39,23 @@ namespace Babylon::Polyfills::Internal Napi::Value GetErrorCode(const Napi::CallbackInfo& info); Napi::Value GetErrorDetail(const Napi::CallbackInfo& info); + // Indices into XMLHttpRequest::EVENT_TYPE_NAMES; used to instantiate the `on` + // property accessors below without needing a distinct method per event type. + enum class EventIndex : size_t + { + ReadyStateChange = 0, + Load = 1, + Error = 2, + LoadEnd = 3, + Abort = 4, + Count = 5, + }; + + static const char* const EVENT_TYPE_NAMES[static_cast(EventIndex::Count)]; + + template Napi::Value GetEventHandler(const Napi::CallbackInfo& info); + template void SetEventHandler(const Napi::CallbackInfo& info, const Napi::Value& value); + void AddEventListener(const Napi::CallbackInfo& info); void RemoveEventListener(const Napi::CallbackInfo& info); void Abort(const Napi::CallbackInfo& info); @@ -53,5 +70,9 @@ namespace Babylon::Polyfills::Internal JsRuntimeScheduler m_runtimeScheduler; ReadyState m_readyState{ReadyState::Unsent}; std::unordered_map> m_eventHandlerRefs; + // The DOM `on` handler properties (onreadystatechange, onload, ...). These are + // kept separate from m_eventHandlerRefs because they have assignment semantics -- setting + // one replaces the previous handler -- whereas addEventListener accumulates. + std::unordered_map m_onEventHandlerRefs; }; } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index cdc9416b..c9f08b87 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -184,6 +184,106 @@ describe("XMLHTTPRequest", function () { expect(result.readyState).to.equal(4); }); + it("should invoke the 'onreadystatechange' handler property", async function () { + // Regression test: the on handler properties were not implemented, so + // `xhr.onreadystatechange = fn` set an ordinary expando property that was never + // invoked and callers waited forever for a callback that could never fire. + this.timeout(30000); + const result = await new Promise<{ states: number[]; status: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const states: number[] = []; + const guard = setTimeout(() => reject(new Error("onreadystatechange never reached readyState 4 within 25s")), 25000); + xhr.onreadystatechange = () => { + states.push(xhr.readyState); + if (xhr.readyState === 4) { + clearTimeout(guard); + resolve({ states, status: xhr.status }); + } + }; + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.states).to.include(4); + expect(result.status).to.equal(200); + }); + + it("should invoke the 'onload' and 'onloadend' handler properties on success", async function () { + this.timeout(30000); + const result = await new Promise<{ loadFired: boolean; loadEndFired: boolean; errorFired: boolean }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let loadFired = false; + let errorFired = false; + const guard = setTimeout(() => reject(new Error("onloadend did not fire within 25s")), 25000); + xhr.onload = () => { loadFired = true; }; + xhr.onerror = () => { errorFired = true; }; + xhr.onloadend = () => { + clearTimeout(guard); + resolve({ loadFired, loadEndFired: true, errorFired }); + }; + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.loadFired).to.equal(true); + expect(result.loadEndFired).to.equal(true); + expect(result.errorFired).to.equal(false); + }); + + it("should invoke the 'onerror' handler property for HTTP 404", async function () { + this.timeout(30000); + const result = await new Promise<{ errorFired: boolean; loadFired: boolean; status: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let errorFired = false; + let loadFired = false; + const guard = setTimeout(() => reject(new Error("onloadend did not fire within 25s")), 25000); + xhr.onerror = () => { errorFired = true; }; + xhr.onload = () => { loadFired = true; }; + xhr.onloadend = () => { + clearTimeout(guard); + resolve({ errorFired, loadFired, status: xhr.status }); + }; + xhr.open("GET", "https://github.com/babylonJS/BabylonNative404"); + xhr.send(); + }); + expect(result.status).to.equal(404); + expect(result.errorFired).to.equal(true); + expect(result.loadFired).to.equal(false); + }); + + it("should let an on property be read back, replaced, and cleared", async function () { + const xhr = new XMLHttpRequest(); + expect(xhr.onload).to.equal(null); + + const first = () => { }; + xhr.onload = first; + expect(xhr.onload).to.equal(first); + + // Assignment replaces rather than accumulates, unlike addEventListener. + const second = () => { }; + xhr.onload = second; + expect(xhr.onload).to.equal(second); + + xhr.onload = null; + expect(xhr.onload).to.equal(null); + }); + + it("should invoke both an on property and addEventListener handlers", async function () { + this.timeout(30000); + const result = await new Promise<{ order: string[] }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const order: string[] = []; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.onload = () => { order.push("onload"); }; + xhr.addEventListener("load", () => { order.push("listener"); }); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ order }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.order).to.have.members(["onload", "listener"]); + }); + it("should expose errorCode/errorDetail diagnostics after a transport failure", async function () { this.timeout(30000); const xhr: any = await createRequest("GET", "http://127.0.0.1:1/"); From c870d43da28801d60910c107dd4792c5af593f1e Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 6 Aug 2026 22:10:00 -0700 Subject: [PATCH 2/2] XMLHttpRequest: dispatch 'abort', harden RaiseEvent, document coercion Addresses review feedback on the `on` handler properties: - `onabort` was exposed but no `abort` event was ever dispatched, so the handler could never fire. `Abort()` now records the caller's intent and the completion continuation reports the outcome as `abort` + `loadend` instead of `error`, matching the DOM. - `RaiseEvent` now snapshots the handler list before dispatching. A handler is free to call `addEventListener`/`removeEventListener` or reassign an `on` property, either of which would reallocate the vector or rehash the map out from under an in-flight dispatch. It also clears pending exceptions between handlers so a throwing handler neither aborts the rest of the dispatch nor escapes into the native completion continuation. This mirrors `FileReader::Dispatch`. - Documented why a non-callable assignment clears the handler rather than throwing: `EventHandler` attributes are `[LegacyTreatNonObjectAsNull]` in WebIDL, so `xhr.onload = 0` yields `null` rather than a TypeError. Adds regression tests for the abort event and the non-callable coercion. --- .../XMLHttpRequest/Source/XMLHttpRequest.cpp | 55 +++++++++++++++---- .../XMLHttpRequest/Source/XMLHttpRequest.h | 2 + Tests/UnitTests/Scripts/tests.ts | 47 ++++++++++++++++ 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 0714e989..e5d481ad 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -89,8 +89,10 @@ namespace Babylon::Polyfills::Internal { const char* eventType = EVENT_TYPE_NAMES[static_cast(Index)]; - // Assigning null/undefined clears the handler, matching the DOM behavior where - // `xhr.onload = null` detaches the previously assigned handler. + // `EventHandler` attributes are declared [LegacyTreatNonObjectAsNull] in WebIDL, so a + // non-callable assignment is coerced to null rather than throwing: `xhr.onload = 0` + // leaves `xhr.onload === null`. We extend that to non-callable objects too -- storing a + // value we could never invoke would only defer the failure to dispatch time. if (!value.IsFunction()) { m_onEventHandlerRefs.erase(eventType); @@ -296,6 +298,10 @@ namespace Babylon::Polyfills::Internal void XMLHttpRequest::Abort(const Napi::CallbackInfo&) { + // Record the caller's intent so the in-flight continuation reports this as an abort + // rather than a transport error. If no request is in flight this is inert, matching the + // DOM, where abort() on an unsent request produces no observable events. + m_aborted = true; m_request.Abort(); } @@ -366,7 +372,13 @@ namespace Babylon::Polyfills::Internal const bool failed = result.has_error() || statusCode < 200 || statusCode >= 300; SetReadyState(ReadyState::Done); - if (failed) + if (m_aborted) + { + // A cancelled request is not a transport failure: the DOM reports it as + // 'abort' + 'loadend' and never raises 'error'. + RaiseEvent(EventType::Abort); + } + else if (failed) { RaiseEvent(EventType::Error); } @@ -393,21 +405,44 @@ namespace Babylon::Polyfills::Internal { std::string traceName = (std::ostringstream{} << "XMLHttpRequest::RaiseEvent [" << eventType << "] [" << m_url << "]").str(); arcana::trace_region raiseEventRegion{traceName.c_str()}; + + Napi::Env env = Env(); + + // Snapshot the handlers before dispatching. A handler may call addEventListener, + // removeEventListener, or reassign an on property while it runs, which would + // otherwise reallocate the vector or rehash the map out from under this dispatch. + // (Mirrors FileReader::Dispatch.) + std::vector handlers{}; + + const auto onIt = m_onEventHandlerRefs.find(eventType); + if (onIt != m_onEventHandlerRefs.end() && !onIt->second.IsEmpty()) + { + handlers.push_back(onIt->second.Value()); + } + const auto it = m_eventHandlerRefs.find(eventType); if (it != m_eventHandlerRefs.end()) { - const auto& eventHandlerRefs = it->second; - for (const auto& eventHandlerRef : eventHandlerRefs) + handlers.reserve(handlers.size() + it->second.size()); + for (const auto& eventHandlerRef : it->second) { - eventHandlerRef.Call({}); + if (!eventHandlerRef.IsEmpty()) + { + handlers.push_back(eventHandlerRef.Value()); + } } } - // The DOM dispatches the `on` handler alongside any addEventListener handlers. - const auto onIt = m_onEventHandlerRefs.find(eventType); - if (onIt != m_onEventHandlerRefs.end()) + for (const auto& handler : handlers) { - onIt->second.Call({}); + handler.Call({}); + + // A throwing handler must not abort the remaining dispatch, and the exception must + // not escape into the native completion continuation that called us. + if (env.IsExceptionPending()) + { + env.GetAndClearPendingException(); + } } } } diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 2f28d95e..ec22818e 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h @@ -69,6 +69,8 @@ namespace Babylon::Polyfills::Internal UrlLib::UrlRequest m_request{}; JsRuntimeScheduler m_runtimeScheduler; ReadyState m_readyState{ReadyState::Unsent}; + // Set by abort(); makes the in-flight continuation report 'abort' instead of 'error'. + bool m_aborted{false}; std::unordered_map> m_eventHandlerRefs; // The DOM `on` handler properties (onreadystatechange, onload, ...). These are // kept separate from m_eventHandlerRefs because they have assignment semantics -- setting diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index c9f08b87..e2af3a53 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -266,6 +266,53 @@ describe("XMLHTTPRequest", function () { expect(xhr.onload).to.equal(null); }); + it("should coerce a non-callable on assignment to null", function () { + // EventHandler attributes are [LegacyTreatNonObjectAsNull] in WebIDL: assigning a + // non-callable value clears the handler rather than throwing a TypeError. + const xhr: any = new XMLHttpRequest(); + xhr.onload = () => { }; + expect(xhr.onload).to.not.equal(null); + + xhr.onload = 0; + expect(xhr.onload).to.equal(null); + + xhr.onload = () => { }; + xhr.onload = "not a function"; + expect(xhr.onload).to.equal(null); + + xhr.onload = () => { }; + xhr.onload = undefined; + expect(xhr.onload).to.equal(null); + }); + + it("should fire 'abort' rather than 'error' when a request is aborted", async function () { + this.timeout(30000); + const result = await new Promise<{ abortFired: boolean; errorFired: boolean; loadFired: boolean; loadEndFired: boolean }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let abortFired = false; + let errorFired = false; + let loadFired = false; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.onabort = () => { abortFired = true; }; + xhr.onerror = () => { errorFired = true; }; + xhr.onload = () => { loadFired = true; }; + xhr.onloadend = () => { + clearTimeout(guard); + resolve({ abortFired, errorFired, loadFired, loadEndFired: true }); + }; + xhr.open("GET", "https://github.com/"); + xhr.send(); + xhr.abort(); + }); + // loadend must always settle the request, whatever the outcome. + expect(result.loadEndFired).to.equal(true); + // The abort was requested before the transfer could complete, so it must be reported + // as an abort -- never as a transport error, and never as a successful load. + expect(result.abortFired).to.equal(true); + expect(result.errorFired).to.equal(false); + expect(result.loadFired).to.equal(false); + }); + it("should invoke both an on property and addEventListener handlers", async function () { this.timeout(30000); const result = await new Promise<{ order: string[] }>((resolve, reject) => {