From 77b60cebb087d9e912b8f1b90e5cde947b19b8b3 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Thu, 2 Apr 2026 17:04:33 +0800 Subject: [PATCH] Move finalizers to pure reference counting Takes https://github.com/lightpanda-io/browser/pull/2024 a step further and changes all reference counting to be explicit. Up until this point, finalizers_callback was seen as a fail-safe to make sure that instances were released no matter what. It exists because v8 might never call a finalizer, so we need to keep track of finalizables and finalize them on behalf of v8. BUT, it was used as more than a fallback for v8...it allowed us to be lazy and acquireRef's in Zig without a matching releaseRef (1), because why not, the finalizer_callback will handle it. This commit redefines finalizer_callbacks as strictly being a fallback for v8. If v8 calls the finalizer, then the finalizer callback is removed (2) - we lose our fail-safe. This means that every acquireRef must be matched with a releaseRef. Everything is explicit now. The most obvious impact of this is that on Page.deinit, we have to releaseRef every MO, IO and blob held by the page. This change removes a number of special-cases to deal with various ownership patterns. For example, Iterators are now properly reference counted and when their RC reaches 0, they can safely releaseRef on their list. This also elimites use-after-free potential when 2 RC objects reference each other. This should eliminate some WPT crashes (e.g. /editing/run/insertimage.html) (1) - We were only ever lazy about releaseRef during shutdown, so this change won't result in more aggressive collection. (2) Since 1 object can be referenced from 0-N IsolatedWorlds, it would be more accurate to say that the finalizer callback is removed when all referencing IsolatedWorld finalize it. --- src/browser/Page.zig | 28 +++++++++++ src/browser/Session.zig | 9 +++- src/browser/js/Local.zig | 52 ++++++++++++++------- src/browser/tests/node/child_nodes.html | 21 +++++---- src/browser/webapi/IntersectionObserver.zig | 34 +++++--------- src/browser/webapi/MutationObserver.zig | 24 +++++----- src/browser/webapi/collections/NodeList.zig | 7 ++- src/browser/webapi/collections/iterator.zig | 27 ++++++----- src/browser/webapi/net/XMLHttpRequest.zig | 17 ++++--- src/lightpanda.zig | 3 -- 10 files changed, 136 insertions(+), 86 deletions(-) diff --git a/src/browser/Page.zig b/src/browser/Page.zig index 3bf8962b..4a586be8 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -351,6 +351,30 @@ pub fn deinit(self: *Page, abort_http: bool) void { session.releaseArena(qn.arena); } + { + // Release all objects we're referencing + { + var it = self._blob_urls.valueIterator(); + while (it.next()) |blob| { + blob.*.releaseRef(session); + } + } + + { + var it: ?*std.DoublyLinkedList.Node = self._mutation_observers.first; + while (it) |node| : (it = node.next) { + const observer: *MutationObserver = @fieldParentPtr("node", node); + observer.releaseRef(session); + } + } + + for (self._intersection_observers.items) |observer| { + observer.releaseRef(session); + } + + self.window._document._selection.releaseRef(session); + } + session.browser.env.destroyContext(self.js); self._script_manager.shutdown = true; @@ -1338,20 +1362,24 @@ pub fn schedulePerformanceObserverDelivery(self: *Page) !void { } pub fn registerMutationObserver(self: *Page, observer: *MutationObserver) !void { + observer.acquireRef(); self._mutation_observers.append(&observer.node); } pub fn unregisterMutationObserver(self: *Page, observer: *MutationObserver) void { + observer.releaseRef(self._session); self._mutation_observers.remove(&observer.node); } pub fn registerIntersectionObserver(self: *Page, observer: *IntersectionObserver) !void { + observer.acquireRef(); try self._intersection_observers.append(self.arena, observer); } pub fn unregisterIntersectionObserver(self: *Page, observer: *IntersectionObserver) void { for (self._intersection_observers.items, 0..) |obs, i| { if (obs == observer) { + observer.releaseRef(self._session); _ = self._intersection_observers.swapRemove(i); return; } diff --git a/src/browser/Session.zig b/src/browser/Session.zig index c4e6d1ca..f063ecf7 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -501,7 +501,11 @@ pub const FinalizerCallback = struct { session: *Session, resolved_ptr_id: usize, finalizer_ptr_id: usize, - _deinit: *const fn (ptr_id: usize, session: *Session) void, + release_ref: *const fn (ptr_id: usize, session: *Session) void, + + // Track how many identities (JS worlds) reference this FC. + // Only cleanup when all identities have finalized. + 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'l be 1. @@ -510,8 +514,9 @@ pub const FinalizerCallback = struct { fc: *Session.FinalizerCallback, }; + // Called during page reset to force cleanup regardless of identity_count. fn deinit(self: *FinalizerCallback, session: *Session) void { - self._deinit(self.finalizer_ptr_id, session); + self.release_ref(self.finalizer_ptr_id, session); session.releaseArena(self.arena); } }; diff --git a/src/browser/js/Local.zig b/src/browser/js/Local.zig index 9543d078..4022a302 100644 --- a/src/browser/js/Local.zig +++ b/src/browser/js/Local.zig @@ -266,7 +266,6 @@ pub fn mapZigInstanceToJs(self: *const Local, js_obj_handle: ?*const v8.Object, v8.v8__Global__New(isolate.handle, js_obj.handle, gop.value_ptr); if (resolved.finalizer) |finalizer| { const finalizer_ptr_id = finalizer.ptr_id; - finalizer.acquireRef(finalizer_ptr_id); const session = ctx.session; const finalizer_gop = try session.finalizer_callbacks.getOrPut(session.page_arena, finalizer_ptr_id); @@ -275,7 +274,8 @@ pub fn mapZigInstanceToJs(self: *const Local, js_obj_handle: ?*const v8.Object, // see this Zig instance. We need to create the FinalizerCallback // so that we can cleanup on page reset if v8 doesn't finalize. errdefer _ = session.finalizer_callbacks.remove(finalizer_ptr_id); - finalizer_gop.value_ptr.* = try self.createFinalizerCallback(resolved_ptr_id, finalizer_ptr_id, finalizer.deinit); + finalizer.acquire_ref(finalizer_ptr_id); + 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 fc.arena.create(Session.FinalizerCallback.Identity); @@ -283,8 +283,9 @@ pub fn mapZigInstanceToJs(self: *const Local, js_obj_handle: ?*const v8.Object, .fc = fc, .identity = ctx.identity, }; + fc.identity_count += 1; - v8.v8__Global__SetWeakFinalizer(gop.value_ptr, identity_finalizer, finalizer.release, v8.kParameter); + v8.v8__Global__SetWeakFinalizer(gop.value_ptr, identity_finalizer, finalizer.release_ref, v8.kParameter); } return js_obj; }, @@ -1128,9 +1129,9 @@ const Resolved = struct { // Resolved.ptr is the most specific value in a chain (e.g. IFrame, not EventTarget, Node, ...) // Finalizer.ptr_id is the most specific value in a chain that defines an acquireRef ptr_id: usize, - deinit: *const fn (ptr_id: usize, session: *Session) void, - acquireRef: *const fn (ptr_id: usize) void, - release: *const fn (handle: ?*const v8.WeakCallbackInfo) callconv(.c) void, + acquire_ref: *const fn (ptr_id: usize) void, + release_ref: *const fn (handle: ?*const v8.WeakCallbackInfo) callconv(.c) void, + release_ref_from_zig: *const fn (ptr_id: usize, session: *Session) void, }; }; pub fn resolveValue(value: anytype) Resolved { @@ -1170,32 +1171,49 @@ fn resolveT(comptime T: type, value: *T) Resolved { const finalizer_ptr = getFinalizerPtr(value); const Wrap = struct { - fn deinit(ptr_id: usize, session: *Session) void { - FT.deinit(@ptrFromInt(ptr_id), session); - } - fn acquireRef(ptr_id: usize) void { FT.acquireRef(@ptrFromInt(ptr_id)); } - fn release(handle: ?*const v8.WeakCallbackInfo) callconv(.c) void { + fn releaseRef(handle: ?*const v8.WeakCallbackInfo) callconv(.c) void { const ptr = v8.v8__WeakCallbackInfo__GetParameter(handle.?).?; const identity_finalizer: *Session.FinalizerCallback.Identity = @ptrCast(@alignCast(ptr)); const fc = identity_finalizer.fc; + const session = fc.session; + const finalizer_ptr_id = fc.finalizer_ptr_id; + + // Remove from this identity's map if (identity_finalizer.identity.identity_map.fetchRemove(fc.resolved_ptr_id)) |kv| { var global = kv.value; v8.v8__Global__Reset(&global); } - FT.releaseRef(@ptrFromInt(fc.finalizer_ptr_id), fc.session); + const identity_count = fc.identity_count; + if (identity_count == 1) { + // All IsolatedWorlds that reference this object have + // released it. Release the instance ref, remove the + // FinalizerCallback and free it. + FT.releaseRef(@ptrFromInt(finalizer_ptr_id), session); + const removed = session.finalizer_callbacks.remove(finalizer_ptr_id); + if (comptime IS_DEBUG) { + std.debug.assert(removed); + } + session.releaseArena(fc.arena); + } else { + fc.identity_count = identity_count - 1; + } + } + + fn releaseRefFromZig(ptr_id: usize, session: *Session) void { + FT.releaseRef(@ptrFromInt(ptr_id), session); } }; break :blk .{ .ptr_id = @intFromPtr(finalizer_ptr), - .deinit = Wrap.deinit, - .acquireRef = Wrap.acquireRef, - .release = Wrap.release, + .acquire_ref = Wrap.acquireRef, + .release_ref = Wrap.releaseRef, + .release_ref_from_zig = Wrap.releaseRefFromZig, }; }, }; @@ -1454,7 +1472,7 @@ fn createFinalizerCallback( // The most specific value where finalizers are defined // What actually gets acquired / released / deinit finalizer_ptr_id: usize, - deinit: *const fn (ptr_id: usize, session: *Session) void, + release_ref: *const fn (ptr_id: usize, session: *Session) void, ) !*Session.FinalizerCallback { const session = self.ctx.session; @@ -1465,7 +1483,7 @@ fn createFinalizerCallback( fc.* = .{ .arena = arena, .session = session, - ._deinit = deinit, + .release_ref = release_ref, .resolved_ptr_id = resolved_ptr_id, .finalizer_ptr_id = finalizer_ptr_id, }; diff --git a/src/browser/tests/node/child_nodes.html b/src/browser/tests/node/child_nodes.html index a5780d4b..603c960b 100644 --- a/src/browser/tests/node/child_nodes.html +++ b/src/browser/tests/node/child_nodes.html @@ -4,7 +4,7 @@

- --> -