Merge pull request #1528 from lightpanda-io/remove_recurise_curl_calls

Remove potential recursive abort call in curl

src/browser/tests/net/xhr.html

@@ -222,3 +222,33 @@
     testing.expectEqual(XMLHttpRequest.UNSENT, req.readyState);
   });
 </script>
+
+<script id=xhr_abort_callback>
+  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');
+      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

@@ -111,6 +111,10 @@ config: *const Config,
 
 cdp_client: ?CDPClient = null,
 
+// keep track of when curl_multi_perform is happening so that we can avoid
+// recursive calls into curl (which it will fail)
+performing: bool = false,
+
 // libcurl can monitor arbitrary sockets, this lets us use libcurl to poll
 // both HTTP data as well as messages from an CDP connection.
 // Furthermore, we have some tension between blocking scripts and request
@@ -735,7 +739,12 @@ pub const PerformStatus = enum {
 fn perform(self: *Client, timeout_ms: c_int) !PerformStatus {
     const multi = self.multi;
     var running: c_int = undefined;
+
+    {
+        self.performing = true;
+        defer self.performing = false;
         try errorMCheck(c.curl_multi_perform(multi, &running));
+    }
 
     // We're potentially going to block for a while until we get data. Process
     // whatever messages we have waiting ahead of time.
@@ -1092,6 +1101,8 @@ pub const Transfer = struct {
     // the headers, and the [encoded] body.
     bytes_received: usize = 0,
 
+    aborted: bool = false,
+
     max_response_size: ?usize = null,
 
     // We'll store the response header here
@@ -1224,10 +1235,27 @@ pub const Transfer = struct {
 
     pub fn abort(self: *Transfer, err: anyerror) void {
         requestFailed(self, err, true);
+        if (self._handle == null) {
+            self.deinit();
+            return;
+        }
+
+        const client = self.client;
+        if (client.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
+            // our callbacks will check for this flag and abort the transfer for
+            // us
+            self.aborted = true;
+            return;
+        }
+
         if (self._handle != null) {
-            self.client.endTransfer(self);
+            client.endTransfer(self);
         }
         self.deinit();
+
     }
 
     pub fn terminate(self: *Transfer) void {
@@ -1359,7 +1387,7 @@ pub const Transfer = struct {
             .transfer = transfer,
         });
 
-        return proceed;
+        return proceed and transfer.aborted == false;
     }
 
     // headerCallback is called by curl on each request's header line read.
@@ -1519,6 +1547,10 @@ pub const Transfer = struct {
             .transfer = transfer,
         });
 
+        if (transfer.aborted) {
+            return -1;
+        }
+
         return @intCast(chunk_len);
     }