Skip to content

XMLHttpRequest: implement the on<event> handler properties #221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bkaradzic-microsoft wants to merge 4 commits into
base: main
Choose a base branch
from
Open

XMLHttpRequest: implement the on<event> handler properties #221

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8166d52
XMLHttpRequest: implement the `on<event>` handler properties
bkaradzic Aug 6, 2026
c870d43
XMLHttpRequest: dispatch 'abort', harden RaiseEvent, document coercion
bkaradzic Aug 7, 2026
20b0c20
XMLHttpRequest: unify on<event> handlers with addEventListener listeners
Copilot Aug 11, 2026
87c5c1b
XMLHttpRequest: make a duplicate addEventListener a no-op
Copilot Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,48 @@ 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)];

// Assigning null/undefined clears the handler, matching the DOM behavior where
// `xhr.onload = null` detaches the previously assigned handler.
if (!value.IsFunction())
{
m_onEventHandlerRefs.erase(eventType);
return;
}

m_onEventHandlerRefs[eventType] = Napi::Persistent(value.As<Napi::Function>());
(void)info;
Comment thread
bkaradzic-microsoft marked this conversation as resolved.
Outdated
}

void XMLHttpRequest::Initialize(Napi::Env env)
{
static constexpr auto JS_XML_HTTP_REQUEST_CONSTRUCTOR_NAME = "XMLHttpRequest";
Expand All @@ -88,6 +127,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>),
Comment thread
bkaradzic-microsoft marked this conversation as resolved.
InstanceMethod("getAllResponseHeaders", &XMLHttpRequest::GetAllResponseHeaders),
InstanceMethod("getResponseHeader", &XMLHttpRequest::GetResponseHeader),
InstanceMethod("setRequestHeader", &XMLHttpRequest::SetRequestHeader),
Expand Down Expand Up @@ -322,11 +370,16 @@ namespace Babylon::Polyfills::Internal
{
RaiseEvent(EventType::Error);
}
else
{
RaiseEvent(EventType::Load);
Comment thread
bghgary marked this conversation as resolved.
}
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();
});
}

Expand All @@ -349,6 +402,13 @@ namespace Babylon::Polyfills::Internal
eventHandlerRef.Call({});
}
}

// The DOM dispatches the `on<event>` handler alongside any addEventListener handlers.
const auto onIt = m_onEventHandlerRefs.find(eventType);
if (onIt != m_onEventHandlerRefs.end())
{
onIt->second.Call({});
}
}
}

Expand Down
21 changes: 21 additions & 0 deletions Polyfills/XMLHttpRequest/Source/XMLHttpRequest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -53,5 +70,9 @@ namespace Babylon::Polyfills::Internal
JsRuntimeScheduler m_runtimeScheduler;
ReadyState m_readyState{ReadyState::Unsent};
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;

@bghgary bghgary Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bkaradzic-microsoft Aug 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bkaradzic-microsoft Aug 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bghgary Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 bkaradzic-microsoft Aug 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
}
100 changes: 100 additions & 0 deletions Tests/UnitTests/Scripts/tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,106 @@ 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 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/");
Expand Down
Loading