XMLHttpRequest: implement the on<event> handler properties

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<size_t>(XMLHttpRequest::EventIndex::Count)] = {
+        EventType::ReadyStateChange,
+        EventType::Load,
+        EventType::Error,
+        EventType::LoadEnd,
+        EventType::Abort,
+    };
+
+    template<XMLHttpRequest::EventIndex Index>
+    Napi::Value XMLHttpRequest::GetEventHandler(const Napi::CallbackInfo&)
+    {
+        const auto it = m_onEventHandlerRefs.find(EVENT_TYPE_NAMES[static_cast<size_t>(Index)]);
+        if (it == m_onEventHandlerRefs.end())
+        {
+            return Env().Null();
+        }
+
+        return it->second.Value();
+    }
+
+    template<XMLHttpRequest::EventIndex Index>
+    void XMLHttpRequest::SetEventHandler(const Napi::CallbackInfo& info, const Napi::Value& value)
+    {
+        const char* eventType = EVENT_TYPE_NAMES[static_cast<size_t>(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<Napi::Function>());
+        (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<event>` 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<EventIndex::ReadyStateChange>, &XMLHttpRequest::SetEventHandler<EventIndex::ReadyStateChange>),
+                InstanceAccessor("onload", &XMLHttpRequest::GetEventHandler<EventIndex::Load>, &XMLHttpRequest::SetEventHandler<EventIndex::Load>),
+                InstanceAccessor("onerror", &XMLHttpRequest::GetEventHandler<EventIndex::Error>, &XMLHttpRequest::SetEventHandler<EventIndex::Error>),
+                InstanceAccessor("onloadend", &XMLHttpRequest::GetEventHandler<EventIndex::LoadEnd>, &XMLHttpRequest::SetEventHandler<EventIndex::LoadEnd>),
+                InstanceAccessor("onabort", &XMLHttpRequest::GetEventHandler<EventIndex::Abort>, &XMLHttpRequest::SetEventHandler<EventIndex::Abort>),
                 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<event> property while it runs, which would
+        // otherwise reallocate the vector or rehash the map out from under this dispatch.
+        // (Mirrors FileReader::Dispatch.)
+        std::vector<Napi::Function> 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();
             }
         }
     }

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<event>`
+        // 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<size_t>(EventIndex::Count)];
+
+        template<EventIndex Index> Napi::Value GetEventHandler(const Napi::CallbackInfo& info);
+        template<EventIndex Index> 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<std::string, std::vector<Napi::FunctionReference>> m_eventHandlerRefs;
+        // The DOM `on<event>` 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<std::string, Napi::FunctionReference> m_onEventHandlerRefs;
     };
 }

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<event> 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<event> 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<event> 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<event> 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/");