From 603e7d922e57d5fb2602c62c2c42d471942f210a Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 20 Feb 2026 16:24:25 +0800 Subject: [PATCH] Improve Context shutdown Under some conditions, a microtask would be executed for a context that was already deinit'd, resulting in various use-after-free. The culprit appears to be WASM compilation being placed in the microtask queue (by a user-script) and then resolved at some point in the future. We guard the microtask queue by a context.shutting_down boolean, but v8 doesn't know anything about this flag. The fact is that, microtasks are tied to an isolate, not a context. This commit introduces a number of changes: 1 - It follows https://github.com/lightpanda-io/browser/commit/309f254c2c8c590a2655972aa1b31d020777634c and stores the zig Context inside of an embedder field. This ensures v8 doesn't consider this when GC'ing, which _could_ extend the lifetime of the v8::Context beyond what we expect 2 - Most significantly, it introduces per-context microtasks queues. Each context gets its own queue. This makes cleanup much simpler and reduces the chance of microtasks outliving the context 3 - pumpMessageLoop is called on context.deinit, this helps to ensure that any tasks v8 has for our context are processed (e.g. wasm compilation) before shtudown 4 - The order of context shutdown is important, we notify the isolate of the context destruction first, then pump the message loop and finally destroy the context's message loop. Depends on https://github.com/lightpanda-io/zig-v8-fork/pull/151 --- .github/actions/install/action.yml | 2 +- Dockerfile | 2 +- build.zig.zon | 4 +-- src/browser/Browser.zig | 6 +--- src/browser/js/Context.zig | 46 ++++++++++++------------------ src/browser/js/Env.zig | 31 +++++++++++++++----- src/browser/js/Inspector.zig | 2 -- src/browser/js/Isolate.zig | 12 -------- src/browser/js/Local.zig | 2 +- src/cdp/cdp.zig | 1 + 10 files changed, 50 insertions(+), 58 deletions(-) diff --git a/.github/actions/install/action.yml b/.github/actions/install/action.yml index 88a672a0..9e47eac2 100644 --- a/.github/actions/install/action.yml +++ b/.github/actions/install/action.yml @@ -13,7 +13,7 @@ inputs: zig-v8: description: 'zig v8 version to install' required: false - default: 'v0.2.9' + default: 'v0.3.0' v8: description: 'v8 version to install' required: false diff --git a/Dockerfile b/Dockerfile index 79ae2627..1aa5d592 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,7 +3,7 @@ FROM debian:stable-slim ARG MINISIG=0.12 ARG ZIG_MINISIG=RWSGOq2NVecA2UPNdBUZykf1CCb147pkmdtYxgb3Ti+JO/wCYvhbAb/U ARG V8=14.0.365.4 -ARG ZIG_V8=v0.2.9 +ARG ZIG_V8=v0.3.0 ARG TARGETPLATFORM RUN apt-get update -yq && \ diff --git a/build.zig.zon b/build.zig.zon index 946210d1..0d115d53 100644 --- a/build.zig.zon +++ b/build.zig.zon @@ -6,8 +6,8 @@ .minimum_zig_version = "0.15.2", .dependencies = .{ .v8 = .{ - .url = "https://github.com/lightpanda-io/zig-v8-fork/archive/refs/tags/v0.2.9.tar.gz", - .hash = "v8-0.0.0-xddH689vBACgpqFVEhT2wxRin-qQQSOcKJoM37MVo0rU", + .url = "https://github.com/lightpanda-io/zig-v8-fork/archive/refs/tags/v0.3.0.tar.gz", + .hash = "v8-0.0.0-xddH69R6BADRXsnhjA8wNnfKfLQACF1I7CSTZvsMAvp8", }, //.v8 = .{ .path = "../zig-v8-fork" }, .@"boringssl-zig" = .{ diff --git a/src/browser/Browser.zig b/src/browser/Browser.zig index 09a78cab..09f7773d 100644 --- a/src/browser/Browser.zig +++ b/src/browser/Browser.zig @@ -96,10 +96,6 @@ pub fn runMacrotasks(self: *Browser) !?u64 { } pub fn runMessageLoop(self: *const Browser) void { - while (self.env.pumpMessageLoop()) { - if (comptime IS_DEBUG) { - log.debug(.browser, "pumpMessageLoop", .{}); - } - } + self.env.pumpMessageLoop(); self.env.runIdleTasks(); } diff --git a/src/browser/js/Context.zig b/src/browser/js/Context.zig index 115b6156..377ced77 100644 --- a/src/browser/js/Context.zig +++ b/src/browser/js/Context.zig @@ -43,6 +43,9 @@ env: *Env, page: *Page, isolate: js.Isolate, +// Per-context microtask queue for isolation between contexts +microtask_queue: *v8.MicrotaskQueue, + // The v8::Global. When necessary, we can create a v8::Local<> // from this, and we can free it when the context is done. handle: v8.Global, @@ -121,10 +124,6 @@ script_manager: ?*ScriptManager, // Our macrotasks scheduler: Scheduler, -// Prevents us from enqueuing a microtask for this context while we're shutting -// down. -shutting_down: bool = false, - unknown_properties: (if (IS_DEBUG) std.StringHashMapUnmanaged(UnknownPropertyStat) else void) = if (IS_DEBUG) .{} else {}, const ModuleEntry = struct { @@ -146,16 +145,11 @@ const ModuleEntry = struct { }; pub fn fromC(c_context: *const v8.Context) *Context { - const data = v8.v8__Context__GetEmbedderData(c_context, 1).?; - const big_int = js.BigInt{ .handle = @ptrCast(data) }; - return @ptrFromInt(big_int.getUint64()); + return @ptrCast(@alignCast(v8.v8__Context__GetAlignedPointerFromEmbedderData(c_context, 1))); } pub fn fromIsolate(isolate: js.Isolate) *Context { - const v8_context = v8.v8__Isolate__GetCurrentContext(isolate.handle).?; - const data = v8.v8__Context__GetEmbedderData(v8_context, 1).?; - const big_int = js.BigInt{ .handle = @ptrCast(data) }; - return @ptrFromInt(big_int.getUint64()); + return fromC(v8.v8__Isolate__GetCurrentContext(isolate.handle).?); } pub fn deinit(self: *Context) void { @@ -169,21 +163,15 @@ pub fn deinit(self: *Context) void { }); } } - defer self.env.app.arena_pool.release(self.arena); + + const env = self.env; + defer env.app.arena_pool.release(self.arena); var hs: js.HandleScope = undefined; const entered = self.enter(&hs); defer entered.exit(); - // We might have microtasks in the isolate that refence this context. The - // only option we have is to run them. But a microtask could queue another - // microtask, so we set the shutting_down flag, so that any such microtask - // will be a noop (this isn't automatic, when v8 calls our microtask callback - // the first thing we'll check is if self.shutting_down == true). - self.shutting_down = true; - self.env.runMicrotasks(); - - // can release objects + // this can release objects self.scheduler.deinit(); { @@ -244,7 +232,13 @@ pub fn deinit(self: *Context) void { v8.v8__Global__Reset(global); } } + v8.v8__Global__Reset(&self.handle); + env.isolate.notifyContextDisposed(); + // There can be other tasks associated with this context that we need to + // purge while the context is still alive. + env.pumpMessageLoop(); + v8.v8__MicrotaskQueue__DELETE(self.microtask_queue); } pub fn weakRef(self: *Context, obj: anytype) void { @@ -992,13 +986,10 @@ pub fn queueSlotchangeDelivery(self: *Context) !void { // But for these Context microtasks, we want to (a) make sure the context isn't // being shut down and (b) that it's entered. fn enqueueMicrotask(self: *Context, callback: anytype) void { - self.isolate.enqueueMicrotask(struct { + // Use context-specific microtask queue instead of isolate queue + v8.v8__MicrotaskQueue__EnqueueMicrotask(self.microtask_queue, self.isolate.handle, struct { fn run(data: ?*anyopaque) callconv(.c) void { const ctx: *Context = @ptrCast(@alignCast(data.?)); - if (ctx.shutting_down) { - return; - } - var hs: js.HandleScope = undefined; const entered = ctx.enter(&hs); defer entered.exit(); @@ -1008,7 +999,8 @@ fn enqueueMicrotask(self: *Context, callback: anytype) void { } pub fn queueMicrotaskFunc(self: *Context, cb: js.Function) void { - self.isolate.enqueueMicrotaskFunc(cb); + // Use context-specific microtask queue instead of isolate queue + v8.v8__MicrotaskQueue__EnqueueMicrotaskFunc(self.microtask_queue, self.isolate.handle, cb.handle); } pub fn createFinalizerCallback(self: *Context, global: v8.Global, ptr: *anyopaque, finalizerFn: *const fn (ptr: *anyopaque) void) !*FinalizerCallback { diff --git a/src/browser/js/Env.zig b/src/browser/js/Env.zig index 458ff099..309285e8 100644 --- a/src/browser/js/Env.zig +++ b/src/browser/js/Env.zig @@ -249,10 +249,19 @@ pub fn createContext(self: *Env, page: *Page) !*Context { hs.init(isolate); defer hs.deinit(); + // Create a per-context microtask queue for isolation + const microtask_queue = v8.v8__MicrotaskQueue__New(isolate.handle, v8.kExplicit).?; + errdefer v8.v8__MicrotaskQueue__DELETE(microtask_queue); + // Get the global template that was created once per isolate const global_template: *const v8.ObjectTemplate = @ptrCast(@alignCast(v8.v8__Eternal__Get(&self.global_template, isolate.handle).?)); v8.v8__ObjectTemplate__SetInternalFieldCount(global_template, comptime Snapshot.countInternalFields(Window.JsApi)); - const v8_context = v8.v8__Context__New(isolate.handle, global_template, null).?; + + const v8_context = v8.v8__Context__New__Config(isolate.handle, &.{ + .global_template = global_template, + .global_object = null, + .microtask_queue = microtask_queue, + }).?; // Create the v8::Context and wrap it in a v8::Global var context_global: v8.Global = undefined; @@ -292,6 +301,7 @@ pub fn createContext(self: *Env, page: *Page) !*Context { .handle = context_global, .templates = self.templates, .call_arena = page.call_arena, + .microtask_queue = microtask_queue, .script_manager = &page._script_manager, .scheduler = .init(context_arena), .finalizer_callback_pool = std.heap.MemoryPool(Context.FinalizerCallback).init(self.app.allocator), @@ -300,8 +310,7 @@ pub fn createContext(self: *Env, page: *Page) !*Context { // Store a pointer to our context inside the v8 context so that, given // a v8 context, we can get our context out - const data = isolate.initBigInt(@intFromPtr(context)); - v8.v8__Context__SetEmbedderData(v8_context, 1, @ptrCast(data.handle)); + v8.v8__Context__SetAlignedPointerInEmbedderData(v8_context, 1, @ptrCast(context)); try self.contexts.append(self.app.allocator, context); return context; @@ -328,11 +337,13 @@ pub fn destroyContext(self: *Env, context: *Context) void { } context.deinit(); - isolate.notifyContextDisposed(); } pub fn runMicrotasks(self: *const Env) void { - self.isolate.performMicrotasksCheckpoint(); + const v8_isolate = self.isolate.handle; + for (self.contexts.items) |ctx| { + v8.v8__MicrotaskQueue__PerformCheckpoint(ctx.microtask_queue, v8_isolate); + } } pub fn runMacrotasks(self: *Env) !?u64 { @@ -360,12 +371,18 @@ pub fn runMacrotasks(self: *Env) !?u64 { return ms_to_next_task; } -pub fn pumpMessageLoop(self: *const Env) bool { +pub fn pumpMessageLoop(self: *const Env) void { var hs: v8.HandleScope = undefined; v8.v8__HandleScope__CONSTRUCT(&hs, self.isolate.handle); defer v8.v8__HandleScope__DESTRUCT(&hs); - return v8.v8__Platform__PumpMessageLoop(self.platform.handle, self.isolate.handle, false); + const isolate = self.isolate.handle; + const platform = self.platform.handle; + while (v8.v8__Platform__PumpMessageLoop(platform, isolate, false)) { + if (comptime IS_DEBUG) { + log.debug(.browser, "pumpMessageLoop", .{}); + } + } } pub fn runIdleTasks(self: *const Env) void { diff --git a/src/browser/js/Inspector.zig b/src/browser/js/Inspector.zig index 245c5582..a9032e33 100644 --- a/src/browser/js/Inspector.zig +++ b/src/browser/js/Inspector.zig @@ -241,8 +241,6 @@ pub const Session = struct { msg.ptr, msg.len, ); - - v8.v8__Isolate__PerformMicrotaskCheckpoint(isolate); } // Gets a value by object ID regardless of which context it is in. diff --git a/src/browser/js/Isolate.zig b/src/browser/js/Isolate.zig index 74974cc0..32b0f7d6 100644 --- a/src/browser/js/Isolate.zig +++ b/src/browser/js/Isolate.zig @@ -41,18 +41,6 @@ pub fn exit(self: Isolate) void { v8.v8__Isolate__Exit(self.handle); } -pub fn performMicrotasksCheckpoint(self: Isolate) void { - v8.v8__Isolate__PerformMicrotaskCheckpoint(self.handle); -} - -pub fn enqueueMicrotask(self: Isolate, callback: anytype, data: anytype) void { - v8.v8__Isolate__EnqueueMicrotask(self.handle, callback, data); -} - -pub fn enqueueMicrotaskFunc(self: Isolate, function: js.Function) void { - v8.v8__Isolate__EnqueueMicrotaskFunc(self.handle, function.handle); -} - pub fn lowMemoryNotification(self: Isolate) void { v8.v8__Isolate__LowMemoryNotification(self.handle); } diff --git a/src/browser/js/Local.zig b/src/browser/js/Local.zig index b51453b5..5937c06c 100644 --- a/src/browser/js/Local.zig +++ b/src/browser/js/Local.zig @@ -82,7 +82,7 @@ pub fn createTypedArray(self: *const Local, comptime array_type: js.ArrayType, s } pub fn runMicrotasks(self: *const Local) void { - self.isolate.performMicrotasksCheckpoint(); + self.ctx.env.runMicrotasks(); } // == Executors == diff --git a/src/cdp/cdp.zig b/src/cdp/cdp.zig index d8ccfb2b..c486e794 100644 --- a/src/cdp/cdp.zig +++ b/src/cdp/cdp.zig @@ -662,6 +662,7 @@ pub fn BrowserContext(comptime CDP_T: type) type { pub fn callInspector(self: *const Self, msg: []const u8) void { self.inspector_session.send(msg); + self.session.browser.env.runMicrotasks(); } pub fn onInspectorResponse(ctx: *anyopaque, _: u32, msg: []const u8) void {