Merge pull request #1468 from lightpanda-io/context_safety

Fixes a few context issues.

src/browser/Page.zig

@@ -243,15 +243,6 @@ pub fn deinit(self: *Page) void {
         // stats.print(&stream) catch unreachable;
     }
 
-    {
-        // some MicroTasks might be referencing the page, we need to drain it while
-        // the page still exists
-        var ls: JS.Local.Scope = undefined;
-        self.js.localScope(&ls);
-        defer ls.deinit();
-        ls.local.runMicrotasks();
-    }
-
     const session = self._session;
     session.browser.env.destroyContext(self.js);
 

src/browser/js/Context.zig

@@ -122,6 +122,10 @@ 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,
+
 const ModuleEntry = struct {
     // Can be null if we're asynchrously loading the module, in
     // which case resolver_promise cannot be null.
@@ -154,12 +158,21 @@ pub fn fromIsolate(isolate: js.Isolate) *Context {
 }
 
 pub fn deinit(self: *Context) void {
-    var page = self.page;
+    defer self.env.app.arena_pool.release(self.arena);
-    const prev_context = page.js;
-    page.js = self;
-    defer page.js = prev_context;
 
-    // This can release JS objects
+    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
     self.scheduler.deinit();
 
     {
@@ -214,13 +227,10 @@ pub fn deinit(self: *Context) void {
     }
 
     if (self.entered) {
-        var ls: js.Local.Scope = undefined;
+        v8.v8__Context__Exit(@ptrCast(v8.v8__Global__Get(&self.handle, self.isolate.handle)));
-        self.localScope(&ls);
-        defer ls.deinit();
-        v8.v8__Context__Exit(ls.local.handle);
     }
+
     v8.v8__Global__Reset(&self.handle);
-    self.env.app.arena_pool.release(self.arena);
 }
 
 pub fn weakRef(self: *Context, obj: anytype) void {
@@ -880,41 +890,90 @@ fn resolveDynamicModule(self: *Context, state: *DynamicModuleResolveState, modul
     };
 }
 
-// Microtasks
+// Used to make temporarily enter and exit a context, updating and restoring
+// page.js:
+//    var hs: js.HandleScope = undefined;
+//    const entered = ctx.enter(&hs);
+//    defer entered.exit();
+pub fn enter(self: *Context, hs: *js.HandleScope) Entered {
+    const isolate = self.isolate;
+    js.HandleScope.init(hs, isolate);
+
+    const page = self.page;
+    const original = page.js;
+    page.js = self;
+
+    const handle: *const v8.Context =  @ptrCast(v8.v8__Global__Get(&self.handle, isolate.handle));
+    v8.v8__Context__Enter(handle);
+    return .{.original = original, .handle = handle, .handle_scope = hs};
+}
+
+const Entered = struct {
+    // the context we should restore on the page
+    original: *Context,
+
+    // the handle of the entered context
+    handle: *const v8.Context,
+
+    handle_scope: *js.HandleScope,
+
+    pub fn exit(self: Entered) void {
+        self.original.page.js = self.original;
+        v8.v8__Context__Exit(self.handle);
+        self.handle_scope.deinit();
+    }
+};
+
 pub fn queueMutationDelivery(self: *Context) !void {
-    self.isolate.enqueueMicrotask(struct {
+    self.enqueueMicrotask(struct {
-        fn run(data: ?*anyopaque) callconv(.c) void {
+        fn run(ctx: *Context) void {
-            const page: *Page = @ptrCast(@alignCast(data.?));
+            ctx.page.deliverMutations();
-            page.deliverMutations();
         }
-    }.run, self.page);
+    }.run);
 }
 
 pub fn queueIntersectionChecks(self: *Context) !void {
-    self.isolate.enqueueMicrotask(struct {
+    self.enqueueMicrotask(struct {
-        fn run(data: ?*anyopaque) callconv(.c) void {
+        fn run(ctx: *Context) void {
-            const page: *Page = @ptrCast(@alignCast(data.?));
+            ctx.page.performScheduledIntersectionChecks();
-            page.performScheduledIntersectionChecks();
         }
-    }.run, self.page);
+    }.run);
 }
 
 pub fn queueIntersectionDelivery(self: *Context) !void {
-    self.isolate.enqueueMicrotask(struct {
+    self.enqueueMicrotask(struct {
-        fn run(data: ?*anyopaque) callconv(.c) void {
+        fn run(ctx: *Context) void {
-            const page: *Page = @ptrCast(@alignCast(data.?));
+            ctx.page.deliverIntersections();
-            page.deliverIntersections();
         }
-    }.run, self.page);
+    }.run);
 }
 
 pub fn queueSlotchangeDelivery(self: *Context) !void {
+    self.enqueueMicrotask(struct {
+        fn run(ctx: *Context) void {
+            ctx.page.deliverSlotchangeEvents();
+        }
+    }.run);
+}
+
+// Helper for executing a Microtask on this Context. In V8, microtasks aren't
+// associated to a Context - they are just functions to execute in an Isolate.
+// 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 {
         fn run(data: ?*anyopaque) callconv(.c) void {
-            const page: *Page = @ptrCast(@alignCast(data.?));
+            const ctx: *Context = @ptrCast(@alignCast(data.?));
-            page.deliverSlotchangeEvents();
+            if (ctx.shutting_down) {
+                return;
+            }
+
+            var hs: js.HandleScope = undefined;
+            const entered = ctx.enter(&hs);
+            defer entered.exit();
+            callback(ctx);
         }
-    }.run, self.page);
+    }.run, self);
 }
 
 pub fn queueMicrotaskFunc(self: *Context, cb: js.Function) void {

src/browser/js/Env.zig

@@ -18,6 +18,8 @@
 
 const std = @import("std");
 const js = @import("js.zig");
+const builtin = @import("builtin");
+
 const v8 = js.v8;
 
 const App = @import("../../App.zig");
@@ -35,7 +37,7 @@ const Window = @import("../webapi/Window.zig");
 
 const JsApis = bridge.JsApis;
 const Allocator = std.mem.Allocator;
-const IS_DEBUG = @import("builtin").mode == .Debug;
+const IS_DEBUG = builtin.mode == .Debug;
 
 // The Env maps to a V8 isolate, which represents a isolated sandbox for
 // executing JavaScript. The Env is where we'll define our V8 <-> Zig bindings,
@@ -284,6 +286,20 @@ pub fn runMicrotasks(self: *const Env) void {
 pub fn runMacrotasks(self: *Env) !?u64 {
     var ms_to_next_task: ?u64 = null;
     for (self.contexts.items) |ctx| {
+        if (comptime builtin.is_test == false) {
+            // I hate this comptime check as much as you do. But we have tests
+            // which rely on short execution before shutdown. In real world, it's
+            // underterministic whether a timer will or won't run before the
+            // page shutsdown. But for tests, we need to run them to their end.
+            if (ctx.scheduler.hasReadyTasks() == false) {
+                continue;
+            }
+        }
+
+        var hs: js.HandleScope = undefined;
+        const entered = ctx.enter(&hs);
+        defer entered.exit();
+
         const ms = (try ctx.scheduler.run()) orelse continue;
         if (ms_to_next_task == null or ms < ms_to_next_task.?) {
             ms_to_next_task = ms;

src/browser/js/Scheduler.zig

@@ -74,11 +74,17 @@ pub fn add(self: *Scheduler, ctx: *anyopaque, cb: Callback, run_in_ms: u32, opts
     });
 }
 
+
 pub fn run(self: *Scheduler) !?u64 {
     _ = try self.runQueue(&self.low_priority);
     return self.runQueue(&self.high_priority);
 }
 
+pub fn hasReadyTasks(self: *Scheduler) bool {
+    const now = milliTimestamp(.monotonic);
+    return queueuHasReadyTask(&self.low_priority, now) or queueuHasReadyTask(&self.high_priority, now);
+}
+
 fn runQueue(self: *Scheduler, queue: *Queue) !?u64 {
     if (queue.count() == 0) {
         return null;
@@ -112,6 +118,11 @@ fn runQueue(self: *Scheduler, queue: *Queue) !?u64 {
     return null;
 }
 
+fn queueuHasReadyTask(queue: *Queue, now: u64) bool {
+    const task = queue.peek() orelse return false;
+    return task.run_at <= now;
+}
+
 fn finalizeTasks(queue: *Queue) void {
     var it = queue.iterator();
     while (it.next()) |t| {