diff --git a/src/browser/Browser.zig b/src/browser/Browser.zig index cf14fb8f..1c6177ee 100644 --- a/src/browser/Browser.zig +++ b/src/browser/Browser.zig @@ -18,18 +18,17 @@ const std = @import("std"); -const Allocator = std.mem.Allocator; +const App = @import("../App.zig"); +const CDP = @import("../cdp/CDP.zig"); +const Notification = @import("../Notification.zig"); const js = @import("js/js.zig"); -const App = @import("../App.zig"); +const Page = @import("Page.zig"); +const Session = @import("Session.zig"); const HttpClient = @import("HttpClient.zig"); -const CDP = @import("../cdp/CDP.zig"); const ArenaPool = App.ArenaPool; - -const Session = @import("Session.zig"); -const Page = @import("Page.zig"); -const Notification = @import("../Notification.zig"); +const Allocator = std.mem.Allocator; // Browser is an instance of the browser. // You can create multiple browser instances. @@ -46,6 +45,14 @@ http_client: HttpClient, // used by sessions to allocate pages. page_pool: std.heap.MemoryPool(Page), +// Pool for FinalizerCallback.Identity structs — the records V8 weak-callback +// parameters point at. Scoped to the Browser (i.e. the V8 Isolate's lifetime) +// rather than the Session: V8 can run a weak finalizer arbitrarily late, any +// time up until the Isolate is torn down, so these must outlive every Session. +// Freed in deinit *after* env.deinit() tears down the Isolate — the point past +// which no finalizer can fire. +fc_identity_pool: std.heap.MemoryPool(js.FinalizerCallback.Identity), + // Monotonic frame-ID generator scoped to this Browser (one per CDP // connection). Lives here, not on Session, because CDP target IDs // (encoded as `FID-{d:0>10}`) must be unique for the lifetime of the @@ -84,6 +91,7 @@ pub fn init(self: *Browser, app: *App, opts: InitOpts, cdp: ?*CDP) !void { .arena_pool = &app.arena_pool, .http_client = undefined, .page_pool = std.heap.MemoryPool(Page).init(allocator), + .fc_identity_pool = .init(allocator), }; try self.http_client.init(allocator, &app.network, cdp); } @@ -91,6 +99,9 @@ pub fn init(self: *Browser, app: *App, opts: InitOpts, cdp: ?*CDP) !void { pub fn deinit(self: *Browser) void { self.closeSession(); self.env.deinit(); + // After env.deinit() the Isolate is gone, so no further weak finalizer can + // fire — only now is it safe to free the pool backing their parameters. + self.fc_identity_pool.deinit(); self.page_pool.deinit(); self.http_client.deinit(); } diff --git a/src/browser/Page.zig b/src/browser/Page.zig index f7d0bebc..9a3f764d 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -75,9 +75,9 @@ identity: js.Identity = .{}, // Finalizer callbacks for Zig instances exposed to v8 in this Page. Keyed by // Zig instance ptr. The backing FinalizerCallback.Identity structs come from -// Session.fc_identity_pool so they outlive the Page for v8 weak-callback -// safety. -finalizer_callbacks: std.AutoHashMapUnmanaged(usize, *Session.FinalizerCallback) = .empty, +// Browser.fc_identity_pool so they outlive the Page (and the Session) for v8 +// weak-callback safety. +finalizer_callbacks: std.AutoHashMapUnmanaged(usize, *js.FinalizerCallback) = .empty, // Tracked global v8 objects that need to be released when the Page tears down. globals: std.ArrayList(v8.Global) = .empty, diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 5f9ede3e..4bd4259f 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -63,10 +63,6 @@ inject_scripts: []const []const u8 = &.{}, // Shared allocator. Used by Session itself and borrowed by Pages. arena_pool: *ArenaPool, -// Pool for FinalizerCallback.Identity structs. These must survive Page -// teardowns so V8 weak callbacks can validate the FC before dereferencing it. -fc_identity_pool: std.heap.MemoryPool(FinalizerCallback.Identity), - // The currently-active Page // flips this pointer. _active: ?*Page = null, @@ -116,7 +112,6 @@ pub fn init(self: *Session, browser: *Browser, notification: *Notification) !voi .storage_shed = .{}, .browser = browser, .notification = notification, - .fc_identity_pool = .init(allocator), .cookie_jar = storage.Cookie.Jar.init(allocator), // CLI defaults; LP.configureLoading can flip these per-session. .subframe_loading_enabled = !browser.app.config.disableSubframes(), @@ -136,13 +131,6 @@ pub fn deinit(self: *Session) void { self.cookie_jar.deinit(); - // Force V8 to flush any remaining weak callbacks while - // fc_identity_pool is still alive. Identity structs allocated from - // this pool back V8 weak-callback parameters; freeing the pool first - // would leave dangling pointers that segfault on the next GC. - self.browser.env.memoryPressureNotification(.critical); - self.fc_identity_pool.deinit(); - self.storage_shed.deinit(self.browser.app.allocator); self.arena_pool.release(self.arena); } @@ -647,49 +635,3 @@ pub fn nextLoaderId(self: *Session) u32 { self.loader_id_gen = id; return id; } - -// Every finalizable instance of Zig gets 1 FinalizerCallback registered in the -// Page. This is to ensure that, if v8 doesn't finalize the value, we can -// release on Page teardown. -pub const FinalizerCallback = struct { - page: *Page, - arena: Allocator, - resolved_ptr_id: usize, - finalizer_ptr_id: usize, - release_ref: *const fn (ptr_id: usize, page: *Page) void, - - // Linked list of Identities referencing this FC. - identities: ?*Identity = null, - // Count of active identities (for knowing when to clean up FC). - identity_count: u8 = 0, - - // For every FinalizerCallback we'll have 1+ FinalizerCallback.Identity: one - // for every identity that gets the instance. In most cases, that'll be 1. - // Allocated from Session.fc_identity_pool so it survives Page teardowns and - // allows the weak callback to safely check the done flag. - pub const Identity = struct { - session: *Session, - // The Page that owns the FinalizerCallback this Identity references. - // Only safe to dereference when `done == false`. When done is true, - // the Page may have been torn down and this pointer is stale. - page: *Page, - identity: *js.Identity, - finalizer_ptr_id: usize, - resolved_ptr_id: usize, - next: ?*Identity = null, - done: bool = false, - }; - - // Called during Page teardown to force cleanup regardless of identities. - pub fn deinit(self: *FinalizerCallback, page: *Page) void { - // Mark all identities as done so stale V8 weak callbacks - // won't find the wrong FC if resolved_ptr_id is reused. - var id = self.identities; - while (id) |identity| { - identity.done = true; - id = identity.next; - } - self.release_ref(self.finalizer_ptr_id, page); - page.releaseArena(self.arena); - } -}; diff --git a/src/browser/js/Local.zig b/src/browser/js/Local.zig index e1ec6be3..900b0b59 100644 --- a/src/browser/js/Local.zig +++ b/src/browser/js/Local.zig @@ -18,10 +18,10 @@ const std = @import("std"); const lp = @import("lightpanda"); + const string = @import("../../string.zig"); const Page = @import("../Page.zig"); -const FinalizerCallback = @import("../Session.zig").FinalizerCallback; const js = @import("js.zig"); const bridge = @import("bridge.zig"); @@ -33,6 +33,7 @@ const TaggedOpaque = @import("TaggedOpaque.zig"); const v8 = js.v8; const log = lp.log; const CallOpts = Caller.CallOpts; +const FinalizerCallback = js.FinalizerCallback; // Where js.Context has a lifetime tied to the frame, and holds the // v8::Global, this has a much shorter lifetime and holds a @@ -293,10 +294,11 @@ pub fn mapZigInstanceToJs(self: *const Local, js_obj_handle: ?*const v8.Object, finalizer_gop.value_ptr.* = try self.createFinalizerCallback(resolved_ptr_id, finalizer_ptr_id, finalizer.release_ref_from_zig); } const fc = finalizer_gop.value_ptr.*; - const identity_finalizer = try session.fc_identity_pool.create(); + const browser = session.browser; + const identity_finalizer = try browser.fc_identity_pool.create(); identity_finalizer.* = .{ + .browser = browser, .page = page, - .session = session, .identity = ctx.identity, .finalizer_ptr_id = finalizer_ptr_id, .resolved_ptr_id = resolved_ptr_id, @@ -1248,23 +1250,30 @@ fn resolveT(comptime T: type, value: *T) Resolved { const ptr = v8.v8__WeakCallbackInfo__GetParameter(handle.?).?; const identity_finalizer: *FinalizerCallback.Identity = @ptrCast(@alignCast(ptr)); - // Identity is allocated from pool, so it's valid even after frame reset. + // The Identity lives in browser.fc_identity_pool, which outlives + // the page and the session, so freeing ourselves is always safe. + // `browser` is the only field we may touch unconditionally. + defer identity_finalizer.browser.fc_identity_pool.destroy(identity_finalizer); + + // If done, the owning scope (page / isolated world) was already + // torn down: its identity.deinit() reset our Global and the FC + // was already released. The page, identity_map and finalizer_ptr_id + // are all stale (the Page may even be reused by a later Session), + // so we must not dereference them — just free ourselves (defer). + if (identity_finalizer.done) { + return; + } + const page = identity_finalizer.page; const resolved_ptr_id = identity_finalizer.resolved_ptr_id; - defer page.session.fc_identity_pool.destroy(identity_finalizer); - // Always clean up the identity map entry + // Clean up the identity map entry: the object is being collected, + // so our Global to it is dead. if (identity_finalizer.identity.identity_map.fetchRemove(resolved_ptr_id)) |kv| { var global = kv.value; v8.v8__Global__Reset(&global); } - // If done, FC was already cleaned up during Page teardown. - // The finalizer_ptr_id may have been reused for a new object, - // so we must not look it up in the map. It's also unsafe to - // dereference identity_finalizer.page after done is true. - if (identity_finalizer.done) return; - const finalizer_ptr_id = identity_finalizer.finalizer_ptr_id; const fc = page.finalizer_callbacks.get(finalizer_ptr_id) orelse return; diff --git a/src/browser/js/bridge.zig b/src/browser/js/bridge.zig index 5de1d1b1..4754f488 100644 --- a/src/browser/js/bridge.zig +++ b/src/browser/js/bridge.zig @@ -19,14 +19,14 @@ const std = @import("std"); const lp = @import("lightpanda"); -const js = @import("js.zig"); const Frame = @import("../Frame.zig"); -const v8 = js.v8; - +const js = @import("js.zig"); const Caller = @import("Caller.zig"); const Context = @import("Context.zig"); +const v8 = js.v8; +const Allocator = std.mem.Allocator; const IS_DEBUG = @import("builtin").mode == .Debug; pub fn Builder(comptime T: type) type { diff --git a/src/browser/js/js.zig b/src/browser/js/js.zig index adc36255..1d4140fe 100644 --- a/src/browser/js/js.zig +++ b/src/browser/js/js.zig @@ -50,6 +50,7 @@ pub const Integer = @import("Integer.zig"); pub const PromiseResolver = @import("PromiseResolver.zig"); pub const PromiseRejection = @import("PromiseRejection.zig"); +const js = @This(); const Allocator = std.mem.Allocator; pub fn Bridge(comptime T: type) type { @@ -359,3 +360,60 @@ test "TaggedAnyOpaque" { // If we grow this, fine, but it should be a conscious decision try std.testing.expectEqual(24, @sizeOf(@import("TaggedOpaque.zig"))); } + +// Every finalizable instance of Zig gets 1 FinalizerCallback registered in the +// Page. This is to ensure that, if v8 doesn't finalize the value, we can +// release on Page teardown. +pub const FinalizerCallback = struct { + page: *Page, + arena: Allocator, + resolved_ptr_id: usize, + finalizer_ptr_id: usize, + release_ref: *const fn (ptr_id: usize, page: *Page) void, + + // Linked list of Identities referencing this FC. + identities: ?*FinalizerCallback.Identity = null, + // Count of active identities (for knowing when to clean up FC). + identity_count: u8 = 0, + + const Page = @import("../Page.zig"); + const Browser = @import("../Browser.zig"); + + // For every FinalizerCallback we'll have 1+ FinalizerCallback.Identity: one + // for every identity that gets the instance. In most cases, that'll be 1. + // Allocated from Browser.fc_identity_pool so it survives Page *and* Session + // teardowns — V8 may fire the weak callback any time before the Isolate is + // torn down — and lets the callback safely check the done flag. + pub const Identity = struct { + // The Page that owns the FinalizerCallback this Identity references. + // Only safe to dereference when `done == false`. When done is true, + // the Page may have been torn down and this pointer is stale. + page: *Page, + + // Stable handle to the pool this struct came from. The weak callback + // reaches the pool through here (not via page/session) so it stays + // valid to self-destruct even when `done` and the page/session are gone. + browser: *Browser, + + // The world's identity map. Only safe to dereference when `done == false` + // (see `browser` above) — its teardown already reset every Global. + identity: *js.Identity, + finalizer_ptr_id: usize, + resolved_ptr_id: usize, + next: ?*FinalizerCallback.Identity = null, + done: bool = false, + }; + + // Called during Page teardown to force cleanup regardless of identities. + pub fn deinit(self: *FinalizerCallback, page: *Page) void { + // Mark all identities as done so stale V8 weak callbacks + // won't find the wrong FC if resolved_ptr_id is reused. + var id = self.identities; + while (id) |identity| { + identity.done = true; + id = identity.next; + } + self.release_ref(self.finalizer_ptr_id, page); + page.releaseArena(self.arena); + } +};