Destroy player and listeners on web unmount#836
Conversation
Code ReviewSolid fix — all four leak vectors identified in the description are addressed correctly. Some observations: What looks good
Minor observations
None of these block merging. The fix correctly addresses the four retention paths described in the PR, and the approach is clean. |
|
For the three minor points: 1 & 3 — Agree these are just GC hygiene. Since the manager and the 2 — Good point. I added a comment to make the multi-player edge case A proper identity guard is not easy here because the closure cannot be |
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:
ChromelessPlayeror nulled either ref.adapter.destroy()was called, butplayer.currentandadapter.currentremained populated.window.playerandwindow.nativePlayerwere assigned on mount and never cleared on unmount, pinning the last-rendered adapter and player globally for the lifetime of the page.WebPresentationModeManagerregistered three listeners — two ondocument(fullscreenchange,fullscreenerror) and one onplayer.presentation(presentationmodechange) — with no matching teardown. The manager (and its_playerfield) was kept alive bydocument's listener table.THEOplayerWebAdapter.destroy()never clearedwindow.__onGCastApiAvailable. When the Cast Sender SDK is loaded (viachromeless.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 thewindow.player/window.nativePlayerglobals — 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 wherewindow.nativePlayer = playerassigned the React ref wrapper rather than the underlyingChromelessPlayer.WebPresentationModeManager.ts— adddestroy()that removes all three listeners. Uses the stable arrow-boundupdatePresentationModereference so removal still matches the original function under monkey-patchedaddEventListenerwrappers (e.g. Sentry's, which keys by the original function).THEOplayerWebAdapter.ts— invoke_presentationModeManager.destroy()from the adapter'sdestroy(), ordered before_player.destroy()so the listener on_player.presentationis removed while the player is still live. Also clearswindow.__onGCastApiAvailable.Behaviour change note
The debug globals
window.player/window.nativePlayerare 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 seeundefined.