fix: prevent memory leak in SimpleAbrManager while destroying (#5149)

SimpleAbrManager was not properly destroyed after a call to player.destroy, leading to SimpleAbrManager instances being kept in memory. This PR aims to resolve it :)
shaka-project · Apr 26, 2023 · f32b11f · f32b11f
1 parent d55479c
commit f32b11f
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 5 deletions.
diff --git a/externs/network_information.js b/externs/network_information.js
@@ -19,5 +19,10 @@ NetworkInformation.prototype.saveData;
  * @param {string} type
  * @param {Function} listener
  */
-NetworkInformation.prototype.addEventListener =
-    function(type, listener) {};
+NetworkInformation.prototype.addEventListener = function(type, listener) {};
+
+/**
+ * @param {string} type
+ * @param {Function} listener
+ */
+NetworkInformation.prototype.removeEventListener = function(type, listener) {};
diff --git a/externs/shaka/abr_manager.js b/externs/shaka/abr_manager.js
@@ -44,6 +44,12 @@ shaka.extern.AbrManager = class {
    */
   stop() {}
 
+  /**
+   * Request that this object release all internal references.
+   * @exportDoc
+   */
+  release() {}
+
   /**
    * Updates manager's variants collection.
    *

diff --git a/lib/abr/simple_abr_manager.js b/lib/abr/simple_abr_manager.js
@@ -6,6 +6,7 @@
 
 goog.provide('shaka.abr.SimpleAbrManager');
 
+goog.require('shaka.util.IReleasable');
 goog.require('goog.asserts');
 goog.require('shaka.abr.EwmaBandwidthEstimator');
 goog.require('shaka.log');
@@ -32,6 +33,7 @@ goog.require('shaka.util.Timer');
  * </p>
  *
  * @implements {shaka.extern.AbrManager}
+ * @implements {shaka.util.IReleasable}
  * @export
  */
 shaka.abr.SimpleAbrManager = class {
@@ -51,7 +53,7 @@ shaka.abr.SimpleAbrManager = class {
     // to the change event to be able to make quick changes in case the type
     // of connectivity changes.
     if (navigator.connection) {
-      navigator.connection.addEventListener('change', () => {
+      this.onNetworkInformationChange_ = () => {
         if (this.config_.useNetworkInformation && this.enabled_) {
           this.bandwidthEstimator_ = new shaka.abr.EwmaBandwidthEstimator();
           if (this.config_) {
@@ -62,7 +64,10 @@ shaka.abr.SimpleAbrManager = class {
             this.switch_(chosenVariant);
           }
         }
-      });
+      };
+
+      navigator.connection.addEventListener(
+          'change', this.onNetworkInformationChange_);
     }
 
     /**
@@ -93,6 +98,9 @@ shaka.abr.SimpleAbrManager = class {
     /** @private {ResizeObserver} */
     this.resizeObserver_ = null;
 
+    /** @private {?function():void} */
+    this.onNetworkInformationChange_ = null;
+
     /** @private {shaka.util.Timer} */
     this.resizeObserverTimer_ = new shaka.util.Timer(() => {
       if (this.config_.restrictToElementSize) {
@@ -128,6 +136,22 @@ shaka.abr.SimpleAbrManager = class {
     // can start using bandwidth estimates right away after init() is called.
   }
 
+  /**
+   * @override
+   * @export
+   */
+  release() {
+    // stop() should already have been called for unload
+
+    if (navigator.connection) {
+      navigator.connection.removeEventListener(
+          'change', this.onNetworkInformationChange_);
+      this.onNetworkInformationChange_ = null;
+    }
+
+    this.resizeObserverTimer_ = null;
+  }
+
 
   /**
    * @override

diff --git a/lib/player.js b/lib/player.js
@@ -897,7 +897,6 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
     }
 
     this.abrManagerFactory_ = null;
-    this.abrManager_ = null;
     this.config_ = null;
     this.stats_ = null;
     this.videoContainer_ = null;
@@ -908,6 +907,11 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
       this.networkingEngine_ = null;
     }
 
+    if (this.abrManager_) {
+      this.abrManager_.release();
+      this.abrManager_ = null;
+    }
+
     // FakeEventTarget implements IReleasable
     super.release();
   }

diff --git a/test/player_unit.js b/test/player_unit.js
@@ -208,6 +208,7 @@ describe('Player', () => {
       await player.destroy();
 
       expect(abrManager.stop).toHaveBeenCalled();
+      expect(abrManager.release).toHaveBeenCalled();
       expect(networkingEngine.destroy).toHaveBeenCalled();
       expect(drmEngine.destroy).toHaveBeenCalled();
       expect(playhead.release).toHaveBeenCalled();
@@ -246,6 +247,7 @@ describe('Player', () => {
       parser.start.and.returnValue(p);
       parser.stop.and.callFake(() => {
         expect(abrManager.stop).not.toHaveBeenCalled();
+        expect(abrManager.release).not.toHaveBeenCalled();
         expect(networkingEngine.destroy).not.toHaveBeenCalled();
       });
       shaka.media.ManifestParser.registerParserByMime(
@@ -255,6 +257,7 @@ describe('Player', () => {
       await shaka.test.Util.shortDelay();
       await player.destroy();
       expect(abrManager.stop).toHaveBeenCalled();
+      expect(abrManager.release).toHaveBeenCalled();
       expect(networkingEngine.destroy).toHaveBeenCalled();
       expect(parser.stop).toHaveBeenCalled();
       await expectAsync(load).toBeRejected();

diff --git a/test/test/util/simple_fakes.js b/test/test/util/simple_fakes.js
@@ -35,6 +35,9 @@ shaka.test.FakeAbrManager = class {
     /** @type {!jasmine.Spy} */
     this.stop = jasmine.createSpy('stop');
 
+    /** @type {!jasmine.Spy} */
+    this.release = jasmine.createSpy('release');
+
     /** @type {!jasmine.Spy} */
     this.enable = jasmine.createSpy('enable');