From e6c3b226ba25cd62b288c75da895531ed27087c0 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Tue, 17 May 2022 08:35:39 -0700 Subject: [PATCH] fix: Fix event listener leaks in Player (#4229) Event listeners were being leaked in Player across load() / unload() cycles. This was fundamentally caused by the difficulty in keeping track of which event listeners to clean up at which stages of the load graph. Everything is cleaned up by Player.destroy() and EventManager.release(), but for a Player with heavy re-use, there was still a small leak. This fixes the leak by splitting the EventManager instance into three instances, each of which is cleaned up in a different part of the load graph life cycle. Listeners which should only live as long as a piece of content is loaded go into loadEventManager_. Listeners which should only live as long as we are attached to the video element go into attachEventManager_. Listeners which should live as long as the Player instance itself go into globalEventManager_. It is now impossible to miss unlistening to a particular event, since we no longer have to unlisten to any individual events at all. The removeAll() method of each event manager will clean up all listeners at the appropriate time. --- lib/player.js | 256 +++++++++++++++++++++++++------------------------- 1 file changed, 129 insertions(+), 127 deletions(-) diff --git a/lib/player.js b/lib/player.js index 81c97c5e59..f82683e3fa 100644 --- a/lib/player.js +++ b/lib/player.js @@ -423,8 +423,23 @@ shaka.Player = class extends shaka.util.FakeEventTarget { */ this.isTextVisible_ = false; - /** @private {shaka.util.EventManager} */ - this.eventManager_ = new shaka.util.EventManager(); + /** + * For listeners scoped to the lifetime of the Player instance. + * @private {shaka.util.EventManager} + */ + this.globalEventManager_ = new shaka.util.EventManager(); + + /** + * For listeners scoped to the lifetime of the media element attachment. + * @private {shaka.util.EventManager} + */ + this.attachEventManager_ = new shaka.util.EventManager(); + + /** + * For listeners scoped to the lifetime of the loaded content. + * @private {shaka.util.EventManager} + */ + this.loadEventManager_ = new shaka.util.EventManager(); /** @private {shaka.net.NetworkingEngine} */ this.networkingEngine_ = null; @@ -564,7 +579,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // If the browser comes back online after being offline, then try to play // again. - this.eventManager_.listen(window, 'online', () => { + this.globalEventManager_.listen(window, 'online', () => { this.retryStreaming(); }); @@ -752,10 +767,18 @@ shaka.Player = class extends shaka.util.FakeEventTarget { }); await this.walker_.destroy(); - // Tear-down the event manager to ensure messages stop moving around. - if (this.eventManager_) { - this.eventManager_.release(); - this.eventManager_ = null; + // Tear-down the event managers to ensure handlers stop firing. + if (this.globalEventManager_) { + this.globalEventManager_.release(); + this.globalEventManager_ = null; + } + if (this.attachEventManager_) { + this.attachEventManager_.release(); + this.attachEventManager_ = null; + } + if (this.loadEventManager_) { + this.loadEventManager_.release(); + this.loadEventManager_ = null; } this.abrManagerFactory_ = null; @@ -1298,7 +1321,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { has.mediaElement = wants.mediaElement; const onError = (error) => this.onVideoError_(error); - this.eventManager_.listen(has.mediaElement, 'error', onError); + this.attachEventManager_.listen(has.mediaElement, 'error', onError); } this.video_ = has.mediaElement; @@ -1325,10 +1348,10 @@ shaka.Player = class extends shaka.util.FakeEventTarget { * @private */ onDetach_(has, wants) { - // If we are going from "detached" to "detached" we wouldn't have + // If we were going from "detached" to "detached" we wouldn't have // a media element to detach from. if (has.mediaElement) { - this.eventManager_.unlisten(has.mediaElement, 'error'); + this.attachEventManager_.removeAll(); has.mediaElement = null; } @@ -1386,11 +1409,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // In most cases we should have a media element. The one exception would // be if there was an error and we, by chance, did not have a media element. if (has.mediaElement) { - this.eventManager_.unlisten(has.mediaElement, 'loadedmetadata'); - this.eventManager_.unlisten(has.mediaElement, 'playing'); - this.eventManager_.unlisten(has.mediaElement, 'pause'); - this.eventManager_.unlisten(has.mediaElement, 'ended'); - this.eventManager_.unlisten(has.mediaElement, 'ratechange'); + this.loadEventManager_.removeAll(); } // Some observers use some playback components, shutting down the observers @@ -1885,18 +1904,18 @@ shaka.Player = class extends shaka.util.FakeEventTarget { this.assetUri_ = assetUri; this.playRateController_ = new shaka.media.PlayRateController({ - getRate: () => has.mediaElement.playbackRate, - getDefaultRate: () => has.mediaElement.defaultPlaybackRate, - setRate: (rate) => { has.mediaElement.playbackRate = rate; }, - movePlayhead: (delta) => { has.mediaElement.currentTime += delta; }, + getRate: () => mediaElement.playbackRate, + getDefaultRate: () => mediaElement.defaultPlaybackRate, + setRate: (rate) => { mediaElement.playbackRate = rate; }, + movePlayhead: (delta) => { mediaElement.currentTime += delta; }, }); const updateStateHistory = () => this.updateStateHistory_(); const onRateChange = () => this.onRateChange_(); - this.eventManager_.listen(mediaElement, 'playing', updateStateHistory); - this.eventManager_.listen(mediaElement, 'pause', updateStateHistory); - this.eventManager_.listen(mediaElement, 'ended', updateStateHistory); - this.eventManager_.listen(mediaElement, 'ratechange', onRateChange); + this.loadEventManager_.listen(mediaElement, 'playing', updateStateHistory); + this.loadEventManager_.listen(mediaElement, 'pause', updateStateHistory); + this.loadEventManager_.listen(mediaElement, 'ended', updateStateHistory); + this.loadEventManager_.listen(mediaElement, 'ratechange', onRateChange); const abrFactory = this.config_.abrFactory; if (!this.abrManager_ || this.abrManagerFactory_ != abrFactory) { @@ -1953,33 +1972,22 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // "streaming" so that they can access internal information. this.loadMode_ = shaka.Player.LoadMode.MEDIA_SOURCE; - // This flag is used below in track management to check if this load was - // canceled before the necessary events fired. - let unloaded = false; - this.cleanupOnUnload_.push(() => { - unloaded = true; - }); - - if (this.video_.textTracks) { - this.eventManager_.listen(this.video_.textTracks, 'addtrack', (e) => { - // If we have moved on to another piece of content while waiting for - // the above event, we should not process tracks here. - if (unloaded) { - return; - } - - const trackEvent = /** @type {!TrackEvent} */(e); - if (trackEvent.track) { - const track = trackEvent.track; - goog.asserts.assert(track instanceof TextTrack, 'Wrong track type!'); - - switch (track.kind) { - case 'chapters': - this.activateChaptersTrack_(track); - break; - } - } - }); + if (mediaElement.textTracks) { + this.loadEventManager_.listen( + mediaElement.textTracks, 'addtrack', (e) => { + const trackEvent = /** @type {!TrackEvent} */(e); + if (trackEvent.track) { + const track = trackEvent.track; + goog.asserts.assert( + track instanceof TextTrack, 'Wrong track type!'); + + switch (track.kind) { + case 'chapters': + this.activateChaptersTrack_(track); + break; + } + } + }); } // The event must be fired after we filter by restrictions but before the @@ -2068,7 +2076,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { } // Wait for the 'loadedmetadata' event to measure load() latency. - this.eventManager_.listenOnce(mediaElement, 'loadedmetadata', () => { + this.loadEventManager_.listenOnce(mediaElement, 'loadedmetadata', () => { const now = Date.now() / 1000; const delta = now - wants.startTimeOfLoad; this.stats_.setLoadLatency(delta); @@ -2221,11 +2229,12 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // Save the uri so that it can be used outside of the load-graph. this.assetUri_ = has.uri; - this.playhead_ = new shaka.media.SrcEqualsPlayhead(has.mediaElement); + const mediaElement = has.mediaElement; + + this.playhead_ = new shaka.media.SrcEqualsPlayhead(mediaElement); - // This flag is used below in the language preference setup and - // track-management to check if this load was canceled before the necessary - // events fired. + // This flag is used below in the language preference setup to check if + // this load was canceled before the necessary awaits completed. let unloaded = false; this.cleanupOnUnload_.push(() => { unloaded = true; @@ -2236,10 +2245,10 @@ shaka.Player = class extends shaka.util.FakeEventTarget { } this.playRateController_ = new shaka.media.PlayRateController({ - getRate: () => has.mediaElement.playbackRate, - getDefaultRate: () => has.mediaElement.defaultPlaybackRate, - setRate: (rate) => { has.mediaElement.playbackRate = rate; }, - movePlayhead: (delta) => { has.mediaElement.currentTime += delta; }, + getRate: () => mediaElement.playbackRate, + getDefaultRate: () => mediaElement.defaultPlaybackRate, + setRate: (rate) => { mediaElement.playbackRate = rate; }, + movePlayhead: (delta) => { mediaElement.currentTime += delta; }, }); // We need to start the buffer management code near the end because it will @@ -2251,17 +2260,17 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // Add all media element listeners. const updateStateHistory = () => this.updateStateHistory_(); const onRateChange = () => this.onRateChange_(); - this.eventManager_.listen(has.mediaElement, 'playing', updateStateHistory); - this.eventManager_.listen(has.mediaElement, 'pause', updateStateHistory); - this.eventManager_.listen(has.mediaElement, 'ended', updateStateHistory); - this.eventManager_.listen(has.mediaElement, 'ratechange', onRateChange); + this.loadEventManager_.listen(mediaElement, 'playing', updateStateHistory); + this.loadEventManager_.listen(mediaElement, 'pause', updateStateHistory); + this.loadEventManager_.listen(mediaElement, 'ended', updateStateHistory); + this.loadEventManager_.listen(mediaElement, 'ratechange', onRateChange); // Wait for the 'loadedmetadata' event to measure load() latency, but only // if preload is set in a way that would result in this event firing // automatically. // See https://github.com/shaka-project/shaka-player/issues/2483 - if (this.video_.preload != 'none') { - this.eventManager_.listenOnce(this.video_, 'loadedmetadata', () => { + if (mediaElement.preload != 'none') { + this.loadEventManager_.listenOnce(mediaElement, 'loadedmetadata', () => { const now = Date.now() / 1000; const delta = now - wants.startTimeOfLoad; this.stats_.setLoadLatency(delta); @@ -2271,49 +2280,47 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // The audio tracks are only available on Safari at the moment, but this // drives the tracks API for Safari's native HLS. So when they change, // fire the corresponding Shaka Player event. - if (this.video_.audioTracks) { - this.eventManager_.listen( - this.video_.audioTracks, 'addtrack', () => this.onTracksChanged_()); - this.eventManager_.listen( - this.video_.audioTracks, 'removetrack', + if (mediaElement.audioTracks) { + this.loadEventManager_.listen( + mediaElement.audioTracks, 'addtrack', () => this.onTracksChanged_()); + this.loadEventManager_.listen( + mediaElement.audioTracks, 'removetrack', () => this.onTracksChanged_()); - this.eventManager_.listen( - this.video_.audioTracks, 'change', () => this.onTracksChanged_()); - } - - if (this.video_.textTracks) { - this.eventManager_.listen(this.video_.textTracks, 'addtrack', (e) => { - // If we have moved on to another piece of content while waiting for - // the above event, we should not process tracks here. - if (unloaded) { - return; - } - - const trackEvent = /** @type {!TrackEvent} */(e); - if (trackEvent.track) { - const track = trackEvent.track; - goog.asserts.assert(track instanceof TextTrack, 'Wrong track type!'); - - switch (track.kind) { - case 'metadata': - this.processTimedMetadataSrcEqls_(track); - break; - - case 'chapters': - this.activateChaptersTrack_(track); - break; - - default: - this.onTracksChanged_(); - break; - } - } - }); + this.loadEventManager_.listen( + mediaElement.audioTracks, 'change', () => this.onTracksChanged_()); + } + + if (mediaElement.textTracks) { + this.loadEventManager_.listen( + mediaElement.textTracks, 'addtrack', (e) => { + const trackEvent = /** @type {!TrackEvent} */(e); + if (trackEvent.track) { + const track = trackEvent.track; + goog.asserts.assert( + track instanceof TextTrack, 'Wrong track type!'); + + switch (track.kind) { + case 'metadata': + this.processTimedMetadataSrcEqls_(track); + break; + + case 'chapters': + this.activateChaptersTrack_(track); + break; + + default: + this.onTracksChanged_(); + break; + } + } + }); - this.eventManager_.listen( - this.video_.textTracks, 'removetrack', () => this.onTracksChanged_()); - this.eventManager_.listen( - this.video_.textTracks, 'change', () => this.onTracksChanged_()); + this.loadEventManager_.listen( + mediaElement.textTracks, 'removetrack', + () => this.onTracksChanged_()); + this.loadEventManager_.listen( + mediaElement.textTracks, 'change', + () => this.onTracksChanged_()); } const extension = shaka.media.ManifestParser.getExtension(has.uri); @@ -2322,14 +2329,14 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // By setting |src| we are done "loading" with src=. We don't need to set // the current time because |playhead| will do that for us. - has.mediaElement.src = this.cmcdManager_.appendSrcData(has.uri, mimeType); + mediaElement.src = this.cmcdManager_.appendSrcData(has.uri, mimeType); // Tizen 3 / WebOS won't load anything unless you call load() explicitly, // no matter the value of the preload attribute. This is harmful on some // other platforms by triggering unbounded loading of media data, but is // necessary here. if (shaka.util.Platform.isTizen() || shaka.util.Platform.isWebOS()) { - has.mediaElement.load(); + mediaElement.load(); } // Set the load mode last so that we know that all our components are @@ -2345,24 +2352,18 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // wait for the full data, that won't happen on Safari until the play button // is hit. const fullyLoaded = new shaka.util.PublicPromise(); - shaka.util.MediaReadyState.waitForReadyState(this.video_, + shaka.util.MediaReadyState.waitForReadyState(mediaElement, HTMLMediaElement.HAVE_METADATA, - this.eventManager_, + this.loadEventManager_, () => { fullyLoaded.resolve(); }); // We can't switch to preferred languages, though, until the data is loaded. - shaka.util.MediaReadyState.waitForReadyState(this.video_, + shaka.util.MediaReadyState.waitForReadyState(mediaElement, HTMLMediaElement.HAVE_CURRENT_DATA, - this.eventManager_, + this.loadEventManager_, async () => { - // If we have moved on to another piece of content while waiting for - // the above event, we should not change tracks here. - if (unloaded) { - return; - } - this.setupPreferredAudioOnSrc_(); // Applying the text preference too soon can result in it being @@ -2370,8 +2371,9 @@ shaka.Player = class extends shaka.util.FakeEventTarget { const textTracks = this.getFilteredTextTracks_(); if (!textTracks.find((t) => t.mode != 'disabled')) { await new Promise((resolve) => { - this.eventManager_.listenOnce( - this.video_.textTracks, 'change', resolve); + this.loadEventManager_.listenOnce( + mediaElement.textTracks, 'change', resolve); + // We expect the event to fire because it does on Safari. // But in case it doesn't on some other platform or future // version, move on in 1 second no matter what. This keeps the @@ -2390,10 +2392,10 @@ shaka.Player = class extends shaka.util.FakeEventTarget { this.setupPreferredTextOnSrc_(); }); - if (this.video_.error) { + if (mediaElement.error) { // Already failed! fullyLoaded.reject(this.videoErrorToShakaError_()); - } else if (this.video_.preload == 'none') { + } else if (mediaElement.preload == 'none') { shaka.log.alwaysWarn( 'With