XMLHttpRequest: implement the on<event> handler properties

[bkaradzic-microsoft]

Problem

XMLHttpRequest::RaiseEvent dispatches only to handlers stored in
m_eventHandlerRefs, and that map is populated exclusively by
addEventListener. The class registers no accessors for the DOM
on<event> handler properties:

void XMLHttpRequest::RaiseEvent(const char* eventType)
{
    const auto it = m_eventHandlerRefs.find(eventType);   // addEventListener only
    ...
}

So this ordinary, spec-compliant code silently does nothing:

const request = new XMLHttpRequest();
request.open("GET", url, true);
request.onreadystatechange = function () {   // stored as a plain expando property
    if (request.readyState === 4) { /* never runs */ }
};
request.onerror = function () { /* never runs */ };
request.send(null);

The failure mode is silent and severe. The request completes normally and the
state is correct -- I instrumented a real case and observed readyState === 4
and status === 200 -- but no callback ever fires. There is no exception and
no diagnostic. The caller simply waits forever.

How this surfaced

In the BabylonNative Playground validation suite, the tests that fetch their
scene script over XHR use request.onreadystatechange. All of them hung until
the harness timeout and were consequently marked excluded on every graphics
API (D3D11, D3D12, OpenGL, Vulkan, Metal, WebGPU), with a misattributed
"scene never becomes ready" reason. The scene was never created at all.

Changes

1. on<event> handler properties. Adds onreadystatechange,
onload, onerror, onloadend and onabort as instance accessors.

They are stored in a map separate from the addEventListener handlers,
because they behave differently:

• assignment replaces the previous handler rather than accumulating;
• the property must be readable (xhr.onload returns what was assigned);
• assigning null/undefined clears it.

RaiseEvent now dispatches the on<event> handler in addition to any
addEventListener handlers, matching the DOM, and Send releases the new
strong references alongside the existing ones when the request settles.

2. Raise the load event on success. load was previously never
raised, so neither onload nor addEventListener("load", ...) could ever
fire -- only loadend, plus error on failure. Success now dispatches
load then loadend; failure continues to dispatch error then
loadend, per the spec.

Tests

Five regression tests in Tests/UnitTests/Scripts/tests.ts:

• onreadystatechange is invoked and reaches readyState 4
• onload + onloadend fire on success, onerror does not
• onerror fires on HTTP 404, onload does not
• the property can be read back, replaced, and cleared with null
• an on<event> property and addEventListener handlers both run

All 221 unit tests pass locally on Windows.

Compatibility

Purely additive. Existing addEventListener behavior is unchanged; the only
behavioral difference for existing code is that load listeners, which
previously could never fire, now do -- which is the documented DOM contract.

[Copilot]

Pull request overview

This PR updates the XMLHttpRequest polyfill to support DOM-style on<event> handler properties (e.g., onreadystatechange, onload) and ensures successful requests also raise the load event (in addition to loadend). It adds unit tests to prevent regressions where on<event> assignments silently did nothing.

Changes:

• Added instance accessors for onreadystatechange, onload, onerror, onloadend, and onabort, stored separately from addEventListener handlers.
• Updated event dispatch to invoke on<event> handlers in addition to addEventListener handlers, and to raise load on success.
• Added regression tests validating on<event> semantics (invocation, readback/replace/clear, and interaction with addEventListener).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
Tests/UnitTests/Scripts/tests.ts	Adds regression tests covering on<event> handler properties and load/loadend behavior.
Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h	Introduces plumbing (event indices + storage) for on<event> handler properties.
Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp	Implements on<event> accessors, dispatches them from RaiseEvent, raises load on success, and clears stored handlers after completion.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bkaradzic-microsoft]

Thanks -- both comments addressed in c870d43.

On the non-callable setter (XMLHttpRequest.cpp:101): throwing a TypeError here would actually diverge from the DOM. EventHandler attributes are declared [LegacyTreatNonObjectAsNull] in WebIDL, so a non-callable assignment is coerced to null rather than rejected -- xhr.onload = 0 leaves xhr.onload === null in every browser, silently. The current clear-on-non-function behavior matches that for primitives.

Strictly, the spec does store non-callable objects (they just never get invoked); we clear those too, because keeping a value we could never call would only defer the failure to dispatch time. I've documented that deliberate narrowing in a code comment and added a regression test (should coerce a non-callable on<event> assignment to null) pinning the no-throw behavior.

On onabort (XMLHttpRequest.cpp:138): good catch, this one was a real defect -- Abort() only called m_request.Abort(), so the completion continuation reported the cancellation as a transport error and onabort was dead API. Abort() now records the intent and the continuation raises abort + loadend instead of error, per the DOM. Covered by a new test asserting that aborting an in-flight request fires abort and never error/load.

While in here I also hardened RaiseEvent along the lines of FileReader::Dispatch: it now snapshots the handler list before dispatching (a handler calling addEventListener/removeEventListener, or reassigning an on<event> property, could reallocate the vector or rehash the map out from under the in-flight dispatch -- a use-after-free) and clears pending exceptions between handlers so a throwing handler neither aborts the remaining dispatch nor escapes into the native completion continuation.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

Two inline.

[bghgary]

failed is statusCode < 200 || statusCode >= 300 (L372), so a 404 takes the error branch and load never fires. Per the XHR spec error is network-level only; a completed 404 dispatches load, with callers branching on xhr.status inside it.

The new 404 test asserts loadFired === false, locking that in. Worth confirming it is intended — the description's "per the spec" holds for transport errors, not HTTP status codes.

The condition predates this PR and is load-bearing for the UWP status-0 case, so this is not a request to simply invert it.

[bkaradzic-microsoft]

You're right, and the test is wrong. A 404 is a completed HTTP transaction — per spec error is for network-level failures only, so load should fire and callers branch on xhr.status inside it.

On the part you flagged as load-bearing: I audited every m_statusCode assignment in UrlLib, and the status-0 case is narrower than the current condition assumes.

UrlStatusCode { None = 0, Ok = 200 }

Every non-HTTP success path sets Ok explicitly — UrlRequest_UWP.cpp:70,78 (local file read), Win32.cpp:78,98, Android.cpp:90,130, Apple.mm:201, Unix.cpp:251, Windows_Shared.h:131,143. None only ever appears as the initial value and in the reset at UrlRequest_Base.h:371.

So statusCode == 0 means precisely "no HTTP response was obtained", which is the transport-failure sentinel the UWP fix relies on — it is never a successful local file read. That makes the condition separable:

const bool failed = result.has_error() || statusCode == 0;

That keeps the missing-local-file-on-UWP behaviour intact (still error + loadend) while letting a completed 404 dispatch load + loadend.

I'll make that change and flip the 404 test to assert loadFired === true, errorFired === false, status === 404.

[bghgary]

on<event> handlers belong in the same listener list as addEventListener, not a parallel map dispatched ahead of it. Two divergences follow from the split:

• Order: browsers dispatch in registration order, so addEventListener("load", a) then xhr.onload = b gives a, b; here it gives b, a.
• xhr.onload = f; xhr.addEventListener("load", f) throws (XMLHttpRequest.cpp L269), where a browser registers both and calls f twice -- per DOM a duplicate add is a silent no-op, not an error.

HTML registers one internal listener on first set whose callback indirects through the stored value, so reassignment keeps its position ("If eventHandler's listener is not null, then return").

One vector<Listener> per type of { FunctionReference callback; bool isEventHandler; } covers it: the setter replaces the flagged entry in place, appends when absent, erases when the value is not callable; the getter reads it back. Drops this map and the on-handler-first branch in RaiseEvent.

The duplicate throw predates this PR.

[bkaradzic-microsoft]

Agreed — the parallel map is the wrong model, and I'll switch to the single-list design you described.

To confirm I've got the shape right:

struct Listener
{
    Napi::FunctionReference callback;
    bool isEventHandler;   // registered via on<event>, not addEventListener
};
std::unordered_map<std::string, std::vector<Listener>> m_listeners;

• setter: find the isEventHandler entry — replace its callback in place if present, append if not, erase if the value isn't callable
• getter: read that entry back, null when absent
• RaiseEvent: one pass over the vector, so dispatch is registration order
• m_onEventHandlerRefs and the on-handler-first branch both go away

That fixes the ordering divergence: addEventListener("load", a) then xhr.onload = b now gives a, b, and reassigning onload keeps its slot rather than moving to the end — matching "If eventHandler's listener is not null, then return".

For the second divergence, I'll scope the duplicate check in AddEventListener to non-isEventHandler entries, so xhr.onload = f; xhr.addEventListener("load", f) registers both and calls f twice, as a browser does.

That leaves the pre-existing addEventListener duplicate throw. Per DOM a duplicate add is a silent no-op, so the throw is also wrong, but it's independent of this bug and nothing currently covers it — happy to drop it here for symmetry, or leave it for a separate PR. Let me know which you'd prefer.

I'll add a test asserting dispatch order across both registration styles.