XMLHttpRequest: implement the on<event> handler properties

[bkaradzic-microsoft]

Problem

RaiseEvent dispatches only to handlers registered via addEventListener. The
DOM on<event> properties had no accessors, so they were stored as plain
expandos and never called:

request.onreadystatechange = function () { /* never runs */ };
request.onerror = function () { /* never runs */ };

The failure is silent: the request completes and the state is correct
(readyState === 4, status === 200), but no callback fires and no error is
reported. The caller waits forever.

This surfaced in the BabylonNative Playground suite, where the tests that fetch
their scene script over XHR use onreadystatechange. All of them hung until
timeout and were marked excluded on every graphics API, with a misattributed
"scene never becomes ready" reason.

Changes

1. on<event> handler properties. Adds onreadystatechange, onload,
onerror, onloadend and onabort as accessors. They share one listener list
per event type with addEventListener, tagged so the single on<event> entry
can be found:

struct Listener
{
    Napi::FunctionReference callback;
    bool isEventHandler;
};

std::unordered_map<std::string, std::vector<Listener>> m_listeners;

One list is what the DOM specifies, and it gets the observable details right:
dispatch follows registration order across both styles; reassigning the
property keeps its position rather than moving to the end; xhr.onload = f and
addEventListener("load", f) are independent registrations, so f is called
twice; removeEventListener does not remove an on<event> handler (assigning
null does). Per WebIDL these are [LegacyTreatNonObjectAsNull], so
xhr.onload = 0 clears the handler rather than throwing.

2. Raise load on success. It was never raised, so neither onload nor
addEventListener("load", ...) could fire — only loadend, plus error on
failure.

3. Raise abort when a request is aborted. Abort() only forwarded to
UrlLib, and the continuation reported the cancelled transfer as a transport
error. A cancelled request now dispatches abort + loadend, never error.

4. A duplicate addEventListener is a no-op. Re-adding an identical
(type, callback) pair threw Cannot add the same event handler twice; per DOM
the second add is silently ignored. The scan still skips the on<event> entry,
so the two-registration case above is unchanged.

5. error is transport-level only.

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

Previously any non-2xx status took the error branch, so a 404 fired error and
load never fired. Per spec error means the transfer did not complete; a 404
is a completed exchange, so it dispatches load and callers branch on
xhr.status.

The condition also covers a missing local file on UWP, where UrlLib leaves
the status at 0. UrlStatusCode::None (0) is only ever the initial value and
the ResetForOpen reset — every path that produces a response assigns an
explicit code, including non-HTTP ones, where a local file read sets Ok. So
statusCode == 0 means precisely "no response was obtained".

Tests

12 regression tests in Tests/UnitTests/Scripts/tests.ts covering: each
handler property firing (and reading back, being replaced, cleared, and
coercing a non-callable to null); 404 dispatching load rather than error
via both registration styles; abort dispatching abort; registration-order
dispatch; a reassigned handler keeping its position; a function registered both
ways being called twice; a duplicate addEventListener not throwing and firing
once; and removeEventListener not removing an on<event> handler.

All 227 unit tests pass on Windows.

Note

Tests/UnitTests/dist/ is gitignored and CMake only copies it, so npm run build in Tests/ is required after editing tests.ts — otherwise the build
silently tests a stale bundle.

Compatibility

The on<event> properties, load and abort are additive: they cover cases
that previously could not fire at all.

One behavioral change worth a close look: a non-2xx response now fires
load instead of error. Code relying on onerror to observe an HTTP 404
must check xhr.status inside onload instead. This matches browsers, and
loadend still fires either way, so anything settling on loadend is
unaffected.

[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]

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.

[bkaradzic-microsoft]

Done in 20b0c20 — switched to the single-list design.

m_onEventHandlerRefs is gone. m_listeners is now one vector<Listener> per event type:

struct Listener
{
    Napi::FunctionReference callback;
    bool isEventHandler;
};

• setter: finds the isEventHandler entry and replaces its callback in place, appends when absent, erases when the assigned value is not callable
• getter: reads that entry back, null when absent
• RaiseEvent: one pass over the vector, so dispatch is registration order
• AddEventListener: the duplicate check skips isEventHandler entries
• RemoveEventListener: the match also skips them, since the property is cleared by assigning null, not by removeEventListener

Four tests cover the behaviour, and each fails against the previous implementation:

Test	Was	Now
addEventListener("load", a), onload = b, addEventListener("load", c)	b, a, c	a, b, c
onload = x, addEventListener("load", l), onload = y	y, l — but only incidentally, since on-handlers always ran first	y, l, because the entry keeps its slot
onload = f, addEventListener("load", f)	threw	f called twice
onload = f, removeEventListener("load", f)	n/a	f still called

On the pre-existing addEventListener duplicate throw: I left it as-is here, since it is independent of this bug and nothing currently covers it. Happy to drop it in a follow-up if you would like DOM-correct silent no-op behaviour.

[bghgary]

[Responded by Copilot on behalf of @bghgary]

Remove it here. You're already in that code, and per DOM a duplicate add is a silent no-op — leaving the throw would park a second known-wrong behaviour next to the one you're fixing.

Your UrlLib audit holds, checked independently: all sixteen m_statusCode assignments set Ok explicitly on the non-HTTP success paths, and the only None is the reset in UrlRequest_Base.h. The blob-handler path returns early via SetError when unhandled, so a successful request can't land at 0 either.

[bkaradzic-microsoft]

Removed in 87c5c1b — the duplicate addEventListener now returns silently instead of throwing, with a comment citing the DOM "append listener" step.

The scan still skips isEventHandler entries, so the two-registration case is unchanged: xhr.onload = f followed by xhr.addEventListener("load", f) still calls f twice. Only two addEventListener calls with the same pair collapse to one.

Added a test pinning the new behaviour (doesn't throw, handler fires exactly once); the existing "called twice" test guards the other direction. Suite is 227 passing on Windows.

And thanks for double-checking the UrlLib status-code paths — good to have that confirmed independently rather than resting on my read alone.

One note: the identical throw also exists in Polyfills/AbortController/Source/AbortSignal.cpp:129. I left it alone since it's outside this PR's surface, but happy to fix it here or file a follow-up, whichever you prefer.

[Copilot]

Pull request overview

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

Suppressed comments (6)

Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp:106

• [LegacyTreatNonObjectAsNull] only converts non-object values to null; a non-callable object must fail callback-function conversion with a TypeError. This branch silently clears assignments such as xhr.onload = {}, which differs from the WebIDL contract. Handle primitive values separately and reject non-callable objects.

        if (!value.IsFunction())

Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp:335

• m_aborted is sticky and is never reset. Calling abort() before a request, or reusing an instance after an aborted request, therefore causes a later successful transfer to dispatch abort instead of load. Scope this flag and the underlying Abort() call to an active send, and reset the per-transfer state when a new send begins.

        m_aborted = true;

Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp:432

• Clearing the unified list now also clears every on<event> property. After completion xhr.onload reads back as null, and reusing the XHR loses all registered listeners; EventTarget registrations should persist until explicitly removed or the object is destroyed.

                m_listeners.clear();

Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp:453

• Snapshotting bare callback functions makes listener mutations during dispatch ineffective. If an earlier callback removes a later listener, the removed function remains in handlers and is still invoked; similarly, reassigning a pending onload invokes the old snapshot. Preserve stable listener records and check their current/removed state before each invocation.

        // 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{};

Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp:472

• This overload invokes the callback with an undefined receiver and no arguments. XHR listeners and handler properties must receive an event and run with this/currentTarget set to the XHR, so code using event.target or this.status breaks. Pass the wrapper object and an event value, as FileReader::Dispatch does in Polyfills/File/Source/FileReader.cpp:223-255.

            handler.Call({});

Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp:158

• The public Polyfills/XMLHttpRequest/Readme.md:5-8 still says onload-style properties are unsupported, omits load/abort, and says non-2xx responses fire error. Update that documentation alongside these accessors so users are not directed away from the newly supported API or given the old error semantics.

This issue also appears in the following locations of the same file:

• line 335
• line 432
• line 449
• line 472

                // 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>),