From 7d90c3f582eefb574b1be878c588504d08f28f48 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 9 Mar 2026 14:16:46 +0800 Subject: [PATCH] Move origin lookup to Session With the last commit, this becomes the more logical place to hold this as it ties into the Session's awareness of the root page's lifetime. --- src/browser/Session.zig | 52 ++++++++++++++++++++++++++++ src/browser/js/Context.zig | 6 ++-- src/browser/js/Env.zig | 69 +++----------------------------------- 3 files changed, 59 insertions(+), 68 deletions(-) diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 42fe8ef5..9cce0ccf 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -62,6 +62,9 @@ factory: Factory, page_arena: Allocator, +// Origin map for same-origin context sharing. Scoped to the root page lifetime. +origins: std.StringHashMapUnmanaged(*js.Origin) = .empty, + // Shared resources for all pages in this session. // These live for the duration of the page tree (root + frames). arena_pool: *ArenaPool, @@ -194,6 +197,41 @@ pub fn releaseArena(self: *Session, allocator: Allocator) void { return self.arena_pool.release(allocator); } +pub fn getOrCreateOrigin(self: *Session, key_: ?[]const u8) !*js.Origin { + const key = key_ orelse { + var opaque_origin: [36]u8 = undefined; + @import("../id.zig").uuidv4(&opaque_origin); + // Origin.init will dupe opaque_origin. It's fine that this doesn't + // get added to self.origins. In fact, it further isolates it. When the + // context is freed, it'll call session.releaseOrigin which will free it. + return js.Origin.init(self.browser.app, self.browser.env.isolate, &opaque_origin); + }; + + const gop = try self.origins.getOrPut(self.arena, key); + if (gop.found_existing) { + const origin = gop.value_ptr.*; + origin.rc += 1; + return origin; + } + + errdefer _ = self.origins.remove(key); + + const origin = try js.Origin.init(self.browser.app, self.browser.env.isolate, key); + gop.key_ptr.* = origin.key; + gop.value_ptr.* = origin; + return origin; +} + +pub fn releaseOrigin(self: *Session, origin: *js.Origin) void { + const rc = origin.rc; + if (rc == 1) { + _ = self.origins.remove(origin.key); + origin.deinit(self.browser.app); + } else { + origin.rc = rc - 1; + } +} + /// Reset page_arena and factory for a clean slate. /// Called when root page is removed. fn resetPageResources(self: *Session) void { @@ -208,6 +246,20 @@ fn resetPageResources(self: *Session) void { self._arena_pool_leak_track.clearRetainingCapacity(); } + // All origins should have been released when contexts were destroyed + if (comptime IS_DEBUG) { + std.debug.assert(self.origins.count() == 0); + } + // Defensive cleanup in case origins leaked + { + const app = self.browser.app; + var it = self.origins.valueIterator(); + while (it.next()) |value| { + value.*.deinit(app); + } + self.origins.clearRetainingCapacity(); + } + // Release old page_arena and acquire fresh one self.frame_id_gen = 0; self.arena_pool.reset(self.page_arena, 64 * 1024); diff --git a/src/browser/js/Context.zig b/src/browser/js/Context.zig index 750cd809..ac6ed3c1 100644 --- a/src/browser/js/Context.zig +++ b/src/browser/js/Context.zig @@ -153,7 +153,7 @@ pub fn deinit(self: *Context) void { v8.v8__Global__Reset(global); } - env.releaseOrigin(self.origin); + self.session.releaseOrigin(self.origin); v8.v8__Global__Reset(&self.handle); env.isolate.notifyContextDisposed(); @@ -167,8 +167,8 @@ pub fn setOrigin(self: *Context, key: ?[]const u8) !void { const env = self.env; const isolate = env.isolate; - const origin = try env.getOrCreateOrigin(key); - errdefer env.releaseOrigin(origin); + const origin = try self.session.getOrCreateOrigin(key); + errdefer self.session.releaseOrigin(origin); try self.origin.transferTo(origin); self.origin.deinit(env.app); diff --git a/src/browser/js/Env.zig b/src/browser/js/Env.zig index 4318394d..5bc2fded 100644 --- a/src/browser/js/Env.zig +++ b/src/browser/js/Env.zig @@ -77,14 +77,6 @@ context_id: usize, // same-origin Contexts. There's a mismatch here between our JS model and our // Browser model. Origins only live as long as the root page of a session exists. // It would be wrong/dangerous to re-use an Origin across root page navigations. -// But we have no mechanism to capture that lifetime in js. We used to have a -// js.BrowserContext which mapped to a Session (oops, I took it out), but even -// that wouldn't match correctly, because 1 session can have have muliple non- -// concurrent pages. We deal with this in destroyContext by checking if we're -// destroying the root context and, if so, making sure origins is empty. But, if -// we ever add multiple Sessions to a Browser or mulitple Pages to a Session, -// this map will have to live in a new, better scoped, container. -origins: std.StringHashMapUnmanaged(*Origin) = .empty, // Global handles that need to be freed on deinit eternal_function_templates: []v8.Eternal, @@ -248,14 +240,6 @@ pub fn deinit(self: *Env) void { const app = self.app; const allocator = app.allocator; - { - var it = self.origins.valueIterator(); - while (it.next()) |value| { - value.*.deinit(app); - } - self.origins.deinit(allocator); - } - if (self.inspector) |i| { i.deinit(allocator); } @@ -323,8 +307,8 @@ pub fn createContext(self: *Env, page: *Page) !*Context { const context_id = self.context_id; self.context_id = context_id + 1; - const origin = try self.getOrCreateOrigin(null); - errdefer self.releaseOrigin(origin); + const origin = try page._session.getOrCreateOrigin(null); + errdefer page._session.releaseOrigin(origin); const context = try context_arena.create(Context); context.* = .{ @@ -383,56 +367,11 @@ pub fn destroyContext(self: *Env, context: *Context, is_root: bool) void { context.deinit(); if (is_root) { - // When the root is destroyed, the all of our contexts should be gone - // and with them, all of our origins. Keep origins around longer than - // intended would cause issues, so we're going to be defensive here and - // clean things up. + // When the root is destroyed, all of our contexts should be gone. + // Origin cleanup happens in Session.resetPageResources. if (comptime IS_DEBUG) { std.debug.assert(self.context_count == 0); - std.debug.assert(self.origins.count() == 0); } - - const app = self.app; - var it = self.origins.valueIterator(); - while (it.next()) |value| { - value.*.deinit(app); - } - self.origins.clearRetainingCapacity(); - } -} - -pub fn getOrCreateOrigin(self: *Env, key_: ?[]const u8) !*Origin { - const key = key_ orelse { - var opaque_origin: [36]u8 = undefined; - @import("../../id.zig").uuidv4(&opaque_origin); - // Origin.init will dupe opaque_origin. It's fine that this doesn't - // get added to self.origins. In fact, it further isolates it. When the - // context is freed, it'll call env.releaseOrigin which will free it. - return Origin.init(self.app, self.isolate, &opaque_origin); - }; - - const gop = try self.origins.getOrPut(self.allocator, key); - if (gop.found_existing) { - const origin = gop.value_ptr.*; - origin.rc += 1; - return origin; - } - - errdefer _ = self.origins.remove(key); - - const origin = try Origin.init(self.app, self.isolate, key); - gop.key_ptr.* = origin.key; - gop.value_ptr.* = origin; - return origin; -} - -pub fn releaseOrigin(self: *Env, origin: *Origin) void { - const rc = origin.rc; - if (rc == 1) { - _ = self.origins.remove(origin.key); - origin.deinit(self.app); - } else { - origin.rc = rc - 1; } }