Merge pull request #1709 from lightpanda-io/expand_event_dispatch_handle_scope

Use a single HandleScope for event dispatch

src/browser/EventManager.zig

@@ -377,12 +377,17 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts
     const page = self.page;
     var was_handled = false;
 
-    defer if (was_handled) {
+    // Create a single scope for all event handlers in this dispatch.
-        var ls: js.Local.Scope = undefined;
+    // This ensures function handles passed to queueMicrotask remain valid
-        page.js.localScope(&ls);
+    // throughout the entire dispatch, preventing crashes when microtasks run.
-        defer ls.deinit();
+    var ls: js.Local.Scope = undefined;
-        ls.local.runMicrotasks();
+    page.js.localScope(&ls);
-    };
+    defer {
+        if (was_handled) {
+            ls.local.runMicrotasks();
+        }
+        ls.deinit();
+    }
 
     const activation_state = ActivationState.create(event, target, page);
 
@@ -461,7 +466,7 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts
             .event_target = @intFromPtr(current_target),
             .type_string = event._type_string,
         })) |list| {
-            try self.dispatchPhase(list, current_target, event, &was_handled, comptime .init(true, opts));
+            try self.dispatchPhase(list, current_target, event, &was_handled, &ls.local, comptime .init(true, opts));
         }
     }
 
@@ -476,10 +481,6 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts
             was_handled = true;
             event._current_target = target_et;
 
-            var ls: js.Local.Scope = undefined;
-            self.page.js.localScope(&ls);
-            defer ls.deinit();
-
             try ls.toLocal(inline_handler).callWithThis(void, target_et, .{event});
 
             if (event._stop_propagation) {
@@ -495,7 +496,7 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts
             .type_string = event._type_string,
             .event_target = @intFromPtr(target_et),
         })) |list| {
-            try self.dispatchPhase(list, target_et, event, &was_handled, comptime .init(null, opts));
+            try self.dispatchPhase(list, target_et, event, &was_handled, &ls.local, comptime .init(null, opts));
             if (event._stop_propagation) {
                 return;
             }
@@ -512,7 +513,7 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts
                 .type_string = event._type_string,
                 .event_target = @intFromPtr(current_target),
             })) |list| {
-                try self.dispatchPhase(list, current_target, event, &was_handled, comptime .init(false, opts));
+                try self.dispatchPhase(list, current_target, event, &was_handled, &ls.local, comptime .init(false, opts));
             }
         }
     }
@@ -530,7 +531,7 @@ const DispatchPhaseOpts = struct {
     }
 };
 
-fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_target: *EventTarget, event: *Event, was_handled: *bool, comptime opts: DispatchPhaseOpts) !void {
+fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_target: *EventTarget, event: *Event, was_handled: *bool, local: *const js.Local, comptime opts: DispatchPhaseOpts) !void {
     const page = self.page;
 
     // Track dispatch depth for deferred removal
@@ -607,18 +608,14 @@ fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_targe
             event._target = getAdjustedTarget(original_target, current_target);
         }
 
-        var ls: js.Local.Scope = undefined;
-        page.js.localScope(&ls);
-        defer ls.deinit();
-
         switch (listener.function) {
-            .value => |value| try ls.toLocal(value).callWithThis(void, current_target, .{event}),
+            .value => |value| try local.toLocal(value).callWithThis(void, current_target, .{event}),
             .string => |string| {
                 const str = try page.call_arena.dupeZ(u8, string.str());
-                try ls.local.eval(str, null);
+                try local.eval(str, null);
             },
             .object => |obj_global| {
-                const obj = ls.toLocal(obj_global);
+                const obj = local.toLocal(obj_global);
                 if (try obj.getFunction("handleEvent")) |handleEvent| {
                     try handleEvent.callWithThis(void, obj, .{event});
                 }

src/browser/js/Context.zig

@@ -1004,6 +1004,13 @@ fn enqueueMicrotask(self: *Context, callback: anytype) void {
     }.run, self);
 }
 
+// There's an assumption here: the js.Function will be alive when microtasks are
+// run. If we're Env.runMicrotasks in all the places that we're supposed to, then
+// this should be safe (I think). In whatever HandleScope a microtask is enqueued,
+// PerformCheckpoint should be run. So the v8::Local<v8::Function> should remain
+// valid. If we have problems with this, a simple solution is to provide a Zig
+// wrapper for these callbacks which references a js.Function.Temp, on callback
+// it executes the function and then releases the global.
 pub fn queueMicrotaskFunc(self: *Context, cb: js.Function) void {
     // Use context-specific microtask queue instead of isolate queue
     v8.v8__MicrotaskQueue__EnqueueMicrotaskFunc(self.microtask_queue, self.isolate.handle, cb.handle);