From fc8b1b854991e1f95ec6adedb96cc93ee40921f0 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Wed, 4 Mar 2026 11:43:09 +0800 Subject: [PATCH] Use a single HandleScope for event dispatch https://github.com/lightpanda-io/browser/pull/1690 narrowed the lifetime of HandleScopes to once per listener. I think that was just an accident of refactoring, and not some intentional choice. The narrower HandleScope lifetime makes it so that when we do run microtask queue at the end of event dispatching, some locals in the queue may not longer be valid. HS1 HS2 queueMicrotask(func) runMicrotask In the above flow, `func` is only valid while HS2 is alive, so when we run the microtask queue in HS1, it is no longer valid. --- src/browser/EventManager.zig | 39 +++++++++++++++++------------------- src/browser/js/Context.zig | 7 +++++++ 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/src/browser/EventManager.zig b/src/browser/EventManager.zig index aeeb2248..17271635 100644 --- a/src/browser/EventManager.zig +++ b/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) { - var ls: js.Local.Scope = undefined; - page.js.localScope(&ls); - defer ls.deinit(); - ls.local.runMicrotasks(); - }; + // Create a single scope for all event handlers in this dispatch. + // This ensures function handles passed to queueMicrotask remain valid + // throughout the entire dispatch, preventing crashes when microtasks run. + var ls: js.Local.Scope = undefined; + 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}); } diff --git a/src/browser/js/Context.zig b/src/browser/js/Context.zig index 6b2e724a..c0242a07 100644 --- a/src/browser/js/Context.zig +++ b/src/browser/js/Context.zig @@ -1007,6 +1007,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 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);