Merge pull request #1614 from lightpanda-io/fix_v8_context_queue_lifetime

Improve Context shutdown

.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

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 && \

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",
+            .url = "https://github.com/lightpanda-io/zig-v8-fork/archive/refs/tags/v0.3.0.tar.gz",
-            .hash = "v8-0.0.0-xddH689vBACgpqFVEhT2wxRin-qQQSOcKJoM37MVo0rU",
+            .hash = "v8-0.0.0-xddH69R6BADRXsnhjA8wNnfKfLQACF1I7CSTZvsMAvp8",
         },
         //.v8 = .{ .path = "../zig-v8-fork" },
         .@"boringssl-zig" = .{

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()) {
+    self.env.pumpMessageLoop();
-        if (comptime IS_DEBUG) {
-            log.debug(.browser, "pumpMessageLoop", .{});
-        }
-    }
     self.env.runIdleTasks();
 }

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<v8::Context>. When necessary, we can create a v8::Local<<v8::Context>>
 // 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).?;
+    return @ptrCast(@alignCast(v8.v8__Context__GetAlignedPointerFromEmbedderData(c_context, 1)));
-    const big_int = js.BigInt{ .handle = @ptrCast(data) };
-    return @ptrFromInt(big_int.getUint64());
 }
 
 pub fn fromIsolate(isolate: js.Isolate) *Context {
-    const v8_context = v8.v8__Isolate__GetCurrentContext(isolate.handle).?;
+    return fromC(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());
 }
 
 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
+    // this can release objects
-    // 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
     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 {

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__SetAlignedPointerInEmbedderData(v8_context, 1, @ptrCast(context));
-    v8.v8__Context__SetEmbedderData(v8_context, 1, @ptrCast(data.handle));
 
     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 {

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.

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);
 }

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 ==

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 {