fix(track-button): properly remove event listeners on dispose to prevent leaks and runtime errors

[nochev]

Description

This PR fixes a memory leak issue in the TrackButton component.
Currently, TrackButton attaches event listeners to TextTrackList and the player instance, but only removes some of them when the player is disposed. If a TrackButton instance itself is disposed (e.g. when custom text tracks are removed/added dynamically), the listeners remain active, causing stale references and potential leaks.

The issue was noticed when dynamically removing and adding text tracks in CaptionsButton or TrackButton components — old listeners were not cleaned up, leading to unexpected behavior, memory usage growth and errors like main.js:1685 VIDEOJS: ERROR: Error: Invalid target for null#on; must be a DOM node or evented object..

Specific Changes proposed

• Store references to updateHandler and disposeHandler as private properties (this.updateHandler_, this.disposeHandler_).
• Override dispose() to:
  • Remove all attached listeners from TextTrackList and the player.
  • Clear the handler references (delete this.updateHandler_, delete this.disposeHandler_).
  • Call super.dispose() afterwards.

Requirements Checklist

• Feature implemented / Bug fixed
• Tests are passing (manual verification, no regressions observed)
• Documentation updated (inline JSDoc for dispose())

Note: Currently using delete this.updateHandler_ for cleanup. If maintainers prefer = null for consistency/performance, I can update accordingly.

[welcome]

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

• Run npm run lint -- --errors locally to catch formatting errors earlier.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[Essk]

Disclosure: This review was drafted with AI assistance (Claude Code) to help trace the dispose lifecycle and identify edge cases.

Thanks for this PR @nochev — the leak you identified is real and we've been wanting this fixed. I've spent some time testing and reviewing the approach and found a few issues that need addressing before this can merge. Happy to help work through them.

1. Double-dispose crash (this is what's failing CI)

Registering this.dispose as the player's 'dispose' handler causes TrackButton.dispose() to run twice during player teardown — once via the event, and again via the normal child disposal chain. The isDisposed_ guard in Component.dispose() would catch this, but your dispose() override does work before calling super.dispose(), so on the second call this.player_ is already null:

TypeError: Cannot read properties of null (reading 'off')
    at AudioTrackButton.dispose
    at ControlBar.dispose
    at Player.dispose

The sequence is:

1. Player.dispose() → Component.dispose() fires 'dispose' event
2. disposeHandler fires → TrackButton.dispose() → super.dispose() → sets this.player_ = null
3. Player continues disposing children → ControlBar → AudioTrackButton
4. TrackButton.dispose() runs again → this.player_.off(...) → 💥

Fix: Use a separate cleanup method instead of wiring the full dispose() to the player's 'dispose' event.

2. Events.off(elem, type, null) removes ALL listeners

This is subtle. In video.js, calling removeEventListener(type, null) (or any falsy fn) hits this path in utils/events.js:

// If no listener was provided, remove all listeners for type
if (!fn) {
    removeType(elem, type);
    return;
}

If you null out the handler references before the second cleanup call (which happens via the child disposal chain as described above), tracks.removeEventListener('removetrack', null) nukes every removetrack listener on the emulated TextTrackList — including the native-to-emulated proxy set up in Html5.proxyNativeTracksForType_(), and any listeners from other consumers. Since player.textTracks() delegates to tech.textTracks() (same object), this affects the entire track event pipeline.

Fix: Guard removeEventListener calls so they only run when the handler reference is still set.

3. Missing null guard on tracks in dispose (minor)

The constructor early-returns when !tracks, meaning updateHandler_ / disposeHandler_ are never created. But dispose() unconditionally accesses this.options_.tracks. In practice textTracks() / audioTracks() always return a TrackList, but for defensive consistency the guard should match the constructor.

Suggested implementation

Here's an approach that addresses all three issues — tested locally with full Safari and Chrome passes:

constructor(player, options) {
    const tracks = options.tracks;

    super(player, options);

    if (this.items.length <= 1) {
      this.hide();
    }

    if (!tracks) {
      return;
    }

    this.updateHandler_ = Fn.bind_(this, this.update);
    this.cleanupHandler_ = Fn.bind_(this, this.cleanupTrackListeners_);

    tracks.addEventListener('removetrack', this.updateHandler_);
    tracks.addEventListener('addtrack', this.updateHandler_);
    tracks.addEventListener('labelchange', this.updateHandler_);
    this.player_.on('ready', this.updateHandler_);
    this.player_.on('dispose', this.cleanupHandler_);
  }

  /**
   * Remove track list event listeners.
   *
   * @private
   */
  cleanupTrackListeners_() {
    const tracks = this.options_.tracks;

    if (tracks && this.updateHandler_) {
      tracks.removeEventListener('removetrack', this.updateHandler_);
      tracks.removeEventListener('addtrack', this.updateHandler_);
      tracks.removeEventListener('labelchange', this.updateHandler_);
    }

    if (this.player_) {
      if (this.updateHandler_) {
        this.player_.off('ready', this.updateHandler_);
      }
      if (this.cleanupHandler_) {
        this.player_.off('dispose', this.cleanupHandler_);
      }
    }

    this.updateHandler_ = null;
    this.cleanupHandler_ = null;
  }

  /**
   * Dispose of the Component and remove all event listeners.
   *
   * @override
   */
  dispose() {
    this.cleanupTrackListeners_();
    super.dispose();
  }

The key ideas:

• Separate cleanup method — the player's 'dispose' handler only cleans up listeners, it doesn't trigger a full component disposal
• Null guards on handler references — prevents the Events.off nuke-all-listeners footgun
• = null instead of delete — matches the convention in Component.dispose()

Happy to answer any questions on any of this. The original diagnosis of the leak was spot on — these are just implementation details to make it safe in video.js's dispose lifecycle.