fix: Fix double-release and other (edge-case) memory issues

Our `Arena` from the `ArenaPool` now tracks if it's already been released. On
a subsequent release, it panics then and there. Without this, the code will
almost certainly panic anyways, but it will panic in a seemingly unrelated
place. Hopefully this makes identifying future cases of this easier (since we'll
get the stack trace of the re-releaser).

Fix 3 separate memory issues, all edge cases.

1 - an XHR error handler that re-open/sends would incorrectly null the new
    transfer (maybe this isn't that odd, maybe it's a common retry-on-error).

2 - On a curl_easy_pause error (from WebSocket.zig) we now unqueue the just-
    queued message, because the error will errdefer the message arena to be
    cleaned up.

3 - ScriptManager now cleans up after itself on a failure prior to submit()
    being called.

src/Arena.zig

@@ -48,6 +48,10 @@ bucket: *ArenaPool.Bucket,
 // Only meaningful while this arena sits in its bucket's free list.
 next: ?*Arena,
 
+// Used to detect a double-release. Allows us to fail when it happens rather
+// than triggering a double-free which will fail in a seemingly unrelated way
+released: bool,
+
 // Bytes _arena holds from the backing allocator, maintained by the vtable
 // below. Changes only when the arena grows or frees a node, so this costs
 // O(log n) updates over an arena's life, not one per allocation.

src/ArenaPool.zig

@@ -146,6 +146,7 @@ fn _acquire(self: *ArenaPool, account: ?*Arena.Account, size_or_bucket: anytype,
     if (bucket.free_list) |entry| {
         bucket.free_list = entry.next;
         bucket.free_list_len -= 1;
+        entry.released = false;
         if (lp.IS_DEBUG) {
             entry.debug = debug;
             const gop = try self._leak_track.getOrPut(self.allocator, debug);
@@ -164,6 +165,7 @@ fn _acquire(self: *ArenaPool, account: ?*Arena.Account, size_or_bucket: anytype,
     const entry = try self.entry_pool.create(self.allocator);
     entry.* = .{
         .next = null,
+        .released = false,
         .pool = self,
         .bucket = bucket,
         .bytes = 0,
@@ -191,23 +193,35 @@ pub fn release(self: *ArenaPool, entry: *Arena) void {
     const arena = &entry._arena;
     const bucket = entry.bucket;
 
-    lp.metrics.arena_inflight.decr(bucket.size);
-
-    if (lp.IS_DEBUG) {
+    {
         self.mutex.lockUncancelable(lp.io);
         defer self.mutex.unlock(lp.io);
-        if (self._leak_track.getPtr(entry.debug)) |count| {
-            count.* -= 1;
-            if (count.* < 0) {
-                log.err(.bug, "ArenaPool double-free", .{ .name = entry.debug });
-                @panic("ArenaPool: double-free detected");
+
+        if (entry.released) {
+            // This arena was already released. It's better to crash here
+            // because it [hopefully] gives us the stack that re-released, else
+            // it'll crash in some random code.
+            lp.assert(false, "ArenaPool double release", .{
+                .bucket = @tagName(bucket.size),
+                .name = if (comptime lp.IS_DEBUG) entry.debug else "",
+            });
+        }
+        entry.released = true;
+
+        if (comptime lp.IS_DEBUG) {
+            if (self._leak_track.getPtr(entry.debug)) |count| {
+                // Can't go negative: the released check above already caught
+                // a double release of this entry.
+                count.* -= 1;
+            } else {
+                log.err(.bug, "ArenaPool release unknown", .{ .name = entry.debug });
+                @panic("ArenaPool: release of untracked arena");
             }
-        } else {
-            log.err(.bug, "ArenaPool release unknown", .{ .name = entry.debug });
-            @panic("ArenaPool: release of untracked arena");
         }
     }
 
+    lp.metrics.arena_inflight.decr(bucket.size);
+
     entry.unpin();
     _ = arena.reset(.{ .retain_with_limit = bucket.retain_bytes });
 

src/browser/ScriptManager.zig

@@ -392,27 +392,38 @@ pub fn addFromElement(self: *ScriptManager, comptime from_parser: bool, script_e
     self.base.is_evaluating = true;
     defer self.base.endEvaluationWindow(was_evaluating);
 
-    errdefer self.base.scriptList(script).remove(&script.node);
-    try frame.makeRequest(.{
-        .ctx = script,
-        .url = remote_url,
-        .method = .GET,
-        .frame_id = frame._frame_id,
-        .loader_id = frame._loader_id,
-        .cookie_jar = &frame._session.cookie_jar,
-        .cookie_origin = frame.url,
-        .resource_type = .script,
-        .notification = frame._session.notification,
-        .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null,
-        .header_callback = Script.headerCallback,
-        .data_callback = Script.dataCallback,
-        .done_callback = Script.doneCallback,
-        .error_callback = Script.errorCallback,
-        // Nothing holds the transfer; teardown cleanup runs through
-        // the manager's script lists.
-        .shutdown_callback = HttpClient.noopShutdown,
-    });
+    const transfer = blk: {
+        errdefer self.base.scriptList(script).remove(&script.node);
+        const transfer = try frame.newRequest(.{
+            .ctx = script,
+            .url = remote_url,
+            .method = .GET,
+            .frame_id = frame._frame_id,
+            .loader_id = frame._loader_id,
+            .cookie_jar = &frame._session.cookie_jar,
+            .cookie_origin = frame.url,
+            .resource_type = .script,
+            .notification = frame._session.notification,
+            .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null,
+            .header_callback = Script.headerCallback,
+            .data_callback = Script.dataCallback,
+            .done_callback = Script.doneCallback,
+            .error_callback = Script.errorCallback,
+            // Nothing holds the transfer; teardown cleanup runs through
+            // the manager's script lists.
+            .shutdown_callback = HttpClient.noopShutdown,
+        });
+        errdefer transfer.deinit();
+        try frame.headersForRequest(transfer);
+        break :blk transfer;
+    };
+
+    // Point of no return: submit() consumes the transfer, and on a synchronous
+    // failure fires Script.errorCallback, which removes the node and deinits
+    // the script (freeing our arena). Its error is already delivered there
+    // (same as Fetch), so there's nothing left for us to unwind.
     handover = true;
+    transfer.submit() catch {};
 }
 
 // A <script> with no src. Runs synchronously right now, except an inline
@@ -628,3 +639,34 @@ test "ScriptManager: waitForPreload stops when teardown is pending" {
 
     try testing.expect(sm.waitForPreload(url) == null);
 }
+
+// Production crash (release overflow on unrelated pooled objects): a
+// synchronous submit() failure fires Script.errorCallback, which deinits the
+// script and releases its arena, and then returns the error, so addFromElement's
+// errdefer released the same arena again — a pooled-arena double release.
+test "ScriptManager: async script whose submit fails synchronously releases its arena once" {
+    const page = try testing.pageTest("mcp_nav.html", .{});
+    defer page.close();
+
+    const frame = page.frame().?;
+    const client = frame._script_manager.base.client;
+    client.test_fail_submit = error.TestSubmitFailure;
+    defer client.test_fail_submit = null;
+
+    // Script.errorCallback logs the fetch error.
+    testing.expectLog(&.{.http});
+
+    var ls: js.Local.Scope = undefined;
+    frame.js.localScope(&ls);
+    defer ls.deinit();
+
+    // A dynamically inserted external script is async, the mode whose
+    // errorCallback tears the script down itself. On the unfixed code the
+    // second release trips ArenaPool.release's double-release assert from
+    // inside this eval — that assert is the test's real check.
+    try ls.local.eval(
+        \\const s = document.createElement('script');
+        \\s.src = 'http://127.0.0.1:9582/fails-at-submit.js';
+        \\document.head.appendChild(s);
+    , null);
+}

src/browser/tests/net/xhr.html

@@ -592,3 +592,53 @@
     });
   }
 </script>
+
+<!--
+Production crash (release overflow on unrelated pooled objects): a re-send from
+inside onerror stored a new transfer that httpErrorCallback then cleared once
+the handler returned. The re-send was orphaned, so the later abort() found an
+empty slot and never cancelled it, and the request ran on into a freed XHR.
+Observable here without the crash: the orphaned re-send is never aborted, so it
+delivers its response (onload) despite abort(). With the slot cleared before the
+handler runs, abort() cancels the re-send and no load is delivered.
+-->
+<script id=xhr_resend_in_onerror_then_abort type=module>
+  {
+    const state = await testing.async();
+    const req = new XMLHttpRequest();
+    let loadFired = false;
+    let abortFired = false;
+    let resent = false;
+
+    req.onload = () => { loadFired = true; };
+    req.onabort = () => { abortFired = true; };
+    req.onerror = () => {
+      if (resent) {
+        return;
+      }
+      resent = true;
+      // Retry inside the error handler, like a reconnecting client. The slow
+      // target keeps the retry in flight until the abort below.
+      req.open('GET', 'http://127.0.0.1:9582/xhr/slow');
+      req.send();
+      // After the handler has returned to httpErrorCallback.
+      setTimeout(() => {
+        req.abort();
+        // Long enough for the aborted-but-orphaned re-send to have delivered
+        // /xhr/slow (100ms) if abort() failed to cancel it.
+        setTimeout(() => state.resolve(), 250);
+      }, 0);
+    };
+
+    // Nothing listens on port 1: a network error, not an HTTP one.
+    req.open('GET', 'http://127.0.0.1:1/');
+    req.send();
+
+    await state.done(() => {
+      testing.expectEqual(true, abortFired);
+      // The aborted re-send must never deliver its response.
+      testing.expectEqual(false, loadFired);
+      testing.expectEqual(XMLHttpRequest.UNSENT, req.readyState);
+    });
+  }
+</script>

src/browser/webapi/net/WebSocket.zig

@@ -522,7 +522,12 @@ fn queueMessage(self: *WebSocket, msg: Message) !void {
     if (was_empty) {
         // Unpause the send callback so libcurl will request data
         if (self._conn) |conn| {
-            try conn.pause(.{ .cont = true });
+            conn.pause(.{ .cont = true }) catch |err| {
+                // our caller is doing `errdefer errdefer arena.release();` which
+                // will free msg. So we have to pop it out.
+                _ = self._send_queue.pop();
+                return err;
+            };
         }
     }
 }
@@ -1054,3 +1059,32 @@ test "WebApi: WebSocket" {
 test "WebApi: WebSocket in worker" {
     try testing.htmlRunner("net/websocket_worker.html", .{});
 }
+
+// Production crash (release overflow on unrelated pooled objects): send()
+// released the message arena on a failed unpause while the message stayed in
+// _send_queue, which released it again later — a pooled-arena double release.
+test "WebApi: WebSocket send owns its message arena once when the unpause fails" {
+    const frame = try testing.createFrame();
+    defer testing.test_session.closeAllPages();
+
+    var ls: js.Local.Scope = undefined;
+    frame.js.localScope(&ls);
+    defer ls.deinit();
+
+    var protocols: [0][]const u8 = .{};
+    const ws = try WebSocket.init("ws://127.0.0.1:9582/ws", &protocols, &frame.js.execution);
+    try testing.expect(ws._conn != null);
+
+    // connect() tracked the easy handle but no tick has performed it, so
+    // libcurl has no connection behind it and curl_easy_pause fails: the
+    // same state as a send() on a socket the peer already closed, before
+    // the close has been dispatched.
+    ws._ready_state = .open;
+
+    const message = try ls.local.exec("'hello'", null);
+    try testing.expectError(error.BadFunctionArgument, ws.send(.{ .js_val = message }));
+
+    // The queued message owns the arena. A failed send must not leave it
+    // queued with its arena already released.
+    try testing.expectEqual(0, ws._send_queue.items.len);
+}

src/browser/webapi/net/XMLHttpRequest.zig

@@ -601,11 +601,9 @@ fn httpDoneCallback(ctx: *anyopaque) !void {
 
 fn httpErrorCallback(ctx: *anyopaque, err: anyerror) void {
     const self: *XMLHttpRequest = @ptrCast(@alignCast(ctx));
-    // http client will close it after an error, it isn't safe to keep around
+    // handleError can execute JS, which could .send() again: clear this now.
+    self._http_transfer = null;
     self.handleError(err);
-    if (self._http_transfer != null) {
-        self._http_transfer = null;
-    }
     self.releaseSelfRef();
 }
 

src/network/HttpClient.zig

@@ -140,6 +140,11 @@ use_proxy: bool,
 // Current TLS verification state, applied per-connection in makeRequest.
 tls_verify: bool = true,
 
+// Test-only fault injection: makes the next submit() fail synchronously from
+// inside the pipeline, the shape where error_callback fires AND the error is
+// returned to the caller (see Transfer.submit).
+test_fail_submit: if (lp.IS_TEST) ?anyerror else void = if (lp.IS_TEST) null else {},
+
 // User agent override set via CDP Emulation.setUserAgentOverride.
 // When set, takes precedence over the config's http_headers value.
 // Allocated from self.allocator when set, null otherwise.
@@ -914,6 +919,12 @@ const SubmitFrom = enum { start, after_intercept, network };
 fn pipeline(self: *Client, transfer: *Transfer, from: SubmitFrom) !void {
     sw: switch (from) {
         .start => {
+            if (comptime lp.IS_TEST) {
+                if (self.test_fail_submit) |err| {
+                    return err;
+                }
+            }
+
             if (self.network.web_bot_auth) |wba| {
                 const authority = URL.getHost(transfer.req.url);
                 try wba.signRequest(transfer, authority);
@@ -3672,6 +3683,7 @@ fn initTestClient(client: *Client, pool: *ArenaPool) void {
         .single_flight = .init(testing.allocator),
     };
     client.url_blocklist = null;
+    client.test_fail_submit = null;
     // isUrlBlocked reaches through here for the adblocker; tests that want
     // one assign it to `client.network` after this returns.
     test_network.adblocker = null;

src/testing.zig

@@ -635,6 +635,16 @@ fn testHTTPHandler(req: *std.http.Server.Request) !void {
         });
     }
 
+    if (std.mem.eql(u8, path, "/xhr/slow")) {
+        // Long enough for a timer scheduled by the requester to fire first.
+        lp.io.sleep(.fromMilliseconds(100), .awake) catch {};
+        return req.respond("slow", .{
+            .extra_headers = &.{
+                .{ .name = "Content-Type", .value = "text/plain" },
+            },
+        });
+    }
+
     if (std.mem.eql(u8, path, "/xhr_empty")) {
         return req.respond("", .{
             .extra_headers = &.{