fix: media source object URL revocation (#5214)

`createMediaSource()` sets `this.url_` for later revocation when the
media source opens. However, `createMediaSource()` is called before the
`url_` property is declared and initialised in the constructor. As
result, `this.url_` actually get declared an initialised with the object
URL in `createMediaSource()` and is later overwritten with an empty
string in the constructor. This compromises the revocation logic as we
end up always passing an empty string to `URL.revokeObjectURL()`.

To fix this, we make sure `this.url_` is declared and assigned default
value in the constructor before any other class methods are called.

lib/media/media_source_engine.js

@@ -112,15 +112,15 @@ shaka.media.MediaSourceEngine = class {
     /** @private {!shaka.util.PublicPromise} */
     this.mediaSourceOpen_ = new shaka.util.PublicPromise();
 
+    /** @private {string} */
+    this.url_ = '';
+
     /** @private {MediaSource} */
     this.mediaSource_ = this.createMediaSource(this.mediaSourceOpen_);
 
     /** @type {!shaka.util.Destroyer} */
     this.destroyer_ = new shaka.util.Destroyer(() => this.doDestroy_());
 
-    /** @private {string} */
-    this.url_ = '';
-
     /** @private {boolean} */
     this.sequenceMode_ = false;
 
@@ -163,6 +163,8 @@ shaka.media.MediaSourceEngine = class {
    * @private
    */
   onSourceOpen_(p) {
+    goog.asserts.assert(this.url_, 'Must have object URL');
+
     // Release the object URL that was previously created, to prevent memory
     // leak.
     // createObjectURL creates a strong reference to the MediaSource object

test/media/media_source_engine_unit.js

@@ -186,9 +186,12 @@ describe('MediaSourceEngine', () => {
   describe('constructor', () => {
     const originalCreateObjectURL =
       shaka.media.MediaSourceEngine.createObjectURL;
+    const originalRevokeObjectURL = window.URL.revokeObjectURL;
     const originalMediaSource = window.MediaSource;
     /** @type {jasmine.Spy} */
     let createObjectURLSpy;
+    /** @type {jasmine.Spy} */
+    let revokeObjectURLSpy;
 
     beforeEach(async () => {
       // Mock out MediaSource so we can test the production version of
@@ -203,6 +206,9 @@ describe('MediaSourceEngine', () => {
       shaka.media.MediaSourceEngine.createObjectURL =
         Util.spyFunc(createObjectURLSpy);
 
+      revokeObjectURLSpy = jasmine.createSpy('revokeObjectURL');
+      window.URL.revokeObjectURL = Util.spyFunc(revokeObjectURLSpy);
+
       const mediaSourceSpy = jasmine.createSpy('MediaSource');
       // Because this is a fake constructor, it must be callable with "new".
       // This will cause jasmine to invoke the callback with "new" as well, so
@@ -220,6 +226,7 @@ describe('MediaSourceEngine', () => {
     afterAll(() => {
       shaka.media.MediaSourceEngine.createObjectURL = originalCreateObjectURL;
       window.MediaSource = originalMediaSource;
+      window.URL.revokeObjectURL = originalRevokeObjectURL;
     });
 
     it('creates a MediaSource object and sets video.src', () => {
@@ -231,6 +238,29 @@ describe('MediaSourceEngine', () => {
       expect(createObjectURLSpy).toHaveBeenCalled();
       expect(mockVideo.src).toBe('blob:foo');
     });
+
+    it('revokes object URL after MediaSource opens', () => {
+      let onSourceOpenListener;
+
+      mockMediaSource.addEventListener.and.callFake((event, callback, _) => {
+        if (event == 'sourceopen') {
+          onSourceOpenListener = callback;
+        }
+      });
+
+      mediaSourceEngine = new shaka.media.MediaSourceEngine(
+          video,
+          new shaka.test.FakeTextDisplayer());
+
+      expect(mockMediaSource.addEventListener).toHaveBeenCalledTimes(1);
+      expect(mockMediaSource.addEventListener.calls.mostRecent().args[0])
+          .toBe('sourceopen');
+      expect(typeof onSourceOpenListener).toBe(typeof Function);
+
+      onSourceOpenListener();
+
+      expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:foo');
+    });
   });
 
   describe('init', () => {