diff --git a/src/workerd/api/basics.c++ b/src/workerd/api/basics.c++ index b592e3b3ed5..aa03600020b 100644 --- a/src/workerd/api/basics.c++ +++ b/src/workerd/api/basics.c++ @@ -32,6 +32,15 @@ constexpr bool isSpecialEventType(kj::StringPtr type) { return type == "fetch" || type == "scheduled" || type == "tail" || type == "trace" || type == "alarm"; } + +// Tests may exercise EventTarget without a fully initialized JS environment, in which case no +// compatibility flags are available. Fall back to the legacy behavior there. +bool specCompliantEventHandlerAttributes(jsg::Lock& js) { + KJ_IF_SOME(flags, FeatureFlags::tryGet(js)) { + return flags.getSpecCompliantEventHandlerAttributes(); + } + return false; +} } // namespace EventTarget::NativeHandler::NativeHandler( @@ -375,6 +384,132 @@ EventTarget::EventHandlerSet& EventTarget::getOrCreate(kj::StringPtr type) { return typeMap.upsert(kj::str(type), EventHandlerSet(), [&](auto&&...) {}).value; } +kj::Maybe EventTarget::getEventHandlerAttribute(jsg::Lock& js, kj::StringPtr type) { + return eventHandlerAttributes.find(type).map( + [&](EventHandlerAttribute& attribute) { return attribute.value.getHandle(js); }); +} + +void EventTarget::setEventHandlerAttribute( + jsg::Lock& js, kj::StringPtr type, jsg::Optional maybeValue) { + kj::Maybe value; + kj::Maybe callback; + + KJ_IF_SOME(v, maybeValue) { + KJ_SWITCH_ONEOF(v) { + KJ_CASE_ONEOF(fn, jsg::Identified) { + // Per the standard, `this` within an event handler attribute is the object the handler + // is set on. + auto& handler = callback.emplace(kj::mv(fn.unwrapped)); + KJ_IF_SOME(self, JSG_THIS.tryGetHandle(js)) { + handler.setReceiver(js.v8Ref(self.As())); + } + value = jsg::JsValue(fn.identity.getHandle(js)); + } + KJ_CASE_ONEOF(other, jsg::JsValue) { + // A non-callable object is retained but never invoked. Anything else -- null, undefined, + // or a primitive -- clears the handler. + if (other.isObject()) { + value = other; + } + } + } + } + + KJ_IF_SOME(v, value) { + KJ_IF_SOME(existing, eventHandlerAttributes.find(type)) { + // Only the value changes. Holding on to the existing registration is what keeps the + // handler in its original position in the listener list. + existing.value = jsg::JsRef(js, v); + existing.callback = kj::mv(callback); + return; + } + + kj::Maybe> listener; + if (specCompliantEventHandlerAttributes(js)) { + listener = newNativeHandler( + js, kj::str(type), [this, type = kj::str(type)](jsg::Lock& js, jsg::Ref event) { + invokeEventHandlerAttribute(js, type, kj::mv(event)); + }); + } + + eventHandlerAttributes.insert(kj::str(type), + EventHandlerAttribute{ + .value = jsg::JsRef(js, v), + .callback = kj::mv(callback), + .listener = kj::mv(listener), + }); + } else { + // Dropping the entry drops the listener registration with it. + eventHandlerAttributes.erase(type); + } +} + +void EventTarget::invokeEventHandlerAttribute( + jsg::Lock& js, kj::StringPtr type, jsg::Ref event) { + KJ_IF_SOME(attribute, eventHandlerAttributes.find(type)) { + KJ_IF_SOME(callback, attribute.callback) { + invokeJavaScriptHandler(js, callback, kj::mv(event), HandlerReturn::FALSE_CANCELS); + } + } +} + +void EventTarget::invokeJavaScriptHandler( + jsg::Lock& js, HandlerFunction& callback, jsg::Ref event, HandlerReturn returnMode) { + // Per the standard, the event listener is not supposed to return any value, and if it + // does, that value is ignored. That can be somewhat problematic if the user passes an + // async function as the event handler. Doing so counts as undefined behavior and can + // introduce subtle and difficult to diagnose bugs. Here, if the handler does return a + // value, we're going to emit a warning but otherwise ignore it. The warning will only + // be emitted at most once per EventEmitter instance. + auto ret = callback(js, event.addRef()); + // Note: We used to run each handler in its own v8::TryCatch. However, due to a + // misunderstanding of the V8 API, we incorrectly believed that TryCatch mishandled + // termination (or maybe it actually did at the time), so we changed things such that + // we don't catch exceptions so the first handler to throw an exception terminates the + // loop, and the exception flows out of dispatchEvent(). In theory if multiple + // handlers were registered then maybe we ought to be running all of them even if one + // fails. This isn't entirely clear, though: in the case of 'fetch' handlers, in + // fail-closed mode, an exception from any handler should make the whole request fail, + // but then who cares if the remaining handlers run? Meanwhile, in fail-open mode, for + // consistency, we should probably trigger fallback behavior if any handler throws, so + // again it doesn't matter. For other types of handlers, e.g. WebSocket 'message', it's + // not clear why one would ever register multiple handlers. + KJ_IF_SOME(r, ret) { + auto handle = r.getHandle(js); + switch (returnMode) { + case HandlerReturn::TRUE_CANCELS: { + if (handle->IsTrue()) { + event->preventDefault(); + } + break; + } + case HandlerReturn::FALSE_CANCELS: { + // Note that the standard's "cancel the event" step sets the canceled flag, which is a + // no-op on an event that is not cancelable. Our preventDefault() does not check that + // itself, and dispatchEvent()'s return value reads the flag without masking it with + // cancelable, so check here rather than changing either of those for every caller. + if (handle->IsFalse() && event->getCancelable()) { + event->preventDefault(); + } + break; + } + } + if (flags.warnOnHandlerReturn && !handle->IsBoolean()) { + flags.warnOnHandlerReturn = false; + // To help make debugging easier, let's tailor the warning a bit if it was a promise. + if (handle->IsPromise()) { + js.logWarning( + kj::str("An event handler returned a promise that will be ignored. Event handlers " + "should not have a return value and should not be async functions.")); + } else { + js.logWarning( + kj::str("An event handler returned a value of type \"", handle->TypeOf(js.v8Isolate), + "\" that will be ignored. Event handlers should not have a return value.")); + } + } + } +} + bool EventTarget::dispatchEventImpl(jsg::Lock& js, jsg::Ref event) { event->beginDispatch(JSG_THIS); KJ_DEFER(event->endDispatch()); @@ -395,17 +530,25 @@ bool EventTarget::dispatchEventImpl(jsg::Lock& js, jsg::Ref event) { // Check if there is an `on` property on this object. If so, we treat that as an event // handler, in addition to the ones registered with addEventListener(). - KJ_IF_SOME(onProp, onEvents.get(js, kj::str("on", event->getType()))) { - // If the on-event is not a function, we silently ignore it rather than raise an error. - KJ_IF_SOME(cb, onProp.tryGet()) { - callbacks.add(Callback{ - .handler = - EventHandler::JavaScriptHandler{ - .identity = nullptr, // won't be used below if oldStyle is true and once is false - .callback = kj::mv(cb), - }, - .oldStyle = true, - }); + // + // This is not a standard behavior: only certain interfaces define `on` handlers, and + // where they do, the handler is a regular listener that fires in registration order rather + // than ahead of everything else. With specCompliantEventHandlerAttributes the lookup goes + // away and those interfaces implement `on` via setEventHandlerAttribute() instead. + if ((flags.legacyOnPropertyLookup || !specCompliantEventHandlerAttributes(js)) && + !hasActiveEventHandlerAttribute(event->getType())) { + KJ_IF_SOME(onProp, onEvents.get(js, kj::str("on", event->getType()))) { + // If the on-event is not a function, we silently ignore it rather than raise an error. + KJ_IF_SOME(cb, onProp.tryGet()) { + callbacks.add(Callback{ + .handler = + EventHandler::JavaScriptHandler{ + .identity = nullptr, // won't be used below if oldStyle is true and once is false + .callback = kj::mv(cb), + }, + .oldStyle = true, + }); + } } } @@ -480,45 +623,7 @@ bool EventTarget::dispatchEventImpl(jsg::Lock& js, jsg::Ref event) { KJ_SWITCH_ONEOF(callback.handler) { KJ_CASE_ONEOF(jsh, EventHandler::JavaScriptHandler) { - // Per the standard, the event listener is not supposed to return any value, and if it - // does, that value is ignored. That can be somewhat problematic if the user passes an - // async function as the event handler. Doing so counts as undefined behavior and can - // introduce subtle and difficult to diagnose bugs. Here, if the handler does return a - // value, we're going to emit a warning but otherwise ignore it. The warning will only - // be emitted at most once per EventEmitter instance. - auto ret = jsh.callback(js, event.addRef()); - // Note: We used to run each handler in its own v8::TryCatch. However, due to a - // misunderstanding of the V8 API, we incorrectly believed that TryCatch mishandled - // termination (or maybe it actually did at the time), so we changed things such that - // we don't catch exceptions so the first handler to throw an exception terminates the - // loop, and the exception flows out of dispatchEvent(). In theory if multiple - // handlers were registered then maybe we ought to be running all of them even if one - // fails. This isn't entirely clear, though: in the case of 'fetch' handlers, in - // fail-closed mode, an exception from any handler should make the whole request fail, - // but then who cares if the remaining handlers run? Meanwhile, in fail-open mode, for - // consistency, we should probably trigger fallback behavior if any handler throws, so - // again it doesn't matter. For other types of handlers, e.g. WebSocket 'message', it's - // not clear why one would ever register multiple handlers. - KJ_IF_SOME(r, ret) { - auto handle = r.getHandle(js); - // Returning true is the same as calling preventDefault() on the event. - if (handle->IsTrue()) { - event->preventDefault(); - } - if (flags.warnOnHandlerReturn && !handle->IsBoolean()) { - flags.warnOnHandlerReturn = false; - // To help make debugging easier, let's tailor the warning a bit if it was a promise. - if (handle->IsPromise()) { - js.logWarning(kj::str( - "An event handler returned a promise that will be ignored. Event handlers " - "should not have a return value and should not be async functions.")); - } else { - js.logWarning(kj::str("An event handler returned a value of type \"", - handle->TypeOf(js.v8Isolate), - "\" that will be ignored. Event handlers should not have a return value.")); - } - } - } + invokeJavaScriptHandler(js, jsh.callback, event.addRef(), HandlerReturn::TRUE_CANCELS); } KJ_CASE_ONEOF(native, EventHandler::NativeHandlerRef) { native.handler(js, event.addRef()); @@ -580,21 +685,14 @@ AbortSignal::AbortSignal(kj::Maybe exception, reason(kj::mv(maybeReason)) {} kj::Maybe AbortSignal::getOnAbort(jsg::Lock& js) { - return onAbortHandler.map( - [&](jsg::JsRef& ref) -> jsg::JsValue { return ref.getHandle(js); }); + return getEventHandlerAttribute(js, kAbortEvent); } -void AbortSignal::setOnAbort(jsg::Lock& js, jsg::Optional handler) { - // We only want to accept the handler if it's a valid handler... For anything - // else, set it to null. - KJ_IF_SOME(h, handler) { - if (h.isFunction() || h.isObject()) { - onAbortHandler = jsg::JsRef(js, h); - subscribeToRpcAbort(js); - return; - } +void AbortSignal::setOnAbort(jsg::Lock& js, jsg::Optional handler) { + setEventHandlerAttribute(js, kAbortEvent, kj::mv(handler)); + if (hasEventHandlerAttribute(kAbortEvent)) { + subscribeToRpcAbort(js); } - onAbortHandler = kj::none; } void AbortSignal::addEventListener(jsg::Lock& js, @@ -730,7 +828,7 @@ jsg::Ref AbortSignal::any(jsg::Lock& js, } void AbortSignal::visitForGc(jsg::GcVisitor& visitor) { - visitor.visit(reason, onAbortHandler); + visitor.visit(reason); } RefcountedCanceler& AbortSignal::getCanceler() { @@ -950,6 +1048,9 @@ void AbortController::abort(jsg::Lock& js, jsg::Optional maybeReas void EventTarget::visitForGc(jsg::GcVisitor& visitor) { visitor.visit(maybeListenerCallback); + for (auto& entry: eventHandlerAttributes) { + visitor.visit(entry.value); + } for (auto& entry: typeMap) { for (auto& handler: entry.value.handlers) { KJ_SWITCH_ONEOF(handler->handler) { @@ -1043,7 +1144,9 @@ CustomEvent::CustomEvent(kj::String ownType, CustomEventInit init) jsg::Ref CustomEvent::constructor( jsg::Lock& js, kj::String type, jsg::Optional init) { - return js.alloc(kj::mv(type), kj::mv(init).orDefault({})); + auto event = js.alloc(kj::mv(type), kj::mv(init).orDefault({})); + event->markConstructedFromJs(); + return event; } jsg::Optional CustomEvent::getDetail(jsg::Lock& js) { @@ -1099,6 +1202,9 @@ void EventTarget::EventHandlerSet::jsgGetMemoryInfo(jsg::MemoryTracker& tracker) void EventTarget::visitForMemoryInfo(jsg::MemoryTracker& tracker) const { tracker.trackField("typeMap", typeMap); + for (const auto& entry: eventHandlerAttributes) { + tracker.trackField("eventHandlerAttribute", entry.value.value); + } } } // namespace workerd::api diff --git a/src/workerd/api/basics.h b/src/workerd/api/basics.h index 474443bbf95..23748b2edfb 100644 --- a/src/workerd/api/basics.h +++ b/src/workerd/api/basics.h @@ -221,6 +221,15 @@ class Event: public jsg::Object { tracker.trackField("target", target); } + protected: + // Per the standard, an event's "is trusted" flag is only set when the runtime itself creates + // and dispatches the event, so an event constructed from JS is never trusted. Subclasses share + // their C++ constructors with internal callers that do produce trusted events, so their JS + // `constructor()` calls this to clear the flag. + inline void markConstructedFromJs() { + flags.trusted = false; + } + private: // listing ownType first so type can be initialized with it in constructor kj::String ownType; @@ -329,6 +338,21 @@ class EventTarget: public jsg::Object { flags.warnOnSpecialEvents = true; } + // Opts this EventTarget into the legacy behavior where dispatching an event looks for an + // `on` property on the object and invokes it before any registered listener, even when + // the specCompliantEventHandlerAttributes compat flag is enabled. + // + // This exists for the global scope only. Some of the global's `on` handlers are + // standardized (`onerror`, `onunhandledrejection` and `onrejectionhandled` on + // WorkerGlobalScope, `onfetch` on ServiceWorkerGlobalScope) and some are workerd-specific + // (`onscheduled`, `ontail`, `ontrace`, `onalarm`, `onqueue`), so aligning the global with the + // standard means picking those apart and adding accessors to the global object. That is left + // for a separate change. The lookup is skipped for any type that has an active event handler + // attribute, so accessors can be introduced a few at a time without double-firing. + inline void enableLegacyOnPropertyLookup() { + flags.legacyOnPropertyLookup = true; + } + // The EventListenerCallback, if given, is called whenever addEventListener // or removeEventListener is invoked to report the number of registered // handlers for the event. @@ -425,6 +449,39 @@ class EventTarget: public jsg::Object { maybeListenerCallback = kj::mv(callback); } + // The value an `on` accessor accepts. A callable is unwrapped into a handler we can + // invoke (keeping the identity around so the getter can hand the original value back), while + // anything else is retained as-is and never invoked. + using EventHandlerAttributeValue = kj::OneOf, jsg::JsValue>; + + // Implements an event handler IDL attribute -- an `on` accessor -- as described by the + // HTML standard. Subclasses that expose `on` accessors should delegate to these. + // + // Assigning a handler registers an ordinary event listener, so the handler is invoked in + // registration order along with listeners added via addEventListener(). Assigning a different + // handler replaces the value without moving that registration, and assigning null (or any + // other non-object) removes it. + // + // When the specCompliantEventHandlerAttributes compat flag is disabled these only store the + // value; dispatchEventImpl() finds it by looking up the `on` property instead, which is + // what produces the non-standard ordering the flag fixes. + kj::Maybe getEventHandlerAttribute(jsg::Lock& js, kj::StringPtr type); + void setEventHandlerAttribute( + jsg::Lock& js, kj::StringPtr type, jsg::Optional value); + bool hasEventHandlerAttribute(kj::StringPtr type) const { + return eventHandlerAttributes.find(type) != kj::none; + } + + // True if an `on` accessor is holding a handler for this type *and* has a listener + // registered for it, meaning dispatch will reach the handler through the listener list rather + // than through the legacy `on` property lookup. + bool hasActiveEventHandlerAttribute(kj::StringPtr type) const { + KJ_IF_SOME(attribute, eventHandlerAttributes.find(type)) { + return attribute.listener != kj::none; + } + return false; + } + private: // RAII-style listener that can be attached to an EventTarget. class NativeHandler { @@ -546,10 +603,55 @@ class EventTarget: public jsg::Object { EventHandlerSet& getOrCreate(kj::StringPtr str) KJ_LIFETIMEBOUND; + // How the value returned by an event handler is interpreted. + enum class HandlerReturn { + // Returning true cancels the event. Listeners are not supposed to return anything at all, + // but we have honored this since long before there was a compat flag to gate it on, so it + // stays the rule for anything registered with addEventListener(). + TRUE_CANCELS, + + // Returning false cancels the event and any other value is ignored, per the standard's event + // handler processing algorithm: + // https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm + FALSE_CANCELS, + }; + + // Runs a JavaScript event listener or event handler, applying `returnMode` to the value it + // returns. + void invokeJavaScriptHandler( + jsg::Lock& js, HandlerFunction& callback, jsg::Ref event, HandlerReturn returnMode); + + // Invokes the `on` handler for the given type, if it is set and callable. Called by the + // listener that setEventHandlerAttribute() registers. + void invokeEventHandlerAttribute(jsg::Lock& js, kj::StringPtr type, jsg::Ref event); + + // The state backing a single `on` accessor. See setEventHandlerAttribute(). + struct EventHandlerAttribute { + // The value that was assigned, handed back as-is by the getter. + jsg::JsRef value; + + // None if `value` is not callable, in which case the handler is never invoked. + kj::Maybe callback; + + // The listener registration that invokes this handler, held for as long as the handler is + // set. Dropping it unregisters the listener, which is what lets the handler keep its place + // in the listener list while it is merely being reassigned. None when the + // specCompliantEventHandlerAttributes compat flag is disabled, since in that case the + // handler is invoked by dispatchEventImpl()'s `on` property lookup instead. + kj::Maybe> listener; + + void visitForGc(jsg::GcVisitor& visitor) { + visitor.visit(value, callback); + } + }; + jsg::PropertyReflection> onEvents; kj::HashMap typeMap; + // Keyed by event type, not by property name, so "abort" rather than "onabort". + kj::HashMap eventHandlerAttributes; + kj::Maybe maybeListenerCallback; struct Flags { @@ -561,6 +663,8 @@ class EventTarget: public jsg::Object { // Event handlers are not supposed to return values. The first time one does, we'll // emit a warning to help users debug things but we'll otherwise ignore it. uint8_t warnOnHandlerReturn : 1 = 1; + // See enableLegacyOnPropertyLookup(). + uint8_t legacyOnPropertyLookup : 1 = 0; }; Flags flags; @@ -569,6 +673,21 @@ class EventTarget: public jsg::Object { friend class NativeHandler; }; +// Defines the getter/setter pair backing an `on` event handler attribute on an +// EventTarget subclass, e.g. WD_EVENT_HANDLER_ATTRIBUTE(Message, "message") defines +// getOnMessage()/setOnMessage() for the "message" event. Register the property itself with +// JSG_PROTOTYPE_PROPERTY(onmessage, getOnMessage, setOnMessage). +// +// See EventTarget::setEventHandlerAttribute() for what the accessors do. +#define WD_EVENT_HANDLER_ATTRIBUTE(name, eventType) \ + kj::Maybe getOn##name(jsg::Lock& js) { \ + return getEventHandlerAttribute(js, eventType##_kj); \ + } \ + void setOn##name(jsg::Lock& js, jsg::Optional value) { \ + setEventHandlerAttribute(js, eventType##_kj, kj::mv(value)); \ + } \ + static_assert(true, "require a trailing semicolon") + // An implementation of the Web Platform Standard AbortSignal API class AbortTriggerRpcClient; @@ -614,12 +733,11 @@ class AbortSignal final: public EventTarget { const jsg::TypeHandler& handler, const jsg::TypeHandler>& eventTargetHandler); - // While AbortSignal extends EventTarget, and our EventTarget implementation will - // automatically support onabort being set as an own property, the spec defines - // onabort as a prototype property on the AbortSignal prototype. Therefore, we - // need to explicitly set it as a prototype property here. + // The spec defines onabort as a prototype property on the AbortSignal prototype, so we + // define it explicitly rather than relying on EventTarget's legacy `on` property + // lookup. kj::Maybe getOnAbort(jsg::Lock& js); - void setOnAbort(jsg::Lock& js, jsg::Optional handler); + void setOnAbort(jsg::Lock& js, jsg::Optional handler); void addEventListener(jsg::Lock& js, kj::String type, @@ -698,7 +816,6 @@ class AbortSignal final: public EventTarget { Flag flag; kj::Maybe> reason; - kj::Maybe> onAbortHandler; static kj::Exception abortException( jsg::Lock& js, const jsg::Optional>& reason); diff --git a/src/workerd/api/events.c++ b/src/workerd/api/events.c++ index ad8a46c453c..69c55e75a40 100644 --- a/src/workerd/api/events.c++ +++ b/src/workerd/api/events.c++ @@ -3,6 +3,8 @@ #include "blob.h" #include "messagechannel.h" +#include + namespace workerd::api { namespace { @@ -60,7 +62,9 @@ MessageEvent::MessageEvent(jsg::Lock& js, jsg::Ref MessageEvent::constructor( jsg::Lock& js, kj::String type, Initializer initializer) { - return js.alloc(js, kj::mv(type), kj::mv(initializer.data)); + auto event = js.alloc(js, kj::mv(type), kj::mv(initializer.data)); + event->markConstructedFromJs(); + return event; } kj::OneOf> MessageEvent::getData(jsg::Lock& js) { @@ -75,8 +79,20 @@ kj::OneOf> MessageEvent::getData(jsg::Lock& js) { KJ_UNREACHABLE; } -kj::Maybe> MessageEvent::getOrigin() { - return maybeOrigin.map([](auto& a) -> kj::ArrayPtr { return a.asPtr(); }); +kj::Maybe> MessageEvent::getOrigin(jsg::Lock& js) { + KJ_IF_SOME(origin, maybeOrigin) { + return origin.asPtr(); + } + + // The origin is internally nullable and the standard's getter reports the empty string for the + // null case. Callers that have a URL to take an origin from (EventSource, and a WebSocket + // opened from a URL) supply one; a MessagePort message or a WebSocketPair endpoint has none. + KJ_IF_SOME(flags, FeatureFlags::tryGet(js)) { + if (flags.getSpecCompliantMessageEventOrigin()) { + return ""_kj.asArray(); + } + } + return kj::none; } kj::StringPtr MessageEvent::getLastEventId() { @@ -132,7 +148,9 @@ ErrorEvent::ErrorEvent(jsg::Lock& js, jsg::JsValue error) jsg::Ref ErrorEvent::constructor( jsg::Lock& js, kj::String type, jsg::Optional init) { - return js.alloc(kj::mv(type), kj::mv(init).orDefault({})); + auto event = js.alloc(kj::mv(type), kj::mv(init).orDefault({})); + event->markConstructedFromJs(); + return event; } kj::StringPtr ErrorEvent::getFilename() { diff --git a/src/workerd/api/events.h b/src/workerd/api/events.h index 2cac81f2aea..95301f1577d 100644 --- a/src/workerd/api/events.h +++ b/src/workerd/api/events.h @@ -51,7 +51,7 @@ class MessageEvent final: public Event { kj::OneOf> getData(jsg::Lock& js); - kj::Maybe> getOrigin(); + kj::Maybe> getOrigin(jsg::Lock& js); kj::StringPtr getLastEventId(); @@ -62,7 +62,7 @@ class MessageEvent final: public Event { kj::ArrayPtr> getPorts(); - JSG_RESOURCE_TYPE(MessageEvent) { + JSG_RESOURCE_TYPE(MessageEvent, CompatibilityFlags::Reader flags) { JSG_INHERIT(Event); JSG_READONLY_INSTANCE_PROPERTY(data, getData); @@ -72,7 +72,13 @@ class MessageEvent final: public Event { JSG_READONLY_INSTANCE_PROPERTY(ports, getPorts); JSG_TS_ROOT(); - JSG_TS_OVERRIDE({ readonly data: any; }); + if (flags.getSpecCompliantMessageEventOrigin()) { + // getOrigin() still returns a kj::Maybe, which maps to `string | null`, but with the flag + // the none case reports the empty string, so `origin` is never actually null. + JSG_TS_OVERRIDE({ readonly data: any; readonly origin: string; }); + } else { + JSG_TS_OVERRIDE({ readonly data: any; }); + } } void visitForMemoryInfo(jsg::MemoryTracker& tracker) const; diff --git a/src/workerd/api/eventsource.c++ b/src/workerd/api/eventsource.c++ index 21e3e35687c..dd0e8ff662e 100644 --- a/src/workerd/api/eventsource.c++ +++ b/src/workerd/api/eventsource.c++ @@ -511,7 +511,7 @@ void EventSource::visitForGc(jsg::GcVisitor& visitor) { KJ_IF_SOME(i, impl) { visitor.visit(i.options.fetcher); } - visitor.visit(abortController, onopenValue, onmessageValue, onerrorValue); + visitor.visit(abortController); } void EventSource::visitForMemoryInfo(jsg::MemoryTracker& tracker) const { @@ -521,9 +521,6 @@ void EventSource::visitForMemoryInfo(jsg::MemoryTracker& tracker) const { } tracker.trackField("abortController", abortController); tracker.trackField("lastEventId", lastEventId); - tracker.trackField("onopen", onopenValue); - tracker.trackField("onmessage", onmessageValue); - tracker.trackField("onerror", onerrorValue); } } // namespace workerd::api diff --git a/src/workerd/api/eventsource.h b/src/workerd/api/eventsource.h index 70158abf21b..fbaf900d9cb 100644 --- a/src/workerd/api/eventsource.h +++ b/src/workerd/api/eventsource.h @@ -68,37 +68,22 @@ class EventSource: public EventTarget { static jsg::Ref from(jsg::Lock& js, JsReadableStream stream); kj::Maybe getOnOpen(jsg::Lock& js) { - return onopenValue.map( - [&](jsg::JsRef& ref) -> jsg::JsValue { return ref.getHandle(js); }); + return getEventHandlerAttribute(js, "open"_kj); } - void setOnOpen(jsg::Lock& js, jsg::JsValue value) { - if (!value.isObject() && !value.isFunction()) { - onopenValue = kj::none; - } else { - onopenValue = jsg::JsRef(js, value); - } + void setOnOpen(jsg::Lock& js, jsg::Optional value) { + setEventHandlerAttribute(js, "open"_kj, kj::mv(value)); } kj::Maybe getOnMessage(jsg::Lock& js) { - return onmessageValue.map( - [&](jsg::JsRef& ref) -> jsg::JsValue { return ref.getHandle(js); }); + return getEventHandlerAttribute(js, "message"_kj); } - void setOnMessage(jsg::Lock& js, jsg::JsValue value) { - if (!value.isObject() && !value.isFunction()) { - onmessageValue = kj::none; - } else { - onmessageValue = jsg::JsRef(js, value); - } + void setOnMessage(jsg::Lock& js, jsg::Optional value) { + setEventHandlerAttribute(js, "message"_kj, kj::mv(value)); } kj::Maybe getOnError(jsg::Lock& js) { - return onerrorValue.map( - [&](jsg::JsRef& ref) -> jsg::JsValue { return ref.getHandle(js); }); + return getEventHandlerAttribute(js, "error"_kj); } - void setOnError(jsg::Lock& js, jsg::JsValue value) { - if (!value.isObject() && !value.isFunction()) { - onerrorValue = kj::none; - } else { - onerrorValue = jsg::JsRef(js, value); - } + void setOnError(jsg::Lock& js, jsg::Optional value) { + setEventHandlerAttribute(js, "error"_kj, kj::mv(value)); } JSG_RESOURCE_TYPE(EventSource) { @@ -170,12 +155,6 @@ class EventSource: public EventTarget { // Indicates that the close method has been previously called. bool closeCalled = false; - // The EventSource spec defines onopen, onmessage, and onerror as prototype - // properties on the class. - kj::Maybe> onopenValue; - kj::Maybe> onmessageValue; - kj::Maybe> onerrorValue; - // The default reconnection wait time. This is fairly arbitrary and is left // entirely up to the implementation. The event stream can provide a new value. static constexpr auto DEFAULT_RECONNECTION_TIME = 2 * kj::SECONDS; diff --git a/src/workerd/api/global-scope.h b/src/workerd/api/global-scope.h index 926963caefe..4fd7c96602b 100644 --- a/src/workerd/api/global-scope.h +++ b/src/workerd/api/global-scope.h @@ -155,6 +155,16 @@ class Cloudflare: public jsg::Object { class WorkerGlobalScope: public EventTarget, public jsg::ContextGlobal { public: + WorkerGlobalScope() { + // The global scope keeps EventTarget's `on` property lookup even when + // specCompliantEventHandlerAttributes is enabled, so `onfetch`, `onscheduled` and the like + // keep working the way they always have. Note that this leaves the global as a deviation: + // some of those handlers are standardized (`onerror`, `onunhandledrejection` and + // `onrejectionhandled` here, `onfetch` on ServiceWorkerGlobalScope) and some are + // workerd-specific. See enableLegacyOnPropertyLookup(). + enableLegacyOnPropertyLookup(); + } + jsg::Unimplemented importScripts(kj::String s) { return {}; }; diff --git a/src/workerd/api/messagechannel.c++ b/src/workerd/api/messagechannel.c++ index 695b170dc82..a10527b3eb3 100644 --- a/src/workerd/api/messagechannel.c++ +++ b/src/workerd/api/messagechannel.c++ @@ -19,13 +19,13 @@ MessagePort::MessagePort(): state(Pending()) { // supports. Specifically, adding a new message listener using the // addEventListener method is *technically* not supposed to start // the port but we're going to do what Node.js does. - if (count > 0 || onmessageValue != kj::none) { + if (count > 0 || hasEventHandlerAttribute("message"_kj)) { start(js); } } KJ_CASE_ONEOF(started, Started) { // If we are in the started state, stop the port if there are no listeners. - if (count == 0 && onmessageValue == kj::none) { + if (count == 0 && !hasEventHandlerAttribute("message"_kj)) { state = Pending(); } } @@ -192,22 +192,17 @@ void MessagePort::start(jsg::Lock& js) { } kj::Maybe MessagePort::getOnMessage(jsg::Lock& js) { - return onmessageValue.map( - [&](jsg::JsRef& ref) -> jsg::JsValue { return ref.getHandle(js); }); + return getEventHandlerAttribute(js, "message"_kj); } -void MessagePort::setOnMessage(jsg::Lock& js, jsg::JsValue value) { - if (!value.isObject() && !value.isFunction()) { - onmessageValue = kj::none; - // If we have no handlers and no onmessage ... - if (getHandlerCount("message"_kj) == 0 && onmessageValue == kj::none) { - // ...Put the port back into a pending state where messages - // will be enqueued until another listener is attached. - state = Pending(); - } - } else { - onmessageValue = jsg::JsRef(js, value); +void MessagePort::setOnMessage(jsg::Lock& js, jsg::Optional value) { + setEventHandlerAttribute(js, "message"_kj, kj::mv(value)); + if (hasEventHandlerAttribute("message"_kj)) { start(js); + } else if (getHandlerCount("message"_kj) == 0 && !state.is()) { + // We have no handlers and no onmessage, so put the port back into a pending state where + // messages will be enqueued until another listener is attached. + state = Pending(); } } diff --git a/src/workerd/api/messagechannel.h b/src/workerd/api/messagechannel.h index 88c0be2b635..90f2579ce9c 100644 --- a/src/workerd/api/messagechannel.h +++ b/src/workerd/api/messagechannel.h @@ -84,14 +84,26 @@ class MessagePort final: public EventTarget { // separately. That's a kind of a weird rule but ok. To support // that we need to define an onmessage getter/setter pair. kj::Maybe getOnMessage(jsg::Lock& js); - void setOnMessage(jsg::Lock& js, jsg::JsValue value); + void setOnMessage(jsg::Lock& js, jsg::Optional value); - JSG_RESOURCE_TYPE(MessagePort) { + WD_EVENT_HANDLER_ATTRIBUTE(MessageError, "messageerror"); + + // The spec puts `onclose` on MessagePort itself, and `onmessage`/`onmessageerror` on the + // MessageEventTarget mixin that MessagePort includes. + WD_EVENT_HANDLER_ATTRIBUTE(Close, "close"); + + JSG_RESOURCE_TYPE(MessagePort, CompatibilityFlags::Reader flags) { JSG_INHERIT(EventTarget); JSG_METHOD(postMessage); JSG_METHOD(close); JSG_METHOD(start); JSG_PROTOTYPE_PROPERTY(onmessage, getOnMessage, setOnMessage); + if (flags.getSpecCompliantEventHandlerAttributes()) { + // Without the flag these are handled by EventTarget's `on` property lookup, so + // defining accessors for them would be a no-op at best. + JSG_PROTOTYPE_PROPERTY(onmessageerror, getOnMessageError, setOnMessageError); + JSG_PROTOTYPE_PROPERTY(onclose, getOnClose, setOnClose); + } } jsg::Ref addRef() { @@ -129,13 +141,11 @@ class MessagePort final: public EventTarget { // To keep them both alive, maintain strong references to both // ports! kj::Maybe> other; - kj::Maybe> onmessageValue; void visitForGc(jsg::GcVisitor& visitor) { KJ_IF_SOME(pending, state.tryGet()) { visitor.visitAll(pending); } - visitor.visit(onmessageValue); } }; diff --git a/src/workerd/api/tests/BUILD.bazel b/src/workerd/api/tests/BUILD.bazel index 45c0adf1426..18b19e31c16 100644 --- a/src/workerd/api/tests/BUILD.bazel +++ b/src/workerd/api/tests/BUILD.bazel @@ -496,6 +496,12 @@ wd_test( data = ["autovuln-63-test.js"], ) +wd_test( + src = "event-handler-attributes-test.wd-test", + args = ["--experimental"], + data = ["event-handler-attributes-test.js"], +) + wd_test( src = "events-test.wd-test", args = ["--experimental"], @@ -905,6 +911,23 @@ wd_test( data = ["websocket-hibernation.js"], ) +js_binary( + name = "websocket-origin-sidecar", + entry_point = "websocket-origin-sidecar.js", +) + +wd_test( + src = "websocket-origin-test.wd-test", + args = ["--experimental"], + data = [ + "websocket-origin-test.js", + ], + sidecar = "websocket-origin-sidecar", + sidecar_port_bindings = [ + "ORIGIN_SERVER_PORT", + ], +) + js_binary( name = "websocket-client-error-sidecar", entry_point = "websocket-client-error-sidecar.js", diff --git a/src/workerd/api/tests/event-handler-attributes-test.js b/src/workerd/api/tests/event-handler-attributes-test.js new file mode 100644 index 00000000000..2e111f27929 --- /dev/null +++ b/src/workerd/api/tests/event-handler-attributes-test.js @@ -0,0 +1,307 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 +import { deepStrictEqual, strictEqual } from 'node:assert'; + +// Tests for `on` event handler attributes on EventTarget and its subclasses. +// See https://github.com/cloudflare/workerd/issues/6022 +// +// Historically, EventTarget itself looked for an `on` property on the object +// whenever an event was dispatched, and invoked it before any listener registered +// with addEventListener(). That is not what the standard says: only specific +// interfaces define `on` accessors, and those accessors register a regular +// listener, so they fire in registration order along with everything else. +// +// The spec_compliant_event_handler_attributes compat flag switches to the standard +// behavior. Every test below runs twice, once per behavior, with env.MODE telling +// us which one to expect. + +function isSpecCompliant(env) { + strictEqual(typeof env.MODE, 'string'); + return env.MODE === 'spec'; +} + +export const plainEventTargetIgnoresOnProperties = { + test(ctrl, env) { + const calls = []; + const target = new EventTarget(); + target.addEventListener('hit', () => { + calls.push('a'); + }); + target.onhit = () => { + calls.push('on'); + }; + target.addEventListener('hit', () => { + calls.push('c'); + }); + target.dispatchEvent(new Event('hit')); + + // A plain EventTarget defines no event handler attributes, so `onhit` is just + // an ordinary property that nothing ever reads. + deepStrictEqual( + calls, + isSpecCompliant(env) ? ['a', 'c'] : ['on', 'a', 'c'] + ); + }, +}; + +export const subclassDefinedOnPropertyFiresOnceInOrder = { + test(ctrl, env) { + const calls = []; + const target = new EventTarget(); + + // This is how a subclass is expected to implement an event handler attribute. + let current = null; + Object.defineProperty(target, 'onhit', { + get: () => current, + set(value) { + if (current) this.removeEventListener('hit', current); + current = value; + if (current) this.addEventListener('hit', current); + }, + }); + + target.addEventListener('hit', () => { + calls.push('a'); + }); + target.onhit = () => { + calls.push('b'); + }; + target.addEventListener('hit', () => { + calls.push('c'); + }); + target.dispatchEvent(new Event('hit')); + + // Without the flag the handler is invoked twice: once by EventTarget's own + // `on` lookup and once as the listener the subclass registered. + deepStrictEqual( + calls, + isSpecCompliant(env) ? ['a', 'b', 'c'] : ['b', 'a', 'b', 'c'] + ); + }, +}; + +export const abortSignalOnAbortFiresInRegistrationOrder = { + test(ctrl, env) { + const calls = []; + const ac = new AbortController(); + ac.signal.addEventListener('abort', () => { + calls.push('a'); + }); + const handler = () => { + calls.push('on'); + }; + ac.signal.onabort = handler; + strictEqual(ac.signal.onabort, handler); + ac.signal.addEventListener('abort', () => { + calls.push('c'); + }); + ac.abort(); + + deepStrictEqual( + calls, + isSpecCompliant(env) ? ['a', 'on', 'c'] : ['on', 'a', 'c'] + ); + }, +}; + +export const abortSignalOnAbortKeepsItsPositionWhenReassigned = { + test(ctrl, env) { + const calls = []; + const ac = new AbortController(); + ac.signal.addEventListener('abort', () => { + calls.push('a'); + }); + ac.signal.onabort = () => { + calls.push('first'); + }; + ac.signal.addEventListener('abort', () => { + calls.push('c'); + }); + // Replacing the handler must not move it to the end of the listener list. + ac.signal.onabort = () => { + calls.push('second'); + }; + ac.abort(); + + deepStrictEqual( + calls, + isSpecCompliant(env) ? ['a', 'second', 'c'] : ['second', 'a', 'c'] + ); + }, +}; + +export const abortSignalOnAbortCanBeCleared = { + test(ctrl, env) { + const calls = []; + const ac = new AbortController(); + ac.signal.addEventListener('abort', () => { + calls.push('a'); + }); + ac.signal.onabort = () => { + calls.push('on'); + }; + ac.signal.addEventListener('abort', () => { + calls.push('c'); + }); + ac.signal.onabort = null; + strictEqual(ac.signal.onabort, null); + ac.abort(); + + deepStrictEqual(calls, ['a', 'c']); + }, +}; + +export const abortSignalOnAbortIgnoresNonCallables = { + test(ctrl, env) { + const calls = []; + const ac = new AbortController(); + ac.signal.addEventListener('abort', () => { + calls.push('a'); + }); + + // Primitives are coerced to null, ... + for (const value of [1, 'a', true, Symbol('test')]) { + ac.signal.onabort = value; + strictEqual(ac.signal.onabort, null); + } + + // ... while a non-callable object is retained but never invoked. + const obj = {}; + ac.signal.onabort = obj; + strictEqual(ac.signal.onabort, obj); + + ac.abort(); + deepStrictEqual(calls, ['a']); + }, +}; + +export const abortSignalOnAbortReceivesTheEvent = { + test(ctrl, env) { + const seen = []; + const ac = new AbortController(); + ac.signal.onabort = function (event) { + seen.push([event.type, this === ac.signal]); + }; + ac.abort(); + // Per the standard, `this` inside the handler is the object it was set on. The legacy + // `on` lookup never bound a receiver, so there `this` is the global object. + deepStrictEqual(seen, [['abort', isSpecCompliant(env)]]); + }, +}; + +export const webSocketOnMessageFiresInRegistrationOrder = { + async test(ctrl, env) { + const calls = []; + const [client, server] = new WebSocketPair(); + server.accept(); + + const { promise, resolve } = Promise.withResolvers(); + server.addEventListener('message', (event) => { + calls.push('a:' + event.data); + }); + const handler = (event) => { + calls.push('on:' + event.data); + }; + server.onmessage = handler; + strictEqual(server.onmessage, handler); + server.addEventListener('message', (event) => { + calls.push('c:' + event.data); + resolve(); + }); + + client.accept(); + client.send('hi'); + await promise; + + deepStrictEqual( + calls, + isSpecCompliant(env) + ? ['a:hi', 'on:hi', 'c:hi'] + : ['on:hi', 'a:hi', 'c:hi'] + ); + + client.close(); + server.close(); + }, +}; + +export const webSocketOnCloseCanBeCleared = { + async test(ctrl, env) { + const calls = []; + const [client, server] = new WebSocketPair(); + server.accept(); + + const { promise, resolve } = Promise.withResolvers(); + server.onclose = () => { + calls.push('on'); + }; + server.onclose = null; + strictEqual(server.onclose, null); + server.addEventListener('close', () => { + calls.push('listener'); + resolve(); + }); + + client.accept(); + client.close(1000, 'done'); + await promise; + + deepStrictEqual(calls, ['listener']); + + server.close(); + }, +}; + +export const onHandlerReturningFalseCancelsTheEvent = { + test(ctrl, env) { + const spec = isSpecCompliant(env); + const signal = new AbortController().signal; + signal.onabort = () => false; + + const event = new Event('abort', { cancelable: true }); + // Per the standard's event handler processing algorithm, returning false cancels the event. + // The legacy `on` lookup ignores false, because it applies the same rule to handlers + // as to listeners: only returning true cancels. + strictEqual(signal.dispatchEvent(event), !spec); + strictEqual(event.defaultPrevented, spec); + }, +}; + +export const onHandlerReturningTrueIsIgnored = { + test(ctrl, env) { + const spec = isSpecCompliant(env); + const signal = new AbortController().signal; + signal.onabort = () => true; + + const event = new Event('abort', { cancelable: true }); + // True has no meaning in the standard's algorithm. + strictEqual(signal.dispatchEvent(event), spec); + strictEqual(event.defaultPrevented, !spec); + }, +}; + +export const onHandlerCannotCancelANonCancelableEvent = { + test(ctrl, env) { + const signal = new AbortController().signal; + signal.onabort = () => false; + + // Cancelling is a no-op on an event that is not cancelable, in either mode. + const event = new Event('abort'); + strictEqual(signal.dispatchEvent(event), true); + strictEqual(event.defaultPrevented, false); + }, +}; + +export const listenerReturningTrueStillCancels = { + test(ctrl, env) { + // Listeners are not supposed to return anything, but a listener that returns true has always + // been treated as calling preventDefault(), and the flag does not change that. + const target = new EventTarget(); + target.addEventListener('hit', () => true); + + const event = new Event('hit', { cancelable: true }); + strictEqual(target.dispatchEvent(event), false); + strictEqual(event.defaultPrevented, true); + }, +}; diff --git a/src/workerd/api/tests/event-handler-attributes-test.wd-test b/src/workerd/api/tests/event-handler-attributes-test.wd-test new file mode 100644 index 00000000000..43270c1fe6f --- /dev/null +++ b/src/workerd/api/tests/event-handler-attributes-test.wd-test @@ -0,0 +1,28 @@ +using Workerd = import "/workerd/workerd.capnp"; + +const unitTests :Workerd.Config = ( + services = [ + ( name = "event-handler-attributes-spec-test", + worker = ( + modules = [ + (name = "worker", esModule = embed "event-handler-attributes-test.js") + ], + bindings = [ + (name = "MODE", text = "spec") + ], + compatibilityFlags = ["nodejs_compat", "spec_compliant_event_handler_attributes"], + ) + ), + ( name = "event-handler-attributes-legacy-test", + worker = ( + modules = [ + (name = "worker", esModule = embed "event-handler-attributes-test.js") + ], + bindings = [ + (name = "MODE", text = "legacy") + ], + compatibilityFlags = ["nodejs_compat", "no_spec_compliant_event_handler_attributes"], + ) + ), + ], +); diff --git a/src/workerd/api/tests/events-test.js b/src/workerd/api/tests/events-test.js index 58f0a4eb705..a93a624cd04 100644 --- a/src/workerd/api/tests/events-test.js +++ b/src/workerd/api/tests/events-test.js @@ -463,3 +463,32 @@ export const handlerThis = { strictEqual(handlerObject.handleEvent.mock.callCount(), 1); }, }; + +export const messageEventOrigin = { + test() { + // Per the spec, MessageEventInit's `origin` member is a USVString defaulting to the empty + // string, so `origin` is never null. Without the + // spec_compliant_message_event_origin compat flag we report null when there is no + // URL to derive an origin from. + const specCompliant = + Cloudflare.compatibilityFlags.spec_compliant_message_event_origin; + const event = new MessageEvent('message', { data: null }); + strictEqual(event.origin, specCompliant ? '' : null); + }, +}; + +export const eventIsTrusted = { + test() { + // Per the standard, an event's "is trusted" flag is only set when the runtime creates and + // dispatches the event, so anything constructed here is untrusted. + for (const event of [ + new Event('foo'), + new MessageEvent('message', { data: null }), + new CustomEvent('foo'), + new ErrorEvent('error'), + new CloseEvent('close'), + ]) { + strictEqual(event.isTrusted, false, event.type); + } + }, +}; diff --git a/src/workerd/api/tests/events-test.wd-test b/src/workerd/api/tests/events-test.wd-test index b87cd682f09..a433ec258e4 100644 --- a/src/workerd/api/tests/events-test.wd-test +++ b/src/workerd/api/tests/events-test.wd-test @@ -10,5 +10,16 @@ const unitTests :Workerd.Config = ( compatibilityFlags = ["nodejs_compat", "set_event_target_this", "workers_api_getters_setters_on_prototype", "dont_substitute_null_on_type_error"] ) ), + # The same tests again with spec_compliant_message_event_origin enabled. The tests that care + # read Cloudflare.compatibilityFlags, so both behaviors are covered without depending on the + # flag having an enable date. + ( name = "events-spec-compliance-test", + worker = ( + modules = [ + (name = "worker", esModule = embed "events-test.js") + ], + compatibilityFlags = ["nodejs_compat", "set_event_target_this", "workers_api_getters_setters_on_prototype", "dont_substitute_null_on_type_error", "spec_compliant_message_event_origin"] + ) + ), ], ); diff --git a/src/workerd/api/tests/eventsource-test.js b/src/workerd/api/tests/eventsource-test.js index 7e3de8b4336..2ed4802a53b 100644 --- a/src/workerd/api/tests/eventsource-test.js +++ b/src/workerd/api/tests/eventsource-test.js @@ -230,7 +230,9 @@ export const eventTest = { }); const { promise, resolve } = Promise.withResolvers(); let count = 0; - eventsource.ontest = (event) => { + // EventSource only defines onopen/onmessage/onerror. Named events like this one have to be + // observed with addEventListener(). + eventsource.addEventListener('test', (event) => { switch (count++) { case 0: { strictEqual(event.data, 'first'); @@ -243,7 +245,7 @@ export const eventTest = { break; } } - }; + }); await promise; }, }; diff --git a/src/workerd/api/tests/http-test.js b/src/workerd/api/tests/http-test.js index d812db558c6..e2b886e4e50 100644 --- a/src/workerd/api/tests/http-test.js +++ b/src/workerd/api/tests/http-test.js @@ -248,7 +248,15 @@ export const test = { assert.notStrictEqual(webSocket, null); // The server-side WebSocketPair socket's binaryType depends on the compat flag. const bt = new WebSocketPair()[0].binaryType; - const wsStr = `WebSocket {\n readyState: 1,\n url: null,\n protocol: '',\n extensions: '',\n binaryType: '${bt}'\n }`; + // The `on` event handler attributes only exist with the + // spec_compliant_event_handler_attributes compat flag, and `origin` is only the empty + // string with the spec_compliant_message_event_origin one. + const flags = Cloudflare.compatibilityFlags; + const onProps = flags.spec_compliant_event_handler_attributes + ? `,\n onopen: null,\n onmessage: null,\n onclose: null,\n onerror: null` + : ''; + const origin = flags.spec_compliant_message_event_origin ? "''" : 'null'; + const wsStr = `WebSocket {\n readyState: 1,\n url: null,\n protocol: '',\n extensions: '',\n binaryType: '${bt}'${onProps}\n }`; const messagePromise = new Promise((resolve) => { webSocket.addEventListener('message', (event) => { assert.strictEqual( @@ -257,7 +265,7 @@ export const test = { ports: [ [length]: 0 ], source: null, lastEventId: '', - origin: null, + origin: ${origin}, data: 'data', type: 'message', eventPhase: 2, diff --git a/src/workerd/api/tests/websocket-origin-sidecar.js b/src/workerd/api/tests/websocket-origin-sidecar.js new file mode 100644 index 00000000000..221743f3815 --- /dev/null +++ b/src/workerd/api/tests/websocket-origin-sidecar.js @@ -0,0 +1,68 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +// A minimal WebSocket server that sends one text message to every client that connects. Used to +// check the origin reported on the message event of a URL-backed WebSocket, which needs a real +// connection: a WebSocketPair endpoint has no URL to take an origin from. + +const http = require('http'); +const crypto = require('crypto'); + +const GUID = '258EAFA5-E914-47DA-95CA-C5AB0DC85B11'; + +function sendTextMessage(socket, message) { + const payload = Buffer.from(message); + // Payloads here are tiny, so the 7-bit length form is always enough. + const header = Buffer.alloc(2); + header[0] = 0x81; // FIN + text opcode + header[1] = payload.length; + socket.write(Buffer.concat([header, payload])); +} + +function sendCloseFrame(socket, code) { + const frame = Buffer.alloc(4); + frame[0] = 0x88; // FIN + close opcode + frame[1] = 2; + frame.writeUInt16BE(code, 2); + socket.write(frame); +} + +function upgradeToWebSocketConnection(req, socket) { + if (req.headers['upgrade'] !== 'websocket') { + socket.end('HTTP/1.1 400 Bad Request'); + return; + } + + const acceptKey = crypto + .createHash('sha1') + .update(req.headers['sec-websocket-key'] + GUID) + .digest('base64'); + + socket.write( + 'HTTP/1.1 101 Switching Protocols\r\n' + + 'Upgrade: websocket\r\n' + + 'Connection: Upgrade\r\n' + + `Sec-WebSocket-Accept: ${acceptKey}\r\n` + + '\r\n' + ); + + sendTextMessage(socket, 'hello'); + + // Close from this end once the message is out. Nothing here reads the client's frames, so + // without this the connection would stay open and the test would hang waiting on it. + sendCloseFrame(socket, 1000); + socket.end(); +} + +const server = http.createServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end('WebSocket server is running'); +}); +server.on('upgrade', upgradeToWebSocketConnection); +server.on('error', (err) => { + console.log(err.message); +}); +server.listen({ port: 0, host: process.env.SIDECAR_HOSTNAME }, () => { + console.log(`ORIGIN_SERVER_PORT=${server.address().port}`); +}); diff --git a/src/workerd/api/tests/websocket-origin-test.js b/src/workerd/api/tests/websocket-origin-test.js new file mode 100644 index 00000000000..407cd7b4cbc --- /dev/null +++ b/src/workerd/api/tests/websocket-origin-test.js @@ -0,0 +1,40 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 +import { strictEqual } from 'node:assert'; + +// The WebSocket standard says a message event's origin is the serialized origin of the socket's +// URL. Checking that needs a real connection, since a WebSocketPair endpoint has no URL, so this +// test talks to a sidecar server that sends one message on connect. +// +// env.MODE says which behavior to expect, because the +// spec_compliant_message_event_origin compat flag has no enable date. + +async function firstMessageOrigin(env) { + const address = `${env.SIDECAR_HOSTNAME}:${env.ORIGIN_SERVER_PORT}`; + const ws = new WebSocket(`ws://${address}/chat`); + try { + const { promise, resolve } = Promise.withResolvers(); + ws.addEventListener('message', (event) => { + resolve({ data: event.data, origin: event.origin }); + }); + return { address, ...(await promise) }; + } finally { + ws.close(); + } +} + +export const messageEventOrigin = { + async test(ctrl, env) { + strictEqual(typeof env.MODE, 'string'); + const { address, data, origin } = await firstMessageOrigin(env); + + strictEqual(data, 'hello'); + if (env.MODE === 'spec') { + // The origin of the URL, so scheme and host but no path. + strictEqual(origin, `ws://${address}`); + } else { + strictEqual(origin, null); + } + }, +}; diff --git a/src/workerd/api/tests/websocket-origin-test.wd-test b/src/workerd/api/tests/websocket-origin-test.wd-test new file mode 100644 index 00000000000..d0bafc0e73c --- /dev/null +++ b/src/workerd/api/tests/websocket-origin-test.wd-test @@ -0,0 +1,33 @@ +using Workerd = import "/workerd/workerd.capnp"; + +const config :Workerd.Config = ( + services = [ + (name = "websocket-origin-spec-test", worker = .specWorker), + (name = "websocket-origin-legacy-test", worker = .legacyWorker), + (name = "internet", network = (allow = ["private"])), + ], +); + +const specWorker :Workerd.Worker = ( + modules = [ + (name = "worker", esModule = embed "websocket-origin-test.js"), + ], + bindings = [ + (name = "MODE", text = "spec"), + (name = "SIDECAR_HOSTNAME", fromEnvironment = "SIDECAR_HOSTNAME"), + (name = "ORIGIN_SERVER_PORT", fromEnvironment = "ORIGIN_SERVER_PORT"), + ], + compatibilityFlags = ["nodejs_compat", "spec_compliant_message_event_origin"], +); + +const legacyWorker :Workerd.Worker = ( + modules = [ + (name = "worker", esModule = embed "websocket-origin-test.js"), + ], + bindings = [ + (name = "MODE", text = "legacy"), + (name = "SIDECAR_HOSTNAME", fromEnvironment = "SIDECAR_HOSTNAME"), + (name = "ORIGIN_SERVER_PORT", fromEnvironment = "ORIGIN_SERVER_PORT"), + ], + compatibilityFlags = ["nodejs_compat", "no_spec_compliant_message_event_origin"], +); diff --git a/src/workerd/api/web-socket.c++ b/src/workerd/api/web-socket.c++ index 098fce96948..d7d678bd2d3 100644 --- a/src/workerd/api/web-socket.c++ +++ b/src/workerd/api/web-socket.c++ @@ -262,6 +262,17 @@ IoOwn LegacyWebSocketAdapter::initNative(IoConte return ioContext.addObject(kj::mv(nativeObj)); } +namespace { +// A message event's origin is the serialized origin of the WebSocket's URL, so keep the URL parsed +// for as long as the socket lives. Sockets with no URL have no origin to report. +kj::Maybe parseUrlForOrigin(kj::Maybe& url) { + KJ_IF_SOME(u, url) { + return jsg::Url::tryParse(u.asPtr()); + } + return kj::none; +} +} // namespace + LegacyWebSocketAdapter::LegacyWebSocketAdapter(jsg::Lock& js, WebSocket& shell, IoContext& ioContext, @@ -279,7 +290,9 @@ LegacyWebSocketAdapter::LegacyWebSocketAdapter(jsg::Lock& js, ws, kj::mv(KJ_REQUIRE_NONNULL(package.maybeTags)), package.closedOutgoingConnection)), - outgoingMessages(IoContext::current().addObject(kj::heap())) {} + outgoingMessages(IoContext::current().addObject(kj::heap())) { + urlForOrigin = parseUrlForOrigin(this->url); +} // This constructor is used when reinstantiating a websocket that had been hibernating, which is // why we can go straight to the Accepted state. However, note that we are actually in the // `Hibernatable` "sub-state"! @@ -306,6 +319,7 @@ LegacyWebSocketAdapter::LegacyWebSocketAdapter(jsg::Lock& js, WebSocket& shell, allowHalfOpen(!FeatureFlags::get(js).getWebSocketAutoReplyToClose()), farNative(nullptr), outgoingMessages(IoContext::current().addObject(kj::heap())) { + urlForOrigin = parseUrlForOrigin(this->url); auto nativeObj = kj::heap(); nativeObj->state.init(); farNative = IoContext::current().addObject(kj::mv(nativeObj)); @@ -1353,20 +1367,30 @@ kj::Promise> LegacyWebSocketAdapter::readLoop( // Emit the mark here, in-scope, so it isn't dropped for want of a perf-counter monitor // scope. The limiter stamps the time (dispatch time, ~= receive time). markWebSocketPerfEvent("ws_received"_kjc); + // Per the WebSocket standard, the message event's origin is the serialized origin of the + // socket's URL. Without specCompliantMessageEventOrigin we report no origin at all, which + // MessageEvent surfaces as null. + kj::Maybe origin; + if (FeatureFlags::get(js).getSpecCompliantMessageEventOrigin()) { + origin = urlForOrigin; + } KJ_SWITCH_ONEOF(message) { KJ_CASE_ONEOF(text, kj::String) { - shell.dispatchEventImpl(js, js.alloc(js, js.str(text))); + shell.dispatchEventImpl( + js, js.alloc(js, js.str(text), kj::String(), kj::none, origin)); } KJ_CASE_ONEOF(data, kj::Array) { if (binaryType_ == BinaryType::BLOB) { // Per the WHATWG spec, deliver binary messages as Blob when binaryType is "blob". auto ab = jsg::JsArrayBuffer::create(js, data); auto blob = js.alloc(js, jsg::JsBufferSource(ab), kj::str()); - shell.dispatchEventImpl( - js, js.alloc(js, kj::str("message"), kj::mv(blob))); + shell.dispatchEventImpl(js, + js.alloc( + js, kj::str("message"), kj::mv(blob), kj::String(), kj::none, origin)); } else { jsg::JsValue ab = jsg::JsArrayBuffer::create(js, data); - shell.dispatchEventImpl(js, js.alloc(js, ab)); + shell.dispatchEventImpl( + js, js.alloc(js, ab, kj::String(), kj::none, origin)); } } KJ_CASE_ONEOF(close, kj::WebSocket::Close) { diff --git a/src/workerd/api/web-socket.h b/src/workerd/api/web-socket.h index 63ee58a1404..cbe1aca72f4 100644 --- a/src/workerd/api/web-socket.h +++ b/src/workerd/api/web-socket.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -53,8 +54,10 @@ class CloseEvent: public Event { static jsg::Ref constructor( jsg::Lock& js, kj::String type, jsg::Optional initializer) { Initializer init = kj::mv(initializer).orDefault({}); - return js.alloc(kj::mv(type), init.code.orDefault(0), + auto event = js.alloc(kj::mv(type), init.code.orDefault(0), kj::mv(init.reason).orDefault(jsg::USVString(kj::str())), init.wasClean.orDefault(false)); + event->markConstructedFromJs(); + return event; } int getCode() { @@ -343,6 +346,15 @@ class WebSocket: public EventTarget { kj::StringPtr getBinaryType(); void setBinaryType(kj::String value); + // The event handler attributes the WebSocket standard defines. These are only registered + // when the specCompliantEventHandlerAttributes compat flag is enabled; without it, assigning + // `onmessage` and friends just sets an ordinary property that EventTarget's legacy + // `on` lookup finds at dispatch time. + WD_EVENT_HANDLER_ATTRIBUTE(Open, "open"); + WD_EVENT_HANDLER_ATTRIBUTE(Message, "message"); + WD_EVENT_HANDLER_ATTRIBUTE(Close, "close"); + WD_EVENT_HANDLER_ATTRIBUTE(Error, "error"); + JSG_RESOURCE_TYPE(WebSocket, CompatibilityFlags::Reader flags) { JSG_INHERIT(EventTarget); JSG_METHOD(accept); @@ -381,6 +393,13 @@ class WebSocket: public EventTarget { JSG_INSTANCE_PROPERTY(binaryType, getBinaryType, setBinaryType); } + if (flags.getSpecCompliantEventHandlerAttributes()) { + JSG_PROTOTYPE_PROPERTY(onopen, getOnOpen, setOnOpen); + JSG_PROTOTYPE_PROPERTY(onmessage, getOnMessage, setOnMessage); + JSG_PROTOTYPE_PROPERTY(onclose, getOnClose, setOnClose); + JSG_PROTOTYPE_PROPERTY(onerror, getOnError, setOnError); + } + JSG_TS_DEFINE(type WebSocketEventMap = { close: CloseEvent; message: MessageEvent; @@ -834,6 +853,12 @@ class LegacyWebSocketAdapter final: public WebSocketAdapter { WebSocket& shell; kj::Maybe url; + + // `url` parsed, so message events can report its origin as the WebSocket standard requires. + // None for a socket that has no URL: a WebSocketPair endpoint, or one taken from an upgraded + // fetch() response. Set by the constructors that receive a URL. + kj::Maybe urlForOrigin; + kj::Maybe protocol = kj::String(); kj::Maybe extensions = kj::String(); // The binaryType attribute per the WHATWG WebSocket spec. Defaults to "blob" when the diff --git a/src/workerd/io/compatibility-date.capnp b/src/workerd/io/compatibility-date.capnp index 29a92c78b09..86de33306c6 100644 --- a/src/workerd/io/compatibility-date.capnp +++ b/src/workerd/io/compatibility-date.capnp @@ -1642,4 +1642,53 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef { $compatEnableDate("2026-08-11"); # Enables fast Workflow engine creation by generating instance IDs with the Durable Object # namespace's `newUniqueId()` method instead of UUIDs. + + specCompliantEventHandlerAttributes @186 :Bool + $compatEnableFlag("spec_compliant_event_handler_attributes") + $compatDisableFlag("no_spec_compliant_event_handler_attributes"); + # Makes `on` event handler attributes behave the way the DOM and HTML standards + # describe them. + # + # Historically our `EventTarget` implementation looked for an `on` property on the + # target itself every time an event was dispatched, and invoked it before any listener + # registered with `addEventListener()`. That behavior is not in any standard and it has + # two visible problems: `on` handlers always run first instead of in registration + # order, and anything that subclasses `EventTarget` and implements `on` properly + # (by registering a listener) gets its handler invoked twice per event. + # + # With this flag, `EventTarget` no longer looks for `on` properties. Instead, the + # interfaces that the standards say have event handler attributes (`AbortSignal`, + # `WebSocket`, `MessagePort`, `EventSource`) implement them as accessors that register a + # regular listener, so they fire in registration order and only once. Assigning a new + # handler keeps the position of the original registration, and assigning null removes it. + # + # The global scope is deliberately excluded and keeps the property lookup for every event + # type, so `onfetch`, `onscheduled` and friends in service-worker syntax are unchanged. This is + # a remaining deviation rather than a fully spec-aligned surface: `WorkerGlobalScope` and + # `ServiceWorkerGlobalScope` do define event handler attributes (`onerror`, + # `onunhandledrejection`, `onrejectionhandled`, `onfetch`, ...), but the global also dispatches + # workerd-specific events (`scheduled`, `tail`, `trace`, `alarm`, `queue`) that no standard + # covers, and `onerror` is an `OnErrorEventHandler`, which has its own arguments and return + # value handling. Turning the standardized subset into real accessors also adds properties to + # the global object, which is a breaking change in its own right. That is left for a separate + # change; the lookup is skipped for any type that does have an event handler attribute, so such + # a change can be made incrementally without double-firing handlers. + + specCompliantMessageEventOrigin @187 :Bool + $compatEnableFlag("spec_compliant_message_event_origin") + $compatDisableFlag("no_spec_compliant_message_event_origin"); + # Makes `MessageEvent.origin` report what the standards say it should. Two things change. + # + # First, an absent origin reports the empty string rather than null. A MessageEvent's origin is + # internally nullable and the standard's getter returns the empty string for the null case: + # https://html.spec.whatwg.org/multipage/comms.html#dom-messageevent-origin + # (`MessageEventInit`'s `""` default for the member is a separate supporting rule.) We returned + # null instead. This is what a `MessagePort` message, or a message from a `WebSocketPair` + # endpoint, now reports, since neither has a URL to take an origin from. + # + # Second, a `WebSocket` opened from a URL reports the serialized origin of that URL, which the + # WebSocket standard requires and we did not do: + # https://websockets.spec.whatwg.org/#feedback-from-the-protocol + # So `new WebSocket("wss://example.com/chat")` now delivers messages with an origin of + # "wss://example.com". `EventSource` already reported the origin of its event stream. } diff --git a/src/wpt/BUILD.bazel b/src/wpt/BUILD.bazel index 71513028c0e..1d39cf9d7e1 100644 --- a/src/wpt/BUILD.bazel +++ b/src/wpt/BUILD.bazel @@ -52,6 +52,9 @@ wpt_test( wpt_test( name = "dom/events", + compat_flags = [ + "spec_compliant_event_handler_attributes", + ], config = "dom/events-test.ts", wpt_directory = "@wpt//:dom/events@module", ) @@ -157,6 +160,7 @@ wpt_test( compat_flags = [ "websocket_standard_binary_type", "web_socket_auto_reply_to_close", + "spec_compliant_event_handler_attributes", ], config = "websockets-test.ts", start_server = True, diff --git a/src/wpt/websockets-test.ts b/src/wpt/websockets-test.ts index 2833928148e..ac1cbfefed9 100644 --- a/src/wpt/websockets-test.ts +++ b/src/wpt/websockets-test.ts @@ -312,30 +312,14 @@ export default { comment: 'Cookie support helper, not an actual test', omittedTests: true, }, - 'eventhandlers.any.js': { - comment: 'TreatNonCallableAsNull behavior differs from spec', - expectedFailures: [ - 'Event handler for open should have [TreatNonCallableAsNull]', - 'Event handler for error should have [TreatNonCallableAsNull]', - 'Event handler for close should have [TreatNonCallableAsNull]', - 'Event handler for message should have [TreatNonCallableAsNull]', - ], - }, + 'eventhandlers.any.js': {}, 'idlharness.any.js': { comment: - 'Some interface/attribute tests still fail due to event handler and inherited property checks', + 'Some interface/attribute tests still fail due to inherited property checks', expectedFailures: [ 'WebSocket interface: existence and properties of interface object', 'WebSocket interface: attribute bufferedAmount', - 'WebSocket interface: attribute onopen', - 'WebSocket interface: attribute onerror', - 'WebSocket interface: attribute onclose', - 'WebSocket interface: attribute onmessage', 'WebSocket interface: new WebSocket("ws://invalid") must inherit property "bufferedAmount" with the proper type', - 'WebSocket interface: new WebSocket("ws://invalid") must inherit property "onopen" with the proper type', - 'WebSocket interface: new WebSocket("ws://invalid") must inherit property "onerror" with the proper type', - 'WebSocket interface: new WebSocket("ws://invalid") must inherit property "onclose" with the proper type', - 'WebSocket interface: new WebSocket("ws://invalid") must inherit property "onmessage" with the proper type', 'CloseEvent interface: existence and properties of interface object', 'CloseEvent interface: attribute wasClean', 'CloseEvent interface: attribute code', diff --git a/tools/base.eslint.config.mjs b/tools/base.eslint.config.mjs index 31c4ffe162c..eb786b3d026 100644 --- a/tools/base.eslint.config.mjs +++ b/tools/base.eslint.config.mjs @@ -18,6 +18,7 @@ const workerdGlobals = { CustomEvent: 'readonly', DecompressionStream: 'readonly', DOMException: 'readonly', + ErrorEvent: 'readonly', Event: 'readonly', EventSource: 'readonly', EventTarget: 'readonly', diff --git a/types/generated-snapshot/experimental/index.d.ts b/types/generated-snapshot/experimental/index.d.ts index 557f4a99a97..5fe94ee5ab8 100755 --- a/types/generated-snapshot/experimental/index.d.ts +++ b/types/generated-snapshot/experimental/index.d.ts @@ -1712,7 +1712,7 @@ declare class MessageEvent extends Event { * * [MDN Reference](https://developer.mozilla.org/docs/Web/API/MessageEvent/origin) */ - readonly origin: string | null; + readonly origin: string; /** * The **`lastEventId`** read-only property of the MessageEvent interface is a string representing a unique ID for the event. * @@ -3880,6 +3880,14 @@ interface WebSocket extends EventTarget { * [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/binaryType) */ binaryType: "blob" | "arraybuffer"; + /* [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/open_event) */ + onopen: any | null; + /* [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/message_event) */ + onmessage: any | null; + /* [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/close_event) */ + onclose: any | null; + /* [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/error_event) */ + onerror: any | null; } interface WebSocketAcceptOptions { /** @@ -4325,6 +4333,10 @@ declare abstract class MessagePort extends EventTarget { start(): void; get onmessage(): any | null; set onmessage(value: any | null); + get onmessageerror(): any | null; + set onmessageerror(value: any | null); + get onclose(): any | null; + set onclose(value: any | null); } /** * The **`MessageChannel`** interface of the Channel Messaging API allows us to create a new message channel and send data through it via its two MessagePort properties. diff --git a/types/generated-snapshot/experimental/index.ts b/types/generated-snapshot/experimental/index.ts index 3f152773310..0f4482b0fa0 100755 --- a/types/generated-snapshot/experimental/index.ts +++ b/types/generated-snapshot/experimental/index.ts @@ -1716,7 +1716,7 @@ export declare class MessageEvent extends Event { * * [MDN Reference](https://developer.mozilla.org/docs/Web/API/MessageEvent/origin) */ - readonly origin: string | null; + readonly origin: string; /** * The **`lastEventId`** read-only property of the MessageEvent interface is a string representing a unique ID for the event. * @@ -3889,6 +3889,14 @@ export interface WebSocket extends EventTarget { * [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/binaryType) */ binaryType: "blob" | "arraybuffer"; + /* [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/open_event) */ + onopen: any | null; + /* [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/message_event) */ + onmessage: any | null; + /* [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/close_event) */ + onclose: any | null; + /* [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebSocket/error_event) */ + onerror: any | null; } export interface WebSocketAcceptOptions { /** @@ -4334,6 +4342,10 @@ export declare abstract class MessagePort extends EventTarget { start(): void; get onmessage(): any | null; set onmessage(value: any | null); + get onmessageerror(): any | null; + set onmessageerror(value: any | null); + get onclose(): any | null; + set onclose(value: any | null); } /** * The **`MessageChannel`** interface of the Channel Messaging API allows us to create a new message channel and send data through it via its two MessagePort properties.