From 8d3b9674a8fc4fb2f385c8f366f6dcb361e8985a Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 23 Jan 2026 19:58:14 +0800 Subject: [PATCH] pumpMessageLoop before inspector deinit Inspector has weak references to v8::Context, if we deinit the inspector and then shutdown contexts, we risk getting a use-after-free as those weak reference callback their finalizers on an inspector which no longer exists. To some degree, this means the Inspector doesn't clean up its weak references on shutdown. I assume this is because it expects us to always clean them up with destroyContext. This also keeps the inspector around until AFTER the page is killed (thus it's still alive to get the finalizer callback) and moves it off the session arena onto the browser context itself. --- src/browser/js/Env.zig | 4 +--- src/cdp/cdp.zig | 38 +++++++++++++++++++------------------- src/cdp/domains/page.zig | 4 ++-- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/browser/js/Env.zig b/src/browser/js/Env.zig index 78fde66a..87605ec9 100644 --- a/src/browser/js/Env.zig +++ b/src/browser/js/Env.zig @@ -160,10 +160,8 @@ pub fn deinit(self: *Env) void { self.allocator.destroy(self.isolate_params); } -pub fn newInspector(self: *Env, arena: Allocator, ctx: anytype) !*Inspector { - const inspector = try arena.create(Inspector); +pub fn newInspector(self: *Env, inspector: *Inspector, ctx: anytype) !void { try Inspector.init(inspector, self.isolate.handle, ctx); - return inspector; } pub fn runMicrotasks(self: *const Env) void { diff --git a/src/cdp/cdp.zig b/src/cdp/cdp.zig index 77378d58..1b71343d 100644 --- a/src/cdp/cdp.zig +++ b/src/cdp/cdp.zig @@ -352,7 +352,7 @@ pub fn BrowserContext(comptime CDP_T: type) type { node_registry: Node.Registry, node_search_list: Node.Search.List, - inspector: *js.Inspector, + inspector: js.Inspector, isolated_worlds: std.ArrayListUnmanaged(IsolatedWorld), http_proxy_changed: bool = false, @@ -376,13 +376,12 @@ pub fn BrowserContext(comptime CDP_T: type) type { const Self = @This(); fn init(self: *Self, id: []const u8, cdp: *CDP_T) !void { + const browser = &cdp.browser; const allocator = cdp.allocator; - const session = try cdp.browser.newSession(); + const session = try browser.newSession(); const arena = session.arena; - const inspector = try cdp.browser.env.newInspector(arena, self); - var registry = Node.Registry.init(allocator); errdefer registry.deinit(); @@ -400,19 +399,21 @@ pub fn BrowserContext(comptime CDP_T: type) type { .node_registry = registry, .node_search_list = undefined, .isolated_worlds = .empty, - .inspector = inspector, + .inspector = undefined, .notification_arena = cdp.notification_arena.allocator(), .intercept_state = try InterceptState.init(allocator), .captured_responses = .empty, .log_interceptor = LogInterceptor(Self).init(allocator, self), }; + try browser.env.newInspector(&self.inspector, self); + self.node_search_list = Node.Search.List.init(allocator, &self.node_registry); errdefer self.deinit(); - try cdp.browser.notification.register(.page_remove, self, onPageRemove); - try cdp.browser.notification.register(.page_created, self, onPageCreated); - try cdp.browser.notification.register(.page_navigate, self, onPageNavigate); - try cdp.browser.notification.register(.page_navigated, self, onPageNavigated); + try browser.notification.register(.page_remove, self, onPageRemove); + try browser.notification.register(.page_created, self, onPageCreated); + try browser.notification.register(.page_navigate, self, onPageNavigate); + try browser.notification.register(.page_navigated, self, onPageNavigated); } pub fn deinit(self: *Self) void { @@ -422,9 +423,10 @@ pub fn BrowserContext(comptime CDP_T: type) type { // Drain microtasks makes sure we don't have inspector's callback // in progress before deinit. - self.cdp.browser.env.runMicrotasks(); + const browser = &self.cdp.browser; + browser.env.runMicrotasks(); + - self.inspector.deinit(); // abort all intercepted requests before closing the sesion/page // since some of these might callback into the page/scriptmanager @@ -440,16 +442,19 @@ pub fn BrowserContext(comptime CDP_T: type) type { // If the session has a page, we need to clear it first. The page // context is always nested inside of the isolated world context, // so we need to shutdown the page one first. - self.cdp.browser.closeSession(); + browser.closeSession(); + + _ = browser.env.pumpMessageLoop(); + self.inspector.deinit(); self.node_registry.deinit(); self.node_search_list.deinit(); - self.cdp.browser.notification.unregisterAll(self); + browser.notification.unregisterAll(self); if (self.http_proxy_changed) { // has to be called after browser.closeSession, since it won't // work if there are active connections. - self.cdp.browser.http_client.restoreOriginalProxy() catch |err| { + browser.http_client.restoreOriginalProxy() catch |err| { log.warn(.http, "restoreOriginalProxy", .{ .err = err }); }; } @@ -771,11 +776,6 @@ const IsolatedWorld = struct { false, ); } - - pub fn createContextAndLoadPolyfills(self: *IsolatedWorld, page: *Page) !void { - // We need to recreate the isolated world context - try self.createContext(page); - } }; // This is a generic because when we send a result we have two different diff --git a/src/cdp/domains/page.zig b/src/cdp/domains/page.zig index 0209ef67..de368821 100644 --- a/src/cdp/domains/page.zig +++ b/src/cdp/domains/page.zig @@ -190,7 +190,7 @@ fn createIsolatedWorld(cmd: anytype) !void { const world = try bc.createIsolatedWorld(params.worldName, params.grantUniveralAccess); const page = bc.session.currentPage() orelse return error.PageNotLoaded; - try world.createContextAndLoadPolyfills(page); + try world.createContext(page); const js_context = &world.executor.context.?; // Create the auxdata json for the contextCreated event @@ -292,7 +292,7 @@ pub fn pageRemove(bc: anytype) !void { pub fn pageCreated(bc: anytype, page: *Page) !void { for (bc.isolated_worlds.items) |*isolated_world| { - try isolated_world.createContextAndLoadPolyfills(page); + try isolated_world.createContext(page); } }