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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion lib/internal/event_target.js
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ const {
Set,
Symbol,
NumberIsNaN,
SymbolFor,
SymbolToStringTag,
} = primordials;

Expand All @@ -16,13 +17,16 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_EVENT_RECURSION,
ERR_OUT_OF_RANGE,
ERR_MISSING_ARGS
ERR_MISSING_ARGS,
ERR_INVALID_THIS,
}
} = require('internal/errors');

const { customInspectSymbol } = require('internal/util');
const { inspect } = require('util');

const kIsEventTarget = SymbolFor('nodejs.event_target');

const kEvents = Symbol('kEvents');
const kStop = Symbol('kStop');
const kTarget = Symbol('kTarget');
Expand Down Expand Up @@ -185,6 +189,10 @@ class Listener {
}

class EventTarget {
// Used in checking whether an object is an EventTarget. This is a well-known
// symbol as EventTarget may be used cross-realm. See discussion in #33661.
static [kIsEventTarget] = true;
Zirak marked this conversation as resolved.
Show resolved Hide resolved

[kEvents] = new Map();
#emitting = new Set();

Expand Down Expand Up @@ -257,6 +265,10 @@ class EventTarget {
throw new ERR_INVALID_ARG_TYPE('event', 'Event', event);
}

if (!isEventTarget(this)) {
throw new ERR_INVALID_THIS('EventTarget');
}

if (this.#emitting.has(event.type) ||
event[kTarget] !== null) {
throw new ERR_EVENT_RECURSION(event.type);
Expand Down Expand Up @@ -447,6 +459,15 @@ function validateEventListenerOptions(options) {
};
}

// Test whether the argument is an event object. This is far from a fool-proof
// test, for example this input will result in a false positive:
// > isEventTarget({ constructor: EventTarget })
// It stands in its current implementation as a compromise. For the relevant
// discussion, see #33661.
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.

}

function addCatch(that, promise, event) {
const then = promise.then;
if (typeof then === 'function') {
Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-eventtarget.js
View file
Open in desktop
Expand Up @@ -439,3 +439,28 @@ ok(EventTarget);
const event = new Event('');
strictEqual(event.toString(), '[object Event]');
}

{
// `this` value of dispatchEvent
const target = new EventTarget();
const target2 = new EventTarget();
const event = new Event('foo');

ok(target.dispatchEvent.call(target2, event));

[
'foo',
{},
[],
1,
null,
undefined,
false,
Symbol(),
/a/
].forEach((i) => {
throws(() => target.dispatchEvent.call(i, event), {
code: 'ERR_INVALID_THIS'
});
});
}