diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index d0220d16..e5d481ad 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -59,9 +59,50 @@ 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)]; + + // `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); + 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 +129,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), @@ -248,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(); } @@ -318,15 +372,26 @@ 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); } + 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(); }); } @@ -340,13 +405,43 @@ 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) + { + if (!eventHandlerRef.IsEmpty()) + { + handlers.push_back(eventHandlerRef.Value()); + } + } + } + + for (const auto& handler : handlers) + { + 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..ec22818e 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); @@ -52,6 +69,12 @@ 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 + // 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..e2af3a53 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -184,6 +184,153 @@ 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 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) => { + 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/");