events: Handle a range of this values for dispatchEvent

[Zirak]

On the web, dispatchEvent is finicky about its this value. An exception is
thrown for this values which are not an EventTarget. However, null-ish
values (null and undefined) do not throw and return immediately.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[addaleax]

An exception is
thrown for this values which are not an EventTarget.

From a quick experiment, it doesn’t look like the web uses instanceof here, and instead performs some kind of brand check that works cross-context (i.e. an EventTarget instance from another iframe is also accepted). I think we should do the same thing, i.e. not use instanceof.

[Zirak]

You're right. Could you please point me somewhere else in node which does a similar thing? Or how it might be implemented?

[jasnell]

From a quick experiment, it doesn’t look like the web uses instanceof here, and instead performs some kind of brand check that works cross-context (i.e. an EventTarget instance from another iframe is also accepted). I think we should do the same thing, i.e. not use instanceof.

Yeah. The fun bit about that is that the browsers all seem to implement EventTarget with native code so I suspect they're doing their brand checking at the native code level also. To make the brand checking easier for us, we'll likely want to using something like an internal Symbol.for() and some duck-typing checks. Didn't pursue this too much yet given that there's no way for another vm context to use EventTarget yet without us passing it in but it's definitely something we should address.

[addaleax]

@Zirak I don’t know, I assumed browsers follow a certain spec on that point but I’m not sure where that is or where to find it. @benjamingr might know.

Didn't pursue this too much yet given that there's no way for another vm context to use EventTarget yet without us passing it in but it's definitely something we should address.

Yeah, the reason I commented was that we may want to look into moving our MessagePort implementation towards using EventTarget – and that already does support multiple contexts.

[benjamingr]

@Zirak I recommend you use a symbol and check for its presence, that could work. Thanks by the way I am in favor for this change with the reservations Anna raised.

[Zirak]

A simple symbol (const kIsEventTarget = Symbol('EventTarget')) won't fly since each realm will have its own distinct symbol.

Using Symbol.for gives us the same symbol for every realm, but feels too global long-term. There is a tiny amount of prior art in node core (see here).

To take a step back from the global-ness of Symbol.for, as @jasnell mentioned, node can have its own symbol registry. We can maybe sidestep the symbol aspect and create a helper in cpp to test these things out.

To summarise, we can:

• Use a global Symbol (Symbol.for)
• Create a private Symbol registry
• Do some cpp magic
• Other?

In my opinion, anything below the first option seems overkill for this point in time.

[benjamingr]

@domenic@annevk any guidance regarding what Node should do here regarding dispatchEventListener.call(targetFromOtherRealm - are there easy ways for Node to behave like the DOM here?

Is this behavior even part of the EventTarget spec?

[domenic]

This behavior is part of Web IDL; see https://heycam.github.io/webidl/#dfn-create-operation-function step 2.1.2. (And thus it's part of the definition of all methods on the web platform, including those of EventTarget and URL.)

I'd suggest Node does whatever it currently does for URL.

[domenic]

Note also that the only reason it works with a null or undefined thisArg is because the global object implements EventTarget. For Node.js I assume that is not the case, in which case a null or undefined thisArg should throw. (I.e., the parenthetical in 2.1.2.1 applies.)

[benjamingr]

Node URL doesn't have any methods except toJSON which uses a symbol and doesn't work cross realm. Node is (probably unknowingly) violating the URL spec here (in a small way).

I think this would have to be solved in C++ since cross realm branding isn't possible without a well known global. Probably a Symbol() like function that works across code. Not sure if to use something like v8::Private makes sense (if we hope to make solving this thread safe or work across worker-threads).

[domenic]

Getters and setters also have a brand check

Workers can't synchronously call methods on other-thread objects so cross-thread is not a concern, at least on the web.

[benjamingr]

@domenic I assumed URLs were transferable like MessagePorts. Apparently they're not so this is not really a concern for the web or Node (for URLs).

I'd still like Node to fix this for MessagePorts and EventTargets if possible.

[domenic]

Transferable objects still don't have this problem. You still can't call methods/getters/setters across realms. The transferred object now lives in the destination realm.

[benjamingr]

@addaleax then I'm confused why this would be an issue for MessagePort.

Or is this only an issue for vm?

[addaleax]

@benjamingr In browsers, you can call any EventTarget.prototype.dispatchEvent function on any MessagePort instance, regardless of which contexts they are from.

I know it’s kind of an edge-case, but the same should work in Node.js, I would say.

[domenic]

@benjamingr In browsers, you can call any EventTarget.prototype.dispatchEvent function on any MessagePort instance, regardless of which contexts they are from.

I think there is still some confusion. Can you give a code example? Note that if you transfer a MessagePort from a worker to a window or vice-versa, this creates a new MessagePort object wrapping the underlying "message port" abstraction. The transferred version is not "from the original context"; it is created newly in the destination context.

[Zirak]

If I understood @addaleax correctly, the concern on the web is something like:

<iframe></iframe><script>// childBody is a cross-realm objectconstchildBody=document.querySelector('iframe').contentDocument.body;document.body.onclick=(e)=>console.log('outer',e);childBody.onclick=(e)=>console.log('inner',e);document.body.dispatchEvent.call(childBody,newEvent('click'))// logs 'inner'</script>

So access such a cross-realm object via same-origin iframes. Do MessagePorts also exhibit such behaviour? Being accessible cross-realm?

[addaleax]

Right – and for completeness, the Node.js equivalent of that, assuming that we make MessagePort implement EventTarget, would be

const{ MessageChannel, moveMessagePortToContext }=require('worker_threads');const{ createContext }=require('vm');constotherContext=createContext();const{ port1, port2 }=newMessageChannel();constport2moved=moveMessagePortToContext(port2,otherContext);const{ dispatchEvent }=port1;dispatchEvent.call(port2moved,newEvent('message'));

[domenic]

I see, yes, if vm contexts are involved, then the concern arises. I thought the discussion was purely about actual threads (created by workers) and thread-safety, not vm contexts.

[Zirak]

moveMessagePortToContext

Very interesting, I didn't know about that, thanks!

It seems like a cpp helper using something like a v8:Private (as @benjamingr pointed out) is the path of some lesser resistance.

How about a pair of helper native functions, one to brand the passed object as an event emitter, and another to check the branding? Is there a file or directory they'd be home in?

[addaleax]

@Zirak That’s possible, yes. I’d probably just stick those helpers in src/node_util.cc, if that works for you.

I don’t want to bikeshed this too much, but there is one problem with that approach, and that is performance. Boundary crossings between C++ and JS are expensive, and a brand check that calls into C++ would have a non-trivial performance impact.

That’s why I upvoted #33661 (comment), and maybe I should have been more explicit about it – I liked not only the summary but also the conclusion, namely that Symbol.for() would be a reasonable option imo.

[benjamingr]

I think a symbol would be a reasonable alternative, under the Node.js "namespace":

const{ SymbolFor }=primordials;// put on the prototype, probablyconstkSymbolEventTargetBrand=Symbol.for('nodejs.event_target.brand');

Or something similar.

I share Anna's concern regarding the performance penalty of a C++ call, I think "leaking" a symbol prefixed with nodejs is a reasonable way to deal with it.

[domenic]

Note that if you're planning to use private fields, then you will not be able to work cross-realm anyway. Private fields have a built-in same-realm brand check. It might be simpler for Node to disallow cross-realm method invocation, at least as long as things are implemented in JS.

[addaleax]

@domenic I don’t think anybody was suggesting that – v8::Private is not the same as a private field.

[domenic]

But EventTarget currently uses private fields, so you would have to remove all of that usage if you wanted to make it work cross-realm.

[benjamingr]

The only private field in EventTarget is emitting which is being removed for spec compliance in #33624 although kEvents, kStop and kTarget would have to be made part of the symbol registry in the same way.

Otherwise indeed things like dispatchEvent or addEventListener wouldn't work cross realm because when you .call on them they change properties by symbols like this[kTarget] which are different per realm.

[Zirak]

[...] namely that Symbol.for() would be a reasonable option imo.

Great, I'll take any reason to not write cpp :)

Addressed with a call to SymbolFor, and leaving out the special case for nullish values as @domenic very correctly pointed out.

Thanks all for your guidance and patience.

[addaleax]

@Zirak Thanks for bearing with us :)

[domenic]

It's not clear how fool-proof you want to be here, but I'll note that people can fool this brand check. For example, EventTarget.prototype.dispatchEvent.call({ constructor: EventTarget }) would pass with this implementation, but fail per spec.

But anything you do will probably be a compromise. If something works cross-realm, then it can be fooled (and thus is not spec-compliant). If it doesn't work cross-realm, then it is not spec compliant either.

So without C++ there will always be compromises. Maybe this version is a fine compromise.

[jasnell]

Nit: We should likely include this explanation in a comment here on isEventTarget() in case this ends up coming up in a question later.

[benjamingr]

LGTM, would be interested in seeing a benchmark run with this - but I think this is the cheapest option we have for the brand check.

We should bring this issue up to V8 and in the summit as it's likely to return again.

[jasnell]

Nit: why define kIsEventTarget in a separate file? It could just be included in this file?

[Zirak]

From looking around the node codebase, it seemed like some calls to SymbolFor happened there. On another glance, it looks like there's more usage inside files, so np, I'll move it in here.

[jasnell]

This will need a rebase

[jasnell]

Closing in favor of #34015 (which combines this and other eventtarget PRs into a single to make landing easier