From d32ac54c8364ccdad99183b022ae34b359c51980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Tyczy=C5=84ski?= Date: Mon, 10 Jun 2024 10:08:16 +0200 Subject: [PATCH] fix webkit memleak --- externs/polyfill.js | 2 +- lib/media/drm_engine.js | 8 ++++- lib/player.js | 7 ++++ lib/polyfill/patchedmediakeys_apple.js | 11 ++++-- lib/polyfill/patchedmediakeys_nop.js | 9 ++++- lib/polyfill/patchedmediakeys_webkit.js | 14 ++++++-- lib/util/platform.js | 11 +++--- test/player_unit.js | 19 +++++++++++ test/polyfill/patchedmediakeys_apple_unit.js | 4 +-- test/util/platform_unit.js | 36 ++++++++++++++++++++ 10 files changed, 107 insertions(+), 14 deletions(-) diff --git a/externs/polyfill.js b/externs/polyfill.js index 89d5f58147..b9b5772e53 100644 --- a/externs/polyfill.js +++ b/externs/polyfill.js @@ -11,5 +11,5 @@ */ -/** @type {boolean} */ +/** @type {string} */ window.shakaMediaKeysPolyfill; diff --git a/lib/media/drm_engine.js b/lib/media/drm_engine.js index 3513679e0d..b0ab5fd0b5 100644 --- a/lib/media/drm_engine.js +++ b/lib/media/drm_engine.js @@ -179,12 +179,18 @@ shaka.media.DrmEngine = class { // |video_| will be |null| if we never attached to a video element. if (this.video_) { - goog.asserts.assert(!this.video_.src, 'video src must be removed first!'); + // Webkit EME implementation requires the src to be defined to clear + // the MediaKeys. + if (!shaka.util.Platform.isMediaKeysPolyfilled('webkit')) { + goog.asserts.assert(!this.video_.src, + 'video src must be removed first!'); + } try { await this.video_.setMediaKeys(null); } catch (error) { // Ignore any failures while removing media keys from the video element. + shaka.log.debug(`DrmEngine.destroyNow_ exception`, error); } this.video_ = null; diff --git a/lib/player.js b/lib/player.js index 7009fbb577..3f3a838f2b 100644 --- a/lib/player.js +++ b/lib/player.js @@ -1356,6 +1356,13 @@ shaka.Player = class extends shaka.util.FakeEventTarget { this.playhead_ = null; } + // EME v0.1b requires the media element to clear the MediaKeys + if (shaka.util.Platform.isMediaKeysPolyfilled('webkit') && + this.drmEngine_) { + await this.drmEngine_.destroy(); + this.drmEngine_ = null; + } + // Media source engine holds onto the media element, and in order to // detach the media keys (with drm engine), we need to break the // connection between media source engine and the media element. diff --git a/lib/polyfill/patchedmediakeys_apple.js b/lib/polyfill/patchedmediakeys_apple.js index f0387a248f..616c1ad33e 100644 --- a/lib/polyfill/patchedmediakeys_apple.js +++ b/lib/polyfill/patchedmediakeys_apple.js @@ -76,7 +76,7 @@ shaka.polyfill.PatchedMediaKeysApple = class { navigator.requestMediaKeySystemAccess = PatchedMediaKeysApple.requestMediaKeySystemAccess; - window.shakaMediaKeysPolyfill = true; + window.shakaMediaKeysPolyfill = PatchedMediaKeysApple.apiName_; } /** @@ -115,7 +115,7 @@ shaka.polyfill.PatchedMediaKeysApple = class { PatchedMediaKeysApple.originalNavigatorRequestMediaKeySystemAccess = null; PatchedMediaKeysApple.originalHTMLMediaElementPrototypeMediaKeys = null; - window.shakaMediaKeysPolyfill = false; + window.shakaMediaKeysPolyfill = ''; } /** @@ -791,3 +791,10 @@ shaka.polyfill.PatchedMediaKeysApple.MediaKeyStatusMap = class { goog.asserts.assert(false, 'Not used! Provided only for the compiler.'); } }; + +/** + * API name. + * + * @private {string} + */ +shaka.polyfill.PatchedMediaKeysApple.apiName_ = 'apple'; diff --git a/lib/polyfill/patchedmediakeys_nop.js b/lib/polyfill/patchedmediakeys_nop.js index 26957e60b6..9b4d8c5a7a 100644 --- a/lib/polyfill/patchedmediakeys_nop.js +++ b/lib/polyfill/patchedmediakeys_nop.js @@ -50,7 +50,7 @@ shaka.polyfill.PatchedMediaKeysNop = class { window.MediaKeys = PatchedMediaKeysNop.MediaKeys; window.MediaKeySystemAccess = PatchedMediaKeysNop.MediaKeySystemAccess; - window.shakaMediaKeysPolyfill = true; + window.shakaMediaKeysPolyfill = PatchedMediaKeysNop.apiName_; } /** @@ -136,6 +136,13 @@ shaka.polyfill.PatchedMediaKeysNop.MediaKeySystemAccess = class { createMediaKeys() {} }; +/** + * API name. + * + * @private {string} + */ +shaka.polyfill.PatchedMediaKeysNop.apiName_ = 'nop'; + // A low priority ensures this is the last and acts as a fallback. shaka.polyfill.register(shaka.polyfill.PatchedMediaKeysNop.install, -10); diff --git a/lib/polyfill/patchedmediakeys_webkit.js b/lib/polyfill/patchedmediakeys_webkit.js index 904d0ff560..43aebcb30f 100644 --- a/lib/polyfill/patchedmediakeys_webkit.js +++ b/lib/polyfill/patchedmediakeys_webkit.js @@ -73,7 +73,7 @@ shaka.polyfill.PatchedMediaKeysWebkit = class { window.MediaKeys = PatchedMediaKeysWebkit.MediaKeys; window.MediaKeySystemAccess = PatchedMediaKeysWebkit.MediaKeySystemAccess; - window.shakaMediaKeysPolyfill = true; + window.shakaMediaKeysPolyfill = PatchedMediaKeysWebkit.apiName_; } /** @@ -860,7 +860,9 @@ class extends shaka.util.FakeEventTarget { PatchedMediaKeysWebkit.prefixApi_('cancelKeyRequest'); try { this.media_[cancelKeyRequestName](this.keySystem_, this.sessionId); - } catch (exception) {} + } catch (exception) { + shaka.log.debug(`${cancelKeyRequestName} exception`, exception); + } } // Resolve the 'closed' promise and return it. @@ -977,4 +979,12 @@ shaka.polyfill.PatchedMediaKeysWebkit.MediaKeyStatusMap = class { shaka.polyfill.PatchedMediaKeysWebkit.prefix_ = ''; +/** + * API name. + * + * @private {string} + */ +shaka.polyfill.PatchedMediaKeysWebkit.apiName_ = 'webkit'; + + shaka.polyfill.register(shaka.polyfill.PatchedMediaKeysWebkit.install); diff --git a/lib/util/platform.js b/lib/util/platform.js index dcaf22155a..7bc6013c9f 100644 --- a/lib/util/platform.js +++ b/lib/util/platform.js @@ -648,16 +648,17 @@ shaka.util.Platform = class { /** * Returns true if MediaKeys is polyfilled * + * @param {string=} polyfillType * @return {boolean} */ - static isMediaKeysPolyfilled() { - if (window.shakaMediaKeysPolyfill) { - return true; + static isMediaKeysPolyfilled(polyfillType) { + if (polyfillType) { + return polyfillType === window.shakaMediaKeysPolyfill; } - - return false; + return !!window.shakaMediaKeysPolyfill; } + /** * Detect the maximum resolution that the platform's hardware can handle. * diff --git a/test/player_unit.js b/test/player_unit.js index 68363afe26..ab8381b079 100644 --- a/test/player_unit.js +++ b/test/player_unit.js @@ -236,6 +236,25 @@ describe('Player', () => { } }); + it('destroys drmEngine before mediaSourceEngine with webkit polyfill', + async () => { + spyOn(shaka.util.Platform, 'isMediaKeysPolyfilled') + .and.returnValue(true); + goog.asserts.assert(manifest, 'Manifest should be non-null'); + + Util.funcSpy(drmEngine.destroy).and.callFake(async () => { + expect(mediaSourceEngine.destroy).not.toHaveBeenCalled(); + await Util.shortDelay(); + expect(mediaSourceEngine.destroy).not.toHaveBeenCalled(); + }); + + await player.load(fakeManifestUri, 0, fakeMimeType); + await player.destroy(); + + expect(drmEngine.destroy).toHaveBeenCalled(); + expect(mediaSourceEngine.destroy).toHaveBeenCalled(); + }); + it('destroys mediaSourceEngine before drmEngine', async () => { goog.asserts.assert(manifest, 'Manifest should be non-null'); diff --git a/test/polyfill/patchedmediakeys_apple_unit.js b/test/polyfill/patchedmediakeys_apple_unit.js index 6e02d96864..98904350d4 100644 --- a/test/polyfill/patchedmediakeys_apple_unit.js +++ b/test/polyfill/patchedmediakeys_apple_unit.js @@ -95,7 +95,7 @@ describe('PatchedMediaKeys_Apple', () => { .toEqual(PatchedMediaKeysApple.MediaKeySystemAccess); expect(navigator.requestMediaKeySystemAccess) .toEqual(PatchedMediaKeysApple.requestMediaKeySystemAccess); - expect(window.shakaMediaKeysPolyfill).toBe(true); + expect(window.shakaMediaKeysPolyfill).toBe('apple'); }); }); @@ -124,7 +124,7 @@ describe('PatchedMediaKeys_Apple', () => { .toEqual(originalWindowMediaKeySystemAccess); expect(navigator.requestMediaKeySystemAccess) .toEqual(originalNavigatorRequestMediaKeySystemAccess); - expect(window.shakaMediaKeysPolyfill).toBe(false); + expect(window.shakaMediaKeysPolyfill).toBe(''); }); }); }); diff --git a/test/util/platform_unit.js b/test/util/platform_unit.js index 0e770d0542..1020b26e56 100644 --- a/test/util/platform_unit.js +++ b/test/util/platform_unit.js @@ -102,6 +102,42 @@ describe('Platform', () => { setUserAgent(webOs5); expect(shaka.util.Platform.isWebOS5()).toBe(true); }); + + describe('isMediaKeysPolyfilled', () => { + let shakaMediaKeysPolyfill; + + beforeAll(() => { + shakaMediaKeysPolyfill = window.shakaMediaKeysPolyfill; + }); + + afterAll(() => { + window.shakaMediaKeysPolyfill = shakaMediaKeysPolyfill; + }); + + it('should return true if media keys are polyfilled', () => { + window.shakaMediaKeysPolyfill = 'webkit'; + const result = shaka.util.Platform.isMediaKeysPolyfilled(); + expect(result).toBe(true); + }); + + it('should return false if media keys are not polyfilled', () => { + window.shakaMediaKeysPolyfill = ''; + const result = shaka.util.Platform.isMediaKeysPolyfilled(); + expect(result).toBe(false); + }); + + it('should return true with a matching polyfill type', () => { + window.shakaMediaKeysPolyfill = 'webkit'; + const result = shaka.util.Platform.isMediaKeysPolyfilled('webkit'); + expect(result).toBe(true); + }); + + it('should return false with a non-matching polyfill type', () => { + window.shakaMediaKeysPolyfill = 'webkit'; + const result = shaka.util.Platform.isMediaKeysPolyfilled('apple'); + expect(result).toBe(false); + }); + }); }); /** @param {string} userAgent */