Skip to content

fix(ext/web): clean up abort handlers when listeners are removed#36226

Open
nathanwhit wants to merge 1 commit into
denoland:mainfrom
nathanwhit:fix/eventtarget-abort-listener-cleanup
Open

fix(ext/web): clean up abort handlers when listeners are removed#36226
nathanwhit wants to merge 1 commit into
denoland:mainfrom
nathanwhit:fix/eventtarget-abort-listener-cleanup

Conversation

@nathanwhit

Copy link
Copy Markdown
Member

Summary

  • detach an AbortSignal abort handler when its associated event listener is explicitly removed
  • apply the same cleanup when a once listener is consumed
  • add regression coverage for both removal paths

Root cause

addEventListener() registered a handler on the supplied abort signal, but the event listener entry did not retain the state needed to unregister that handler. Removing the event listener therefore left the abort handler attached until the signal itself was aborted.

The listener entry now tracks the paired signal and abort handler. A shared removal helper removes the event listener and detaches that handler for both explicit and once removal.

Tests

  • cargo test -p unit_tests --test unit -- event_target_test

@bartlomieju bartlomieju changed the title fix(web): clean up abort handlers when listeners are removed fix(ext/web): clean up abort handlers when listeners are removed Jul 22, 2026

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The fix is sound: I traced the abort-during-dispatch reentrancy (safe thanks to the slice copy for >1 listener and the ArrayPrototypeIncludes guard), confirmed both once-fires-first and abort-fires-first orderings, that the abort handler is registered strictly after the dedup/aborted early-returns (so no orphan handler), that only the two existing splice sites touch listener arrays and both were converted, and that the new fields are nulled on removal so the target<->signal retention is fully broken. Both new tests are genuine regressions. One minor efficiency note inline.

Comment thread ext/web/02_event.js
listener.callback === callback
) {
ArrayPrototypeSplice(listeners[type], i, 1);
removeListener(listeners, type, listener);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the loop just above already located this entry at index i, but removeListener re-runs ArrayPrototypeIndexOf (line ~874) to rediscover the same index, so removeEventListener now does up to 2x O(n). Consider giving removeListener an optional index param and passing i here; the once path can keep passing none since it iterates a cloned handlers array and genuinely needs the lookup. Listener arrays per type are usually tiny so it's low priority, just a free win and a cleaner factoring.

(Also cosmetic, at line ~880: listener.signal && listener.abortListener — the two are always set/cleared together, so listener.abortListener alone would do.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants