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.

Sign up for GitHub

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: make abort_controller event trusted #35811

Closed
wants to merge 1 commit into from
Closed

events: make abort_controller event trusted #35811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions doc/api/events.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1282,9 +1282,10 @@ This is not used in Node.js and is provided purely for completeness.
added: v14.5.0
-->

* Type: {boolean} Always returns `false`.
* Type: {boolean} True for Node.js internal events, false otherwise.

This is not used in Node.js and is provided purely for completeness.
Currently only `AbortSignal`s' `"abort"` event is fired with `isTrusted`
Trott marked this conversation as resolved.
Show resolved Hide resolved
set to `true`.

#### `event.preventDefault()`
<!-- YAML
Expand Down
7 changes: 5 additions & 2 deletions lib/internal/abort_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ const {

const {
EventTarget,
Event
Event,
kTrustEvent
} = require('internal/event_target');
const {
customInspectSymbol,
Expand Down Expand Up @@ -49,7 +50,9 @@ ObjectDefineProperties(AbortSignal.prototype, {
function abortSignal(signal) {
if (signal[kAborted]) return;
signal[kAborted] = true;
const event = new Event('abort');
const event = new Event('abort', {
[kTrustEvent]: true
});
if (typeof signal.onabort === 'function') {
signal.onabort(event);
}
Expand Down
15 changes: 14 additions & 1 deletion lib/internal/event_target.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ const {
ObjectAssign,
ObjectDefineProperties,
ObjectDefineProperty,
ObjectGetOwnPropertyDescriptor,
String,
Symbol,
SymbolFor,
SymbolToStringTag,
SafeWeakSet,
} = primordials;

const {
Expand Down Expand Up @@ -41,6 +43,7 @@ const kRemoveListener = Symbol('kRemoveListener');
const kIsNodeStyleListener = Symbol('kIsNodeStyleListener');
const kMaxListeners = Symbol('kMaxListeners');
const kMaxListenersWarned = Symbol('kMaxListenersWarned');
const kTrustEvent = Symbol('kTrustEvent');

// Lazy load perf_hooks to avoid the additional overhead on startup
let perf_hooks;
Expand All @@ -61,7 +64,12 @@ const kBubbles = Symbol('bubbles');
const kComposed = Symbol('composed');
const kPropagationStopped = Symbol('propagationStopped');

const isTrusted = () => false;
const isTrustedSet = new SafeWeakSet();
const isTrusted = ObjectGetOwnPropertyDescriptor({
get isTrusted() {
return isTrustedSet.has(this);
}
}, 'isTrusted').get;
Comment on lines +68 to +72
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ExE-Boss ExE-Boss Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ExE-Boss ExE-Boss Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


class Event {
constructor(type, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ExE-Boss ExE-Boss Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed it in the other PR :]

Expand All @@ -77,6 +85,10 @@ class Event {
this[kDefaultPrevented] = false;
this[kTimestamp] = lazyNow();
this[kPropagationStopped] = false;
if (options != null && options[kTrustEvent]) {
isTrustedSet.add(this);
}

// isTrusted is special (LegacyUnforgeable)
ObjectDefineProperty(this, 'isTrusted', {
get: isTrusted,
Expand Down Expand Up @@ -571,5 +583,6 @@ module.exports = {
initNodeEventTarget,
kCreateEvent,
kNewListener,
kTrustEvent,
kRemoveListener,
};
4 changes: 4 additions & 0 deletions lib/internal/per_context/primordials.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ primordials.SafeSet = makeSafe(
Set,
class SafeSet extends Set {}
);
primordials.SafeWeakSet = makeSafe(
WeakSet,
class SafeWeakSet extends WeakSet {}
);
primordials.SafePromise = makeSafe(
Promise,
class SafePromise extends Promise {}
Expand Down
34 changes: 33 additions & 1 deletion test/parallel/test-abortcontroller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
// Flags: --no-warnings
// Flags: --no-warnings --expose-internals
'use strict';

const common = require('../common');

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

@ExE-Boss ExE-Boss Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

(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)

Copy link
Contributor

@ExE-Boss ExE-Boss Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@ExE-Boss very cool I missed this!


{
// Tests that abort is fired with the correct event type on AbortControllers
const ac = new AbortController();
ok(ac.signal);
ac.signal.onabort = common.mustCall((event) => {
Expand All @@ -20,3 +22,33 @@ const { ok, strictEqual } = require('assert');
ac.abort();
ok(ac.signal.aborted);
}

{
// Tests that abort events are trusted
const ac = new AbortController();
ac.signal.addEventListener('abort', common.mustCall((event) => {
ok(event.isTrusted);
}));
ac.abort();
}

{
// Tests that abort events have the same `isTrusted` reference
const first = new AbortController();
const second = new AbortController();
let ev1, ev2;
const ev3 = new Event('abort');
first.signal.addEventListener('abort', common.mustCall((event) => {
ev1 = event;
}));
second.signal.addEventListener('abort', common.mustCall((event) => {
ev2 = event;
}));
first.abort();
second.abort();
const firstTrusted = Reflect.getOwnPropertyDescriptor(ev1, 'isTrusted').get;
const secondTrusted = Reflect.getOwnPropertyDescriptor(ev2, 'isTrusted').get;
const untrusted = Reflect.getOwnPropertyDescriptor(ev3, 'isTrusted').get;
strictEqual(firstTrusted, secondTrusted);
strictEqual(untrusted, firstTrusted);
}