Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: Handle a range of this values for dispatchEvent #33661

Closed
wants to merge 1 commit into from

Conversation

Zirak
Copy link
Contributor

@Zirak Zirak commented May 30, 2020

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
Copy link
Member

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
Copy link
Contributor Author

Zirak commented May 30, 2020

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

@jasnell
Copy link
Member

jasnell commented May 30, 2020

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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

Zirak commented May 30, 2020

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
Copy link
Member

@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?

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@domenic
Copy link
Contributor

domenic commented May 31, 2020

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
Copy link
Contributor

domenic commented May 31, 2020

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
Copy link
Member

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
Copy link
Contributor

domenic commented May 31, 2020

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
Copy link
Member

@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
Copy link
Contributor

domenic commented May 31, 2020

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
Copy link
Member

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

Or is this only an issue for vm?

@addaleax
Copy link
Member

addaleax commented Jun 1, 2020

@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
Copy link
Contributor

domenic commented Jun 1, 2020

@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
Copy link
Contributor Author

Zirak commented Jun 1, 2020

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

<iframe></iframe>

<script>
// childBody is a cross-realm object
const childBody = 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, new Event('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
Copy link
Member

addaleax commented Jun 1, 2020

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');

const otherContext = createContext();
const { port1, port2 } = new MessageChannel();
const port2moved = moveMessagePortToContext(port2, otherContext);

const { dispatchEvent } = port1;
dispatchEvent.call(port2moved, new Event('message'));

@domenic
Copy link
Contributor

domenic commented Jun 1, 2020

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
Copy link
Contributor Author

Zirak commented Jun 1, 2020

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
Copy link
Member

addaleax commented Jun 1, 2020

@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
Copy link
Member

benjamingr commented Jun 1, 2020

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

const { SymbolFor } = primordials;
// put on the prototype, probably
const kSymbolEventTargetBrand = 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
Copy link
Contributor

domenic commented Jun 1, 2020

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
Copy link
Member

addaleax commented Jun 1, 2020

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

@domenic
Copy link
Contributor

domenic commented Jun 1, 2020

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
Copy link
Member

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 Zirak force-pushed the eventtarget-thisval branch 2 times, most recently from 70daf3e to 0184632 Compare June 1, 2020 21:25
@Zirak
Copy link
Contributor Author

Zirak commented Jun 1, 2020

[...] 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.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zirak Thanks for bearing with us :)

@@ -417,6 +423,10 @@ function validateEventListenerOptions(options) {
};
}

function isEventTarget(obj) {
return obj && obj.constructor && obj.constructor[kIsEventTarget];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
} = require('internal/errors');

const perf_hooks = require('perf_hooks');
const { customInspectSymbol } = require('internal/util');
const { customInspectSymbol, kIsEventTarget } = require('internal/util');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

jasnell commented Jun 17, 2020

This will need a rebase

@jasnell jasnell added the eventtarget Issues and PRs related to the EventTarget implementation. label Jun 19, 2020
@Zirak Zirak force-pushed the eventtarget-thisval branch 2 times, most recently from 20b3123 to 69e2720 Compare June 19, 2020 18:01
On the web, dispatchEvent is finicky about its `this` value. An exception is
thrown for `this` values which are not an EventTarget.
@jasnell
Copy link
Member

jasnell commented Jun 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eventtarget Issues and PRs related to the EventTarget implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants