From e018aaaaba5b952510b18a70850aedfdbbe9c19f Mon Sep 17 00:00:00 2001
From: Joey Parrish <joeyparrish@google.com>
Date: Tue, 25 Feb 2020 19:02:02 -0800
Subject: [PATCH] Fix potential memory leak in SegmentIndex

SegmentIndex has had a destroy() method for a long time, but it has
never been called.  Now that SegmentIndex has a timer which adds new
references for DASH SegmentTemplate live streams, it is more important
than ever to properly stop this active part of SegmentIndex.

This change replaces async destroy() with synchronous release() and
calls it from Player when the manifest is being disposed of.  This
will ensure that SegmentIndexes don't grow out of control after
content is unloaded.

This would not have affected v2.5, since we didn't have this
timer-driven growth of DASH SegmentTemplate live streams in that
release branch.

Related to #1339 (fixes issues introduced for period flattening)

Change-Id: I419a06a62eaa507d92132e20d4cc2d6e45c83ff2
---
 lib/media/segment_index.js     |  8 +++-----
 lib/player.js                  | 18 ++++++++++++++++++
 test/player_unit.js            | 20 ++++++++++++++++++++
 test/test/util/simple_fakes.js |  3 +--
 4 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/lib/media/segment_index.js b/lib/media/segment_index.js
index 5480a186c..3ce484131 100644
--- a/lib/media/segment_index.js
+++ b/lib/media/segment_index.js
@@ -8,14 +8,14 @@ goog.provide('shaka.media.SegmentIterator');
 
 goog.require('goog.asserts');
 goog.require('shaka.media.SegmentReference');
-goog.require('shaka.util.IDestroyable');
+goog.require('shaka.util.IReleasable');
 goog.require('shaka.util.Timer');
 
 
 /**
  * SegmentIndex.
  *
- * @implements {shaka.util.IDestroyable}
+ * @implements {shaka.util.IReleasable}
  * @implements {Iterable.<!shaka.media.SegmentReference>}
  * @export
  */
@@ -50,15 +50,13 @@ shaka.media.SegmentIndex = class {
    * @override
    * @export
    */
-  destroy() {
+  release() {
     this.references_ = [];
 
     if (this.timer_) {
       this.timer_.stop();
     }
     this.timer_ = null;
-
-    return Promise.resolve();
   }
 
 
diff --git a/lib/player.js b/lib/player.js
index bf6f9ad23..28a6c9b64 100644
--- a/lib/player.js
+++ b/lib/player.js
@@ -1346,6 +1346,24 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
     this.assetUri_ = null;
     this.bufferObserver_ = null;
     this.loadingTextStreams_.clear();
+
+    if (this.manifest_) {
+      for (const period of this.manifest_.periods) {
+        for (const variant of period.variants) {
+          for (const stream of [variant.audio, variant.video]) {
+            if (stream && stream.segmentIndex) {
+              stream.segmentIndex.release();
+            }
+          }
+        }
+        for (const stream of period.textStreams) {
+          if (stream.segmentIndex) {
+            stream.segmentIndex.release();
+          }
+        }
+      }
+    }
+
     this.manifest_ = null;
     this.stats_ = new shaka.util.Stats(); // Replace with a clean stats object.
     this.lastTextFactory_ = null;
diff --git a/test/player_unit.js b/test/player_unit.js
index 9c3d7cd2a..9f54338ca 100644
--- a/test/player_unit.js
+++ b/test/player_unit.js
@@ -159,6 +159,26 @@ describe('Player', () => {
       expect(playhead.release).toHaveBeenCalled();
       expect(mediaSourceEngine.destroy).toHaveBeenCalled();
       expect(streamingEngine.destroy).toHaveBeenCalled();
+
+      const segmentIndexes = [];
+      for (const period of manifest.periods) {
+        for (const variant of period.variants) {
+          if (variant.audio) {
+            segmentIndexes.push(variant.audio.segmentIndex);
+          }
+          if (variant.video) {
+            segmentIndexes.push(variant.video.segmentIndex);
+          }
+        }
+        for (const textStream of period.textStreams) {
+          segmentIndexes.push(textStream.segmentIndex);
+        }
+      }
+      for (const segmentIndex of segmentIndexes) {
+        if (segmentIndex) {
+          expect(segmentIndex.release).toHaveBeenCalled();
+        }
+      }
     });
 
     it('destroys mediaSourceEngine before drmEngine', async () => {
diff --git a/test/test/util/simple_fakes.js b/test/test/util/simple_fakes.js
index 45287d59f..14e7c812c 100644
--- a/test/test/util/simple_fakes.js
+++ b/test/test/util/simple_fakes.js
@@ -427,8 +427,7 @@ shaka.test.FakeClosedCaptionParser = class {
 shaka.test.FakeSegmentIndex = class {
   constructor() {
     /** @type {!jasmine.Spy} */
-    this.destroy =
-        jasmine.createSpy('destroy').and.returnValue(Promise.resolve());
+    this.release = jasmine.createSpy('release');
 
     /** @type {!jasmine.Spy} */
     this.find = jasmine.createSpy('find').and.returnValue(null);