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

[EventTarget] Abort controller from a past subscription can remove events from future subscriptions #1346

Open
rubennorte opened this issue Dec 31, 2024 · 2 comments
Labels
interop Implementations are not interoperable with each other needs tests Moving the issue forward requires someone to write tests topic: events

Comments

@rubennorte
Copy link

What is the issue with the DOM Standard?

The addEventListener method for the EventTarget interface includes a step to add an abort step to remove the subscription: https://dom.spec.whatwg.org/#add-an-event-listener.

In the removeEventListener method (in the same interface) there is no step to remove the previous abort step (https://dom.spec.whatwg.org/#remove-an-event-listener), which means that the abort controller will still try to remove the listener when aborted, even if the previous listener was removed.

That can cause the abort controller to remove subscriptions where we abort controller itself was not used.

Example (which works according to the step and can be reproduced in browsers like Google Chrome):

/*
 * Setup
 */

const eventTarget = new EventTarget();
const callback = (event) => console.log('Event was called');
const abortController = new AbortController();

/*
 * Previous subscription
 */

eventTarget.addEventListener('customEvent', callback, {signal: abortController.signal});

eventTarget.dispatchEvent(new CustomEvent('customEvent'));
// console logs "Event was called", as expected

eventTarget.removeEventListener('customEvent', callback);

eventTarget.dispatchEvent(new CustomEvent('customEvent'));
// console does NOT log "Event was called", as expected

/*
 * New subscription
 */

// Note that we did NOT use a signal
eventTarget.addEventListener('customEvent', callback);

eventTarget.dispatchEvent(new CustomEvent('customEvent'));
// console logs "Event was called", as expected

// Aborting the controller that is not used in this subscription
abortController.abort();

eventTarget.dispatchEvent(new CustomEvent('customEvent'));
// console does NOT log "Event was called" (not as expected?)

This behavior does not match user expectations. Is it a bug in the specification, followed by implementers? Is this something that can be fixed at this point?

Thanks.

rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 2, 2025
Summary:
Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 3, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 4, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 4, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 4, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 4, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 5, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 5, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 5, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 5, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in the Web spec: whatwg/dom#1346

Differential Revision: D67758702
@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Jan 6, 2025
@annevk
Copy link
Member

annevk commented Jan 6, 2025

I don't think Chrome is following the specification here. While the callback is the same, the listener is not (we create a new listener for each call to addEventListener()). And it's the listener that we attempt to remove.

Firefox and Safari handle this correctly.

Nice discovery though! We should add test coverage for this. I think we probably don't want to add steps to clean up the signal's abort algorithms as it's not observable and would add some amount of complexity.

Would you be willing to write web-platform-tests?

@annevk annevk added interop Implementations are not interoperable with each other topic: aborting AbortController and AbortSignal topic: events and removed topic: aborting AbortController and AbortSignal labels Jan 6, 2025
@rubennorte
Copy link
Author

While the callback is the same, the listener is not (we create a new listener for each call to addEventListener()). And it's the listener that we attempt to remove.

Oh, that's a good point and the spec is indeed correct about this then.

Would you be willing to write web-platform-tests?

I hope I can find some time to do that in the new couple of weeks.

rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 6, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 7, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 7, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
rubennorte added a commit to rubennorte/react-native that referenced this issue Jan 7, 2025
Summary:

Changelog: [internal]

Adds a regression test to make sure we implement the correct spec-compliant behavior for a possible bug in ~~the Web spec~~ __Chrome__: whatwg/dom#1346

Edit: the bug is in the Chrome implementation, not in the spec.

Differential Revision: D67758702
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other needs tests Moving the issue forward requires someone to write tests topic: events
Development

No branches or pull requests

2 participants