mirror of
https://github.com/lightpanda-io/browser.git
synced 2026-03-22 04:34:44 +00:00
Remove potential recursive abort call in curl
Curl doesn't like recursive calls. For example, you can't call
curl_multi_remove_handle from within a dataCallback.
This specifically means that, as-is, transfer.abort() calls aren't safe to be
called during a libcurl callback. Consider this code:
```
req.open('GET', 'http://127.0.0.1:9582/xhr');
req.onreadystatechange = (e) => {
req.abort();
}
req.send();
```
onreadystatechange is triggered by network events, i.e. it executes in libcurl
callback. Thus, the above code fails to truly "abort" the request with
`curl_multi_remove_handle` error, saying it's a recursive call.
To solve this, transfer.abort() now sets an `aborted = true` flag. Callbacks can
now use this flag to signal to libcurl to stop the transfer.
A test was added which reproduced this issue, but this comes from:
https://github.com/lightpanda-io/browser/issues/1527 which I wasn't able to
reliably reproduce. I did see it happen regularly, just not always. It seems
like this commit fixes that issue.
This commit is contained in:
@@ -222,3 +222,34 @@
|
|||||||
testing.expectEqual(XMLHttpRequest.UNSENT, req.readyState);
|
testing.expectEqual(XMLHttpRequest.UNSENT, req.readyState);
|
||||||
});
|
});
|
||||||
</script>
|
</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>
|
||||||
|
-->
|
||||||
|
|||||||
@@ -1092,6 +1092,8 @@ pub const Transfer = struct {
|
|||||||
// the headers, and the [encoded] body.
|
// the headers, and the [encoded] body.
|
||||||
bytes_received: usize = 0,
|
bytes_received: usize = 0,
|
||||||
|
|
||||||
|
aborted: bool = false,
|
||||||
|
|
||||||
max_response_size: ?usize = null,
|
max_response_size: ?usize = null,
|
||||||
|
|
||||||
// We'll store the response header here
|
// We'll store the response header here
|
||||||
@@ -1224,10 +1226,18 @@ pub const Transfer = struct {
|
|||||||
|
|
||||||
pub fn abort(self: *Transfer, err: anyerror) void {
|
pub fn abort(self: *Transfer, err: anyerror) void {
|
||||||
requestFailed(self, err, true);
|
requestFailed(self, err, true);
|
||||||
if (self._handle != null) {
|
if (self._handle == null) {
|
||||||
self.client.endTransfer(self);
|
self.deinit();
|
||||||
|
return;
|
||||||
}
|
}
|
||||||
self.deinit();
|
|
||||||
|
// abort can be called from a libcurl callback, e.g. we get data, we
|
||||||
|
// do the header done callback, the client aborts.
|
||||||
|
// libcurl doesn't support re-entrant calls. We can't remove the
|
||||||
|
// handle from the multi during a callback. Instead, we flag this as
|
||||||
|
// aborted, which will signal the callbacks to stop processing the
|
||||||
|
// transfer
|
||||||
|
self.aborted = true;
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn terminate(self: *Transfer) void {
|
pub fn terminate(self: *Transfer) void {
|
||||||
@@ -1359,7 +1369,7 @@ pub const Transfer = struct {
|
|||||||
.transfer = transfer,
|
.transfer = transfer,
|
||||||
});
|
});
|
||||||
|
|
||||||
return proceed;
|
return proceed and transfer.aborted == false;
|
||||||
}
|
}
|
||||||
|
|
||||||
// headerCallback is called by curl on each request's header line read.
|
// headerCallback is called by curl on each request's header line read.
|
||||||
@@ -1519,6 +1529,10 @@ pub const Transfer = struct {
|
|||||||
.transfer = transfer,
|
.transfer = transfer,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
if (transfer.aborted) {
|
||||||
|
return -1;
|
||||||
|
}
|
||||||
|
|
||||||
return @intCast(chunk_len);
|
return @intCast(chunk_len);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user