fix(ext/web): Copy EventTarget list before dispatch

[vrugtehagel]

Related issue: #19358.

This is a regression that seems to have been introduced in #18905. It looks to have been a performance optimization.

The issue is probably easiest described with some code:

const target = new EventTarget();
const event = new Event("foo");
target.addEventListener("foo", () => {
  console.log('base');
  target.addEventListener("foo", () => {
    console.log('nested');
  });
});
target.dispatchEvent(event);

Essentially, the second event listener is being attached while the foo event is still being dispatched. It should then not fire that second event listener, but Deno currently does.

[CLAassistant]

All committers have signed the CLA.

[mmastrac]

I believe this was a useful optimization for us to keep our websocket perf up. Rather than reverting this and always cloning the array, I would suggest that we memoize the handlers.length instead so that in the case where we're not slicing the array, we'll only ever go through the loop once.

[mmastrac]

Github won't allow me to added suggestions on deleted lines, but I think it would look like the code below:

  let handlers = targetListeners[type];
  const handlersLength = handlers.length;
  // Copy event listeners before iterating since the list can be modified during the iteration.
  if (handlersLength > 1) {
    handlers = ArrayPrototypeSlice(targetListeners[type]);
  }
  
for (let i = 0; i < handlersLength; i++) {

[vrugtehagel]

Sounds good! Your suggestion has been committed 🙂

[aapoalas]

Does this bug appear in the reverse? If you remove a listener when firing an event?

Partially sounds to me like the listener list should be shallowe copied upon add/remove of listeners, and triggers should take an object reference to keep "snapshotted" list

[vrugtehagel]

Hey @aapoalas! No, it does not happen in reverse; it would if the bug existed in the general case, but that's not true. The optimization done (see link in PR description) was just "no shallow copy if there is only one listener attached" which breaks in the example from the test in this PR, but this also means that when adding two or more listeners, things behave like they should. That means that this bug does not exist in the "removal" variant, because the buggy path only exists for when there is one event listener in the first place.

The solution here (after the suggested change) is to

• If there are more than one listeners, shallow copy (as before);
• Otherwise, if there is only one, loop through only the first item.

This is functionally equivalent to shallow copying, except avoids shallow copying in the case where there is one listener only (as a performance optimization).

[mmastrac]

LGTM!