From f32b11f0a5376ee79144aa66293477de4fe6947f Mon Sep 17 00:00:00 2001 From: Vincent Valot Date: Sat, 15 Apr 2023 00:18:04 +0200 Subject: [PATCH] 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 :) --- externs/network_information.js | 9 +++++++-- externs/shaka/abr_manager.js | 6 ++++++ lib/abr/simple_abr_manager.js | 28 ++++++++++++++++++++++++++-- lib/player.js | 6 +++++- test/player_unit.js | 3 +++ test/test/util/simple_fakes.js | 3 +++ 6 files changed, 50 insertions(+), 5 deletions(-) diff --git a/externs/network_information.js b/externs/network_information.js index 75ecfaf739..a8e23415e1 100644 --- 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 index f421ac7f41..0a2328a094 100644 --- 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 index 19bec41518..1029333055 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.IReleasable'); 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.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 index 621d422331..dfd67bf017 100644 --- 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 index 5f19a3f5cc..7f7caf2408 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.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 index 9ff9d6d278..fe301338d0 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.release = jasmine.createSpy('release'); + /** @type {!jasmine.Spy} */ this.enable = jasmine.createSpy('enable');