diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index d0220d16..399613e8 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -59,6 +60,68 @@ 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_listeners.find(EVENT_TYPE_NAMES[static_cast(Index)]); + if (it != m_listeners.end()) + { + for (const auto& listener : it->second) + { + if (listener.isEventHandler) + { + return listener.callback.Value(); + } + } + } + + return Env().Null(); + } + + template + 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; + }); + + // `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()) + { + if (it != listeners.end()) + { + listeners.erase(it); + } + + return; + } + + 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}); } } @@ -88,6 +151,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), @@ -215,31 +287,40 @@ 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 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; } } - 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; } } @@ -248,6 +329,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(); } @@ -315,18 +400,36 @@ 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 (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); } + 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_listeners.clear(); }); } @@ -340,13 +443,39 @@ namespace Babylon::Polyfills::Internal { std::string traceName = (std::ostringstream{} << "XMLHttpRequest::RaiseEvent [" << eventType << "] [" << m_url << "]").str(); arcana::trace_region raiseEventRegion{traceName.c_str()}; - const auto it = m_eventHandlerRefs.find(eventType); - if (it != m_eventHandlerRefs.end()) + + 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 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()); + } + } + } + + for (const auto& handler : handlers) { - const auto& eventHandlerRefs = it->second; - for (const auto& eventHandlerRef : eventHandlerRefs) + 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()) { - eventHandlerRef.Call({}); + env.GetAndClearPendingException(); } } } diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h index 74d2c3b9..5cd3627d 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); @@ -48,10 +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}; - std::unordered_map> m_eventHandlerRefs; + // Set by abort(); makes the in-flight continuation report 'abort' instead of 'error'. + bool m_aborted{false}; + std::unordered_map> m_listeners; }; } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index cdc9416b..c700b7fb 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -159,31 +159,275 @@ 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); }); + 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 '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(); + 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.loadFired).to.equal(true); + expect(result.errorFired).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 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 dispatch on properties and addEventListener handlers in registration order", async function () { + // on handlers and addEventListener listeners share one list per event type, so + // dispatch follows registration order across both styles rather than running all the + // on handlers first. + this.timeout(30000); + const result = await new Promise<{ order: string[] }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const order: string[] = []; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.addEventListener("load", () => { order.push("first"); }); + xhr.onload = () => { order.push("onload"); }; + xhr.addEventListener("load", () => { order.push("last"); }); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ order }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.order).to.deep.equal(["first", "onload", "last"]); + }); + + it("should keep an on handler's position in the dispatch order when reassigned", async function () { + // Per HTML the internal listener is registered on first set and reused thereafter ("If + // eventHandler's listener is not null, then return"), so reassigning the property must + // not move it to the end of the list. + this.timeout(30000); + const result = await new Promise<{ order: string[] }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + const order: string[] = []; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + xhr.onload = () => { order.push("replaced"); }; + xhr.addEventListener("load", () => { order.push("listener"); }); + xhr.onload = () => { order.push("onload"); }; + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ order }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.order).to.deep.equal(["onload", "listener"]); + }); + + it("should invoke a function registered both as an on property and via addEventListener twice", async function () { + // These are two independent registrations, so the duplicate-registration check must not + // see the on entry: a browser calls the shared function once for each. + this.timeout(30000); + const result = await new Promise<{ calls: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let calls = 0; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + const handler = () => { calls++; }; + xhr.onload = handler; + xhr.addEventListener("load", handler); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ calls }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.calls).to.equal(2); + }); + + it("should treat a duplicate addEventListener registration as a no-op", async function () { + // Per DOM, re-adding an identical (type, callback) pair is a silent no-op rather than an + // error, and the listener stays registered once, so it is dispatched once. + this.timeout(30000); + const result = await new Promise<{ calls: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let calls = 0; + const guard = setTimeout(() => reject(new Error("loadend did not fire within 25s")), 25000); + const handler = () => { calls++; }; + xhr.addEventListener("load", handler); + expect(() => xhr.addEventListener("load", handler)).to.not.throw(); + xhr.addEventListener("loadend", () => { + clearTimeout(guard); + resolve({ calls }); + }); + xhr.open("GET", "app:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.calls).to.equal(1); + }); + + it("should not let removeEventListener remove an on handler", async function () { + // The property is cleared by assigning null, not by removeEventListener. + this.timeout(30000); + 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:///Scripts/symlink_target.js"); + xhr.send(); + }); + expect(result.order).to.deep.equal(["onload"]); + }); + 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/");