Filtering only trusted events in addEventListener

[Jamesernator]

See my second comment for modified suggestion.

As a note, this issue is primarily targeted at AbortSignal's "abort" event, however the infrastructure could be useful to many event types where listeners after a certain point are probably a mistake.

Anyway, currently with AbortSignal we currently have a problem where AbortSignals may live a long time (possibly very long times if they're forwarded from program level). This means if we pass an AbortSignal around a program, it is very easy to leak memory accidentally by using .addEventListener("abort", () => doSomethingOnAbort()). Similarly if an event handler is subscribed late, it will never be collected (due to the existence of .dispatchEvent)

A related issue is also due to the existence of .dispatchEvent, and that's that even if we add a "abort" handler, if we don't write awkward boilerplate to check if the event .isTrusted, then there is no guarantee about consistency with .aborted or with behaviour of web APIs, i.e.:

const controller = new AbortController();
const { signal } = controller;

const req = await fetch("/some/url", { signal });
signal.addEventListener("abort", () => {
    console.log("Some cleanup action!");
});

// This triggers listeners, but leaves an inconsistent state as
// fetch does not care about this event and so the fetch is not cancelled
// Additionally because more .dispatchEvent or real events could be fired
// on the signal, the handler will be never be removed
signal.dispatchEvent(new Event("abort"));

I'd like to propose a solution that solves both of these issues at once, and that is the idea I'm referring to as "robust events". The basic idea of "robust events" is essentially that certain events are marked so that .dispatchEvent is not allowed.

Essentially the idea here is that some APIs (in particular AbortSignal "abort") would declare their events as robust, what this entails is that only events which are trusted can be dispatched on the event target, further the API can declare when a certain type of event may no longer be fired and as such mark all listeners as garbage collectable.

With this idea effectively we get this:

const controller = new AbortController();
const { signal } = controller;

const req = await fetch("/some/url", { signal });

const finalizationRegistry = new FinalizationRegistry((handler) => {
    // This will eventually happen, assuming normal GC, despite no
    // explicit removal of the event listener
    console.log("Collected the handler!");
});

let listener = () => {
    // Guaranteed to be true
    console.log(signal.aborted);
};

signal.addEventListener("abort", listener);

// Make only reference to listener inside signal event handler list
listener = null;

try {
    signal.dispatchEvent(new Event("");
} catch {
    // This happens because dispatchEvent throws an error
    console.log("Some error was thrown!");
}

// Triggers the listener, this works as expected and the listener is fired
// After this the handlers are all eligible for collection
controller.abort();

Note that this might be useful outside of AbortSignal, there's actually a number of events where only real (.isTrusted) events are generally of interest. For example audioNode.onended events, animation.onfinished. It's also worth noting that this isn't just useful for "one-time" events, but rather any events which never occur again after a certain point (i.e. maybe websocket messages, or similar).

Some places where this idea is not a good idea would be all DOM events, DOM events are often dispatched explicitly in order to transform events from one kind to another so if you have a handler you probably do want fake events in the majority of cases.

Some open questions remain with this idea though:

• Is changing "abort" to disallow .dispatchEvent web-compatible?
  • If not this could be done as an option to .addEventListener, i.e. .addEventListener("abort", callback, { robust: true }), when robust: true is set then only trusted events are considered, and robust callbacks can be collected
  • Note in this sub-idea: regular callbacks will still be called for non-trusted events and will never be collected as long as the event handler lives
• Similarly if other APIs want to use this are changing those APIs web-compatible? Certainly new APis would be, but there are probably cases where old APIs may wish to allow GC of handlers
• Should we have the event handler option anyyway, this means .dispatchEvent would never throw, but in general whether or not .addEventListener observes those events would be based on some set default, i.e. for AbortSignal we'd declare that "abort" handlers are robust by default, so they would both ignore .dispatchEvent events, and be released for garbage collection on the real event

• Should we allow userland EventTarget instances the ability to have robust events?

  • If so we'd do something like

    const target = new EventTarget({
      robustEvents: ["my-event-1", "my-event-2"],
      // Dunno what the best API shape would be here
      start: async ([dispatchEvent1, dispatchEvent2]) => {
          // On garbage collection of either dispatchEvent1 or dispatchEvent2
          // the handlers for those events could be collected also
    
          // When passing this event into dispatchEvent1 it also gets marked
          // as .isTrusted === true, this makes it more similar to a UA event
          dispatchEvent1(new Event("my-event-1"));
      },
    });

~

[annevk]

863 considered isTrusted a legacy feature, but yeah, I think there is some value in it for cases like this. I think the correct approach here would be to add a feature that event listeners can filter for this, not to forbid dispatchEvent(). The latter seems too drastic and quite a departure from the existing model, whereas the former fits in quite naturally.

As for new EventTarget(), perhaps https://blog.domenic.me/the-revealing-constructor-pattern/ would be a way to expose a dispatchEvent() that can dispatch trusted Event instances. Although really you want the Event instance itself to be created or blessed in such a way (so ev.isTrusted is correct at all times), so this might need some more thought.

[Jamesernator]

863 considered isTrusted a legacy feature, but yeah, I think there is some value in it for cases like this. I think the correct approach here would be to add a feature that event listeners can filter for this, not to forbid dispatchEvent(). The latter seems too drastic and quite a departure from the existing model, whereas the former fits in quite naturally.

Yeah thinking about I agree it would be simpler for it just to be an option, maybe something like eventTarget.addEventListener("some-event", listener, { trustedOnly: true }).

Something I'm torn on is whether or not event targets should allow configuring the default value of this option for certain events, in particular because the naive usage of abortSignal.addEventListner("abort", memoryLeakingListener). I feel like the majority of people would appreciate having the naive version automatically clean up the listener after abort happens, people who want non-trusted events on AbortSignal could just pass { trustedOnly: false } explictly.

As for new EventTarget(), perhaps https://blog.domenic.me/the-revealing-constructor-pattern/ would be a way to expose a dispatchEvent() that can dispatch trusted Event instances. Although really you want the Event instance itself to be created or blessed in such a way (so ev.isTrusted is correct at all times), so this might need some more thought.

Yeah, the tricky thing here is that events can be subclassed so even if there was something like:

const target = new EventTarget({
    start(controller) {
        controller.dispatchTrustedEvent({ name: "some-event" });
    },
});

there would no way to create instances of arbitrary classes. Now we could certainly have something like:

class MyEvent extends Event {
    constructor() {
        super("my-event");
    }

    myEventMethod() {
        // Frobulate the foobar
    }
}

const target = new EventTarget({
    start(controller) {
        const event = new MyEvent();
        controller.markTrusted(event);
        controller.dispatchEvent(event);
    },
});

however this would beg some questions, like what would happen if we dispatched the event on a different event listener? Throw an error? clear the trusted mark? Only allow dispatchEvent on the same controller to be used?

I feel like whoever is creating the events won't really care that .isTrusted would be initially false but set to true during dispatch simply because in the majority of cases the event is created immediately before dispatch anyway. Even if they are going to something with the event object before dispatching (calling methods or whatever), it can just be assumed that such actions are trusted as whoever called the constructor has full control of the event object anyway (i.e. if a method is called on the event, and the method wants to check .isTrusted, instead it can consider event.eventPhase === Event.NONE as trustworthy as well).

The userland part of the idea still clearly needs more thought, so I don't know if it would be worth specifying { trustedOnly } by itself to allow abortSignal (and probably some other specs eventually) to use this, and revisit userland trusted events as a follow-on proposal.

[annevk]

Another change that's needed for reasonable userland AbortSignal implementations is that all event listeners get run, regardless of stopImmediatePropagation() invocation.

@rniwa @smaug---- @mfreed7 @domenic do you think it's reasonable if we created an event dispatch feature that allows for disabling stopImmediatePropagation() (and perhaps stopPropagation() at the same time)?

(Maybe I should split this into a separate issue as this seems like a requirement, whereas filtering for isTrusted is more of a nice-to-have.)

[smaug----]

Why would we disable stopImmediatePropagation or stopPropagation. I don't see any strong reasons for that.

About listeners only for trusted events, Gecko has had wantsTrusted option / 4th param for privileged JS for ages https://searchfox.org/mozilla-central/rev/85b4f7363292b272eb9b606e00de2c37a6be73f0/dom/webidl/EventTarget.webidl#21,26,42

[annevk]

@smaug---- the idea is that you can hand out a signal to someone safely without them being able to stop the signal from doing something. However, if you can prevent further listeners from running you have a problem in this scenario:

giveSignalToSomeone(mysignal);
userlandFetch(..., { signal: mysignal });

giveSignalToSomeone() could prevent userlandFetch() from seeing the signal was aborted by using stopImmediatePropagation().

[Jamesernator]

(Maybe I should split this into a separate issue as this seems like a requirement

This would make sense, trustedOnly is useful for other events as well not just abort related events.

I will change the title of this issue to reflect the scope of what I was suggesting.

Also I know there are discussions in AbortSignal.any about weak listeners, as mentioned in one of my previous comments, if trustedOnly were specified one could make such event listeners weak on the associated AbortController(s) (as one can't make isTrusted: true events without access to the controller).

[smaug----]

giveSignalToSomeone() could prevent userlandFetch() from seeing the signal was aborted by using stopImmediatePropagation().

...if using event, and that is the whole point of stop*propagation. The same applies to all the event targets. And if one wants to follow signals, any() should help here, since it can be used to basically clone the signal object.

[annevk]

Part of @smaug----'s comment above was on-topic, repeating that here:

About listeners only for trusted events, Gecko has had wantsTrusted option / 4th param for privileged JS for ages https://searchfox.org/mozilla-central/rev/85b4f7363292b272eb9b606e00de2c37a6be73f0/dom/webidl/EventTarget.webidl#21,26,42