diff --git a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp index 344169a8..4e11278f 100644 --- a/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp +++ b/Polyfills/XMLHttpRequest/Source/XMLHttpRequest.cpp @@ -57,6 +57,7 @@ namespace Babylon::Polyfills::Internal { constexpr const char* ReadyStateChange = "readystatechange"; constexpr const char* LoadEnd = "loadend"; + constexpr const char* Error = "error"; } } @@ -262,19 +263,24 @@ namespace Babylon::Polyfills::Internal .then(arcana::inline_scheduler, arcana::cancellation::none(), [sendRegion{std::move(sendRegion)}]() mutable { sendRegion.reset(); }) - .then(m_runtimeScheduler, arcana::cancellation::none(), [this]() { + .then(m_runtimeScheduler, arcana::cancellation::none(), [this](const arcana::expected& result) { + // Run on every outcome -- transport exception OR underlying request succeeded but ended in a non-2xx + // status (e.g. a missing local file on UWP, where UrlLib silently retains status 0). The previous + // success-only continuation here skipped readyState=Done / loadend / error and let the JS observer + // hang. + const auto statusCode = arcana::underlying_cast(m_request.StatusCode()); + const bool failed = result.has_error() || statusCode < 200 || statusCode >= 300; + SetReadyState(ReadyState::Done); + if (failed) + { + RaiseEvent(EventType::Error); + } 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(); - }) - .then(arcana::inline_scheduler, arcana::cancellation::none(), [env = info.Env()](arcana::expected result) { - if (result.has_error()) - { - Napi::Error::New(env, result.error()).ThrowAsJavaScriptException(); - } }); } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 764c838b..8f7e7295 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -121,6 +121,31 @@ describe("XMLHTTPRequest", function () { expect(xhr.status).to.equal(404); }); + it("should fire 'error' event for a remote URL that returns HTTP 404", async function () { + // Regression test: previously the success-only continuation in XMLHttpRequest::Send + // skipped 'error' on async failures including non-2xx HTTP responses, so onerror + // observers never ran. See https://github.com/BabylonJS/JsRuntimeHost/pull/165. + this.timeout(30000); + const result = await new Promise<{ errorFired: boolean; loadendFired: boolean; status: number; readyState: number }>((resolve, reject) => { + const xhr = new XMLHttpRequest(); + let errorFired = false; + let loadendFired = false; + const guard = setTimeout(() => reject(new Error("XHR neither errored nor loadended within 25s")), 25000); + xhr.addEventListener("error", () => { errorFired = true; }); + xhr.addEventListener("loadend", () => { + loadendFired = true; + clearTimeout(guard); + resolve({ errorFired, loadendFired, status: xhr.status, readyState: xhr.readyState }); + }); + xhr.open("GET", "https://github.com/babylonJS/BabylonNative404"); + xhr.send(); + }); + expect(result.status).to.equal(404); + expect(result.errorFired).to.equal(true); + expect(result.loadendFired).to.equal(true); + expect(result.readyState).to.equal(4); + }); + it("should throw something when opening //", async function () { function openDoubleSlash() { const xhr = new XMLHttpRequest();