Fix event handler attributes

[rexxars]

EventTarget reads an on<type> property off the target on every dispatch and invokes it ahead of any listener registered with addEventListener(). No standard describes that, so on<type> handlers run out of order, and a class that implements an on<type> accessor the usual way (a getter, plus addEventListener() in the setter) has its handler invoked twice per event. The eventsource package does exactly that, so on Workers es.onmessage = handler runs handler twice for every message.

Three commits. Two add a compatibility flag, neither with an enable date yet:

• spec_compliant_event_handler_attributes fixes the above.
• spec_compliant_message_event_origin reports "" instead of null when a MessageEvent has no origin, and gives a URL-backed WebSocket the origin of its URL.

The third has no flag, on review feedback that it does not need one: isTrusted is now false on MessageEvent, CustomEvent, ErrorEvent and CloseEvent constructed from JavaScript, matching new Event(), which already got it right.

Two things need your call: which enable dates you want for the two flags, and whether the global scope carve-out is acceptable. Those and the rest of the reasoning are below, folded up.

Repros, and why our own classes never double-fired

The issue's repro. A plain EventTarget has no event handler attributes, so onhit should be an inert property that nothing reads:

const x = new EventTarget();
x.addEventListener('hit', () => console.log('a'));
x.onhit = () => console.log('[SHOULD NOT FIRE]');
x.addEventListener('hit', () => console.log('c'));
x.dispatchEvent(new Event('hit'));

workerd prints [SHOULD NOT FIRE] a c. Node, Chrome, Safari, Firefox, Deno and Bun print a c.

The double invocation needs an accessor that registers a listener, which is what eventsource does:

public get onmessage() {
  return this.#onMessage
}
public set onmessage(value) {
  if (this.#onMessage) {
    this.removeEventListener('message', this.#onMessage)
  }
  this.#onMessage = value
  if (value) {
    this.addEventListener('message', value)
  }
}

Our lookup finds the getter, and the listener the setter registered is in the table, so the handler runs twice.

Our own classes never hit that, because their on<type> accessors only stored a value and relied on the lookup to invoke it rather than registering a listener. basics.h said so out loud for AbortSignal: "our EventTarget implementation will automatically support onabort being set as an own property". So they fire once, but always first. Setting onmessage between two addEventListener('message') calls on the built-in EventSource gives:

• today: onmessage, listener-1, listener-2
• with the flag: listener-1, onmessage, listener-2

That workaround is not available to user code, which cannot opt out of the lookup. It also only bites accessors that have a getter, since the lookup reads the property: a setter-only accessor fires once.

What spec_compliant_event_handler_attributes changes

EventTarget no longer looks up on<type> properties. EventTarget::setEventHandlerAttribute() implements the HTML standard's event handler IDL attribute behavior instead, and the interfaces that the standards give those attributes to delegate to it:

• assigning a handler registers an ordinary listener, so it fires in registration order and only once
• reassigning replaces the value without moving the registration, so the handler keeps its position in the listener list
• assigning null, or anything that is not an object, removes it
• a non-callable object is retained and handed back by the getter, but never invoked
• this inside the handler is the object the handler was set on
• returning false cancels the event, and any other return value is ignored, per the event handler processing algorithm

Listeners registered with addEventListener() keep the opposite workerd rule, where returning true cancels. That is deliberate: the standard ignores a listener's return value entirely, so there is no spec behavior to move them to, and changing it would be a far wider break than this flag.

Covers AbortSignal.onabort, MessagePort.onmessage/onmessageerror/onclose, EventSource.onopen/onmessage/onerror, and WebSocket.onopen/onmessage/onclose/onerror. WebSocket had no accessors at all and depended entirely on the property lookup, so those four are new. MessagePort gets all three of its spec'd handlers rather than only onmessage: the HTML IDL puts onclose on MessagePort itself and the other two on the MessageEventTarget mixin it includes.

Two knock-on effects. Setting on<customtype> for an event type the interface does not define stops doing anything, which is standard behavior but is more than a reordering; eventsource-test.js relied on it via eventsource.ontest and now uses addEventListener. And twelve WPT websocket cases move from expected failure to passing, so their entries come out of src/wpt/websockets-test.ts.

The global scope keeps the old behavior, and I would like a second opinion

WorkerGlobalScope opts back into the property lookup regardless of the flag, so onfetch, onscheduled and friends are unchanged. That leaves the global as a deliberate remaining deviation. Some of its handlers are standardized (onerror, onunhandledrejection and onrejectionhandled on WorkerGlobalScope, onfetch on ServiceWorkerGlobalScope) and some are workerd-specific (scheduled, tail, trace, alarm, queue), so aligning it means picking those apart. Real accessors would also add properties to the global object, which WorkerGlobalScope's own warning comment treats as a breaking change in itself.

Two things surfaced while scoping it, worth recording before anyone attempts the conversion:

• onerror cancels on a true return, not false, because it is an OnErrorEventHandler. The global dispatches ErrorEvent at global-scope.c++:1116 and uses the dispatch result to decide whether to log to the console, so converting it with the ordinary handler rule would silently break globalThis.onerror = () => true suppressing that log.
• globalThis.onscheduled and onqueue have never worked. Both paths bail out on getHandlerCount(...) == 0 (global-scope.c++:557, queue.c++:654), and the property lookup does not contribute to that count, so they throw "No event listener registered" before dispatch.

So the global wants a designed pass on which handlers should exist as properties, not a mechanical conversion. To keep that path open, the lookup is skipped for any type that has an active event handler attribute, so accessors can be added there a few at a time without double-firing.

The alternative is dropping the carve-out entirely and letting the flag remove the lookup on the global too, which breaks globalThis.onfetch = fn for service-worker Workers. Smaller change than doing the global properly, and I am happy to make it instead.

The origin flag, and the unflagged isTrusted fix

Neither has an issue behind it. Both came out of the same eventsource port, which is where #6995 came from too, so the motivation is porting libraries written against standard event semantics. Each is its own commit, so either can be dropped without touching the work above.

spec_compliant_message_event_origin. A message event's origin is internally nullable, and the standard's getter reports the empty string for the null case; MessageEventInit's "" default is a separate supporting rule. That covers a MessagePort message and a WebSocketPair endpoint. Separately, a WebSocket opened from a URL now reports the serialized origin of that URL, which the WebSocket standard requires and we did not do: new WebSocket("wss://example.com/chat") delivers messages with an origin of "wss://example.com". LegacyWebSocketAdapter parses its URL once, in the two constructors that receive one. Worth having because an origin check is how postMessage-shaped code decides whether to trust a message, and on a WebSocket you currently cannot perform one at all. getOrigin() still returns a kj::Maybe, so the generated type is narrowed to string with a flag-conditional JSG_TS_OVERRIDE.

isTrusted, no flag. False on MessageEvent, CustomEvent, ErrorEvent and CloseEvent constructed from JS. new Event() already got this right. Worth having because isTrusted exists so code can tell runtime-generated events from script-constructed ones, and reporting true for the latter made it a property that lied. This started behind a flag and lost it on review feedback, so it takes effect as soon as it lands. Those four share their C++ constructors with internal callers that legitimately produce trusted events, so the JS-facing constructor() factories call Event::markConstructedFromJs() rather than changing the constructor default. Events the runtime creates stay trusted, including the ones behind ExtendableEvent::waitUntil()'s trusted-event requirement. One unrelated line rides along: tools/base.eslint.config.mjs was missing ErrorEvent from its globals list.

Enable dates, testing, and why this is three flags in one PR

Neither flag has an enable date, so they are opt-in until you decide on dates and the documentation is agreed, which is what docs/reference/adding-a-compatibility-flag.md asks for. What dates do you want, and in this PR or a follow-up? The matching cloudflare-docs PR is open and needs the same dates in enable_date and sort_date, so I will update both together.

Because the flags have no date, the @all-compat-flags variants do not exercise them, so every test that depends on one names it explicitly. event-handler-attributes-test.wd-test and websocket-origin-test.wd-test run the same file twice with the flag on and off, events-test.wd-test gained a second service, and the websockets and dom/events WPT targets list the handler flag in compat_flags. The isTrusted test needs none of that, since it is unconditional. Checking the WebSocket origin needs a real connection, since a WebSocketPair endpoint has no URL, so that test runs against a small sidecar server.

Separate commits because they are three observable surfaces with different migration risks, so they stay independently revertible. One PR because they are logically independent but touch the same files, and splitting would duplicate test scaffolding. Happy to split if you would rather review them apart; the natural seam is the first commit on its own, since it is the largest and carries the open question above.

Finally, #6995 rewrites MessageEvent's constructors and adds origin to MessageEventInit. It does not change the origin default, and it asks the compatibility flag question these commits answer. The two overlap textually in events.h and events.c++ but not semantically. Whichever lands second, I will rebase.

Fixes #6022

[jasnell]

Thank you for the contribution. One note... I appreciate the PR description but it could have been way shorter ;-) ... given how verbose it is, i'm assuming it was written by an agent. On future PRs, if you happen to make any, ask the agent to trim it down and be more concise :-) ...

We'll have to give some thought to this. Eliminating the behavior on some but not all EventTargets can add confusion.

And I an curious, your original example:

const x = new EventTarget();
x.addEventListener('hit', () => console.log('a'));
x.onhit = () => console.log('[SHOULD NOT FIRE]');
x.addEventListener('hit', () => console.log('c'));
x.dispatchEvent(new Event('hit'));

With the result: workerd prints [SHOULD NOT FIRE] a c. Node, Chrome, Safari, Firefox, Deno and Bun print a c.

Your claim is, "anything that subclasses EventTarget and implements an on accessor using addEventListener() gets its handler invoked twice per event" ... but I'm not seeing anything invoked twice. There's one dispatch with one set of handlers, of which the on* is included. The handlers are invoked only once.

Good catch on the trusted issue with that not carrying through, I think that's a change that's worth making separately. And honest... I don't think we really actually need a compat flag for. The likelihood that anyone is relying on trusted: true for these is exceedingly low to non-existent.

[rexxars]

I appreciate the PR description but it could have been way shorter ;-)

Thanks for the heads up - it could have, but I chose not to. I had a fun time trying to understand the behavior here, and went back and forth many times on how much or little to change, whether to split things up into multiple PRs etc - so I figured I'd rather leave it verbose so that any reviewer could understand why the changes appear to be as broad as they are. But suppose I could put some of those salient details into a... <details> block :)

Will update.

We'll have to give some thought to this. Eliminating the behavior on some but not all EventTargets can add confusion.

Agreed! Again, went back and forth on this - ideally I'd like to see all of them use the same behavior, but it felt like a scarier change to make - I could see more people relying on the existing behavior than with the other changes in this PR.

Your claim is, "anything that subclasses EventTarget and implements an on accessor using addEventListener() gets its handler invoked twice per event" ... but I'm not seeing anything invoked twice. There's one dispatch with one set of handlers, of which the on* is included. The handlers are invoked only once.

You're right, I was mixing up two examples from #6022. The problem in the original snippet was that it fired at all, since you shouldn't be able to attach event handlers using on* on plain EventTargets attributes (…and that it fired ahead of the listeners).

The double invocation is the second repro in #6022 - something like this shows the double invocation:

class Emitter extends EventTarget {
  #onhit;
  get onhit() {
    return this.#onhit;
  }
  set onhit(fn) {
    this.#onhit = fn;
    this.addEventListener('hit', fn);
  }
}

const x = new Emitter();
x.onhit = () => console.log('b');
x.dispatchEvent(new Event('hit'));
// Fires 'b' twice

Good catch on the trusted issue with that not carrying through, I think that's a change that's worth making separately. And honest... I don't think we really actually need a compat flag for. The likelihood that anyone is relying on trusted: true for these is exceedingly low to non-existent.

Wanted to be on the safe side, but more than happy to drop the flag - I've force-pushed now.

[rexxars]

Oh, also wanted to mention: #6995 is perhaps an easier approval? Smaller in scope, and it blocks the eventsource module from working on workerd. The on* handlers also does, for full compatibility - but most people use addEventListener, so if you stay away from the former, the rest should at least work.