From a079b482b7631ed5efed169d8af9f9c9467cec1f Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Wed, 29 Jun 2022 20:26:18 +0900 Subject: [PATCH] events: improve `EventListener` validation This fixes validating `EventListener` given to `add/removeEventListener` to improve the Web API compatibility. According to the WPT test failure with the current validation, `addEventListener` should not require `handleEvent` to be defined on an `EventListener`". IIUC, the same can be applied to `removeEventListener` also. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: https://github.com/nodejs/node/pull/43491 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- lib/internal/event_target.js | 19 ++- test/parallel/test-eventtarget.js | 25 +++- ...twg-events-eventtarget-this-of-listener.js | 119 ++++++++++++++++++ 3 files changed, 156 insertions(+), 7 deletions(-) create mode 100644 test/parallel/test-whatwg-events-eventtarget-this-of-listener.js diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js index 27c1263acc4..af12eb95229 100644 --- a/lib/internal/event_target.js +++ b/lib/internal/event_target.js @@ -4,7 +4,6 @@ const { ArrayFrom, Boolean, Error, - FunctionPrototypeBind, FunctionPrototypeCall, NumberIsInteger, ObjectAssign, @@ -385,7 +384,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; } } @@ -467,7 +469,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, @@ -478,7 +480,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 @@ -551,7 +553,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); @@ -860,7 +862,7 @@ ObjectDefineProperties(NodeEventTarget.prototype, { // EventTarget API -function shouldAddListener(listener) { +function validateEventListener(listener) { if (typeof listener === 'function' || typeof listener?.handleEvent === 'function') { return true; @@ -869,6 +871,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); } diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js index 0211dc964d2..c3521d35d82 100644 --- a/test/parallel/test-eventtarget.js +++ b/test/parallel/test-eventtarget.js @@ -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'); @@ -308,7 +332,6 @@ let asyncTest = Promise.resolve(); [ 'foo', 1, - {}, // No handleEvent function false, ].forEach((i) => throws(() => target.addEventListener('foo', i), err(i))); } diff --git a/test/parallel/test-whatwg-events-eventtarget-this-of-listener.js b/test/parallel/test-whatwg-events-eventtarget-this-of-listener.js new file mode 100644 index 00000000000..16ee14feabe --- /dev/null +++ b/test/parallel/test-whatwg-events-eventtarget-this-of-listener.js @@ -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');