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)
shaka-project · Feb 15, 2022 · f1c1585 · f1c1585
1 parent 46bb67b
commit f1c1585
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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.<shaka.util.FakeEventTarget.ListenerType>}
+     * @private {shaka.util.MultiMap.<shaka.util.FakeEventTarget.ListenerType>}
      */
     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
@@ -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
@@ -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.<string, !Map.<string, string>>}
      */
     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));
     }