From ea5d7c0dee25ae4dbfcca82fce660d5236a5f80b Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 23 Feb 2026 17:28:14 +0800 Subject: [PATCH] Use EventManager.dispatch for Script events Should be updated and merged after: https://github.com/lightpanda-io/browser/pull/1623 else we'll have a double-free. The ScriptManager used to directly call the "onload" and "onerror" attributes. The implementation predates EventManager.dispatch support attribute-based callbacks. But now the EventManager is attribute-aware and correctly times the attribute dispatch AND details such as cancellation. So this commit moves the old attribute-only ScriptManager-specific callback to the EventManager. With one little wrinkle: 'load' listeners added during a script's execution should NOT receive a 'load' event when the script finishes. This makes no sense to me. The EventManager now maintains an ignore_list for "load" events which is reset after each script execution. A comptime flag is passed to dispatch to indicate whether the ignore list should be checked. This is only ever set when the ScriptManager dispatches the 'load' event, so there's no overhead to dispatch for most events. --- src/browser/EventManager.zig | 69 +++++++++++++++---- src/browser/ScriptManager.zig | 30 ++++---- .../tests/element/html/script/empty.js | 0 .../tests/element/html/script/script.html | 27 ++++++-- src/browser/webapi/element/html/Script.zig | 38 +--------- 5 files changed, 94 insertions(+), 70 deletions(-) create mode 100644 src/browser/tests/element/html/script/empty.js diff --git a/src/browser/EventManager.zig b/src/browser/EventManager.zig index 42de7d55..53e2c6ad 100644 --- a/src/browser/EventManager.zig +++ b/src/browser/EventManager.zig @@ -61,6 +61,7 @@ arena: Allocator, // 'load' event (e.g. amazon product page has no listener and ~350 resources) has_dom_load_listener: bool, listener_pool: std.heap.MemoryPool(Listener), +ignore_list: std.ArrayList(*Listener), list_pool: std.heap.MemoryPool(std.DoublyLinkedList), lookup: std.HashMapUnmanaged( EventKey, @@ -76,6 +77,7 @@ pub fn init(arena: Allocator, page: *Page) EventManager { .page = page, .lookup = .{}, .arena = arena, + .ignore_list = .{}, .list_pool = .init(arena), .listener_pool = .init(arena), .dispatch_depth = 0, @@ -155,6 +157,11 @@ pub fn register(self: *EventManager, target: *EventTarget, typ: []const u8, call }; // append the listener to the list of listeners for this target gop.value_ptr.*.append(&listener.node); + + // Track load listeners for script execution ignore list + if (type_string.eql(comptime .wrap("load"))) { + try self.ignore_list.append(self.arena, listener); + } } pub fn remove(self: *EventManager, target: *EventTarget, typ: []const u8, callback: Callback, use_capture: bool) void { @@ -167,6 +174,10 @@ pub fn remove(self: *EventManager, target: *EventTarget, typ: []const u8, callba } } +pub fn clearIgnoreList(self: *EventManager) void { + self.ignore_list.clearRetainingCapacity(); +} + // Dispatching can be recursive from the compiler's point of view, so we need to // give it an explicit error set so that other parts of the code can use and // inferred error. @@ -178,7 +189,21 @@ const DispatchError = error{ ExecutionError, JsException, }; + +pub const DispatchOpts = struct { + // A "load" event triggered by a script (in ScriptManager) should not trigger + // a "load" listener added within that script. Therefore, any "load" listener + // that we add go into an ignore list until after the script finishes executing. + // The ignore list is only checked when apply_ignore == true, which is only + // set by the ScriptManager when raising the script's "load" event. + apply_ignore: bool = false, +}; + pub fn dispatch(self: *EventManager, target: *EventTarget, event: *Event) DispatchError!void { + return self.dispatchOpts(target, event, .{}); +} + +pub fn dispatchOpts(self: *EventManager, target: *EventTarget, event: *Event, comptime opts: DispatchOpts) DispatchError!void { defer if (!event._v8_handoff) event.deinit(false, self.page); if (comptime IS_DEBUG) { @@ -197,7 +222,7 @@ pub fn dispatch(self: *EventManager, target: *EventTarget, event: *Event) Dispat }; switch (target._type) { - .node => |node| try self.dispatchNode(node, event, &was_handled), + .node => |node| try self.dispatchNode(node, event, &was_handled, opts), .xhr, .window, .abort_signal, @@ -214,7 +239,7 @@ pub fn dispatch(self: *EventManager, target: *EventTarget, event: *Event) Dispat .event_target = @intFromPtr(target), .type_string = event._type_string, }) orelse return; - try self.dispatchAll(list, target, event, &was_handled); + try self.dispatchAll(list, target, event, &was_handled, opts); }, } } @@ -262,10 +287,10 @@ pub fn dispatchWithFunction(self: *EventManager, target: *EventTarget, event: *E .event_target = @intFromPtr(target), .type_string = event._type_string, }) orelse return; - try self.dispatchAll(list, target, event, &was_dispatched); + try self.dispatchAll(list, target, event, &was_dispatched, .{}); } -fn dispatchNode(self: *EventManager, target: *Node, event: *Event, was_handled: *bool) !void { +fn dispatchNode(self: *EventManager, target: *Node, event: *Event, was_handled: *bool, comptime opts: DispatchOpts) !void { const ShadowRoot = @import("webapi/ShadowRoot.zig"); const page = self.page; @@ -346,7 +371,7 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, was_handled: .event_target = @intFromPtr(current_target), .type_string = event._type_string, })) |list| { - try self.dispatchPhase(list, current_target, event, was_handled, true); + try self.dispatchPhase(list, current_target, event, was_handled, comptime .init(true, opts)); } } @@ -380,7 +405,7 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, was_handled: .type_string = event._type_string, .event_target = @intFromPtr(target_et), })) |list| { - try self.dispatchPhase(list, target_et, event, was_handled, null); + try self.dispatchPhase(list, target_et, event, was_handled, comptime .init(null, opts)); if (event._stop_propagation) { return; } @@ -397,13 +422,25 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, was_handled: .type_string = event._type_string, .event_target = @intFromPtr(current_target), })) |list| { - try self.dispatchPhase(list, current_target, event, was_handled, false); + try self.dispatchPhase(list, current_target, event, was_handled, comptime .init(false, opts)); } } } } -fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_target: *EventTarget, event: *Event, was_handled: *bool, comptime capture_only: ?bool) !void { +const DispatchPhaseOpts = struct { + capture_only: ?bool = null, + apply_ignore: bool = false, + + fn init(capture_only: ?bool, opts: DispatchOpts) DispatchPhaseOpts { + return .{ + .capture_only = capture_only, + .apply_ignore = opts.apply_ignore, + }; + } +}; + +fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_target: *EventTarget, event: *Event, was_handled: *bool, comptime opts: DispatchPhaseOpts) !void { const page = self.page; // Track dispatch depth for deferred removal @@ -429,7 +466,7 @@ fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_targe // Iterate through the list, stopping after we've encountered the last_listener var node = list.first; var is_done = false; - while (node) |n| { + node_loop: while (node) |n| { if (is_done) { break; } @@ -439,7 +476,7 @@ fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_targe node = n.next; // Skip non-matching listeners - if (comptime capture_only) |capture| { + if (comptime opts.capture_only) |capture| { if (listener.capture != capture) { continue; } @@ -458,6 +495,14 @@ fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_targe } } + if (comptime opts.apply_ignore) { + for (self.ignore_list.items) |ignored| { + if (ignored == listener) { + continue :node_loop; + } + } + } + // Remove "once" listeners BEFORE calling them so nested dispatches don't see them if (listener.once) { self.removeListener(list, listener); @@ -502,8 +547,8 @@ fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_targe } // Non-Node dispatching (XHR, Window without propagation) -fn dispatchAll(self: *EventManager, list: *std.DoublyLinkedList, current_target: *EventTarget, event: *Event, was_handled: *bool) !void { - return self.dispatchPhase(list, current_target, event, was_handled, null); +fn dispatchAll(self: *EventManager, list: *std.DoublyLinkedList, current_target: *EventTarget, event: *Event, was_handled: *bool, comptime opts: DispatchOpts) !void { + return self.dispatchPhase(list, current_target, event, was_handled, comptime .init(null, opts)); } fn getInlineHandler(self: *EventManager, target: *EventTarget, event: *Event) ?js.Function.Global { diff --git a/src/browser/ScriptManager.zig b/src/browser/ScriptManager.zig index f4d95230..b536e2db 100644 --- a/src/browser/ScriptManager.zig +++ b/src/browser/ScriptManager.zig @@ -20,13 +20,14 @@ const std = @import("std"); const lp = @import("lightpanda"); const builtin = @import("builtin"); -const js = @import("js/js.zig"); const log = @import("../log.zig"); +const Http = @import("../http/Http.zig"); +const String = @import("../string.zig").String; +const js = @import("js/js.zig"); const URL = @import("URL.zig"); const Page = @import("Page.zig"); const Browser = @import("Browser.zig"); -const Http = @import("../http/Http.zig"); const Element = @import("webapi/Element.zig"); @@ -830,13 +831,15 @@ pub const Script = struct { .kind = self.kind, .cacheable = cacheable, }); - self.executeCallback("error", local.toLocal(script_element._on_error), page); + self.executeCallback(comptime .wrap("error"), page); return; }; - self.executeCallback("load", local.toLocal(script_element._on_load), page); + self.executeCallback(comptime .wrap("load"), page); return; } + defer page._event_manager.clearIgnoreList(); + var try_catch: js.TryCatch = undefined; try_catch.init(local); defer try_catch.deinit(); @@ -855,7 +858,7 @@ pub const Script = struct { }; if (comptime IS_DEBUG) { - log.debug(.browser, "executed script", .{ .src = url, .success = success, .on_load = script_element._on_load != null }); + log.debug(.browser, "executed script", .{ .src = url, .success = success }); } defer { @@ -867,7 +870,7 @@ pub const Script = struct { } if (success) { - self.executeCallback("load", local.toLocal(script_element._on_load), page); + self.executeCallback(comptime .wrap("load"), page); return; } @@ -878,14 +881,12 @@ pub const Script = struct { .cacheable = cacheable, }); - self.executeCallback("error", local.toLocal(script_element._on_error), page); + self.executeCallback(comptime .wrap("error"), page); } - fn executeCallback(self: *const Script, comptime typ: []const u8, cb_: ?js.Function, page: *Page) void { - const cb = cb_ orelse return; - + fn executeCallback(self: *const Script, typ: String, page: *Page) void { const Event = @import("webapi/Event.zig"); - const event = Event.initTrusted(comptime .wrap(typ), .{}, page) catch |err| { + const event = Event.initTrusted(typ, .{}, page) catch |err| { log.warn(.js, "script internal callback", .{ .url = self.url, .type = typ, @@ -893,14 +894,11 @@ pub const Script = struct { }); return; }; - defer if (!event._v8_handoff) event.deinit(false, self.manager.page); - - var caught: js.TryCatch.Caught = undefined; - cb.tryCall(void, .{event}, &caught) catch { + page._event_manager.dispatchOpts(self.script_element.?.asNode().asEventTarget(), event, .{ .apply_ignore = true }) catch |err| { log.warn(.js, "script callback", .{ .url = self.url, .type = typ, - .caught = caught, + .err = err, }); }; } diff --git a/src/browser/tests/element/html/script/empty.js b/src/browser/tests/element/html/script/empty.js new file mode 100644 index 00000000..e69de29b diff --git a/src/browser/tests/element/html/script/script.html b/src/browser/tests/element/html/script/script.html index 3629cf1a..75547d40 100644 --- a/src/browser/tests/element/html/script/script.html +++ b/src/browser/tests/element/html/script/script.html @@ -2,11 +2,28 @@ diff --git a/src/browser/webapi/element/html/Script.zig b/src/browser/webapi/element/html/Script.zig index 92b37199..41899bb4 100644 --- a/src/browser/webapi/element/html/Script.zig +++ b/src/browser/webapi/element/html/Script.zig @@ -30,8 +30,6 @@ const Script = @This(); _proto: *HtmlElement, _src: []const u8 = "", -_on_load: ?js.Function.Global = null, -_on_error: ?js.Function.Global = null, _executed: bool = false, pub fn asElement(self: *Script) *Element { @@ -108,22 +106,6 @@ pub fn setDefer(self: *Script, value: bool, page: *Page) !void { } } -pub fn getOnLoad(self: *const Script) ?js.Function.Global { - return self._on_load; -} - -pub fn setOnLoad(self: *Script, cb: ?js.Function.Global) void { - self._on_load = cb; -} - -pub fn getOnError(self: *const Script) ?js.Function.Global { - return self._on_error; -} - -pub fn setOnError(self: *Script, cb: ?js.Function.Global) void { - self._on_error = cb; -} - pub fn getNoModule(self: *const Script) bool { return self.asConstElement().getAttributeSafe(comptime .wrap("nomodule")) != null; } @@ -147,8 +129,6 @@ pub const JsApi = struct { pub const @"type" = bridge.accessor(Script.getType, Script.setType, .{}); pub const nonce = bridge.accessor(Script.getNonce, Script.setNonce, .{}); pub const charset = bridge.accessor(Script.getCharset, Script.setCharset, .{}); - pub const onload = bridge.accessor(Script.getOnLoad, Script.setOnLoad, .{}); - pub const onerror = bridge.accessor(Script.getOnError, Script.setOnError, .{}); pub const noModule = bridge.accessor(Script.getNoModule, null, .{}); pub const innerText = bridge.accessor(_innerText, Script.setInnerText, .{}); fn _innerText(self: *Script, page: *const Page) ![]const u8 { @@ -160,26 +140,10 @@ pub const JsApi = struct { }; pub const Build = struct { - pub fn complete(node: *Node, page: *Page) !void { + pub fn complete(node: *Node, _: *Page) !void { const self = node.as(Script); const element = self.asElement(); self._src = element.getAttributeSafe(comptime .wrap("src")) orelse ""; - - if (element.getAttributeSafe(comptime .wrap("onload"))) |on_load| { - if (page.js.stringToPersistedFunction(on_load)) |func| { - self._on_load = func; - } else |err| { - log.err(.js, "script.onload", .{ .err = err, .str = on_load }); - } - } - - if (element.getAttributeSafe(comptime .wrap("onerror"))) |on_error| { - if (page.js.stringToPersistedFunction(on_error)) |func| { - self._on_error = func; - } else |err| { - log.err(.js, "script.onerror", .{ .err = err, .str = on_error }); - } - } } };