events: make abort_controller event trusted #35811

benjamingr · 2020-10-26T11:58:07Z

Fire abort event as trusted, fixes #35748

Checklist

[ x make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax · 2020-10-26T12:03:32Z

lib/internal/event_target.js

-      get: isTrusted,
+      get() {
+        return this[kTrustEvent];
+      },


We should avoid this:

It tanks perf, see events: re-use the same isTrusted getter #34459 / https://twitter.com/tverwaes/status/1285497796540473344

If we make kTrustEvent a symbol property, it’s not unforgeable anymore, which means we might as well stop using an own property here

@addaleax given these conflicting requirements (unforgeable and the fact we need some events to be trusted internally) - what should we do?

Regarding performance - I am not sure the performance of isTrusted for EventTarget is that important (is it?)

@addaleax hmm, actually I can capture it from the closure - let me try and let me know what you think - though it will likely tank performance just as bad.

I am not sure the performance of isTrusted for EventTarget is that important (is it?)

It’s not the performance of isTrusted, it’s the performance of new Event() that matters here :)

@addaleax please take a look. I think what I ended up doing shouldn't tank performance or introduce a closure. Should I run benchmarks?

(Also thanks, I didn't know this tanks perf 🙏 )

Should I run benchmarks?

I think that would be helpful, yes :) And maybe also update the benchmarks to account for situations in which sometimes new Event() creates a trusted event and sometimes not :)

addaleax

LGTM if the benchmarks are good with it :)

benjamingr · 2020-10-26T12:19:36Z

Looks mostly OK:

benjamingruenbaum@Benjamins-MBP node % git checkout fix-abortcontroller-trusted-event
Switched to branch 'fix-abortcontroller-trusted-event'
benjamingruenbaum@Benjamins-MBP node % make -j12 > /dev/null                         
benjamingruenbaum@Benjamins-MBP node % ./out/Release/node ./benchmark/events/eventtarget.js
events/eventtarget.js listeners=1 n=1000000: 3,067,260.432666775
events/eventtarget.js listeners=5 n=1000000: 2,797,382.2678592885
events/eventtarget.js listeners=10 n=1000000: 2,318,102.157338759
benjamingruenbaum@Benjamins-MBP node % git checkout master
Switched to branch 'master'
benjamingruenbaum@Benjamins-MBP node % make -j12 > /dev/null 
benjamingruenbaum@Benjamins-MBP node % ./out/Release/node ./benchmark/events/eventtarget.js
events/eventtarget.js listeners=1 n=1000000: 3,144,785.5756887244
events/eventtarget.js listeners=5 n=1000000: 2,704,624.177365229
events/eventtarget.js listeners=10 n=1000000: 2,340,452.7737892433
benjamingruenbaum@Benjamins-MBP node %

Just for sport - here are benchmarks with the "old" (property) fix:

benjamingruenbaum@Benjamins-MBP node % ./out/Release/node ./benchmark/events/eventtarget.js
events/eventtarget.js listeners=1 n=1000000: 1,039,763.9506252201
events/eventtarget.js listeners=5 n=1000000: 937,062.0643370639
events/eventtarget.js listeners=10 n=1000000: 829,347.3311018195

I ran each a few times (but didn't run the t-test thing). Anyway - good catch @addaleax :]

ExE-Boss · 2020-10-26T12:17:41Z

lib/internal/event_target.js

+const isTrustedFalse = () => false;
+const isTrustedTrue = () => true;


This should use a SafeWeakSet, so that the isTrusted getter function between a trusted and untrusted event is the same:

Object.getOwnPropertyDescriptor(trustedEvent, "isTrusted").get === Object.getOwnPropertyDescriptor(untrustedEvent, 'isTrusted').get;

Suggested change

const isTrustedFalse = () => false;

const isTrustedTrue = () => true;

const isTrustedSet = new SafeWeakSet();

const isTrusted = ObjectGetOwnPropertyDescriptor({

get isTrusted() {

return isTrustedSet.has(this);

}

}, 'isTrusted').get;

Please see Anna's comment above - this would introduce a (rather big) performance regression. The initial implementation was a bit like this (except without a WeakSet which IIRC incurs its own separate performance penalty in 8).

That said - that's a very good point (regarding wanting the reference to be the same for false/true cases.

Hmm, @addaleax I would like to (if possible) make the issue @ExE-Boss raised work, namely:

Object.getOwnPropertyDescriptor(trustedEvent, "isTrusted").get === Object.getOwnPropertyDescriptor(untrustedEvent, 'isTrusted').get;

Any clever trick or idea for getting the reference of different functions to equal without creating an additional closure for every event target or running into the "have a non-configurable own accessor property on an object without the overhead of creating the dictionary for slow properties" thing?

Hm … I think the above suggestion by @ExE-Boss might work rather well, actually? If we always use the same getter, then performance-wise we’re good, and SafeWeakSet should give us the guarantees we’d want here in terms of unforgeability

Oh, I think I misunderstood the solution - this is actually pretty clever (creating the temp object and getting the reference to avoid the different property.

Benchmarks look good.

ExE-Boss · 2020-10-26T12:17:51Z

lib/internal/event_target.js

    // isTrusted is special (LegacyUnforgeable)
    ObjectDefineProperty(this, 'isTrusted', {
-      get: isTrusted,
+      get: shouldTrust ? isTrustedTrue : isTrustedFalse,


Suggested change

get: shouldTrust ? isTrustedTrue : isTrustedFalse,

get: isTrusted,

ExE-Boss · 2020-10-26T12:19:32Z

lib/internal/event_target.js

@@ -75,9 +77,11 @@ class Event {
    this[kDefaultPrevented] = false;
    this[kTimestamp] = lazyNow();
    this[kPropagationStopped] = false;
+    const shouldTrust = (options != null && options[kTrustEvent]);


Suggested change

const shouldTrust = (options != null && options[kTrustEvent]);

if (options != null && options[kTrustEvent]) {

isTrustedSet.add(this);

}

benjamingr · 2020-10-26T12:45:36Z

PTAL @ExE-Boss - neat trick 🙏

benjamingr · 2020-10-26T12:54:19Z

@ExE-Boss

P.S. since I applied the changes manually and I feel like your contribution to this PR was important - I have added you as Co-Authored-By (same as if I pressed ""Commit Suggestion" in GitHub when squashing).

I have used the same username/email combo GitHub used for you when this happened in the past - but if you prefer a different email/name combo please let me know and I'll gladly change it to whatever. Alternatively if you want me to remove you as "Co-Authored-By" let me know and I will do that instead. Whatever you prefer.

jasnell · 2020-10-26T15:29:46Z

The docs should be updated also: See: https://nodejs.org/dist/latest-v15.x/docs/api/events.html#events_event_istrusted

Trott

LGTM with any necessary doc updates applied

benjamingr · 2020-10-26T15:46:32Z

Any idea on what the docs should say?

> Type: {boolean} Returns `false` for user constructed events and `true` for Node.js internal events. 

Currently only `AbortSignal`s' `"abort"` event is fired with `isTrusted` set to `true`.

Or something?

jasnell · 2020-10-26T16:01:02Z

That works, I think. We can revisit it later if necessary.

ExE-Boss · 2020-10-26T16:00:34Z

lib/internal/event_target.js

  Symbol,
  SymbolFor,
+  SafeWeakSet,
+  ObjectGetOwnPropertyDescriptor,


This should be sorted alphabetically:

Suggested change

Symbol,

SymbolFor,

SafeWeakSet,

ObjectGetOwnPropertyDescriptor,

ObjectGetOwnPropertyDescriptor,

SafeWeakSet,

Symbol,

SymbolFor,

ExE-Boss · 2020-10-26T16:04:15Z

lib/internal/event_target.js

+  get isTrusted() {
+    return isTrustedSet.has(this);
+  }
+}, 'isTrusted').get;

 class Event {
  constructor(type, options) {


To ensure that the Event constructor has the correct length, options should be initialised to null:

Suggested change

constructor(type, options) {

constructor(type, options = null) {

This also makes it possible to use strict equality comparison when checking options !== null.

This sounds like a separate change from this one - @jasnell given past experience - would you prefer it if I made it here or if we opened an issue and it went on a separate PR?

Either works I think. If you do it here make it a separate Commit but a separate pr also works

Actually, I don't understand this - IIUC length is determined by the number of arguments - so Event.length should be 2 anyway.

I can add a test for that though - I'll push it in #35806

Added in https://github.com/nodejs/node/pull/35806/files#diff-7177d219f295c5297f8e5d119cea177fc0b50c1df33161729ec0e1213f6c3b24R412

@benjamingr

The length property defaults to the number of leading parameters without an initializer, and options = null makes the parameter have an initializer, thus the length will be 1:

https://tc39.es/ecma262/#sec-function-definitions-static-semantics-expectedargumentcount

I fixed it in the other PR :]

devsnek · 2020-10-26T16:16:46Z

lib/internal/event_target.js

+const isTrusted = ObjectGetOwnPropertyDescriptor({
+  get isTrusted() {
+    return isTrustedSet.has(this);
+  }
+}, 'isTrusted').get;


Suggested change

const isTrusted = ObjectGetOwnPropertyDescriptor({

get isTrusted() {

return isTrustedSet.has(this);

}

}, 'isTrusted').get;

function isTrusted() {

return isTrustedSet.has(this);

}

That will make the isTrusted getter function incorrectly have an own prototype property and the [[Construct]] internal method.

It will also cause it to have the wrong name.

@ExE-Boss would you mind explaining why this doesn't work?

Because the following will no longer throw a TypeError:

new (Object.getOwnPropertyDescriptor(new Event('foo'), "isTrusted").get)();

And the following will return "object" instead of "undefined":

typeof Object.getOwnPropertyDescriptor(new Event('foo'), "isTrusted").get.prototype;

And the following will return true instead of false:

Object.getOwnPropertyDescriptor(new Event('foo'), "isTrusted").get.hasOwnProperty("prototype");

We should add a test for this across the EventTarget properties :]

Trott · 2020-10-26T16:46:59Z

doc/api/events.md

@@ -1282,9 +1282,11 @@ This is not used in Node.js and is provided purely for completeness.
 added: v14.5.0
 -->

-* Type: {boolean} Always returns `false`.
+* Type: {boolean} Returns `false` for user constructed events and `true`


Suggested change

* Type: {boolean} Returns `false` for user constructed events and `true`

* Type: {boolean} Value is `false` for user-constructed events and `true`

All other properties (like event.returnValue) just use "True if..." so I will change to that?

Sure, that works.

doc/api/events.md

Trott · 2020-10-26T16:55:54Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/673/

Update: Results show no significant change:

10:08:54                                               confidence improvement accuracy (*)   (**)   (***)
10:08:54  events/eventtarget.js listeners=10 n=1000000                 3.45 %       ±5.08% ±6.82%  ±9.02%
10:08:54  events/eventtarget.js listeners=1 n=1000000                 -2.23 %       ±4.07% ±5.47%  ±7.22%
10:08:54  events/eventtarget.js listeners=5 n=1000000                  6.69 %       ±7.09% ±9.49% ±12.47%
10:08:54

ExE-Boss · 2020-10-26T17:20:26Z

test/parallel/test-abortcontroller.js

@@ -4,8 +4,10 @@
 const common = require('../common');

 const { ok, strictEqual } = require('assert');
+const { Event } = require('internal/event_target');


This line is unnecessary, as the Event constructor is available as a global property.

Suggested change

const { Event } = require('internal/event_target');

It also causes the test to fail without the ‑‑expose‑internals flag.

Event is currently not exposed globally as far as I know. Good catch on the flag - I'll add that.

(Event is not exposed at all at the moment because as you can tell - there are still a bunch of rough edges we need to iron out - I intended to work on this a lot more a few months ago and am only getting to it now)

Event is exposed since #35496, which shipped in v15.0.0:

node/lib/internal/bootstrap/node.js

Lines 143 to 148 in 9ff25b1

const {

EventTarget,

Event,

} = require('internal/event_target');

exposeInterface(global, 'EventTarget', EventTarget);

exposeInterface(global, 'Event', Event);

/ping @benjamingr Good to make this change or not so much?

I have no strong feelings regarding this - I didn't realize that we expose event globally and totally missed it.

@ExE-Boss very cool I missed this!

The AbortController abort event is trusted, currently we fire all events with isTrusted: false. Allow dispatching events internally with `isTrusted: true` and add a test for it. Co-Authored-By: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> Fixes: nodejs#35748

benjamingr · 2020-10-26T18:43:28Z

Thanks for the review and benchmark run Trott - please take a look at the wording :]

Trott · 2020-10-28T15:20:37Z

@Trott what is the process now? Do I just add the commit queue label?

You can apply the commit-queue label on a PR like this, but it will fail because CI has not been run. There's a label for that too, though! request-ci

I'll add the request-ci label now.

In general, there are a number of cases where commit-queue can fail and you will need to manually land or at least adjust things. I don't use the commit-queue label much (yet!) so I'm not familiar with all of them, but I'm sure others who use it have a pretty good handle on when it works and when it doesn't.

The up-to-date process for landing a PR would ideally be in https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#landing-pull-requests but I see that the commit-queue label is not mentioned there, so uh...it's still good if you need to go the manual route.

nodejs-github-bot · 2020-10-28T15:35:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33911/

benjamingr · 2020-10-28T16:10:01Z

@Trott ok - I see there are two failed tests in the CI that are unrelated - how should I proceed?

Trott · 2020-10-28T17:50:10Z

@Trott ok - I see there are two failed tests in the CI that are unrelated - how should I proceed?

Best thing to do with unrelated failures is to use "Resume Build" on the left nav. That will rebuild only the Jenkins subtasks that failed before. I'll go ahead and click "Resume Build" right now so you don't have to.

Optional thing you can do if you want to be a good citizen: Check to make sure that there are open issues for the tests that failed.

If not, open an issue.
If there are open issues and they haven't been updated in a while, consider leaving a comment and a copy/paste of the error trace so that it's clear that the issue is ongoing (and a recent trace will be more accurate/useful than a dated one).
If there are open issues and they have been updated recently, consider opening a PR to mark the test flaky.

nodejs-github-bot · 2020-10-28T17:50:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33914/

github-actions · 2020-10-28T18:33:08Z

Landed in 81ba3ae...f20a783

The AbortController abort event is trusted, currently we fire all events with isTrusted: false. Allow dispatching events internally with `isTrusted: true` and add a test for it. Co-Authored-By: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> Fixes: #35748 PR-URL: #35811 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

benjamingr · 2020-10-28T19:28:29Z

@Trott it worked - this is pretty cool!

The AbortController abort event is trusted, currently we fire all events with isTrusted: false. Allow dispatching events internally with `isTrusted: true` and add a test for it. Co-Authored-By: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> Fixes: #35748 PR-URL: #35811 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The AbortController abort event is trusted, currently we fire all events with isTrusted: false. Allow dispatching events internally with `isTrusted: true` and add a test for it. Co-Authored-By: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> Fixes: nodejs#35748 PR-URL: nodejs#35811 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The AbortController abort event is trusted, currently we fire all events with isTrusted: false. Allow dispatching events internally with `isTrusted: true` and add a test for it. Co-Authored-By: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> Fixes: #35748 PR-URL: #35811 Backport-PR-URL: #38386 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

benjamingr requested review from jasnell, Trott and addaleax October 26, 2020 11:58

addaleax requested changes Oct 26, 2020

View reviewed changes

addaleax approved these changes Oct 26, 2020

View reviewed changes

ExE-Boss reviewed Oct 26, 2020

View reviewed changes

benjamingr force-pushed the fix-abortcontroller-trusted-event branch from d66fba0 to 2bc7831 Compare October 26, 2020 12:52

benjamingr force-pushed the fix-abortcontroller-trusted-event branch from ebec379 to 7fc3d02 Compare October 26, 2020 14:01

Trott approved these changes Oct 26, 2020

View reviewed changes

ExE-Boss reviewed Oct 26, 2020

View reviewed changes

benjamingr force-pushed the fix-abortcontroller-trusted-event branch 2 times, most recently from ed336bd to 0e3da51 Compare October 26, 2020 16:13

devsnek reviewed Oct 26, 2020

View reviewed changes

benjamingr force-pushed the fix-abortcontroller-trusted-event branch from 565cb05 to 521bc99 Compare October 26, 2020 16:19

jasnell approved these changes Oct 26, 2020

View reviewed changes

Trott reviewed Oct 26, 2020

View reviewed changes

doc/api/events.md Show resolved Hide resolved

ExE-Boss reviewed Oct 26, 2020

View reviewed changes

benjamingr force-pushed the fix-abortcontroller-trusted-event branch from 521bc99 to 5ccc2a9 Compare October 26, 2020 18:43

Trott approved these changes Oct 26, 2020

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 28, 2020

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020

benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 28, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 28, 2020

github-actions bot closed this Oct 28, 2020

benjamingr deleted the fix-abortcontroller-trusted-event branch October 28, 2020 19:28

targos mentioned this pull request Nov 3, 2020

v15.1.0 release proposal #35948

Merged

ExE-Boss mentioned this pull request Dec 2, 2020

lib: add uncurried accessor properties to primordials #36329

Closed

3 tasks

BethGriggs added the dont-land-on-v14.x label Dec 7, 2020

targos added backport-open-v14.x and removed dont-land-on-v14.x labels Apr 24, 2021

targos mentioned this pull request Apr 24, 2021

[v14.x] Backport AbortController and friends #38386

Closed

targos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v14.x labels Apr 30, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

		const isTrustedFalse = () => false;
		const isTrustedTrue = () => true;

	get: shouldTrust ? isTrustedTrue : isTrustedFalse,
	get: isTrusted,

-    const shouldTrust = (options != null && options[kTrustEvent]);
+    if (options != null && options[kTrustEvent]) {
+      isTrustedSet.add(this);
+    }

	constructor(type, options) {
	constructor(type, options = null) {

	* Type: {boolean} Returns `false` for user constructed events and `true`
	* Type: {boolean} Value is `false` for user-constructed events and `true`

	const {
	EventTarget,
	Event,
	} = require('internal/event_target');
	exposeInterface(global, 'EventTarget', EventTarget);
	exposeInterface(global, 'Event', Event);

events: make abort_controller event trusted #35811

events: make abort_controller event trusted #35811

Conversation

benjamingr commented Oct 26, 2020

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

benjamingr commented Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Oct 26, 2020

benjamingr commented Oct 26, 2020

jasnell commented Oct 26, 2020

Trott left a comment

Choose a reason for hiding this comment

benjamingr commented Oct 26, 2020 • edited Loading

jasnell commented Oct 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Oct 26, 2020 • edited Loading

ExE-Boss Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Oct 26, 2020

Trott commented Oct 28, 2020

nodejs-github-bot commented Oct 28, 2020

benjamingr commented Oct 28, 2020

Trott commented Oct 28, 2020

nodejs-github-bot commented Oct 28, 2020

github-actions bot commented Oct 28, 2020

benjamingr commented Oct 28, 2020

benjamingr commented Oct 26, 2020 •

edited

Loading

benjamingr commented Oct 26, 2020 •

edited

Loading

ExE-Boss Oct 26, 2020 •

edited

Loading

ExE-Boss Oct 26, 2020 •

edited

Loading

ExE-Boss Oct 26, 2020 •

edited

Loading

Trott commented Oct 26, 2020 •

edited

Loading

ExE-Boss Oct 26, 2020 •

edited

Loading

ExE-Boss Oct 26, 2020 •

edited

Loading