From f1c1585afb2cfa3eb6b7465c8b32c9bad8e62d15 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Tue, 15 Feb 2022 12:06:26 -0800 Subject: [PATCH] fix: Add explicit release() for FakeEventTarget (#3950) Before, we would count on all event listeners for FakeEventTargets to be cleaned up by the object that listens. Now, FakeEventTarget implements IReleasable, so that all listeners are removed when owners call release(). For objects extending FakeEventTarget and also implementing IDestroyable, the destroy() methods will call out to super.release() to clean up listeners then. The owner should use destroy() in those cases. Issue #3949 (memory leak in DASH live streams with inband EventStream) --- lib/ads/ad_manager.js | 1 + lib/cast/cast_proxy.js | 3 +++ lib/cast/cast_receiver.js | 3 +++ lib/net/networking_engine.js | 4 +++ lib/player.js | 3 +++ lib/util/fake_event_target.js | 22 +++++++++++++++- ui/controls.js | 3 +++ ui/localization.js | 48 +++++++++-------------------------- 8 files changed, 50 insertions(+), 37 deletions(-) diff --git a/lib/ads/ad_manager.js b/lib/ads/ad_manager.js index 0f3ebf46ff..35e3a0cc0c 100644 --- a/lib/ads/ad_manager.js +++ b/lib/ads/ad_manager.js @@ -448,6 +448,7 @@ shaka.ads.AdManager = class extends shaka.util.FakeEventTarget { this.ssAdManager_.release(); this.ssAdManager_ = null; } + super.release(); } diff --git a/lib/cast/cast_proxy.js b/lib/cast/cast_proxy.js index c453cb80ea..d10984cd11 100644 --- a/lib/cast/cast_proxy.js +++ b/lib/cast/cast_proxy.js @@ -120,6 +120,9 @@ shaka.cast.CastProxy = class extends shaka.util.FakeEventTarget { this.videoProxy_ = null; this.playerProxy_ = null; + // FakeEventTarget implements IReleasable + super.release(); + return Promise.all(waitFor); } diff --git a/lib/cast/cast_receiver.js b/lib/cast/cast_receiver.js index e5dbfb0364..6d67d3a5a5 100644 --- a/lib/cast/cast_receiver.js +++ b/lib/cast/cast_receiver.js @@ -238,6 +238,9 @@ shaka.cast.CastReceiver = class extends shaka.util.FakeEventTarget { this.shakaBus_ = null; this.genericBus_ = null; + // FakeEventTarget implements IReleasable + super.release(); + await Promise.all(waitFor); const manager = cast.receiver.CastReceiverManager.getInstance(); diff --git a/lib/net/networking_engine.js b/lib/net/networking_engine.js index a7df748cc5..626990ca7c 100644 --- a/lib/net/networking_engine.js +++ b/lib/net/networking_engine.js @@ -232,6 +232,10 @@ shaka.net.NetworkingEngine = class extends shaka.util.FakeEventTarget { this.destroyed_ = true; this.requestFilters_.clear(); this.responseFilters_.clear(); + + // FakeEventTarget implements IReleasable + super.release(); + return this.operationManager_.destroy(); } diff --git a/lib/player.js b/lib/player.js index 137e5cfb3c..c50ad975a1 100644 --- a/lib/player.js +++ b/lib/player.js @@ -787,6 +787,9 @@ shaka.Player = class extends shaka.util.FakeEventTarget { await this.networkingEngine_.destroy(); this.networkingEngine_ = null; } + + // FakeEventTarget implements IReleasable + super.release(); } /** diff --git a/lib/util/fake_event_target.js b/lib/util/fake_event_target.js index 7412edef44..fc57f1f89d 100644 --- a/lib/util/fake_event_target.js +++ b/lib/util/fake_event_target.js @@ -9,6 +9,7 @@ goog.provide('shaka.util.FakeEventTarget'); goog.require('goog.asserts'); goog.require('shaka.log'); goog.require('shaka.util.FakeEvent'); +goog.require('shaka.util.IReleasable'); goog.require('shaka.util.MultiMap'); @@ -18,13 +19,14 @@ goog.require('shaka.util.MultiMap'); * to non-DOM classes. Only FakeEvents should be dispatched. * * @implements {EventTarget} + * @implements {shaka.util.IReleasable} * @exportInterface */ shaka.util.FakeEventTarget = class { /** */ constructor() { /** - * @private {!shaka.util.MultiMap.} + * @private {shaka.util.MultiMap.} */ this.listeners_ = new shaka.util.MultiMap(); @@ -46,6 +48,9 @@ shaka.util.FakeEventTarget = class { * @exportInterface */ addEventListener(type, listener, options) { + if (!this.listeners_) { + return; + } this.listeners_.push(type, listener); } @@ -73,6 +78,9 @@ shaka.util.FakeEventTarget = class { * @exportInterface */ removeEventListener(type, listener, options) { + if (!this.listeners_) { + return; + } this.listeners_.remove(type, listener); } @@ -90,6 +98,10 @@ shaka.util.FakeEventTarget = class { goog.asserts.assert(event instanceof shaka.util.FakeEvent, 'FakeEventTarget can only dispatch FakeEvents!'); + if (!this.listeners_) { + return true; + } + let listeners = this.listeners_.get(event.type) || []; const universalListeners = this.listeners_.get(shaka.util.FakeEventTarget.ALL_EVENTS_); @@ -129,6 +141,14 @@ shaka.util.FakeEventTarget = class { return event.defaultPrevented; } + + /** + * @override + * @exportInterface + */ + release() { + this.listeners_ = null; + } }; /** diff --git a/ui/controls.js b/ui/controls.js index 1b27040cfb..6fe8d393ed 100644 --- a/ui/controls.js +++ b/ui/controls.js @@ -254,6 +254,9 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { this.localization_ = null; this.pressedKeys_.clear(); + + // FakeEventTarget implements IReleasable + super.release(); } diff --git a/ui/localization.js b/ui/localization.js index fad4c1af11..e161e6921d 100644 --- a/ui/localization.js +++ b/ui/localization.js @@ -23,17 +23,18 @@ goog.require('shaka.util.LanguageUtils'); * If a string is not available, it will return the localized * form in the closest related locale. * - * @implements {EventTarget} * @final * @export */ -shaka.ui.Localization = class { +shaka.ui.Localization = class extends shaka.util.FakeEventTarget { /** * @param {string} fallbackLocale * The fallback locale that should be used. It will be assumed that this * locale should have entries for just about every request. */ constructor(fallbackLocale) { + super(); + /** @private {string} */ this.fallbackLocale_ = shaka.util.LanguageUtils.normalize(fallbackLocale); @@ -62,41 +63,16 @@ shaka.ui.Localization = class { * @private {!Map.>} */ this.localizations_ = new Map(); - - /** - * The event target that we will wrap so that we can fire events - * without having to manage the listeners directly. - * - * @private {!EventTarget} - */ - this.events_ = new shaka.util.FakeEventTarget(); - } - - /** - * @override - * @export - */ - addEventListener(type, listener, options) { - this.events_.addEventListener(type, listener, options); - } - - /** - * @override - * @export - */ - removeEventListener(type, listener, options) { - // Apparently Closure says we can be passed a null |option|, but we can't - // pass a null option, so if we get have a null-like |option|, force it to - // be undefined. - this.events_.removeEventListener(type, listener, options || undefined); } /** * @override * @export */ - dispatchEvent(event) { - return this.events_.dispatchEvent(event); + release() { + // Placeholder so that readers know this implements IReleasable (via + // FakeEventTarget) + super.release(); } /** @@ -130,7 +106,7 @@ shaka.ui.Localization = class { (locale) => !this.localizations_.has(locale)); if (missing.length) { - this.events_.dispatchEvent(new shaka.util.FakeEvent( + this.dispatchEvent(new shaka.util.FakeEvent( Class.UNKNOWN_LOCALES, (new Map()).set('locales', missing))); } @@ -141,7 +117,7 @@ shaka.ui.Localization = class { const data = (new Map()).set( 'locales', found.length ? found : [this.fallbackLocale_]); - this.events_.dispatchEvent(new shaka.util.FakeEvent( + this.dispatchEvent(new shaka.util.FakeEvent( Class.LOCALE_CHANGED, data)); } @@ -193,7 +169,7 @@ shaka.ui.Localization = class { // data from. this.updateCurrentMap_(); - this.events_.dispatchEvent(new FakeEvent(Class.LOCALE_UPDATED)); + this.dispatchEvent(new FakeEvent(Class.LOCALE_UPDATED)); return this; } @@ -249,7 +225,7 @@ shaka.ui.Localization = class { // Make a copy to avoid leaking references. .set('locales', Array.from(this.currentLocales_)) .set('missing', id); - this.events_.dispatchEvent(new FakeEvent(Class.UNKNOWN_LOCALIZATION, data)); + this.dispatchEvent(new FakeEvent(Class.UNKNOWN_LOCALIZATION, data)); return ''; } @@ -377,7 +353,7 @@ shaka.ui.Localization = class { // an array. .set('missing', Array.from(missing)); - this.events_.dispatchEvent(new shaka.util.FakeEvent( + this.dispatchEvent(new shaka.util.FakeEvent( shaka.ui.Localization.MISSING_LOCALIZATIONS, data)); }