mirror/browser
1
0
Fork 0
mirror of https://github.com/lightpanda-io/browser.git synced 2026-06-11 01:25:53 -04:00
Code Issues Packages Projects Releases Wiki Activity

Improve / fix InterceptLayer.intercepted count tracking

Browse Source 
The intercept state is currently split and hard to keep consistent and even
just reason about. InterceptLayer keeps the `intercepted` count, but CDP's
`BrowserContext` has its intercepted lookup. This isn't a problem per se, but
you BrowserContext.deinit tries to decrement `InterceptLayer.intercepted` which
is only safe if we can guarantee that the two are in sync. Which we can't.

This commit simplifies the upkeep of `InterceptLayer.intercepted` and uses the
Transfer's state on unpark/deinit to decrement it. The CDP layer no longer
cares about / has to maintain the count.

Driven by this crash report:

BrowserContext.deinit.intercepted
---
value: 0
/home/runner/work/browser/browser/src/lightpanda.zig:279:25: 0x2871842 in deinit (lightpanda)
/home/runner/work/browser/browser/src/cdp/CDP.zig:127:18: 0x28c3f45 in deinit (lightpanda)
/home/runner/work/browser/browser/src/Server.zig:186:21: 0x2827997 in handleConnection (lightpanda)
/home/runner/work/_temp/6dc322a8-c74f-4990-9660-4cc6dcfb9352/zig-x86_64-linux-0.15.2/lib/std/Thread.zig:509:13: 0x269c233 in entryFn (lightpanda)
???:?:?: 0x7fce7ccabd57 in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7fce7ccabd57` was not available, trace may be incomplete

on 1.0.0-nightly.6542+94ba0791
This commit is contained in:
Karl Seguin
2026-06-03 08:24:47 +08:00
parent f61b110b1c
commit f7a32c05a8
4 changed files with 44 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
src/browser/HttpClient.zig
View File

@@ -1564,10 +1564,28 @@ pub const Transfer = struct {
// post-perform would need to be improved),
pub fn unpark(self: *Transfer) void {
lp.assert(self.state == .parked, "Transfer.unpark", .{ .state = self.state });
self.leaveIntercept();
self.state = .created;
}
// Decrement the interception counter iff this transfer is currently
// parked for CDP interception.
fn leaveIntercept(self: *Transfer) void {
if (self.state != .parked) {
return;
}
switch (self.state.parked) {
.robots => {},
.intercept_request, .intercept_auth => {
const intercept_layer = &self.client.interception_layer;
lp.assert(intercept_layer.intercepted > 0, "Transfer.leaveIntercept", .{ .value = intercept_layer.intercepted });
intercept_layer.intercepted -= 1;
},
}
}
pub fn deinit(self: *Transfer) void {
self.leaveIntercept();
if (self._conn) |c| {
self.client.removeConn(c);
self._conn = null;
@@ -1904,9 +1922,9 @@ pub const Transfer = struct {
log.debug(.http, "abort auth transfer", .{ .intercepted = self.client.interception_layer.intercepted });
}
self.client.interception_layer.intercepted -= 1;
// The transfer is still .parked(.intercept_auth)
// abort -> deinit -> leaveIntercept decrements the counter.
self.abort(error.AbortAuthChallenge);
return;
}
// headerDoneCallback is called once the headers have been read.
@@ -2067,13 +2085,16 @@ pub const Transfer = struct {
pub fn continueTransfer(self: *Client, transfer: *Transfer) !void {
if (comptime IS_DEBUG) {
lp.assert(self.interception_layer.intercepted > 0, "HttpClient.continueTransfer", .{ .value = self.interception_layer.intercepted });
log.debug(.http, "continue transfer", .{ .intercepted = self.interception_layer.intercepted });
}
self.interception_layer.intercepted -= 1;
transfer.unpark();
return self.process(transfer);
self.process(transfer) catch |err| {
if (transfer.state == .created) {
transfer.abort(err);
}
return err;
};
}
const Noop = struct {

11
src/cdp/CDP.zig
View File

@@ -618,18 +618,9 @@ pub const BrowserContext = struct {
// abort all intercepted requests before closing the session/page
// since some of these might callback into the page/scriptmanager.
// intercept_state stores ids — look each one up; if it's already
// gone (out-of-band destroy), there's nothing to abort, but the
// intercepted counter still needs decrementing because we
// incremented it on pause.
// intercept_state stores ids.
const http_client = &browser.http_client;
for (self.intercept_state.pendingIntercepts()) |transfer_id| {
lp.assert(
http_client.interception_layer.intercepted > 0,
"BrowserContext.deinit.intercepted",
.{ .value = http_client.interception_layer.intercepted },
);
http_client.interception_layer.intercepted -= 1;
if (http_client.findTransfer(transfer_id)) |transfer| {
transfer.abort(error.ClientDisconnect);
}

34
src/cdp/domains/fetch.zig
View File

@@ -198,6 +198,7 @@ pub fn requestIntercept(bc: *CDP.BrowserContext, intercept: *const Notification.
const transfer = intercept.transfer;
try bc.intercept_state.put(transfer.id);
errdefer _ = bc.intercept_state.remove(transfer.id);
try bc.cdp.sendEvent("Fetch.requestPaused", .{
.requestId = &id.toInterceptId(transfer.id),
@@ -332,23 +333,21 @@ fn continueWithAuth(cmd: *CDP.Command) !void {
return cmd.sendResult(null, .{});
}
// TODO: double-decrement of interception_layer.intercepted if
// continueTransfer fails: continueTransfer decrements unconditionally,
// and the errdefer below decrements again via abortAuthChallenge.
// Worse: if continueTransfer's failure path destroys the transfer
// (start_callback fail in makeRequest), this errdefer hits a freed
// transfer. Pre-existing; needs makeRequest failure-semantics cleanup.
errdefer transfer.abortAuthChallenge();
transfer.updateCredentials(try std.fmt.allocPrintSentinel(
transfer.arena,
"{s}:{s}",
.{
params.authChallengeResponse.username,
params.authChallengeResponse.password,
},
0,
));
{
// The transfer is still parked here; if building the credentials
// fails, release it. Scoped so the errdefer does NOT cover
// continueTransfer (which owns its failures).
errdefer transfer.abortAuthChallenge();
transfer.updateCredentials(try std.fmt.allocPrintSentinel(
transfer.arena,
"{s}:{s}",
.{
params.authChallengeResponse.username,
params.authChallengeResponse.password,
},
0,
));
}
try client.continueTransfer(transfer);
return cmd.sendResult(null, .{});
@@ -440,6 +439,7 @@ pub fn requestAuthRequired(bc: *CDP.BrowserContext, intercept: *const Notificati
const transfer = intercept.transfer;
try bc.intercept_state.put(transfer.id);
errdefer _ = bc.intercept_state.remove(transfer.id);
const request = &transfer.req;
const challenge = transfer._auth_challenge orelse return error.NullAuthChallenge;

3
src/network/layer/InterceptionLayer.zig
View File

@@ -194,7 +194,6 @@ pub fn continueRequest(self: *InterceptionLayer, transfer: *Transfer) anyerror!v
lp.assert(self.intercepted > 0, "InterceptionLayer.continueRequest", .{ .value = self.intercepted });
log.debug(.http, "continue transfer", .{ .intercepted = self.intercepted });
}
self.intercepted -= 1;
// Resume the layer chain. Ownership is re-handed to whichever subsequent
// layer commits the transfer (queue, multi, or another park). If the
@@ -214,7 +213,6 @@ pub fn abortRequest(self: *InterceptionLayer, transfer: *Transfer) void {
lp.assert(self.intercepted > 0, "InterceptionLayer.abortRequest", .{ .value = self.intercepted });
log.debug(.http, "abort transfer", .{ .intercepted = self.intercepted });
}
self.intercepted -= 1;
transfer.abort(error.Abort);
}
@@ -229,7 +227,6 @@ pub fn fulfillRequest(
lp.assert(self.intercepted > 0, "InterceptionLayer.fulfillRequest", .{ .value = self.intercepted });
log.debug(.http, "fulfill transfer", .{ .intercepted = self.intercepted });
}
self.intercepted -= 1;
// `done` flips true once we've called the user's done_callback. If
// done_callback itself throws, the user already saw their end-of-flow