From f7a32c05a8603699372d681fc2e6f34508ebb008 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Wed, 3 Jun 2026 08:24:47 +0800 Subject: [PATCH] Improve / fix InterceptLayer.intercepted count tracking 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 --- src/browser/HttpClient.zig | 31 ++++++++++++++++++---- src/cdp/CDP.zig | 11 +------- src/cdp/domains/fetch.zig | 34 ++++++++++++------------- src/network/layer/InterceptionLayer.zig | 3 --- 4 files changed, 44 insertions(+), 35 deletions(-) diff --git a/src/browser/HttpClient.zig b/src/browser/HttpClient.zig index 874ed98b..e62e9f20 100644 --- a/src/browser/HttpClient.zig +++ b/src/browser/HttpClient.zig @@ -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 { diff --git a/src/cdp/CDP.zig b/src/cdp/CDP.zig index 57bef94c..05e66933 100644 --- a/src/cdp/CDP.zig +++ b/src/cdp/CDP.zig @@ -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); } diff --git a/src/cdp/domains/fetch.zig b/src/cdp/domains/fetch.zig index 0d8f2347..cc62921d 100644 --- a/src/cdp/domains/fetch.zig +++ b/src/cdp/domains/fetch.zig @@ -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; diff --git a/src/network/layer/InterceptionLayer.zig b/src/network/layer/InterceptionLayer.zig index 25c49ae9..9c391572 100644 --- a/src/network/layer/InterceptionLayer.zig +++ b/src/network/layer/InterceptionLayer.zig @@ -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