From d8b4f5aaa1f63a11a26e93bb9bdd3d264b1e73b6 Mon Sep 17 00:00:00 2001 From: Vincent Valot Date: Thu, 13 Apr 2023 10:40:29 +0200 Subject: [PATCH 1/4] fix: prevent memory leak in SimpleAbrManager while destroying --- externs/network_information.js | 7 +++++++ externs/shaka/abr_manager.js | 10 ++++++++++ lib/abr/simple_abr_manager.js | 30 ++++++++++++++++++++++++++++-- lib/player.js | 6 +++++- test/player_unit.js | 3 +++ test/test/util/simple_fakes.js | 3 +++ 6 files changed, 56 insertions(+), 3 deletions(-) diff --git a/externs/network_information.js b/externs/network_information.js index 75ecfaf739..9724455b33 100644 --- a/externs/network_information.js +++ b/externs/network_information.js @@ -21,3 +21,10 @@ NetworkInformation.prototype.saveData; */ 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 index f421ac7f41..ca5b7cdbeb 100644 --- a/externs/shaka/abr_manager.js +++ b/externs/shaka/abr_manager.js @@ -44,6 +44,16 @@ shaka.extern.AbrManager = class { */ stop() {} + /** + * Request that this object be destroyed, releasing all resources and shutting + * down all operations. Returns a Promise which is resolved when destruction + * is complete. This Promise should never be rejected. + * + * @return {!Promise} + * @exportDoc + */ + destroy() {} + /** * Updates manager's variants collection. * diff --git a/lib/abr/simple_abr_manager.js b/lib/abr/simple_abr_manager.js index 19bec41518..29bb414e18 100644 --- 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.IDestroyable'); goog.require('goog.asserts'); goog.require('shaka.abr.EwmaBandwidthEstimator'); goog.require('shaka.log'); @@ -32,6 +33,7 @@ goog.require('shaka.util.Timer'); *

* * @implements {shaka.extern.AbrManager} + * @implements {shaka.util.IDestroyable} * @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,24 @@ shaka.abr.SimpleAbrManager = class { // can start using bandwidth estimates right away after init() is called. } + /** + * @override + * @export + */ + destroy() { + // stop() should already have been called for unload + + if (navigator.connection) { + navigator.connection.removeEventListener( + 'change', () => this.onNetworkInformationChange_); + this.onNetworkInformationChange_ = null; + } + + this.resizeObserverTimer_ = null; + + return Promise.resolve(); + } + /** * @override diff --git a/lib/player.js b/lib/player.js index 0fa0b655f8..62f3d1efa4 100644 --- a/lib/player.js +++ b/lib/player.js @@ -901,7 +901,6 @@ shaka.Player = class extends shaka.util.FakeEventTarget { } this.abrManagerFactory_ = null; - this.abrManager_ = null; this.config_ = null; this.stats_ = null; this.videoContainer_ = null; @@ -912,6 +911,11 @@ shaka.Player = class extends shaka.util.FakeEventTarget { this.networkingEngine_ = null; } + if (this.abrManager_) { + await this.abrManager_.destroy(); + this.abrManager_ = null; + } + // FakeEventTarget implements IReleasable super.release(); } diff --git a/test/player_unit.js b/test/player_unit.js index 68a23ef541..a3c7df4adb 100644 --- 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.destroy).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.destroy).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.destroy).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 index 9a1908ba61..715cb13882 100644 --- 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.destroy = jasmine.createSpy('destroy'); + /** @type {!jasmine.Spy} */ this.enable = jasmine.createSpy('enable'); From 95740c7601bb498402bbe06967b001ca7d634e1f Mon Sep 17 00:00:00 2001 From: Vincent Valot Date: Thu, 13 Apr 2023 10:45:57 +0200 Subject: [PATCH 2/4] fixup --- lib/abr/simple_abr_manager.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/abr/simple_abr_manager.js b/lib/abr/simple_abr_manager.js index 29bb414e18..7acea8f7a9 100644 --- a/lib/abr/simple_abr_manager.js +++ b/lib/abr/simple_abr_manager.js @@ -67,7 +67,7 @@ shaka.abr.SimpleAbrManager = class { }; navigator.connection.addEventListener( - 'change', () => this.onNetworkInformationChange_); + 'change', this.onNetworkInformationChange_); } /** @@ -145,7 +145,7 @@ shaka.abr.SimpleAbrManager = class { if (navigator.connection) { navigator.connection.removeEventListener( - 'change', () => this.onNetworkInformationChange_); + 'change', this.onNetworkInformationChange_); this.onNetworkInformationChange_ = null; } From 4aa47e1c4e6cf9af156b98e42894b2c74d4b8deb Mon Sep 17 00:00:00 2001 From: Vincent Valot Date: Fri, 14 Apr 2023 09:26:12 +0200 Subject: [PATCH 3/4] one liner --- externs/network_information.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/externs/network_information.js b/externs/network_information.js index 9724455b33..a8e23415e1 100644 --- a/externs/network_information.js +++ b/externs/network_information.js @@ -19,12 +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) {}; +NetworkInformation.prototype.removeEventListener = function(type, listener) {}; From 4b6990d53d313de1e9e0464e4bff6724445f0c25 Mon Sep 17 00:00:00 2001 From: Vincent Valot Date: Fri, 14 Apr 2023 09:27:33 +0200 Subject: [PATCH 4/4] implement IReleasable --- externs/shaka/abr_manager.js | 8 ++------ lib/abr/simple_abr_manager.js | 8 +++----- lib/player.js | 2 +- test/player_unit.js | 6 +++--- test/test/util/simple_fakes.js | 2 +- 5 files changed, 10 insertions(+), 16 deletions(-) diff --git a/externs/shaka/abr_manager.js b/externs/shaka/abr_manager.js index ca5b7cdbeb..0a2328a094 100644 --- a/externs/shaka/abr_manager.js +++ b/externs/shaka/abr_manager.js @@ -45,14 +45,10 @@ shaka.extern.AbrManager = class { stop() {} /** - * Request that this object be destroyed, releasing all resources and shutting - * down all operations. Returns a Promise which is resolved when destruction - * is complete. This Promise should never be rejected. - * - * @return {!Promise} + * Request that this object release all internal references. * @exportDoc */ - destroy() {} + release() {} /** * Updates manager's variants collection. diff --git a/lib/abr/simple_abr_manager.js b/lib/abr/simple_abr_manager.js index 7acea8f7a9..1029333055 100644 --- a/lib/abr/simple_abr_manager.js +++ b/lib/abr/simple_abr_manager.js @@ -6,7 +6,7 @@ goog.provide('shaka.abr.SimpleAbrManager'); -goog.require('shaka.util.IDestroyable'); +goog.require('shaka.util.IReleasable'); goog.require('goog.asserts'); goog.require('shaka.abr.EwmaBandwidthEstimator'); goog.require('shaka.log'); @@ -33,7 +33,7 @@ goog.require('shaka.util.Timer'); *

* * @implements {shaka.extern.AbrManager} - * @implements {shaka.util.IDestroyable} + * @implements {shaka.util.IReleasable} * @export */ shaka.abr.SimpleAbrManager = class { @@ -140,7 +140,7 @@ shaka.abr.SimpleAbrManager = class { * @override * @export */ - destroy() { + release() { // stop() should already have been called for unload if (navigator.connection) { @@ -150,8 +150,6 @@ shaka.abr.SimpleAbrManager = class { } this.resizeObserverTimer_ = null; - - return Promise.resolve(); } diff --git a/lib/player.js b/lib/player.js index 62f3d1efa4..b8f5b2c22f 100644 --- a/lib/player.js +++ b/lib/player.js @@ -912,7 +912,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { } if (this.abrManager_) { - await this.abrManager_.destroy(); + this.abrManager_.release(); this.abrManager_ = null; } diff --git a/test/player_unit.js b/test/player_unit.js index a3c7df4adb..323c1ebcb0 100644 --- a/test/player_unit.js +++ b/test/player_unit.js @@ -208,7 +208,7 @@ describe('Player', () => { await player.destroy(); expect(abrManager.stop).toHaveBeenCalled(); - expect(abrManager.destroy).toHaveBeenCalled(); + expect(abrManager.release).toHaveBeenCalled(); expect(networkingEngine.destroy).toHaveBeenCalled(); expect(drmEngine.destroy).toHaveBeenCalled(); expect(playhead.release).toHaveBeenCalled(); @@ -247,7 +247,7 @@ describe('Player', () => { parser.start.and.returnValue(p); parser.stop.and.callFake(() => { expect(abrManager.stop).not.toHaveBeenCalled(); - expect(abrManager.destroy).not.toHaveBeenCalled(); + expect(abrManager.release).not.toHaveBeenCalled(); expect(networkingEngine.destroy).not.toHaveBeenCalled(); }); shaka.media.ManifestParser.registerParserByMime( @@ -257,7 +257,7 @@ describe('Player', () => { await shaka.test.Util.shortDelay(); await player.destroy(); expect(abrManager.stop).toHaveBeenCalled(); - expect(abrManager.destroy).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 index 715cb13882..63a6fee44d 100644 --- a/test/test/util/simple_fakes.js +++ b/test/test/util/simple_fakes.js @@ -36,7 +36,7 @@ shaka.test.FakeAbrManager = class { this.stop = jasmine.createSpy('stop'); /** @type {!jasmine.Spy} */ - this.destroy = jasmine.createSpy('destroy'); + this.release = jasmine.createSpy('release'); /** @type {!jasmine.Spy} */ this.enable = jasmine.createSpy('enable');