Add origins safety cleanup when destroying the context for the root page

This commit is contained in:
Karl Seguin
2026-03-09 08:47:43 +08:00
parent 94ce5edd20
commit 753391b7e2
3 changed files with 34 additions and 5 deletions

View File

@@ -345,11 +345,12 @@ pub fn deinit(self: *Page, abort_http: bool) void {
} }
const session = self._session; const session = self._session;
session.browser.env.destroyContext(self.js); const is_root = self.parent == null;
session.browser.env.destroyContext(self.js, is_root);
self._script_manager.shutdown = true; self._script_manager.shutdown = true;
if (self.parent == null) { if (is_root) {
session.browser.http_client.abort(); session.browser.http_client.abort();
} else if (abort_http) { } else if (abort_http) {
// a small optimization, it's faster to abort _everything_ on the root // a small optimization, it's faster to abort _everything_ on the root

View File

@@ -73,7 +73,17 @@ isolate_params: *v8.CreateParams,
context_id: usize, context_id: usize,
// Maps origin -> shared Origin contains, for v8 values shared across same-origin Contexts // Maps origin -> shared Origin contains, for v8 values shared across
// 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, origins: std.StringHashMapUnmanaged(*Origin) = .empty,
// Global handles that need to be freed on deinit // Global handles that need to be freed on deinit
@@ -348,7 +358,7 @@ pub fn createContext(self: *Env, page: *Page) !*Context {
return context; return context;
} }
pub fn destroyContext(self: *Env, context: *Context) void { pub fn destroyContext(self: *Env, context: *Context, is_root: bool) void {
for (self.contexts[0..self.context_count], 0..) |ctx, i| { for (self.contexts[0..self.context_count], 0..) |ctx, i| {
if (ctx == context) { if (ctx == context) {
// Swap with last element and decrement count // Swap with last element and decrement count
@@ -371,6 +381,24 @@ pub fn destroyContext(self: *Env, context: *Context) void {
} }
context.deinit(); 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.
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 { pub fn getOrCreateOrigin(self: *Env, key_: ?[]const u8) !*Origin {

View File

@@ -759,7 +759,7 @@ const IsolatedWorld = struct {
pub fn removeContext(self: *IsolatedWorld) !void { pub fn removeContext(self: *IsolatedWorld) !void {
const ctx = self.context orelse return error.NoIsolatedContextToRemove; const ctx = self.context orelse return error.NoIsolatedContextToRemove;
self.browser.env.destroyContext(ctx); self.browser.env.destroyContext(ctx, false);
self.context = null; self.context = null;
} }