Merge pull request #3159 from lightpanda-io/custom-element-errors

webapi: Report CustomElement errors via window.reportError

src/browser/EventManagerBase.zig

@@ -219,6 +219,7 @@ pub const DispatchError = error{
 pub const DispatchDirectOptions = struct {
     context: []const u8 = "dispatchDirect",
     inject_target: bool = true,
+    run_microtasks: bool = true,
 };
 
 /// Direct dispatch for non-DOM targets. No propagation - just calls the property
@@ -249,7 +250,9 @@ pub fn dispatchDirect(
     var ls: js.Local.Scope = undefined;
     ctx.localScope(&ls);
     defer {
-        ls.local.runMicrotasks();
+        if (comptime opts.run_microtasks) {
+            ls.local.runMicrotasks();
+        }
         ls.deinit();
     }
 

src/browser/Frame.zig

@@ -232,6 +232,10 @@ _customized_builtin_disconnected_callback_invoked: std.AutoHashMapUnmanaged(*Ele
 // The constructor can access this to get the element being upgraded.
 _upgrading_element: ?*Node = null,
 
+// _upgrading_element can be consumed once. A second HTMLElement construction
+// during upgrade is a TypeError.
+_upgrading_consumed: bool = false,
+
 // Set when materializing the fragment parser's context element. The element
 // is never inserted into the tree so if its a custom element ,we must not run
 // its constructor (else we'll end up in an endless loop if the constructor

src/browser/frame/node_factory.zig

@@ -948,11 +948,15 @@ pub fn createElementNS(frame: *Frame, namespace: Element.Namespace, name: []cons
 // Runs a custom element constructor for a token being created (parser or
 // createElement), and validates the result against the post-conditions.
 fn constructForToken(frame: *Frame, definition: *CustomElementDefinition, tag_name: String, comptime from_parser: bool) !*Element {
-
     // This is a creation, not an upgrade. super() ha to build a new element
     const prev_upgrading = frame._upgrading_element;
+    const prev_consumed = frame._upgrading_consumed;
     frame._upgrading_element = null;
-    defer frame._upgrading_element = prev_upgrading;
+    frame._upgrading_consumed = false;
+    defer {
+        frame._upgrading_element = prev_upgrading;
+        frame._upgrading_consumed = prev_consumed;
+    }
 
     var ls: JS.Local.Scope = undefined;
     frame.js.localScope(&ls);
@@ -968,38 +972,64 @@ fn constructForToken(frame: *Frame, definition: *CustomElementDefinition, tag_na
     };
 
     const name = tag_name.str();
-    var caught: JS.TryCatch.Caught = .{};
-    const object = ls.toLocal(definition.constructor).newInstance(&caught) catch |err| {
-        log.warn(.js, "custom element constructor", .{ .name = name, .err = err, .caught = caught, .type = frame._type, .url = frame.url });
+    const local = &ls.local;
+
+    var try_catch: JS.TryCatch = undefined;
+    try_catch.init(local);
+    defer try_catch.deinit();
+
+    const object = ls.toLocal(definition.constructor).newInstanceThrow() catch |err| {
+        if (err != error.ExecutionTerminated) {
+            log.warn(.js, "custom element constructor", .{ .name = name, .err = err, .type = frame._type, .url = frame.url });
+            if (try_catch.exceptionValue()) |exc| {
+                // Spec: report the exception
+                frame.window.reportError(exc, frame) catch {};
+            }
+        }
         return err;
     };
 
-    const reason: []const u8 = blk: {
-        // validate the result
-        const node = object.toZig(*Node) catch break :blk "not a node";
-        const element = node.is(Element) orelse break :blk "not an element";
+    const reason: []const u8, const exc: JS.Value = blk: {
+        // Validate the result. A result that isn't an HTMLElement is a
+        // TypeError; any other violation is a NotSupportedError.
+        const node = object.toZig(*Node) catch break :blk .{ "not a node", typeError(local) };
+        const element = node.is(Element) orelse break :blk .{ "not an element", typeError(local) };
         if (element._namespace != .html) {
-            break :blk "wrong namespace";
+            break :blk .{ "wrong namespace", typeError(local) };
         }
         if (node._parent != null) {
-            break :blk "has a parent";
+            break :blk .{ "has a parent", notSupportedError(local) };
         }
         if (node.firstChild() != null) {
-            break :blk "has children";
+            break :blk .{ "has children", notSupportedError(local) };
         }
         if (element._attributes.isEmpty() == false) {
-            break :blk "has attributes";
+            break :blk .{ "has attributes", notSupportedError(local) };
+        }
+        if (node.ownerDocument(frame) != frame.document) {
+            break :blk .{ "wrong document", notSupportedError(local) };
         }
         if (std.mem.eql(u8, element.getTagNameLower(), name) == false) {
-            break :blk "wrong local name";
+            break :blk .{ "wrong local name", notSupportedError(local) };
         }
         return element;
     };
 
     log.warn(.js, "custom element not usable", .{ .name = name, .reason = reason, .type = frame._type, .url = frame.url });
+    frame.window.reportError(exc, frame) catch {};
     return error.CustomElementConstructionFailed;
 }
 
+fn typeError(local: *const JS.Local) JS.Value {
+    return .{ .local = local, .handle = local.isolate.createTypeError("Invalid custom element constructor return value") };
+}
+
+fn notSupportedError(local: *const JS.Local) JS.Value {
+    const DOMException = @import("../webapi/DOMException.zig");
+    const ex = DOMException.fromError(error.NotSupported).?;
+    return local.zigValueToJs(ex, .{}) catch .{ .local = local, .handle = local.isolate.createError("not supported") };
+}
+
 fn createHtmlElementT(frame: *Frame, comptime E: type, namespace: Element.Namespace, attribute_iterator: anytype, html_element: E) !*Node {
     const html_element_ptr = try frame._factory.htmlElement(html_element);
     const element = html_element_ptr.asElement();

src/browser/js/Function.zig

@@ -49,6 +49,21 @@ pub fn withThis(self: *const Function, value: anytype) !Function {
 }
 
 pub fn newInstance(self: *const Function, caught: *js.TryCatch.Caught) !js.Object {
+    var try_catch: js.TryCatch = undefined;
+    try_catch.init(self.local);
+    defer try_catch.deinit();
+
+    return self.newInstanceThrow() catch |err| {
+        if (err == error.JsConstructorFailed) {
+            caught.* = try_catch.caughtOrError(self.local.call_arena, error.Unknown);
+        }
+        return err;
+    };
+}
+
+// Like newInstance, but with no TryCatch of our own. Gives more flexibility to
+// the caller on how to handle the error (e.g. window.reportError)
+pub fn newInstanceThrow(self: *const Function) !js.Object {
     const local = self.local;
 
     if (comptime lp.IS_DEBUG == false) {
@@ -70,17 +85,12 @@ pub fn newInstance(self: *const Function, caught: *js.TryCatch.Caught) !js.Objec
         return error.ExecutionTerminated;
     }
 
-    var try_catch: js.TryCatch = undefined;
-    try_catch.init(local);
-    defer try_catch.deinit();
-
     // This creates a new instance using this Function as a constructor.
     // const c_args = @as(?[*]const ?*c.Value, @ptrCast(&.{}));
     const handle = v8.v8__Function__NewInstance(self.handle, local.handle, 0, null) orelse {
         if (local.ctx.env.terminatePending()) {
             return error.ExecutionTerminated;
         }
-        caught.* = try_catch.caughtOrError(local.call_arena, error.Unknown);
         return error.JsConstructorFailed;
     };
 

src/browser/tests/custom_elements/error_reporting.html

@@ -0,0 +1,286 @@
+<!DOCTYPE html>
+<script src="../testing.js"></script>
+
+<script id="create_element_reports_thrown_exception">
+    {
+        // https://dom.spec.whatwg.org/#concept-create-element -- a failed
+        // synchronous construction is reported to the global, and the very
+        // exception object must reach window.onerror.
+        const thrown = new Error('from constructor');
+        class ThrowsInConstructor extends HTMLElement {
+            constructor() {
+                super();
+                throw thrown;
+            }
+        }
+        customElements.define('ce-report-throw', ThrowsInConstructor);
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        const el = document.createElement('ce-report-throw');
+        window.onerror = null;
+
+        testing.expectEqual(true, reported === thrown);
+        testing.expectEqual(true, el instanceof HTMLUnknownElement);
+    }
+</script>
+
+<script id="create_element_reports_type_error_for_bad_return">
+    {
+        class ReturnsObject extends HTMLElement {
+            constructor() {
+                super();
+                return {};
+            }
+        }
+        customElements.define('ce-report-bad-return', ReturnsObject);
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        document.createElement('ce-report-bad-return');
+        window.onerror = null;
+
+        testing.expectEqual('TypeError', reported.name);
+    }
+</script>
+
+<script id="create_element_reports_not_supported_for_state_violation">
+    {
+        class AddsAttribute extends HTMLElement {
+            constructor() {
+                super();
+                this.setAttribute('id', 'nope');
+            }
+        }
+        customElements.define('ce-report-attribute', AddsAttribute);
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        document.createElement('ce-report-attribute');
+        window.onerror = null;
+
+        testing.expectEqual('NotSupportedError', reported.name);
+    }
+</script>
+
+<script id="create_element_rejects_adoption_into_another_document">
+    {
+        const other = document.implementation.createHTMLDocument();
+        class AdoptsSelf extends HTMLElement {
+            constructor() {
+                super();
+                other.adoptNode(this);
+            }
+        }
+        customElements.define('ce-report-adopts-self', AdoptsSelf);
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        const el = document.createElement('ce-report-adopts-self');
+        window.onerror = null;
+
+        testing.expectEqual('NotSupportedError', reported.name);
+        testing.expectEqual(true, el instanceof HTMLUnknownElement);
+    }
+</script>
+
+<script id="upgrade_rejects_constructor_returning_another_element">
+    {
+        // https://html.spec.whatwg.org/#upgrades -- the construction result
+        // must be SameValue with the element being upgraded.
+        const a = document.createElement('ce-report-same-value');
+        const b = document.createElement('ce-report-same-value');
+        document.documentElement.appendChild(a);
+        document.documentElement.appendChild(b);
+
+        class ReturnsOther extends HTMLElement {
+            constructor() {
+                super();
+                if (this === a) {
+                    return b;
+                }
+            }
+        }
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        customElements.define('ce-report-same-value', ReturnsOther);
+        window.onerror = null;
+
+        testing.expectEqual('TypeError', reported.name);
+        // a's failed upgrade must not prevent b's.
+        testing.expectEqual(true, b instanceof ReturnsOther);
+    }
+</script>
+
+<script id="upgrade_rejects_self_instantiation">
+    {
+        // The construction stack's "already constructed" marker: a constructor
+        // instantiating itself during its own upgrade is a TypeError.
+        class InstantiatesItself extends HTMLElement {
+            constructor(stop) {
+                super();
+                if (!stop) {
+                    new InstantiatesItself(true);
+                }
+            }
+        }
+        const el = document.createElement('ce-instantiates-itself');
+        document.documentElement.appendChild(el);
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        customElements.define('ce-instantiates-itself', InstantiatesItself);
+        window.onerror = null;
+
+        testing.expectEqual('TypeError', reported.name);
+    }
+</script>
+
+<script id="failed_upgrade_is_not_retried">
+    {
+        let attempts = 0;
+        class FailsToUpgrade extends HTMLElement {
+            constructor() {
+                super();
+                attempts += 1;
+                throw new Error('nope');
+            }
+        }
+        const el = document.createElement('ce-fails-to-upgrade');
+        document.documentElement.appendChild(el);
+
+        window.onerror = () => true;
+        customElements.define('ce-fails-to-upgrade', FailsToUpgrade);
+
+        // Reconnecting a "failed" element must not attempt another upgrade.
+        el.remove();
+        document.documentElement.appendChild(el);
+        window.onerror = null;
+
+        testing.expectEqual(1, attempts);
+    }
+</script>
+
+<script id="parser_reports_constructor_exception">
+    {
+        // The parser's create-element path must report too, and still insert
+        // the HTMLUnknownElement fallback.
+        const thrown = new Error('from parser constructor');
+        class ParserThrows extends HTMLElement {
+            constructor() {
+                super();
+                throw thrown;
+            }
+        }
+        customElements.define('ce-parser-throws', ParserThrows);
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        const container = document.createElement('div');
+        container.innerHTML = '<ce-parser-throws></ce-parser-throws>';
+        window.onerror = null;
+
+        testing.expectEqual(true, reported === thrown);
+        testing.expectEqual(1, container.children.length);
+        testing.expectEqual(true, container.firstElementChild instanceof HTMLUnknownElement);
+    }
+</script>
+
+<script id="upgrade_rejects_self_instantiation_before_super">
+    {
+        // The before-super variant: the nested construction consumes the
+        // upgrading element, so the outer super() finds the marker set.
+        class InstantiatesBeforeSuper extends HTMLElement {
+            constructor(stop) {
+                if (!stop) {
+                    new InstantiatesBeforeSuper(true);
+                }
+                super();
+            }
+        }
+        const el = document.createElement('ce-before-super');
+        document.documentElement.appendChild(el);
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        customElements.define('ce-before-super', InstantiatesBeforeSuper);
+        window.onerror = null;
+
+        testing.expectEqual('TypeError', reported.name);
+    }
+</script>
+
+<script id="construction_during_upgrade_is_isolated">
+    {
+        // createElement of another custom element inside an upgrading
+        // constructor must not inherit the outer construction's
+        // already-constructed marker.
+        class Inner extends HTMLElement {}
+        customElements.define('ce-marker-inner', Inner);
+
+        let inner = null;
+        class Outer extends HTMLElement {
+            constructor() {
+                super();
+                inner = document.createElement('ce-marker-inner');
+            }
+        }
+        const el = document.createElement('ce-marker-outer');
+        document.documentElement.appendChild(el);
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        customElements.define('ce-marker-outer', Outer);
+        window.onerror = null;
+
+        testing.expectEqual(null, reported);
+        testing.expectEqual(true, inner instanceof Inner);
+        testing.expectEqual(true, el instanceof Outer);
+    }
+</script>
+
+<script id="report_during_report_dispatch_is_not_swallowed">
+    {
+        // Regression test: reportError's event dispatch must not pump the
+        // microtask queue. It used to, and a report fired by a pumped
+        // continuation was silently dropped by the error-reporting-mode
+        // guard.
+        class ThrowsSync extends HTMLElement {
+            constructor() {
+                super();
+                throw new Error('sync');
+            }
+        }
+        class ThrowsNested extends HTMLElement {
+            constructor() {
+                super();
+                throw new Error('nested');
+            }
+        }
+        customElements.define('ce-swallow-sync', ThrowsSync);
+        customElements.define('ce-swallow-nested', ThrowsNested);
+
+        window.__nested_report = null;
+        Promise.resolve().then(() => {
+            window.onerror = (m, u, l, c, error) => { window.__nested_report = error; return true; };
+            document.createElement('ce-swallow-nested');
+            window.onerror = null;
+        });
+
+        let reported = null;
+        window.onerror = (m, u, l, c, error) => { reported = error; return true; };
+        document.createElement('ce-swallow-sync');
+        window.onerror = null;
+        testing.expectEqual('sync', reported.message);
+    }
+</script>
+
+<script id="report_during_report_dispatch_is_not_swallowed_check">
+    {
+        // The continuation queued by the previous block has run by now (the
+        // microtask checkpoint sits between the two script evaluations) and
+        // its report must have been delivered.
+        testing.expectEqual('nested', window.__nested_report.message);
+    }
+</script>

src/browser/webapi/CustomElementRegistry.zig

@@ -181,7 +181,9 @@ fn upgradeElement(self: *CustomElementRegistry, element: *Element, frame: *Frame
         return Custom.checkAndAttachBuiltIn(element, frame);
     };
 
-    if (custom._definition != null) return;
+    if (custom._definition != null or custom._upgrade_failed) {
+        return;
+    }
 
     const name = custom._tag_name.str();
     const definition = self._definitions.get(name) orelse return;
@@ -198,19 +200,49 @@ pub fn upgradeCustomElement(custom: *Custom, definition: *CustomElementDefinitio
 
     const node = custom.asNode();
     const prev_upgrading = frame._upgrading_element;
+    const prev_consumed = frame._upgrading_consumed;
     frame._upgrading_element = node;
-    defer frame._upgrading_element = prev_upgrading;
+    frame._upgrading_consumed = false;
+    defer {
+        frame._upgrading_element = prev_upgrading;
+        frame._upgrading_consumed = prev_consumed;
+    }
 
     var ls: js.Local.Scope = undefined;
     frame.js.localScope(&ls);
     defer ls.deinit();
 
-    var caught: js.TryCatch.Caught = .{};
-    _ = ls.toLocal(definition.constructor).newInstance(&caught) catch |err| {
-        log.warn(.js, "custom element upgrade", .{ .name = definition.name, .err = err, .caught = caught });
+    const local = &ls.local;
+    var try_catch: js.TryCatch = undefined;
+    try_catch.init(local);
+    defer try_catch.deinit();
+
+    const object = ls.toLocal(definition.constructor).newInstanceThrow() catch |err| {
+        if (err == error.ExecutionTerminated) {
+            custom._definition = null;
+            return err;
+        }
+        log.warn(.js, "custom element upgrade", .{ .name = definition.name, .err = err });
+        upgradeFailed(custom);
+        if (try_catch.exceptionValue()) |exc| {
+            frame.window.reportError(exc, frame) catch {};
+        }
         return error.CustomElementUpgradeFailed;
     };
 
+    const same = if (object.toZig(*Node)) |result| result == node else |_| false;
+    if (!same) {
+        // the construction result must be the element being upgraded.
+        log.warn(.js, "custom element upgrade", .{ .name = definition.name, .reason = "constructor returned another value" });
+        upgradeFailed(custom);
+        const exc: js.Value = .{
+            .local = local,
+            .handle = local.isolate.createTypeError("custom element constructor must return the upgraded element"),
+        };
+        frame.window.reportError(exc, frame) catch {};
+        return error.CustomElementUpgradeFailed;
+    }
+
     // Enqueue attributeChangedCallback for existing observed attributes
     const element = custom.asElement();
     for (element.attributeEntries()) |*attr| {
@@ -227,6 +259,11 @@ pub fn upgradeCustomElement(custom: *Custom, definition: *CustomElementDefinitio
     }
 }
 
+fn upgradeFailed(custom: *Custom) void {
+    custom._definition = null;
+    custom._upgrade_failed = true;
+}
+
 fn validateName(name: []const u8) !void {
     if (name.len == 0) {
         return error.SyntaxError;
@@ -288,6 +325,6 @@ pub const JsApi = struct {
 
 const testing = @import("../../testing.zig");
 test "WebApi: CustomElementRegistry" {
-    testing.expectLog(&.{ .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js });
+    testing.expectLog(&.{ .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js, .js });
     try testing.htmlRunner("custom_elements", .{});
 }

src/browser/webapi/Window.zig

@@ -639,6 +639,7 @@ pub fn reportError(self: *Window, err: js.Value, frame: *Frame) !void {
     // We still dispatch so that addEventListener('error', ...) listeners fire.
     try frame._event_manager.dispatchDirect(target, event, null, .{
         .context = "window.reportError",
+        .run_microtasks = false,
     });
 
     if (comptime lp.IS_TEST == false) {

src/browser/webapi/element/Html.zig

@@ -116,6 +116,10 @@ _proto_canary: if (lp.IS_DEBUG) *Element else void = undefined,
 //    which custom element class was invoked; look it up in the registry.
 pub fn construct(new_target: js.Function, frame: *Frame) !*Element {
     if (frame._upgrading_element) |node| {
+        if (frame._upgrading_consumed) {
+            return error.TypeError;
+        }
+        frame._upgrading_consumed = true;
         return node.is(Element) orelse return error.IllegalConstructor;
     }
     return Frame.node_factory.constructCustomElement(frame, new_target);
@@ -127,6 +131,10 @@ pub fn construct(new_target: js.Function, frame: *Frame) !*Element {
 // constructors routed here.
 pub fn upgradeConstruct(frame: *Frame) !*Element {
     const node = frame._upgrading_element orelse return error.TypeError;
+    if (frame._upgrading_consumed) {
+        return error.TypeError;
+    }
+    frame._upgrading_consumed = true;
     return node.is(Element) orelse return error.TypeError;
 }
 

src/browser/webapi/element/html/Custom.zig

@@ -41,6 +41,7 @@ _tag_name: String,
 _definition: ?*CustomElementDefinition,
 _connected_callback_invoked: bool = false,
 _disconnected_callback_invoked: bool = false,
+_upgrade_failed: bool = false, // a failed upgrade is never retried
 
 pub fn asElement(self: *Custom) *Element {
     return Factory.protoOf(self).asElement();
@@ -62,6 +63,9 @@ pub fn enqueueConnectedCallbackOnElement(comptime from_parser: bool, element: *E
     if (element.is(Custom)) |custom| {
         // Upgrade if a definition exists but isn't yet attached
         if (custom._definition == null) {
+            if (custom._upgrade_failed) {
+                return;
+            }
             const name = custom._tag_name.str();
             if (frame.window._custom_elements._definitions.get(name)) |definition| {
                 const CustomElementRegistry = @import("../../CustomElementRegistry.zig");
@@ -257,9 +261,14 @@ pub fn checkAndAttachBuiltIn(element: *Element, frame: *Frame) !void {
 
     // Invoke constructor
     const prev_upgrading = frame._upgrading_element;
+    const prev_consumed = frame._upgrading_consumed;
     const node = element.asNode();
     frame._upgrading_element = node;
-    defer frame._upgrading_element = prev_upgrading;
+    frame._upgrading_consumed = false;
+    defer {
+        frame._upgrading_element = prev_upgrading;
+        frame._upgrading_consumed = prev_consumed;
+    }
 
     // PERFORMANCE OPTIMIZATION: This pattern is discouraged in general code.
     // Used here because: (1) multiple early returns before needing Local,