Fix a few issues in Client

Most significantly, if removing from the multi fails, the connection
is added to a "dirty" list for the removal to be retried later. Looking at
the curl source code, remove fails on a recursive call, and we've struggled with
recursive calls before, so I _think_ this might be happening (it fails in other
cases, but I suspect if it _is_ happening, it's for this reason). The retry
happens _after_ `perform`, so it cannot fail for due to recursiveness. If it
fails at this point, we @panic. This is harsh, but it isn't easily recoverable
and before putting effort into it, I'd like to know that it's actually happening.

Fix potential use of undefined when a 401-407 request is received, but no
'WWW-Authenticate' or 'Proxy-Authenticate' header is received.

Don't call `curl_multi_remove_handle` on an easy that hasn't been added yet do
to error. Specifically, if `makeRequest` fails during setup, transfer_conn is
nulled so that `transfer.deinit()` doesn't try to remove the connection. And the
conn is removed from the `in_use` queue and made `available` again.

On Abort, if getting the private fails (extremely unlikely), we now still try
to remove the connection from the multi.

Added a few more fields to the famous "ScriptManager.Header recall" assertion.

src/Net.zig

@@ -174,16 +174,16 @@ const HeaderValue = struct {
 
 pub const AuthChallenge = struct {
     status: u16,
-    source: enum { server, proxy },
+    source: ?enum { server, proxy },
-    scheme: enum { basic, digest },
+    scheme: ?enum { basic, digest },
-    realm: []const u8,
+    realm: ?[]const u8,
 
     pub fn parse(status: u16, header: []const u8) !AuthChallenge {
         var ac: AuthChallenge = .{
             .status = status,
-            .source = undefined,
+            .source = null,
-            .realm = "TODO", // TODO parser and set realm
+            .realm = null,
-            .scheme = undefined,
+            .scheme = null,
         };
 
         const sep = std.mem.indexOfPos(u8, header, 0, ": ") orelse return error.InvalidHeader;
@@ -471,6 +471,7 @@ pub const Connection = struct {
 
 pub const Handles = struct {
     connections: []Connection,
+    dirty: HandleList,
     in_use: HandleList,
     available: HandleList,
     multi: *libcurl.CurlM,
@@ -501,6 +502,7 @@ pub const Handles = struct {
         }
 
         return .{
+            .dirty = .{},
             .in_use = .{},
             .connections = connections,
             .available = available,
@@ -522,8 +524,6 @@ pub const Handles = struct {
 
     pub fn get(self: *Handles) ?*Connection {
         if (self.available.popFirst()) |node| {
-            node.prev = null;
-            node.next = null;
             self.in_use.append(node);
             return @as(*Connection, @fieldParentPtr("node", node));
         }
@@ -535,21 +535,46 @@ pub const Handles = struct {
     }
 
     pub fn remove(self: *Handles, conn: *Connection) void {
-        libcurl.curl_multi_remove_handle(self.multi, conn.easy) catch |err| {
+        if (libcurl.curl_multi_remove_handle(self.multi, conn.easy)) {
-            log.fatal(.http, "multi remove handle", .{ .err = err });
+            self.isAvailable(conn);
-        };
+        } else |err| {
-        var node = &conn.node;
+            // can happen if we're in a perform() call, so we'll queue this
+            // for cleanup later.
+            const node = &conn.node;
+            self.in_use.remove(node);
+            self.dirty.append(node);
+            log.warn(.http, "multi remove handle", .{ .err = err });
+        }
+    }
+
+    pub fn isAvailable(self: *Handles, conn: *Connection) void {
+        const node = &conn.node;
         self.in_use.remove(node);
-        node.prev = null;
-        node.next = null;
         self.available.append(node);
     }
 
     pub fn perform(self: *Handles) !c_int {
-        var running: c_int = undefined;
         self.performing = true;
         defer self.performing = false;
+
+        const multi = self.multi;
+        var running: c_int = undefined;
         try libcurl.curl_multi_perform(self.multi, &running);
+
+        {
+            const list = &self.dirty;
+            while (list.first) |node| {
+                list.remove(node);
+                const conn: *Connection = @fieldParentPtr("node", node);
+                if (libcurl.curl_multi_remove_handle(multi, conn.easy)) {
+                    self.available.append(node);
+                } else |err| {
+                    log.fatal(.http, "multi remove handle", .{ .err = err, .src = "perform" });
+                    @panic("multi_remove_handle");
+                }
+            }
+        }
+
         return running;
     }
 

src/browser/ScriptManager.zig

@@ -634,6 +634,8 @@ pub const Script = struct {
     debug_transfer_notified_fail: bool = false,
     debug_transfer_redirecting: bool = false,
     debug_transfer_intercept_state: u8 = 0,
+    debug_transfer_auth_challenge: bool = false,
+    debug_transfer_easy_id: usize = 0,
 
     const Kind = enum {
         module,
@@ -711,6 +713,8 @@ pub const Script = struct {
                 .a5 = self.debug_transfer_notified_fail,
                 .a6 = self.debug_transfer_redirecting,
                 .a7 = self.debug_transfer_intercept_state,
+                .a8 = self.debug_transfer_auth_challenge,
+                .a9 = self.debug_transfer_easy_id,
                 .b1 = transfer.id,
                 .b2 = transfer._tries,
                 .b3 = transfer.aborted,
@@ -718,6 +722,8 @@ pub const Script = struct {
                 .b5 = transfer._notified_fail,
                 .b6 = transfer._redirecting,
                 .b7 = @intFromEnum(transfer._intercept_state),
+                .b8 = transfer._auth_challenge != null,
+                .b9 = if (transfer._conn) |c| @intFromPtr(c.easy) else 0,
             });
             self.header_callback_called = true;
             self.debug_transfer_id = transfer.id;
@@ -727,6 +733,8 @@ pub const Script = struct {
             self.debug_transfer_notified_fail = transfer._notified_fail;
             self.debug_transfer_redirecting = transfer._redirecting;
             self.debug_transfer_intercept_state = @intFromEnum(transfer._intercept_state);
+            self.debug_transfer_auth_challenge = transfer._auth_challenge != null;
+            self.debug_transfer_easy_id = if (transfer._conn) |c| @intFromPtr(c.easy) else 0;
         }
 
         lp.assert(self.source.remote.capacity == 0, "ScriptManager.Header buffer", .{ .capacity = self.source.remote.capacity });

src/cdp/domains/fetch.zig

@@ -406,10 +406,10 @@ pub fn requestAuthRequired(bc: anytype, intercept: *const Notification.RequestAu
             .fetch => "Fetch",
         },
         .authChallenge = .{
-            .source = if (challenge.source == .server) "Server" else "Proxy",
             .origin = "", // TODO get origin, could be the proxy address for example.
-            .scheme = if (challenge.scheme == .digest) "digest" else "basic",
+            .source = if (challenge.source) |s| (if (s == .server) "Server" else "Proxy") else "",
-            .realm = challenge.realm,
+            .scheme = if (challenge.scheme) |s| (if (s == .digest) "digest" else "basic") else "",
+            .realm = challenge.realm orelse "",
         },
         .networkId = &id.toRequestId(transfer.id),
     }, .{ .session_id = session_id });

src/http/Client.zig

@@ -66,7 +66,7 @@ active: usize,
 intercepted: usize,
 
 // Our easy handles, managed by a curl multi.
-handles: Handles,
+handles: Net.Handles,
 
 // Use to generate the next request ID
 next_request_id: u32 = 0,
@@ -128,7 +128,7 @@ pub fn init(allocator: Allocator, ca_blob: ?Net.Blob, robot_store: *RobotStore,
     const client = try allocator.create(Client);
     errdefer allocator.destroy(client);
 
-    var handles = try Handles.init(allocator, ca_blob, config);
+    var handles = try Net.Handles.init(allocator, ca_blob, config);
     errdefer handles.deinit(allocator);
 
     // Set transfer callbacks on each connection.
@@ -191,6 +191,8 @@ fn _abort(self: *Client, comptime abort_all: bool, frame_id: u32) void {
             n = node.next;
             const conn: *Net.Connection = @fieldParentPtr("node", node);
             var transfer = Transfer.fromConnection(conn) catch |err| {
+                // Let's cleanup what we can
+                self.handles.remove(conn);
                 log.err(.http, "get private info", .{ .err = err, .source = "abort" });
                 continue;
             };
@@ -665,7 +667,7 @@ pub fn restoreOriginalProxy(self: *Client) !void {
 }
 
 // Enable TLS verification on all connections.
-pub fn enableTlsVerify(self: *const Client) !void {
+pub fn enableTlsVerify(self: *Client) !void {
     // Remove inflight connections check on enable TLS b/c chromiumoxide calls
     // the command during navigate and Curl seems to accept it...
 
@@ -675,7 +677,7 @@ pub fn enableTlsVerify(self: *const Client) !void {
 }
 
 // Disable TLS verification on all connections.
-pub fn disableTlsVerify(self: *const Client) !void {
+pub fn disableTlsVerify(self: *Client) !void {
     // Remove inflight connections check on disable TLS b/c chromiumoxide calls
     // the command during navigate and Curl seems to accept it...
 
@@ -689,7 +691,11 @@ fn makeRequest(self: *Client, conn: *Net.Connection, transfer: *Transfer) anyerr
 
     {
         transfer._conn = conn;
-        errdefer transfer.deinit();
+        errdefer {
+            transfer._conn = null;
+            transfer.deinit();
+            self.handles.isAvailable(conn);
+        }
 
         try conn.setURL(req.url);
         try conn.setMethod(req.method);
@@ -716,17 +722,20 @@ fn makeRequest(self: *Client, conn: *Net.Connection, transfer: *Transfer) anyerr
         }
     }
 
-    // Once soon as this is called, our "perform" loop is responsible for
+    // As soon as this is called, our "perform" loop is responsible for
     // cleaning things up. That's why the above code is in a block. If anything
-    // fails BEFORE `curl_multi_add_handle` suceeds, the we still need to do
+    // fails BEFORE `curl_multi_add_handle` succeeds, the we still need to do
     // cleanup. But if things fail after `curl_multi_add_handle`, we expect
     // perfom to pickup the failure and cleanup.
-    try self.handles.add(conn);
+    self.handles.add(conn) catch |err| {
+        transfer._conn = null;
+        transfer.deinit();
+        self.handles.isAvailable(conn);
+        return err;
+    };
 
     if (req.start_callback) |cb| {
         cb(transfer) catch |err| {
-            self.handles.remove(conn);
-            transfer._conn = null;
             transfer.deinit();
             return err;
         };
@@ -834,7 +843,7 @@ fn processMessages(self: *Client) !bool {
                 // 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 });
+                    log.err(.http, "header_done_callback2", .{ .err = err });
                     requestFailed(transfer, err, true);
                     continue;
                 };
@@ -872,8 +881,6 @@ fn ensureNoActiveConnection(self: *const Client) !void {
     }
 }
 
-const Handles = Net.Handles;
-
 pub const RequestCookie = struct {
     is_http: bool,
     jar: *CookieJar,
@@ -1300,9 +1307,9 @@ pub const Transfer = struct {
                 // WWW-Authenticate or Proxy-Authenticate header.
                 transfer._auth_challenge = .{
                     .status = status,
-                    .source = undefined,
+                    .source = null,
-                    .scheme = undefined,
+                    .scheme = null,
-                    .realm = undefined,
+                    .realm = null,
                 };
                 return buf_len;
             }