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: improve EventListener validation #43491

Merged
merged 1 commit into from
Jun 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 13 additions & 6 deletions lib/internal/event_target.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ const {
ArrayFrom,
Boolean,
Error,
FunctionPrototypeBind,
FunctionPrototypeCall,
NumberIsInteger,
ObjectAssign,
Expand Down Expand Up @@ -381,7 +380,10 @@ class Listener {
this.callback = listener;
this.listener = listener;
} else {
this.callback = FunctionPrototypeBind(listener.handleEvent, listener);
this.callback = async (...args) => {
if (listener.handleEvent)
await ReflectApply(listener.handleEvent, listener, args);
};
this.listener = listener;
}
}
Expand Down Expand Up @@ -463,7 +465,7 @@ class EventTarget {
if (arguments.length < 2)
throw new ERR_MISSING_ARGS('type', 'listener');

// We validateOptions before the shouldAddListeners check because the spec
// We validateOptions before the validateListener check because the spec
// requires us to hit getters.
const {
once,
Expand All @@ -474,7 +476,7 @@ class EventTarget {
weak,
} = validateEventListenerOptions(options);

if (!shouldAddListener(listener)) {
if (!validateEventListener(listener)) {
// The DOM silently allows passing undefined as a second argument
// No error code for this since it is a Warning
// eslint-disable-next-line no-restricted-syntax
Expand Down Expand Up @@ -547,7 +549,7 @@ class EventTarget {
removeEventListener(type, listener, options = kEmptyObject) {
if (!isEventTarget(this))
throw new ERR_INVALID_THIS('EventTarget');
if (!shouldAddListener(listener))
if (!validateEventListener(listener))
return;

type = String(type);
Expand Down Expand Up @@ -856,7 +858,7 @@ ObjectDefineProperties(NodeEventTarget.prototype, {

// EventTarget API

function shouldAddListener(listener) {
function validateEventListener(listener) {
if (typeof listener === 'function' ||
typeof listener?.handleEvent === 'function') {
return true;
Expand All @@ -865,6 +867,11 @@ function shouldAddListener(listener) {
if (listener == null)
return false;

if (typeof listener === 'object') {
// Require `handleEvent` lazily.
return true;
}

throw new ERR_INVALID_ARG_TYPE('listener', 'EventListener', listener);
}

Expand Down
25 changes: 24 additions & 1 deletion test/parallel/test-eventtarget.js
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,30 @@ let asyncTest = Promise.resolve();
eventTarget.dispatchEvent(event);
}

{
const target = new EventTarget();
const listener = {};
// AddEventListener should not require handleEvent to be
// defined on an EventListener.
target.addEventListener('foo', listener);
listener.handleEvent = common.mustCall(function(event) {
strictEqual(event.type, 'foo');
strictEqual(this, listener);
});
target.dispatchEvent(new Event('foo'));
}

{
const target = new EventTarget();
const listener = {};
// do not throw
target.removeEventListener('foo', listener);
target.addEventListener('foo', listener);
target.removeEventListener('foo', listener);
listener.handleEvent = common.mustNotCall();
target.dispatchEvent(new Event('foo'));
}

{
const uncaughtException = common.mustCall((err, origin) => {
strictEqual(err.message, 'boom');
Expand Down Expand Up @@ -308,7 +332,6 @@ let asyncTest = Promise.resolve();
[
'foo',
1,
{}, // No handleEvent function
false,
].forEach((i) => throws(() => target.addEventListener('foo', i), err(i)));
}
Expand Down
119 changes: 119 additions & 0 deletions test/parallel/test-whatwg-events-eventtarget-this-of-listener.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
'use strict';

require('../common');
const { test, assert_equals, assert_unreached } =
require('../common/wpt').harness;

// Manually ported from: https://github.com/web-platform-tests/wpt/blob/6cef1d2087d6a07d7cc6cee8cf207eec92e27c5f/dom/events/EventTarget-this-of-listener.html

// Mock document
const document = {
createElement: () => new EventTarget(),
createTextNode: () => new EventTarget(),
createDocumentFragment: () => new EventTarget(),
createComment: () => new EventTarget(),
createProcessingInstruction: () => new EventTarget(),
};

test(() => {
const nodes = [
document.createElement('p'),
document.createTextNode('some text'),
document.createDocumentFragment(),
document.createComment('a comment'),
document.createProcessingInstruction('target', 'data'),
];

let callCount = 0;
for (const node of nodes) {
node.addEventListener('someevent', function() {
++callCount;
assert_equals(this, node);
});

node.dispatchEvent(new Event('someevent'));
}

assert_equals(callCount, nodes.length);
}, 'the this value inside the event listener callback should be the node');

test(() => {
const nodes = [
document.createElement('p'),
document.createTextNode('some text'),
document.createDocumentFragment(),
document.createComment('a comment'),
document.createProcessingInstruction('target', 'data'),
];

let callCount = 0;
for (const node of nodes) {
const handler = {};

node.addEventListener('someevent', handler);
handler.handleEvent = function() {
++callCount;
assert_equals(this, handler);
};

node.dispatchEvent(new Event('someevent'));
}

assert_equals(callCount, nodes.length);
}, 'addEventListener should not require handleEvent to be defined on object listeners');

test(() => {
const nodes = [
document.createElement('p'),
document.createTextNode('some text'),
document.createDocumentFragment(),
document.createComment('a comment'),
document.createProcessingInstruction('target', 'data'),
];

let callCount = 0;
for (const node of nodes) {
function handler() {
++callCount;
assert_equals(this, node);
}

handler.handleEvent = () => {
assert_unreached('should not call the handleEvent method on a function');
};

node.addEventListener('someevent', handler);

node.dispatchEvent(new Event('someevent'));
}

assert_equals(callCount, nodes.length);
}, 'handleEvent properties added to a function before addEventListener are not reached');

test(() => {
const nodes = [
document.createElement('p'),
document.createTextNode('some text'),
document.createDocumentFragment(),
document.createComment('a comment'),
document.createProcessingInstruction('target', 'data'),
];

let callCount = 0;
for (const node of nodes) {
function handler() {
++callCount;
assert_equals(this, node);
}

node.addEventListener('someevent', handler);

handler.handleEvent = () => {
assert_unreached('should not call the handleEvent method on a function');
};

node.dispatchEvent(new Event('someevent'));
}

assert_equals(callCount, nodes.length);
}, 'handleEvent properties added to a function after addEventListener are not reached');