Skip to content

fix: Patch Memory Leaks#218

Merged
R-Delfino95 merged 7 commits intomuxinc:mainfrom
R-Delfino95:fix/memory-leaks-main-packages
Mar 19, 2026
Merged

fix: Patch Memory Leaks#218
R-Delfino95 merged 7 commits intomuxinc:mainfrom
R-Delfino95:fix/memory-leaks-main-packages

Conversation

@R-Delfino95
Copy link
Copy Markdown
Contributor

@R-Delfino95 R-Delfino95 commented Mar 13, 2026

Description

This PR focuses on lifecycle management and breaking reference cycles that prevent garbage collection in mux-player-react.

Changes by Package:

castable-video

  • Teardown Logic: Added disconnectedCallback and a destroy() method to properly unmount the element and its properties during casting.
  • Remote Getter Guard: Added a !this.isConnected check to the remote getter. This prevents the accidental creation of a new RemotePlayback instance during the disconnection process (e.g., when other mixins call super.disconnectedCallback() and access properties that hit this getter).

custom-media-element

  • Listener Management: Missing removeEventListener calls were added.
  • Lifecycle Refactor: Listeners are now removed in disconnectedCallback and re-created in connectedCallback (if already initialized), skipping a full init().
  • State Preservation: #isInit remains true during disconnect. This prevents other mixins in the class hierarchy from inadvertently re-triggering #init() via property access (like this.nativeEl or this.currentTime) while the component is being torn down.
  • V8/esbuild Optimization: Addressed an issue where esbuild compiles private fields into WeakMap entries. If a closure captures this, it can create an ephemeron cycle that V8 cannot resolve. These fields are now nulled in disconnectedCallback to break the cycle.

media-tracks

  • Weak References: Replaced strong media references with WeakRef(media) in track lists, rendition lists, and mixin init functions. Access sites now use media?.deref() with optional chaining.
  • Named Listeners: Extracted anonymous native track event listeners (change, addtrack, removetrack) into named references to ensure they are properly removed during cleanup.

Related Issues

Relates to Issue #1235


Testing Instructions

To verify these changes and the overall fix for memory leaks, please refer to the manual testing tool and the integration example provided in:

👉 PR #1285 - Manual Testing Tool & Examples

The tool allows for local testing using Yalc to link media-elements and media-chrome without relying on NPM, and includes a script to restore the environment once finished.


Note

Medium Risk
Touches custom element lifecycle and Cast remote playback teardown; mistakes could cause missed events or regress casting/track sync. Changes are mostly cleanup/guard logic but span multiple core media abstractions.

Overview
Fixes several lifecycle-related memory leaks across media components by ensuring teardown happens on disconnect and by breaking strong reference cycles.

In castable-video, the remote getter now avoids instantiating RemotePlayback while disconnected, and disconnectedCallback() explicitly destroys the remote instance and clears stored privateProps. RemotePlayback gains a destroy() method to remove text track and cast controller listeners and clear cast element refs when the media element is removed mid-cast.

In custom-media-element, listener setup is refactored into #setupListeners() with a tracked slotchange handler; disconnectedCallback() now removes all shadowRoot listeners, disconnects observers, removes cloned children, and nulls #nativeEl to allow GC, while connectedCallback() re-attaches listeners on remount.

In media-tracks, track/rendition objects now store media as WeakRef and all access sites use media?.deref() with null-guards, preventing track lists from keeping media elements alive after removal.

Written by Cursor Bugbot for commit e7a26e7. This will update automatically on new commits. Configure here.

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 13, 2026

@R-Delfino95 is attempting to deploy a commit to the Mux Team on Vercel.

A member of the Team first needs to authorize it.

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 13, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@R-Delfino95 R-Delfino95 marked this pull request as ready for review March 13, 2026 19:39
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.


nativeTracks.addEventListener('change', onChange);
nativeTracks.addEventListener('addtrack', onAddTrack);
nativeTracks.addEventListener('removetrack', onRemoveTrack);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Native track listeners stored locally, never removable

Medium Severity

The onChange, onAddTrack, and onRemoveTrack listeners are extracted into named local variables but never stored anywhere accessible for later removal. These references are lost when getVideoTracks/getAudioTracks returns, so the listeners on nativeTracks can never be cleaned up. Since these closures capture media (a strong reference to the custom element), they keep the media element alive as long as the native track list exists — undermining the WeakRef work done elsewhere in this PR.

Additional Locations (1)
Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This case is for Safari only:

These listeners (onChange, onAddTrack, onRemoveTrack) are anonymous and are attached to nativeTracks, which is the track list of the native

Therefore, this isn't a leak because the listener target (nativeTracks) shares the same lifecycle as the element itself. This differs from listeners on globalThis or the custom element itself, which do require explicit cleanup

Copy link
Copy Markdown
Collaborator

@luwes luwes left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@R-Delfino95 R-Delfino95 merged commit 4708538 into muxinc:main Mar 19, 2026
5 of 6 checks passed
@R-Delfino95 R-Delfino95 deleted the fix/memory-leaks-main-packages branch March 19, 2026 14:50
@github-actions github-actions bot mentioned this pull request Mar 19, 2026
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.

3 participants