Fix XMLHttpRequest::Send swallowing async failure

[bkaradzic-microsoft]

Problem

XMLHttpRequest::Send's .then chain has a success-only middle continuation. On async failure the chain skips SetReadyState(ReadyState::Done), RaiseEvent(LoadEnd) and the event handler cleanup, so:

• No error event is ever raised.
• No loadend event is raised.
• readystatechange(4) is never fired.
• The trailing continuation calls ThrowAsJavaScriptException() on the eventually-failed task, but throwing into the empty C++ stack does not propagate to the XHR's JS error handlers.

Net effect: every BABYLON.Tools.LoadFile async failure (e.g. local file not found via the app:/// scheme, where UrlLib's UWP path silently retains status code 0) hangs the JS observer or surfaces as an unrelated uncaught exception elsewhere.

Fix

Replace the two-step success+catch with a single arcana::expected continuation that:

• always advances readyState to Done (which fires readystatechange);
• raises an error event when the request errored (has_error()) or finished with a non-2xx status code;
• always raises loadend and clears event handler refs.

The non-2xx check is needed because UrlLib's Windows-shared and UWP paths return task_from_result<>() on HTTP-level failures and local-file lookup failures respectively, so has_error() is false even when nothing was actually loaded.

Tests

Two new tests in Tests/UnitTests/Scripts/tests.ts:

• should fire 'error' event when an async request fails -- exercises the local-file-not-found case via an app:/// URL pointing at a nonexistent script.
• should fire 'error' event for a remote URL that returns HTTP 404 -- exercises the HTTP non-2xx case via the existing GitHub 404 URL.

Both verify that error and loadend fire and that readyState reaches 4.

[Copilot]

Pull request overview

This PR fixes XMLHttpRequest::Send so that async failures (including “successful” tasks that end with non-2xx status codes) still advance readyState to Done, raise error/loadend, and release event-handler references, preventing JS callers from hanging on failure.

Changes:

• Update the XHR async continuation to run for both success and failure outcomes, always finalizing state and events.
• Treat non-2xx HTTP status codes as failures for purposes of raising the error event.
• Add unit tests covering async transport failure (missing app:/// file) and HTTP 404 error behavior.

Reviewed changes

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

File	Description
Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp	Ensures XHR completion logic (readyState/events/cleanup) runs even when the async task fails or returns non-2xx.
Tests/UnitTests/Scripts/tests.ts	Adds regression tests validating error + loadend behavior and readyState completion for failure cases.

---

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

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

LGTM; +1'd the AI-reviewer's three concerns to address before merge.

[bkaradzic-microsoft]

CI failure analysis -- dropped one test and force-pushed.

The original should fire 'error' event when an async request fails test issued an XMLHttpRequest GET against a deliberately-missing app:/// URL. That surfaced two unrelated pre-existing UrlLib bugs that crash the host process instead of producing an error event on the XHR. I removed that test (the remaining HTTP 404 test exercises the fix path equally well -- success-with-non-2xx is the same code path as has_error() in the new continuation) and force-pushed.

For visibility -- both UrlLib bugs should be fixed so consumers can rely on receiving an error event for missing local files. I tried to file an issue on UrlLib but issues are disabled on that repo, so noting here:

Bug 1 (Unix) -- Source/UrlRequest_Unix.cpp's worker std::thread calls curl_check(curl_easy_perform(m_curl)). curl_check throws std::runtime_error on CURLE_FILE_COULDNT_READ_FILE (37) for missing local file:// (or post-rewrite app:///) targets. The uncaught exception in the worker thread calls std::terminate:

[log] should have status=404 for a file that does not exist (204ms) terminate called after throwing an instance of 'std::runtime_error' what(): CURL call failed with code (37) ./UnitTests: line 1: 3639 Aborted (core dumped)

Suggested fix: try/catch in the thread function and signal completion with status 0 to match Apple/UWP's "client-side error" convention.

Bug 2 (Apple) -- Source/UrlRequest_Apple.mm's Open throws std::runtime_error{"No file exists at local path"} synchronously when an app:/// resource lookup fails. This breaks platform parity: callers get a thrown xhr.open(...) on macOS/iOS instead of the deferred status=0 + loadend path the rest of the platforms (UWP catches winrt::hresult_error and retains status 0, Win32 returns task_from_result<>(), the same is true for the Apple SendAsync path with m_url == nil).

Suggested fix: leave m_url = nil (or set a sentinel) in Open instead of throwing, then SendAsync's existing if (m_url == nil) branch handles the rest.

Happy to send PRs to UrlLib for both. cc @bghgary -- want to confirm desired semantics (status=0 + loadend to match the rest, or complete_exception so the new has_error() branch fires?) before I write them.

[bghgary]

[Responded by Copilot on behalf of @bghgary]

want to confirm desired semantics (status=0 + loadend to match the rest, or complete_exception so the new has_error() branch fires?) before I write them.

Thanks for the investigation — I closed JsRuntimeHost#167 since its premise was wrong; your analysis is accurate.

Preference: status=0 + loadend for both fixes.

• It's already the convention in every UrlLib path that handles local-failure correctly today (Win32 catch(winrt::hresult_error), UWP same, Apple's m_url==nil SendAsync branch). The Unix and Apple bugs are each platform not following that convention; fixing them to match is the smaller change.
• With Fix XMLHttpRequest::Send swallowing async failure #165 in place, JS sees the same error event either way (the new continuation fires error for both has_error() and status<200), so this only matters for direct C++ callers.
• For those direct callers, status=0+loadend keeps the contract uniform with HTTP failures — one check (status code) covers all failure modes. complete_exception would split the contract and force consumers to handle a wildly platform-dependent exception_ptr.

Per-platform error-message detail ("Failed to load file 'X'", "No file exists at local path") drops from the C++ surface — fine for our usage; platform-specific logging can capture detail if needed.

Go ahead with both PRs whenever you're ready.

[bkaradzic-microsoft]

UrlLib PR with both fixes is up: BabylonJS/UrlLib#28 (status=0 + loadend semantics, per your preference).

Once that lands too, I can re-add the app:/// missing-file regression test that originally surfaced these UrlLib bugs -- as a follow-up to this PR (or in the BN PR #1688 commit that bumps the JsRuntimeHost GIT_TAG).