Merge pull request #1689 from lightpanda-io/protect_xhr_abort_during_callback

Protect against transfer.abort() being called during callback

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,

src/browser/tests/net/xhr.html

@@ -252,3 +252,34 @@
     testing.expectEqual(XMLHttpRequest.UNSENT, req.readyState);
   });
 </script>
+
+
+<script id=xhr_abort_callback_nobody>
+  testing.async(async (restore) => {
+    const req = new XMLHttpRequest();
+    let abortFired = false;
+    let errorFired = false;
+    let loadEndFired = false;
+
+    await new Promise((resolve) => {
+      req.onabort = () => { abortFired = true; };
+      req.onerror = () => { errorFired = true; };
+      req.onloadend = () => {
+        loadEndFired = true;
+        resolve();
+      };
+
+      req.open('GET', 'http://127.0.0.1:9582/xhr_empty');
+      req.onreadystatechange = (e) => {
+        req.abort();
+      }
+      req.send();
+    });
+
+    restore();
+    testing.expectEqual(true, abortFired);
+    testing.expectEqual(true, errorFired);
+    testing.expectEqual(true, loadEndFired);
+    testing.expectEqual(XMLHttpRequest.UNSENT, req.readyState);
+  });
+</script>

src/http/Client.zig

@@ -496,8 +496,12 @@ 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);
+    // 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);
+        }
     }
 
     self.queue.append(&transfer._node);
@@ -789,9 +793,14 @@ 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.
+            // 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.
                 const proceed = transfer.headerDoneCallback(&msg.conn) catch |err| {
                     log.err(.http, "header_done_callback", .{ .err = err });
                     requestFailed(transfer, err, true);
@@ -937,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 = .{},
@@ -1041,13 +1051,9 @@ 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) {
+        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

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 = &.{