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.

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

[matthargett]

Heads-up on a rooting hazard that this PR mirrors from FileReader::Dispatch ("Mirrors FileReader::Dispatch"): the snapshot is a std::vector<Napi::Function> of raw values held across the listener calls. On JavaScriptCore nothing roots a napi_value that lives only on the C++ heap (the backend's handle scopes are stubs and it scans just the C stack), so if a listener removes a later listener during dispatch, that later Napi::Function can be collected before the loop reaches it — the same class of bug as the CompressionStream output-chunk corruption fixed on #211, which reproduces deterministically under JSC_collectContinuously=1. Snapshotting the FunctionReferences (or a JS array) instead of the values avoids it. I was going to send a small PR for FileReader; since this PR adds a second copy of the pattern, either I fold both into one follow-up after this lands, or you take the FunctionReference snapshot here and I do FileReader only — your call.

[matthargett]

Follow-up: the FileReader side of this is now its own PR, #248 (snapshot Napi::FunctionReferences via Napi::Persistent instead of bare values, released when dispatch returns). If you mirror that shape in XMLHttpRequest::Dispatch here, both stay consistent; happy to rebase #248 on top of this if it lands first.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

Concerns inline.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

Two concerns remain.

[Copilot]

Copilot review overview

🟡 Changes recommended

Event-handler conversion, completed-request abort state, and exception-time propagation stopping remain incorrect.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Reset completed XHR response state on abort · New
• Reject non-callable object event handlers · New
• Check immediate-propagation stop after listener exception · New

[bkaradzic-microsoft]

Rebased onto latest main (62818ee, including the #257 test-layout split). Native/JS coverage now lives under Tests/UnitTests/Source/ (Tests.XMLHttpRequest.cpp + tests.xmlHttpRequest.ts); app:/// fixtures use Assets/ instead of Scripts/.