Implement EventTarget and Event builtin

[andreiltd]

This patch sets the stage for incoming AbortController and AbortSignal:

• implements Event interface: https://dom.spec.whatwg.org/#interface-event,
• implements CustomEvent interface: https://dom.spec.whatwg.org/#interface-customevent,
• implement EventTarget interface: https://dom.spec.whatwg.org/#interface-eventtarget,
• adds a global EventTarget instance,
• makes FetchEvent a subclass of Event and uses global EventTarget.

Closes: #156

[guybedford]

Great to see this. Strictly speaking FetchEvent is supposed to be a subclass of this Event. Not sure how practical that will be though, but I think that's the WinterTC expectation.

[andreiltd]

Great to see this. Strictly speaking FetchEvent is supposed to be a subclass of this Event. Not sure how practical that will be though, but I think that's the WinterTC expectation.

Yeah, I think it should be doable similar to how we made File extending Blob. I will look into this when I have a moment. I think the other nice to have event would be CustomEvent.

[andreiltd]

Strictly speaking FetchEvent is supposed to be a subclass of this Event.

So this is now working as expected, for instance this code shows FetchEvent using Event interface (event.type).

async function handler(event) {
  console.log(event.type);
}

addEventListener("fetch", (event) =>
  event.respondWith(
    (async () => {
      await handler(event);
      return new Response(`Success`);
    })(),
  ),
);

[tschneidereit]

This looks great, thank you for the detailed work!

I left a bunch of comments, and there are a number of things that I'd like to take a second look at, but it's quite close!

[tschneidereit]

I think it'd be great if we could skip this here. Instead, just set the removed flag on the listener here, perhaps add a bool listeners_removed outparam to this function, and then filter the target's listeners list once after the loop. It'd require changing list to JS::HandleVector<&EventListener>, but that seems like a good change to do regardless: no need to actually copy all those listener instances.

Do you think that makes sense?

[andreiltd]

I think the problem is that there is nothing in the spec that would prevent a listener to modify the EventTarget (.e.g. removing itself from the target) during the dispatch and I believe this is why the spec explicitly calls for cloning the listeners. I think holding onto listener references during dispatch is not sound but moving removals to after the dispatch sounds like a good optimization - we can mark cloned listeners as removed and do actual removing outside dispatch loop. WDYT?

[tschneidereit]

Looking at the note under step 6 of https://dom.spec.whatwg.org/#concept-event-listener-invoke, it seems like we actively have to make the EventListener entries be references? If, during the invoke operation, a listener is removed, its removed flag should be set, and the listener not be invoked. But with the current implementation, we won't see the flag here, because all the entries are copies.

[tschneidereit]

more generally, as mentioned in that same note in the spec, the goal of the list copy is to prevent added listeners from running (presumably because that could result in infinite loops), not preventing absolutely any modifications to the entries

[andreiltd]

That might be correct behavior, yes. The technical difficulty I mentioned stems from maintaining valid references during dispatch. Imagine code like this:

std::vector<EventListener> listeners = EventTarget::listeners();
std::vector<&EventListener> cloned =  cloned();  // derived from listeners

for listener in cloned {
    dispatch(|_| {
        add_new_listener();
        // adding new listeners triggers reallocate of `listeners`
        // continuing the loop is UB
    });
}

I guess this could be solved by having std::vector<shared_ptr<EventListener>> in EventTarget instead, with the performance penalty: each listener is now separate allocation so iterating is slower due to potential cache misses and no prefetch. But maybe that's fine. 🙂

[andreiltd]

OK so the problem is that if we defer removal of once listener to after the dispatch loop instead of removing them immediately, the listener dispatch function cannot re-add the handler because it already exists. There is a specific WPT test for this.

In theory we could still delay the removal if we have an additional check in addEventListener function for: if listener exists but it's marked for removal then add it anyway (or simply toggle removed). I'm happy to do that if we think that the potential performance gains justify diverging from spec.

[tschneidereit]

In theory we could still delay the removal if we have an additional check in addEventListener function for: if listener exists but it's marked for removal then add it anyway (or simply toggle removed). I'm happy to do that if we think that the potential performance gains justify diverging from spec.

We should definitely not diverge from the spec in actual behavior here. I don't think we need to, though: instead, we can check if the removed flag is set, and if so reset the flag and move the entry to the end of the list. I think that should preserve specified behavior? (The fact that moving the listener to the end is a slightly more costly operation is entirely fine: this is decidedly a corner case.)

[andreiltd]

Sorry I don't follow 😅 do you mean something like:

  for (auto &listener : list) {
    ...
    if (listener->once) {
       listener->removed = true;
       // move to the end
    }
    
    // dispatch
  }
  
   // defer removal
   target_list->eraseIf([&listener](const auto &other) { return listener->removed; });

[tschneidereit]

Not quite, but close. IIUC, the problem is that it's possible to re-add a once listener during dispatching, in which case it needs to be kept in the list, but moved to the end, right? If so, I'd imagine something like this:

  for (auto &listener : list) {
    ...
    if (listener->once) {
       listener->removed = true;
    }
    
    // dispatch
  }
  
   // defer removal
   target_list->eraseIf([&listener](const auto &other) { return listener->removed; });

// in add_listener:
  if (found(listener) && listener.removed) {
    listener.removed = false;
    // move to the end
  }

So if add_listener is calling during dispatching, we reset the removed flag, but also move the listener to the end, because it shouldn't keep its previous position in the list.

[andreiltd]

Ah yes, sure! That makes sense, let me try this.

EDIT: Seems to work like a charm.

[tschneidereit]

I am so, so sorry for taking so long with this review! :(

This is in great shape, with only a handful of comments below. Not all of them need addressing, but I think at least one is a correctness issue.

Still, with that addressed, this will be ready to go!

[tschneidereit]

I think we need to ensure that the various fields are updated correctly here. Only type, callback_val, and capture are compared in the search, so for all other fields we can have differences between the old and the new value. E.g., a listener could first be registered with once set, and then during its execution be re-registered without once.

Would be great if we could have that in the integration test, too.

[andreiltd]

That's a great catch! I updated the code and added integration test.

[tschneidereit]

Looks great! Feel free to merge whenever