public/browser
1
0
Fork 0
mirror of https://github.com/lightpanda-io/browser.git synced 2026-02-04 06:23:45 +00:00
Code Issues Actions 5 Packages Projects Releases Wiki Activity

pumpMessageLoop before inspector deinit

Browse Source 
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.
This commit is contained in:
Karl Seguin
2026-01-23 19:58:14 +08:00
parent d1e7f46994
commit 8d3b9674a8
3 changed files with 22 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
src/browser/js/Env.zig
View File

@@ -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 {

38
src/cdp/cdp.zig
View File

@@ -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

4
src/cdp/domains/page.zig
View File

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