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

@@ -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 {
+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, null);
+    return self.dispatchPhase(list, current_target, event, was_handled, comptime .init(null, opts));
 }
 
 fn getInlineHandler(self: *EventManager, target: *EventTarget, event: *Event) ?js.Function.Global {

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 {
+    fn executeCallback(self: *const Script, typ: String, page: *Page) void {
-        const cb = cb_ orelse return;
-
         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);
+        page._event_manager.dispatchOpts(self.script_element.?.asNode().asEventTarget(), event, .{ .apply_ignore = true }) catch |err| {
-
-        var caught: js.TryCatch.Caught = undefined;
-        cb.tryCall(void, .{event}, &caught) catch {
             log.warn(.js, "script callback", .{
                 .url = self.url,
                 .type = typ,
-                .caught = caught,
+                .err = err,
             });
         };
     }

src/browser/tests/element/html/script/script.html

@@ -2,11 +2,28 @@
 <script src="../../../testing.js"></script>
 
 <script id="script">
-   {
+{
-    let s = document.createElement('script');
+   let dom_load = false;
-    testing.expectEqual('', s.src);
+   let attribute_load = false;
 
-    s.src = 'over.9000.js';
+   let s = document.createElement('script');
-    testing.expectEqual(testing.BASE_URL + 'element/html/script/over.9000.js', s.src);
+   document.documentElement.addEventListener('load', (e) => {
+      testing.expectEqual(s, e.target);
+      dom_load = true;
+   }, true);
+
+   testing.expectEqual('', s.src);
+   s.onload = function(e) {
+      testing.expectEqual(s, e.target);
+      attribute_load = true;
    }
+   s.src = 'empty.js';
+   testing.expectEqual(testing.BASE_URL + 'element/html/script/empty.js', s.src);
+   document.head.appendChild(s);
+
+   testing.eventually(() => {
+      testing.expectEqual(true, dom_load);
+      testing.expectEqual(true, attribute_load);
+   });
+}
 </script>

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