Skip to content

Destroy player and listeners on web unmount#836

Open
mikolajadamowicz wants to merge 2 commits into
THEOplayer:developfrom
mikolajadamowicz:bugfix/web-player-memory-leak
Open

Destroy player and listeners on web unmount#836
mikolajadamowicz wants to merge 2 commits into
THEOplayer:developfrom
mikolajadamowicz:bugfix/web-player-memory-leak

Conversation

@mikolajadamowicz

Copy link
Copy Markdown

Problem

On web, mounting and unmounting <THEOplayerView> leaks the full player graph (ChromelessPlayer, THEOplayerWebAdapter, its sub-managers, the underlying video element + buffers, registered listeners). Repeated mount/unmount cycles (i've tested 10 mounts and unmounts) accumulate one detached player per cycle. On memory-constrained environments (webOS TVs, low-end web) this leads to RAM exhaustion.

Four independent retainers were keeping the graph alive after unmount:

  1. The component's cleanup never destroyed the raw ChromelessPlayer or nulled either ref. adapter.destroy() was called, but player.current and adapter.current remained populated.
  2. window.player and window.nativePlayer were assigned on mount and never cleared on unmount, pinning the last-rendered adapter and player globally for the lifetime of the page.
  3. WebPresentationModeManager registered three listeners — two on document (fullscreenchange, fullscreenerror) and one on player.presentation (presentationmodechange) — with no matching teardown. The manager (and its _player field) was kept alive by document's listener table.
  4. THEOplayerWebAdapter.destroy() never cleared window.__onGCastApiAvailable. When the Cast Sender SDK is loaded (via chromeless.js), this global is assigned a closure that captures the player; until cleared, the closure retains the player.

Fix

  • THEOplayerView.web.tsx — rework the effect cleanup to destroy the adapter (which internally destroys the player), null both refs, and clear the window.player / window.nativePlayer globals — but only if they still point to this component's instances (identity-guarded, so a second mounted player isn't nulled by the first one's cleanup). Also fixes a pre-existing bug where window.nativePlayer = player assigned the React ref wrapper rather than the underlying ChromelessPlayer.
  • WebPresentationModeManager.ts — add destroy() that removes all three listeners. Uses the stable arrow-bound updatePresentationMode reference so removal still matches the original function under monkey-patched addEventListener wrappers (e.g. Sentry's, which keys by the original function).
  • THEOplayerWebAdapter.ts — invoke _presentationModeManager.destroy() from the adapter's destroy(), ordered before _player.destroy() so the listener on _player.presentation is removed while the player is still live. Also clears window.__onGCastApiAvailable.

Behaviour change note

The debug globals window.player / window.nativePlayer are still assigned on mount as before, but now cleared on unmount (with the identity guard). Any consumer code that reads these globals after the player screen unmounts will now see undefined.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Code Review

Solid fix — all four leak vectors identified in the description are addressed correctly. Some observations:

What looks good

  • Identity-guarded global cleanup in THEOplayerView.web.tsxwindow.player === adapterRef prevents one component's cleanup from clobbering another mounted instance's globals.
  • Correct window.nativePlayer fix — was previously assigning the React ref wrapper (player) instead of the underlying ChromelessPlayer (player.current). Good catch.
  • Destroy ordering in THEOplayerWebAdapter.destroy()_presentationModeManager.destroy() before _player.destroy() so the player.presentation listener is removed while the player is still alive.
  • updatePresentationMode is an arrow property (stable reference), so removeEventListener will correctly match the originally registered listener, even under monkey-patched addEventListener wrappers.
  • Defensive else if (playerRef) branch — handles the edge case where ChromelessPlayer construction succeeded but THEOplayerWebAdapter construction threw.

Minor observations

  1. _presentationModeManager not nulled after destroy: _eventForwarder, _mediaSession, _cmcdConnector, and _player are all set to undefined after their respective teardown, but _presentationModeManager is not. This is not a bug (it's readonly and removeEventListener is idempotent), but for consistency and to release the reference for GC, you could consider adding this._presentationModeManager = undefined (would require dropping readonly or using a ! assertion).

  2. __onGCastApiAvailable cleanup is unconditional: The condition checks window.__onGCastApiAvailable is truthy, so if the Cast SDK was never loaded, this is a no-op — which is fine. But if a different player instance set this global (e.g. two players on the same page), this destroy will clear the other player's callback. An identity guard like the one used for window.player isn't straightforward here since the closure isn't easily compared, but worth noting for multi-player scenarios.

  3. WebPresentationModeManager.destroy() doesn't clear its own _player / _eventForwarder references: Similar to point 1 — the manager retains references to the player and event forwarder after destroy. Since the adapter owns the manager and both go out of scope together, this is a cosmetic concern, not a leak.

None of these block merging. The fix correctly addresses the four retention paths described in the PR, and the approach is clean.

@tvanlaerhoven tvanlaerhoven self-requested a review June 10, 2026 11:56
@mikolajadamowicz

Copy link
Copy Markdown
Author

For the three minor points:

1 & 3 — Agree these are just GC hygiene. Since the manager and the
player share the same lifetime as the adapter, they all become
unreachable together when the adapter is dropped, so there is no real
leak here. I left them as is to avoid dropping readonly only to release
a reference that is already going out of scope.

2 — Good point. I added a comment to make the multi-player edge case
explicit:

  // We clear this global always. If there are two players on the same 
  page,
  // this can also clear the callback of the other player. This is fine,
  // because using Cast with multiple players on web is not supported.

A proper identity guard is not easy here because the closure cannot be
compared, and Cast with multiple players on the same page is not a
supported setup, so I think this is acceptable for now.

@tvanlaerhoven tvanlaerhoven added the enhancement New feature or request label Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants