webapi: Fix script load/error events

1 - inline scripts don't fire anything
2 - load fires even on a JS throw

https://github.com/lightpanda-io/browser/pull/1629, so that is now removed.

Tested the behavior extensively in chrome/firefox (new test files) and matched
our implementation to those tests.

Fixes https://github.com/lightpanda-io/browser/issues/3069

src/browser/EventManager.zig

@@ -50,12 +50,9 @@ base: EventManagerBase,
 // 'load' event (e.g. amazon product page has no listener and ~350 resources)
 has_dom_load_listener: bool,
 
-ignore_list: std.ArrayList(*Listener),
-
 pub fn init(arena: Allocator, frame: *Frame) EventManager {
     return .{
         .frame = frame,
-        .ignore_list = .empty,
         .has_dom_load_listener = false,
         .base = EventManagerBase.init(arena),
     };
@@ -64,14 +61,9 @@ pub fn init(arena: Allocator, frame: *Frame) EventManager {
 pub fn register(self: *EventManager, target: *EventTarget, typ: []const u8, callback: Callback, opts: RegisterOptions) !void {
     const listener = (try self.base.register(target, typ, callback, opts)) orelse return;
 
-    if (listener.typ.eql(comptime .wrap("load"))) {
-        if (target._type == .node) {
-            // Track load listeners on DOM nodes for optimization
-            self.has_dom_load_listener = true;
-        }
-        // Track load listeners for script execution ignore list. See the
-        // `apply_ignore` field of DispatchOpts
-        try self.ignore_list.append(self.base.arena, listener);
+    // Track load listeners on DOM nodes for optimization
+    if (target._type == .node and listener.typ.eql(comptime .wrap("load"))) {
+        self.has_dom_load_listener = true;
     }
 }
 
@@ -79,27 +71,10 @@ pub fn remove(self: *EventManager, target: *EventTarget, typ: []const u8, callba
     self.base.remove(target, typ, callback, use_capture);
 }
 
-pub fn clearIgnoreList(self: *EventManager) void {
-    self.ignore_list.clearRetainingCapacity();
-}
-
 // Re-export DispatchError from base
 pub const DispatchError = EventManagerBase.DispatchError;
 
-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 {
     event.acquireRef();
     defer _ = event.releaseRef(self.frame._page);
 
@@ -111,7 +86,7 @@ pub fn dispatchOpts(self: *EventManager, target: *EventTarget, event: *Event, co
     }
 
     switch (target._type) {
-        .node => |node| try self.dispatchNode(node, event, opts),
+        .node => |node| try self.dispatchNode(node, event),
         .xhr => |xhr| try self.dispatchDirect(target, event, xhr.inlineHandler(event._type_string), .{ .context = "dispatch" }),
         .window => |w| try self.dispatchDirect(target, event, windowInlineHandler(w, event._type_string), .{ .context = "dispatch" }),
         else => try self.dispatchDirect(target, event, null, .{ .context = "dispatch" }),
@@ -161,7 +136,7 @@ pub fn hasDirectListeners(self: *EventManager, target: *EventTarget, typ: []cons
     return self.base.hasDirectListeners(target, typ, handler);
 }
 
-fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts: DispatchOpts) !void {
+fn dispatchNode(self: *EventManager, target: *Node, event: *Event) !void {
     {
         const et = target.asEventTarget();
         event._target = et;
@@ -329,7 +304,7 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts
         if (event._stop_propagation) return;
         const current_target = path[i];
         if (self.base.getListeners(current_target, event._type_string)) |list| {
-            try self.dispatchPhase(list, current_target, event, &was_handled, &ls.local, comptime .init(true, opts));
+            try self.dispatchPhase(list, current_target, event, &was_handled, &ls.local, true);
         }
     }
 
@@ -372,13 +347,13 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts
         // of the listener list: a bubble listener added while running the
         // target's capture listeners must run.
         if (self.base.getListeners(target_et, event._type_string)) |list| {
-            try self.dispatchPhase(list, target_et, event, &was_handled, &ls.local, comptime .init(true, opts));
+            try self.dispatchPhase(list, target_et, event, &was_handled, &ls.local, true);
             if (event._stop_propagation) {
                 return;
             }
         }
         if (self.base.getListeners(target_et, event._type_string)) |list| {
-            try self.dispatchPhase(list, target_et, event, &was_handled, &ls.local, comptime .init(false, opts));
+            try self.dispatchPhase(list, target_et, event, &was_handled, &ls.local, false);
             if (event._stop_propagation) {
                 return;
             }
@@ -428,7 +403,7 @@ fn dispatchNode(self: *EventManager, target: *Node, event: *Event, comptime opts
             }
 
             if (self.base.getListeners(current_target, event._type_string)) |list| {
-                try self.dispatchPhase(list, current_target, event, &was_handled, &ls.local, comptime .init(false, opts));
+                try self.dispatchPhase(list, current_target, event, &was_handled, &ls.local, false);
             }
         }
     }
@@ -447,19 +422,7 @@ fn currentEventForTarget(target: *EventTarget, event: *Event) ?*Event {
     return if (rootIsShadowRoot(target)) null else event;
 }
 
-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, local: *const js.Local, comptime opts: DispatchPhaseOpts) !void {
+fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_target: *EventTarget, event: *Event, was_handled: *bool, local: *const js.Local, comptime capture_only: ?bool) !void {
     const frame = self.frame;
     const base = &self.base;
 
@@ -489,7 +452,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;
-    node_loop: while (node) |n| {
+    while (node) |n| {
         if (is_done) {
             break;
         }
@@ -499,7 +462,7 @@ fn dispatchPhase(self: *EventManager, list: *std.DoublyLinkedList, current_targe
         node = n.next;
 
         // Skip non-matching listeners
-        if (comptime opts.capture_only) |capture| {
+        if (comptime capture_only) |capture| {
             if (listener.capture != capture) {
                 continue;
             }
@@ -518,14 +481,6 @@ 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) {
             base.removeListener(list, listener);

src/browser/ScriptManagerBase.zig

@@ -930,8 +930,6 @@ pub const Script = struct {
             return;
         }
 
-        defer frame._event_manager.clearIgnoreList();
-
         var try_catch: js.TryCatch = undefined;
         try_catch.init(local);
         defer try_catch.deinit();
@@ -986,12 +984,23 @@ pub const Script = struct {
             };
         }
 
-        self.executeCallback(comptime .wrap("error"));
+        // a classic script that throws is still "loaded". For a module, we can't
+        // currently tell the difference between loaded, but errors, and failed
+        // to load, so we stick with just error.
+        self.executeCallback(switch (fe.kind) {
+            .javascript => comptime .wrap("load"),
+            else => comptime .wrap("error"),
+        });
     }
 
     // Frame-only: fires load/error on the <script> element itself,
     // synchronously. Hint <link> events go through queueHintEvent instead.
     pub fn executeCallback(self: *const Script, typ: String) void {
+        if (self.source != .remote) {
+            // an inline script fires nothing
+            return;
+        }
+
         const fe = self.extra.frame;
         const frame = fe.frame;
         const Event = @import("webapi/Event.zig");
@@ -1003,7 +1012,7 @@ pub const Script = struct {
             });
             return;
         };
-        frame._event_manager.dispatchOpts(fe.script_element.asNode().asEventTarget(), event, .{ .apply_ignore = true }) catch |err| {
+        frame._event_manager.dispatch(fe.script_element.asNode().asEventTarget(), event) catch |err| {
             log.warn(.js, "script callback", .{
                 .url = self.url,
                 .type = typ,

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

@@ -0,0 +1,81 @@
+<!DOCTYPE html>
+<head></head>
+<script src="../../../testing.js"></script>
+
+<!--
+  A <script> fires load/error at its own element only when it is "from an
+  external file": the last step of the spec's "execute the script element" is
+  conditional on that flag, and it is not conditional on the script having run
+  cleanly. So an inline script fires nothing, and an external script fires load
+  even when it throws (the exception goes to window, not to an error event on
+  the element).
+-->
+
+<script id=recorder>
+  window.loads = [];
+  window.errors = [];
+  const record = (into) => (e) => {
+    const el = e.target;
+    into.push(el.src ? el.src.split('/').pop() : 'inline');
+  };
+  document.addEventListener('load', record(loads), true);
+  document.addEventListener('error', record(errors), true);
+  testing.expectEqual([], loads);
+</script>
+
+<script>window.parser_inline_ran = true;</script>
+
+<!-- an inline script that throws reports to window.onerror, it does not fire
+     an error event at its element -->
+<script>throw new Error('deliberate');</script>
+
+<script id=inline_fires_nothing>
+  testing.expectTrue(window.parser_inline_ran);
+
+  // an appended inline script executes synchronously, still fires nothing
+  const s = document.createElement('script');
+  s.textContent = 'window.appended_inline_ran = true;';
+  document.head.appendChild(s);
+  testing.expectTrue(window.appended_inline_ran);
+
+  testing.expectEqual([], loads);
+  testing.expectEqual([], errors);
+</script>
+
+<script id=external_self_listener>
+  testing.async().then((state) => {
+    window.self_load = 0;
+    const s = document.createElement('script');
+    s.src = 'load_self.js';
+    // resolve on the next task: onload is registered before the listener that
+    // load_self.js adds to itself, and there's a microtask checkpoint between
+    // two listeners of the same dispatch
+    s.onload = () => setTimeout(() => state.resolve(), 0);
+    document.head.appendChild(s);
+    setTimeout(() => state.resolve(), 500);
+
+    state.done(() => {
+      // load_self.js registers a load listener on itself while it runs
+      testing.expectEqual(1, window.self_load);
+      testing.expectEqual(true, loads.includes('load_self.js'));
+    });
+  });
+</script>
+
+<script id=external_throws>
+  testing.async().then((state) => {
+    window.throws_ran = false;
+    const s = document.createElement('script');
+    s.src = 'throws.js';
+    s.onload = () => setTimeout(() => state.resolve(), 0);
+    s.onerror = () => setTimeout(() => state.resolve(), 0);
+    document.head.appendChild(s);
+    setTimeout(() => state.resolve(), 500);
+
+    state.done(() => {
+      testing.expectTrue(window.throws_ran);
+      testing.expectEqual(true, loads.includes('throws.js'));
+      testing.expectEqual(false, errors.includes('throws.js'));
+    });
+  });
+</script>

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

@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<head></head>
+<script src="../../../testing.js"></script>
+
+<!--
+  https://github.com/lightpanda-io/browser/issues/3069
+
+  The listener is registered from a timer, i.e. outside of any script execution,
+  and must still receive the load event of the script inserted by that same
+  callback. This is the shape every "load a script, resolve a promise from its
+  load event" loader uses.
+
+  Keep this file free of any other external script: a listener registered
+  outside of a script execution used to be suppressed only until the *next*
+  script ran, so a second script here would hide the regression.
+-->
+
+<script id=listener_from_timer>
+  testing.async().then((state) => {
+    window.load_fired = 0;
+    window.error_fired = 0;
+
+    setTimeout(() => {
+      const s = document.createElement('script');
+      s.addEventListener('load', () => {
+        window.load_fired += 1;
+        state.resolve();
+      });
+      s.addEventListener('error', () => {
+        window.error_fired += 1;
+        state.resolve();
+      });
+      s.src = 'empty.js';
+      document.head.appendChild(s);
+    }, 20);
+
+    // so that a missing event is an assertion failure, not a runner timeout
+    setTimeout(() => state.resolve(), 500);
+
+    state.done(() => {
+      testing.expectEqual(1, window.load_fired);
+      testing.expectEqual(0, window.error_fired);
+    });
+  });
+</script>

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

@@ -0,0 +1,6 @@
+// The load event is fired at the element *after* this script finishes
+// executing, so a listener registered here, from inside the script itself,
+// still receives it.
+document.currentScript.addEventListener('load', () => {
+  window.self_load += 1;
+});

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

@@ -0,0 +1,2 @@
+window.throws_ran = true;
+throw new Error('external script that throws still fires load, not error');

src/browser/tests/testing.js

@@ -76,7 +76,9 @@
   }
 
   async function async() {
-    const script_id = (IS_TEST_RUNNER) ? document.currentScript.id : 'cannot track module id in FF/Chrome';
+    // currentScript is null while a module runs in FF/Chrome (the test runner
+    // does set it), which leaves us with no way to attribute the observations.
+    const script_id = document.currentScript ? document.currentScript.id : 'cannot track module id in FF/Chrome';
 
     if (async_seen.has(script_id) && IS_TEST_RUNNER) {
       throw new Error(`testing.async() called more than once for script '${script_id}'. A script may only register one async block (the runner can declare success in the gap between two of them); split the test into separate <script> tags.`);