From 82e3f126ff38d4401bd806465c25fff724f1467e Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 2 Mar 2026 11:44:42 +0800 Subject: [PATCH 1/3] Protect against transfer.abort() being called during callback This was already handled in most cases, but not for a body-less response. It's safe to call transfer.abort() during a callback, so long as the performing flag is set to true. This was set during the normal libcurl callbacks, but for a body-less response, we manually invoke the header_done_callback and were not setting the performing flag. --- src/browser/tests/net/xhr.html | 31 +++++++++++++++++++++++++++ src/http/Client.zig | 39 +++++++++++++++++----------------- src/testing.zig | 8 +++++++ 3 files changed, 59 insertions(+), 19 deletions(-) diff --git a/src/browser/tests/net/xhr.html b/src/browser/tests/net/xhr.html index 7dc89a23..64fac5c3 100644 --- a/src/browser/tests/net/xhr.html +++ b/src/browser/tests/net/xhr.html @@ -252,3 +252,34 @@ testing.expectEqual(XMLHttpRequest.UNSENT, req.readyState); }); + + + diff --git a/src/http/Client.zig b/src/http/Client.zig index 1670883c..db3ec161 100644 --- a/src/http/Client.zig +++ b/src/http/Client.zig @@ -789,25 +789,30 @@ fn processMessages(self: *Client) !bool { if (msg.err) |err| { requestFailed(transfer, err, true); } else blk: { - // In case of request w/o data, we need to call the header done - // callback now. - if (!transfer._header_done_called) { - const proceed = transfer.headerDoneCallback(&msg.conn) catch |err| { - log.err(.http, "header_done_callback", .{ .err = err }); + { + self.handles.performing = true; + defer self.handles.performing = false; + + // In case of request w/o data, we need to call the header done + // callback now. + if (!transfer._header_done_called) { + const proceed = transfer.headerDoneCallback(&msg.conn) catch |err| { + log.err(.http, "header_done_callback", .{ .err = err }); + requestFailed(transfer, err, true); + continue; + }; + if (!proceed) { + requestFailed(transfer, error.Abort, true); + break :blk; + } + } + transfer.req.done_callback(transfer.ctx) catch |err| { + // transfer isn't valid at this point, don't use it. + log.err(.http, "done_callback", .{ .err = err }); requestFailed(transfer, err, true); continue; }; - if (!proceed) { - requestFailed(transfer, error.Abort, true); - break :blk; - } } - transfer.req.done_callback(transfer.ctx) catch |err| { - // transfer isn't valid at this point, don't use it. - log.err(.http, "done_callback", .{ .err = err }); - requestFailed(transfer, err, true); - continue; - }; transfer.req.notification.dispatch(.http_request_done, &.{ .transfer = transfer, @@ -1041,10 +1046,6 @@ pub const Transfer = struct { pub fn abort(self: *Transfer, err: anyerror) void { requestFailed(self, err, true); - if (self._conn == null) { - self.deinit(); - return; - } const client = self.client; if (client.handles.performing) { diff --git a/src/testing.zig b/src/testing.zig index 62ec8870..16b06a35 100644 --- a/src/testing.zig +++ b/src/testing.zig @@ -561,6 +561,14 @@ fn testHTTPHandler(req: *std.http.Server.Request) !void { }); } + if (std.mem.eql(u8, path, "/xhr_empty")) { + return req.respond("", .{ + .extra_headers = &.{ + .{ .name = "Content-Type", .value = "text/html; charset=utf-8" }, + }, + }); + } + if (std.mem.eql(u8, path, "/xhr/json")) { return req.respond("{\"over\":\"9000!!!\",\"updated_at\":1765867200000}", .{ .extra_headers = &.{ From b104c3bfe889355ffa070be31345fdd2c506fc19 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 2 Mar 2026 12:04:02 +0800 Subject: [PATCH 2/3] Don't start request during callback Fixes a separate but similar issue to https://github.com/lightpanda-io/browser/pull/1689 Specifically, it prevents starting a request from within a libcurl handler, thus avoiding an illegal recursive call. (This commit also removes the failed function call debug logging for DOMExceptions, as these aren't particularly abnormal / log-worthy) --- src/browser/js/Caller.zig | 12 ++++++++---- src/http/Client.zig | 6 ++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/browser/js/Caller.zig b/src/browser/js/Caller.zig index 6ab014c4..5f34a5b9 100644 --- a/src/browser/js/Caller.zig +++ b/src/browser/js/Caller.zig @@ -328,9 +328,13 @@ fn nameToString(local: *const Local, comptime T: type, name: *const v8.Name) !T fn handleError(comptime T: type, comptime F: type, local: *const Local, err: anyerror, info: anytype, comptime opts: CallOpts) void { const isolate = local.isolate; - if (comptime @import("builtin").mode == .Debug and @TypeOf(info) == FunctionCallbackInfo) { - if (log.enabled(.js, .warn)) { - logFunctionCallError(local, @typeName(T), @typeName(F), err, info); + if (comptime IS_DEBUG and @TypeOf(info) == FunctionCallbackInfo) { + if (log.enabled(.js, .debug)) { + const DOMException = @import("../webapi/DOMException.zig"); + if (DOMException.fromError(err) == null) { + // This isn't a DOMException, let's log it + logFunctionCallError(local, @typeName(T), @typeName(F), err, info); + } } } @@ -360,7 +364,7 @@ fn handleError(comptime T: type, comptime F: type, local: *const Local, err: any // this can add as much as 10 seconds of compilation time. fn logFunctionCallError(local: *const Local, type_name: []const u8, func: []const u8, err: anyerror, info: FunctionCallbackInfo) void { const args_dump = serializeFunctionArgs(local, info) catch "failed to serialize args"; - log.info(.js, "function call error", .{ + log.debug(.js, "function call error", .{ .type = type_name, .func = func, .err = err, diff --git a/src/http/Client.zig b/src/http/Client.zig index db3ec161..3d8ddc3c 100644 --- a/src/http/Client.zig +++ b/src/http/Client.zig @@ -496,8 +496,10 @@ fn waitForInterceptedResponse(self: *Client, transfer: *Transfer) !bool { // cases, the interecptor is expected to call resume to continue the transfer // or transfer.abort() to abort it. fn process(self: *Client, transfer: *Transfer) !void { - if (self.handles.get()) |conn| { - return self.makeRequest(conn, transfer); + if (self.handles.performing == false) { + if (self.handles.get()) |conn| { + return self.makeRequest(conn, transfer); + } } self.queue.append(&transfer._node); From 328c681a8fa3a7a8016cc1983a0cd0392b8f285c Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 2 Mar 2026 17:29:47 +0800 Subject: [PATCH 3/3] Add transfer-specific "performing" flag In the previous commits, two separte crash resolution conspired to introduce 1 tick delay in request handling. When we're in a libcurl perform, we can't re-enter libcurl. So we added a check before processing new requests to make sure we weren't "performing" and, if we were, we'd queue the request (hence the 1 tick delay). But for another issue, we set the same "performing" check when manually triggering callbacks. This extended the situations where the above check fired thus causing the 1-tick delay to happen under more (and even common) situation. This commit improves this - instead of relying on the global "performing" check when processing 1 transfer explicitly, we now have a per-transfer performing check. This prevents the transfer from being deinitialized during a callback but does not block requests from being started immediately. --- src/http/Client.zig | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/http/Client.zig b/src/http/Client.zig index 3d8ddc3c..6405c8ae 100644 --- a/src/http/Client.zig +++ b/src/http/Client.zig @@ -496,6 +496,8 @@ fn waitForInterceptedResponse(self: *Client, transfer: *Transfer) !bool { // cases, the interecptor is expected to call resume to continue the transfer // or transfer.abort() to abort it. fn process(self: *Client, transfer: *Transfer) !void { + // libcurl doesn't allow recursive calls, if we're in a `perform()` operation + // then we _have_ to queue this. if (self.handles.performing == false) { if (self.handles.get()) |conn| { return self.makeRequest(conn, transfer); @@ -791,30 +793,30 @@ fn processMessages(self: *Client) !bool { if (msg.err) |err| { requestFailed(transfer, err, true); } else blk: { - { - self.handles.performing = true; - defer self.handles.performing = false; + // make sure the transfer can't be immediately aborted from a callback + // since we still need it here. + transfer._performing = true; + defer transfer._performing = false; + if (!transfer._header_done_called) { // In case of request w/o data, we need to call the header done // callback now. - if (!transfer._header_done_called) { - const proceed = transfer.headerDoneCallback(&msg.conn) catch |err| { - log.err(.http, "header_done_callback", .{ .err = err }); - requestFailed(transfer, err, true); - continue; - }; - if (!proceed) { - requestFailed(transfer, error.Abort, true); - break :blk; - } - } - transfer.req.done_callback(transfer.ctx) catch |err| { - // transfer isn't valid at this point, don't use it. - log.err(.http, "done_callback", .{ .err = err }); + const proceed = transfer.headerDoneCallback(&msg.conn) catch |err| { + log.err(.http, "header_done_callback", .{ .err = err }); requestFailed(transfer, err, true); continue; }; + if (!proceed) { + requestFailed(transfer, error.Abort, true); + break :blk; + } } + transfer.req.done_callback(transfer.ctx) catch |err| { + // transfer isn't valid at this point, don't use it. + log.err(.http, "done_callback", .{ .err = err }); + requestFailed(transfer, err, true); + continue; + }; transfer.req.notification.dispatch(.http_request_done, &.{ .transfer = transfer, @@ -944,6 +946,7 @@ pub const Transfer = struct { // number of times the transfer has been tried. // incremented by reset func. _tries: u8 = 0, + _performing: bool = false, // for when a Transfer is queued in the client.queue _node: std.DoublyLinkedList.Node = .{}, @@ -1050,7 +1053,7 @@ pub const Transfer = struct { requestFailed(self, err, true); const client = self.client; - if (client.handles.performing) { + if (self._performing or client.handles.performing) { // We're currently in a curl_multi_perform. We cannot call endTransfer // as that calls curl_multi_remove_handle, and you can't do that // from a curl callback. Instead, we flag this transfer and all of