From a9cf87e0b0a695f8ab192ecc34bc9a10e62b7423 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Wed, 20 May 2026 15:02:48 +0800 Subject: [PATCH] Remove reentrency teardown protection This largely reverts 92607ad7650f19ab73a3b101239b6be35b10bf03 (captured in PR: https://github.com/lightpanda-io/browser/pull/2398). https://github.com/lightpanda-io/browser/pull/2495 introduces protection against execution arbitrary CDP command during JavaScript callbacks. Claude initially made the case for keeping the existing code as a safety net, but sycophanted when I pushed by. My reason for removing it is that it isn't a low-maintenance guard. It's a flag that serves a real purpose (ensuring 1 JS script is finished before executing another one), that has been expended to solve these issues. It needs to be set (and reverted) at every callsite that makes a blocking call, and it needs to be checked (recursively across all frames) in any place that can teardown the page/ frame. Claude called the allowlist "load-bearing in a non-obvious way", but I think it's purpose built specifically for this case. Extended the comment atop `allowDuringSyncWait` so that future-selves remember this. --- src/browser/Frame.zig | 18 ------------------ src/browser/HttpClient.zig | 5 +++++ src/browser/Session.zig | 12 +----------- src/browser/webapi/Worker.zig | 15 --------------- src/browser/webapi/WorkerGlobalScope.zig | 20 -------------------- src/cdp/CDP.zig | 10 ---------- src/cdp/Connection.zig | 12 ------------ src/network/WS.zig | 1 - 8 files changed, 6 insertions(+), 87 deletions(-) diff --git a/src/browser/Frame.zig b/src/browser/Frame.zig index 1f081ded..0d559d57 100644 --- a/src/browser/Frame.zig +++ b/src/browser/Frame.zig @@ -1256,24 +1256,6 @@ pub fn isGoingAway(self: *const Frame) bool { return parent.isGoingAway(); } -// True if this frame, any descendant frame, or any worker owned by any -// of those frames is currently inside script evaluation. Used as a -// reentrancy guard before tearing down a page from a CDP message that -// may have been drained while a Zig->JS->Zig stack (e.g. Worker -// importScripts -> syncRequest -> blocking_read) is mid-flight. -// Recursive over child frames so an evaluating subframe also defers -// parent teardown. -pub fn anyScriptEvaluating(self: *const Frame) bool { - if (self._script_manager.base.is_evaluating) return true; - for (self.workers.items) |worker| { - if (worker._worker_scope._script_manager.is_evaluating) return true; - } - for (self.child_frames.items) |child| { - if (child.anyScriptEvaluating()) return true; - } - return false; -} - pub fn scriptAddedCallback(self: *Frame, comptime from_parser: bool, script: *Element.Html.Script) !void { if (self.isGoingAway()) { // if we're planning on navigating to another frame, don't run this script diff --git a/src/browser/HttpClient.zig b/src/browser/HttpClient.zig index 3641511c..c16c499f 100644 --- a/src/browser/HttpClient.zig +++ b/src/browser/HttpClient.zig @@ -736,6 +736,11 @@ fn drainInbox(self: *Client, mode: DrainMode) !void { // transfer state via InterceptionLayer; they don't touch page / // session / V8 state). The check is exact on the parsed `method` // field — no substring matching against raw JSON. +// +// Every method listed here must be safe to dispatch with +// JS on the stack — meaning it must NO reach any other code +// path that frees Page/Session/Frame/Worker state the unwinding +// eval frame above us will dereference. fn allowDuringSyncWait(msg: *Inbox.Message) bool { return switch (msg.payload) { .ping, .close, .disconnect => true, diff --git a/src/browser/Session.zig b/src/browser/Session.zig index aa34370d..7f7420fa 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -236,18 +236,8 @@ pub fn createPage(self: *Session) !*Frame { } pub fn removePage(self: *Session) void { - const page = self._active orelse { + if (self._active == null) { lp.assert(false, "Session.removePage - page is null", .{}); - }; - - if (page.frame.anyScriptEvaluating()) { - // Reentrant teardown from a CDP message drained inside syncRequest; - // either the page's own script (frame ScriptManager.is_evaluating) - // or a Worker eval (Worker.loadInitialScript marks its - // _worker_scope._script_manager.is_evaluating). Tearing down here - // would free the arena/identity_map underneath the active eval. - // Session.deinit reclaims the page when the connection closes. - return; } // If a navigation is in flight, drop the pending Page first. Its diff --git a/src/browser/webapi/Worker.zig b/src/browser/webapi/Worker.zig index ec3d1473..4c96a0e4 100644 --- a/src/browser/webapi/Worker.zig +++ b/src/browser/webapi/Worker.zig @@ -207,21 +207,6 @@ fn loadInitialScript(self: *Worker, script: []const u8) !void { try_catch.init(&ls.local); defer try_catch.deinit(); - // Mark this worker's ScriptManager as evaluating for the lifetime of - // the eval. Worker scripts can call importScripts() which performs a - // synchronous HTTP request that pumps the CDP socket while waiting - // (HttpClient.syncRequest -> cdp.blocking_read). A CDP message such - // as Target.closeTarget arriving on that socket would otherwise tear - // down the page (Session.removePage -> Page.deinit -> Frame.deinit -> - // Worker.deinit) while this eval is mid-flight, freeing the worker's - // arena and identity_map underneath us. Session.removePage walks - // every frame's workers and bails out when any is_evaluating, so the - // teardown is deferred until the eval unwinds. - const sm = &self._worker_scope._script_manager; - const was_evaluating = sm.is_evaluating; - sm.is_evaluating = true; - defer sm.is_evaluating = was_evaluating; - _ = ls.local.eval(script, self._url) catch |err| { const caught = try_catch.caughtOrError(self._arena, err); log.err(.browser, "worker script error", .{ .url = self._url, .caught = caught }); diff --git a/src/browser/webapi/WorkerGlobalScope.zig b/src/browser/webapi/WorkerGlobalScope.zig index bbf92424..fd691c61 100644 --- a/src/browser/webapi/WorkerGlobalScope.zig +++ b/src/browser/webapi/WorkerGlobalScope.zig @@ -428,26 +428,6 @@ fn importScript(self: *WorkerGlobalScope, arena: Allocator, url: [:0]const u8) ! var headers = try http_client.newHeaders(); try self.headersForRequest(&headers); - // Mark the worker's ScriptManager as evaluating for the duration of - // the synchronous fetch. syncRequest pumps the CDP socket while - // waiting (HttpClient.syncRequest -> cdp.blocking_read). A CDP - // message such as Target.closeTarget arriving on that socket would - // otherwise tear down the page (Session.removePage -> Page.deinit -> - // Frame.deinit -> Worker.deinit) while we're mid-fetch, freeing the - // worker's arena and identity_map underneath us. Frame.anyScriptEvaluating - // walks every frame's workers, so the teardown is deferred until the - // outer call unwinds. - // - // The typical caller (Worker.loadInitialScript) already sets this - // around its own eval, so this is a defense-in-depth nesting: a worker - // script that calls importScripts() from a setTimeout callback or a - // microtask wouldn't have the outer guard, but would still be safe - // because of this one. - const sm = &self._script_manager; - const was_evaluating = sm.is_evaluating; - sm.is_evaluating = true; - defer sm.is_evaluating = was_evaluating; - const response = http_client.syncRequest(arena, .{ .url = resolved_url, .method = .GET, diff --git a/src/cdp/CDP.zig b/src/cdp/CDP.zig index c29c91f9..7adfa455 100644 --- a/src/cdp/CDP.zig +++ b/src/cdp/CDP.zig @@ -423,16 +423,6 @@ pub fn disposeBrowserContext(self: *CDP, browser_context_id: []const u8) bool { if (std.mem.eql(u8, bc.id, browser_context_id) == false) { return false; } - // Reentrant teardown from a CDP message drained inside HttpClient.syncRequest. - // Tearing down the browser context here would free Session/Page state - // that the unwinding script-eval frame above us is about to dereference - // (see Session.removePage's matching guard). Defer cleanup to - // CDP.deinit at connection close, by which time eval has unwound. - if (bc.session.currentPage()) |page| { - if (page.frame.anyScriptEvaluating()) { - return true; - } - } bc.deinit(); self.browser.closeSession(); self.browser_context = null; diff --git a/src/cdp/Connection.zig b/src/cdp/Connection.zig index 05811414..f42f2cf7 100644 --- a/src/cdp/Connection.zig +++ b/src/cdp/Connection.zig @@ -500,22 +500,10 @@ pub fn getAddress(self: *Connection) !std.net.Address { return address; } -pub fn sendClose(self: *Connection) void { - self.send(&WS.CLOSE_GOING_AWAY) catch {}; -} - pub fn shutdown(self: *Connection) void { posix.shutdown(self.socket, .recv) catch {}; } -pub fn setBlocking(self: *Connection, blocking: bool) !void { - if (blocking) { - _ = try posix.fcntl(self.socket, posix.F.SETFL, self.socket_flags & ~@as(u32, @bitCast(posix.O{ .NONBLOCK = true }))); - } else { - _ = try posix.fcntl(self.socket, posix.F.SETFL, self.socket_flags); - } -} - fn fillWebsocketHeader(buf: std.ArrayList(u8)) []const u8 { // can't use buf[0..10] here, because the header length // is variable. If it's just 2 bytes, for example, we need the diff --git a/src/network/WS.zig b/src/network/WS.zig index 097179a7..5678c804 100644 --- a/src/network/WS.zig +++ b/src/network/WS.zig @@ -26,7 +26,6 @@ pub const EMPTY_PONG = [_]u8{ 138, 0 }; // CLOSE, 2 length, code pub const CLOSE_NORMAL = [_]u8{ 136, 2, 3, 232 }; // code: 1000 -pub const CLOSE_GOING_AWAY = [_]u8{ 136, 2, 3, 233 }; // code: 1001 pub const CLOSE_TOO_BIG = [_]u8{ 136, 2, 3, 241 }; // 1009 pub const CLOSE_PROTOCOL_ERROR = [_]u8{ 136, 2, 3, 234 }; //code: 1002