Repository navigation
fix(youtube-video-element): destroy the player on disconnect to fix memory leak #256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,6 +147,9 @@ class YoutubeVideoElement extends MediaPlayedRangesMixin(globalThis.HTMLElement | |
| loadComplete = new PublicPromise(); | ||
| #loadRequested; | ||
| #hasLoaded; | ||
| #wasDisconnected = false; | ||
| #loadId = 0; | ||
| #timers = []; | ||
| #readyState = 0; | ||
| #seeking = false; | ||
| #seekComplete; | ||
|
|
@@ -172,6 +175,12 @@ class YoutubeVideoElement extends MediaPlayedRangesMixin(globalThis.HTMLElement | |
| async load() { | ||
| if (this.#loadRequested) return; | ||
|
|
||
| // Identifies this load attempt. `load()` awaits the API script, so the | ||
| // element can be disconnected (or asked to load again) while this one is | ||
| // still in flight; those bump #loadId and this attempt bails out below | ||
| // instead of attaching a player nothing will ever destroy. | ||
| const loadId = ++this.#loadId; | ||
|
|
||
| if (!this.shadowRoot) { | ||
| this.attachShadow({ mode: 'open' }); | ||
| } | ||
|
|
@@ -193,12 +202,13 @@ class YoutubeVideoElement extends MediaPlayedRangesMixin(globalThis.HTMLElement | |
|
|
||
| let oldApi = this.api; | ||
| this.api = null; | ||
| // A new player is always constructed below, so the previous one is never | ||
| // reused. Release it rather than just dropping the reference, otherwise it | ||
| // stays in the API's registries holding its (now discarded) iframe. | ||
| // Removes the <iframe> containing the player. | ||
| oldApi?.destroy(); | ||
|
|
||
| if (!this.src) { | ||
| // Removes the <iframe> containing the player. | ||
| oldApi?.destroy(); | ||
| return; | ||
| } | ||
| if (!this.src) return; | ||
|
|
||
| this.#textTracksVideo = document.createElement('video'); | ||
| this.textTracks = this.#textTracksVideo.textTracks; | ||
|
|
@@ -223,6 +233,10 @@ class YoutubeVideoElement extends MediaPlayedRangesMixin(globalThis.HTMLElement | |
| } | ||
|
|
||
| const YT = await loadScript(API_URL, API_GLOBAL, API_GLOBAL_READY); | ||
|
|
||
| // Superseded by a disconnect or a newer load() while awaiting the API. | ||
| if (loadId !== this.#loadId) return; | ||
|
|
||
|
cursor[bot] marked this conversation as resolved.
Resolved
|
||
| this.api = new YT.Player(iframe, { | ||
| events: { | ||
| onReady: () => { | ||
|
|
@@ -328,8 +342,14 @@ class YoutubeVideoElement extends MediaPlayedRangesMixin(globalThis.HTMLElement | |
|
|
||
| await this.loadComplete; | ||
|
|
||
| // Superseded while awaiting loadComplete; don't start pollers for a player | ||
| // that is already gone. | ||
| if (loadId !== this.#loadId) return; | ||
|
|
||
| this.#clearTimers(); | ||
|
|
||
| let lastCurrentTime = 0; | ||
| setInterval(() => { | ||
| this.#timers.push(setInterval(() => { | ||
| const diff = Math.abs(this.currentTime - lastCurrentTime); | ||
| const bufferedEnd = this.buffered.end(this.buffered.length - 1); | ||
| if (this.seeking && bufferedEnd > 0.1) { | ||
|
|
@@ -341,7 +361,7 @@ class YoutubeVideoElement extends MediaPlayedRangesMixin(globalThis.HTMLElement | |
| this.dispatchEvent(new Event('seeking')); | ||
| } | ||
| lastCurrentTime = this.currentTime; | ||
| }, 50); | ||
| }, 50)); | ||
|
|
||
| let lastBufferedEnd; | ||
| const progressInterval = setInterval(() => { | ||
|
|
@@ -355,6 +375,40 @@ class YoutubeVideoElement extends MediaPlayedRangesMixin(globalThis.HTMLElement | |
| this.dispatchEvent(new Event('progress')); | ||
| } | ||
| }, 100); | ||
| this.#timers.push(progressInterval); | ||
| } | ||
|
|
||
| #clearTimers() { | ||
| for (const id of this.#timers) clearInterval(id); | ||
| this.#timers = []; | ||
| } | ||
|
|
||
| connectedCallback() { | ||
|
cursor[bot] marked this conversation as resolved.
Resolved
|
||
| // `load()` is only triggered by attributeChangedCallback, so an element that | ||
| // is moved in the DOM (disconnected and reconnected without its attributes | ||
| // changing) has to ask for a new player itself. | ||
| if (this.#wasDisconnected) { | ||
| this.#wasDisconnected = false; | ||
| this.load(); | ||
| } | ||
| super.connectedCallback?.(); | ||
| } | ||
|
|
||
| disconnectedCallback() { | ||
| this.#wasDisconnected = true; | ||
| this.#loadId++; | ||
| this.#clearTimers(); | ||
| this.#loadRequested = null; | ||
| this.#hasLoaded = null; | ||
| this.isLoaded = false; | ||
| this.loadComplete = new PublicPromise(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If disconnect happens between building the player and Since this.loadComplete.reject(new Error('disconnected before load completed')); Same gap exists in vimeo-video-element.js, so it is not new here, but worth closing now since this PR is the one making reconnect a supported, expected flow.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, fixed in db5a5e3. Two changes from the one-liner, both checked locally:
The reject by itself causes unhandled rejections. Three things are listening on
One related spot, from before this PR: |
||
| // The YouTube iframe API holds a reference to every player it creates, so | ||
| // dropping the element is not enough to release the <iframe>: it stays | ||
| // detached but reachable from window.YT. destroy() removes the iframe and | ||
| // deregisters the player. | ||
| this.api?.destroy?.(); | ||
| this.api = null; | ||
| super.disconnectedCallback?.(); | ||
| } | ||
|
|
||
| async attributeChangedCallback(attrName, oldValue, newValue) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test creates the element, calls
remove()right away, and assertsvideo.api === nullimmediately after. That assertion is true by construction,disconnectedCallbacksetsthis.api = nullunconditionally, regardless of whether the#loadIdguard actually did its job.It does not currently prove the thing it is meant to prove, that the in flight continuation (
await loadScript(...)orawait this.loadComplete) does not go on to build a second player after the element already disconnected.To actually cover that race, the test needs to let that continuation resume after
remove(), for example by awaiting a tick or intercepting theYT.Playerconstruction, and then assertvideo.apiis stillnullonce it does. As written this passes even if the#loadIdguard were removed entirely.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and that test was mine.
api === nullright afterremove()is guaranteed bydisconnectedCallback, andt.ok(true, …)doesn't check anything.Rewritten in db5a5e3 as
disconnecting mid-load does not build a player afterwards. It waits forYT.Playerto exist, gives the pendingload()100ms to run, and only then assertsapiis stillnull. To check it actually proves something, I deleted the#loadIdguard: the old test still passes, and the new one fails.I also added
disconnecting before ready settles the pending loadCompletefor the other fix. It waits for the player to be constructed, disconnects, and then racesloadCompleteagainst a 3s timeout. It fails on 3992fa2 (the promise stays pending) and passes with the fix, where it rejects withAbortError. It only checks that the promise doesn't hang, not that it rejects, so it won't flake ifonReadyhappens to fire before the disconnect.