Merge pull request #2501 from lightpanda-io/remove_reentrency_teardown_protection

Remove reentrency teardown protection

src/browser/Frame.zig

@@ -1290,24 +1290,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

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,

src/browser/Session.zig

@@ -246,18 +246,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

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 });

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,

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;

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

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