Be more explicit in DrmEngine destroy

We do a lot of tear down in DrmEngine at the same time. While this often
works well for common tasks, when tasks focus on different parts, it can
introduce some race conditions.

This change looks at being a little more strict about the order we do things
in DrmEngine destroy.

While this does not fix #1728, the hope is that this will correct the
"read property 'catch' of undefined".

Issue #1728

Change-Id: Id3cd25b416969ecd75a8cdd6d36959e34673fa1b

lib/media/drm_engine.js

@@ -23,7 +23,6 @@ goog.require('shaka.net.NetworkingEngine');
 goog.require('shaka.util.Error');
 goog.require('shaka.util.EventManager');
 goog.require('shaka.util.FakeEvent');
-goog.require('shaka.util.Functional');
 goog.require('shaka.util.IDestroyable');
 goog.require('shaka.util.Iterables');
 goog.require('shaka.util.MapUtils');
@@ -105,8 +104,24 @@ shaka.media.DrmEngine = function(playerInterface) {
   this.keyStatusTimer_ =
       new shaka.util.Timer(() => this.processKeyStatusChanges_());
 
-  /** @private {boolean} */
-  this.destroyed_ = false;
+  /**
+   * A flag to signal when have started destroying ourselves. This will:
+   *  1. Stop later calls to |destroy| from trying to destroy the already
+   *     destroyed (or currently destroying) DrmEngine.
+   *  2. Stop in-progress async operations from continuing.
+   *
+   * @private {boolean}
+   */
+  this.isDestroying_ = false;
+
+  /**
+   * A promise that will only resolve once we have finished destroying
+   * ourselves, this is used to ensure that subsequent calls to |destroy| don't
+   * resolve before the first call to |destroy|.
+   *
+   * @private {!shaka.util.PublicPromise}
+   */
+  this.finishedDestroyingPromise_ = new shaka.util.PublicPromise();
 
   /** @private {boolean} */
   this.usePersistentLicenses_ = false;
@@ -181,66 +196,82 @@ shaka.media.DrmEngine.PlayerInterface;
 
 
 /** @override */
-shaka.media.DrmEngine.prototype.destroy = function() {
-  const Functional = shaka.util.Functional;
-  this.destroyed_ = true;
-
-  let async = [];
-
-  // Wait for sessions to close when destroying.
-  const sessions = this.activeSessions_.keys();
-  for (const session of sessions) {
-    shaka.log.v1('Closing session', session.sessionId);
-    // Ignore any errors when closing the sessions.  One such error would be
-    // an invalid state error triggered by closing a session which has not
-    // generated any key requests.
-    let isClosed = false;
-    let close =
-        session.close().then(() => { isClosed = true; }, Functional.noop);
-    // Due to a bug in Chrome, sometimes the Promise returned by close()
-    // never resolves.  See issue #1093 and https://crbug.com/690583.
-    let closeTimeout =
-        shaka.media.DrmEngine.timeout_(shaka.media.DrmEngine.CLOSE_TIMEOUT_)
-            .then(() => {
-              if (!isClosed) {
-                shaka.log.warning('Timeout waiting for session close');
-              }
-            });
-    async.push(Promise.race([close, closeTimeout]));
+shaka.media.DrmEngine.prototype.destroy = async function() {
+  // If we have started destroying ourselves, wait for the common "I am finished
+  // being destroyed" promise to be resolved.
+  if (this.isDestroying_) {
+   await this.finishedDestroyingPromise_;
+  } else {
+    this.isDestroying_ = true;
+    await this.destroyNow_();
+    this.finishedDestroyingPromise_.resolve();
   }
+};
+
+
+/**
+ * Destroy this instance of DrmEngine. This assumes that all other checks about
+ * "if it should" have passed.
+ *
+ * @private
+ */
+shaka.media.DrmEngine.prototype.destroyNow_ = async function() {
+  // |eventManager_| should only be |null| after we call |destroy|. Destroy it
+  // first so that we will stop responding to events.
+  await this.eventManager_.destroy();
+  this.eventManager_ = null;
+
+  // Since we are destroying ourselves, we don't want to react to the "all
+  // sessions loaded" event.
   this.allSessionsLoaded_.reject();
 
-  if (this.eventManager_) {
-    async.push(this.eventManager_.destroy());
-  }
+  // Stop all timers. This will ensure that they do not start any new work while
+  // we are destroying ourselves.
+  this.expirationTimer_.stop();
+  this.expirationTimer_ = null;
 
+  this.keyStatusTimer_.stop();
+  this.keyStatusTimer_ = null;
+
+  // Close all open sessions.
+  const openSessions = Array.from(this.activeSessions_.keys());
+  this.activeSessions_.clear();
+
+  // Close all sessions before we remove media keys from the video element.
+  await Promise.all(openSessions.map((session) => {
+    return Promise.resolve().then(async () => {
+      shaka.log.v1('Closing session', session.sessionId);
+
+      try {
+        await shaka.media.DrmEngine.closeSession_(session);
+      } catch (error) {
+        // Ignore errors when closing the sessions. Closing a session that
+        // generated no key requests will throw an error.
+      }
+    });
+  }));
+
+  // |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!');
-    async.push(this.video_.setMediaKeys(null).catch(Functional.noop));
-  }
 
-  if (this.expirationTimer_) {
-    this.expirationTimer_.stop();
-    this.expirationTimer_ = null;
-  }
+    try {
+      await this.video_.setMediaKeys(null);
+    } catch (error) {
+      // Ignore any failures while removing media keys from the video element.
+    }
 
-  if (this.keyStatusTimer_) {
-    this.keyStatusTimer_.stop();
-    this.keyStatusTimer_ = null;
+    this.video_ = null;
   }
 
+  // Break references to everything else we hold internally.
   this.currentDrmInfo_ = null;
   this.supportedTypes_.clear();
   this.mediaKeys_ = null;
-  this.video_ = null;
-  this.eventManager_ = null;
-  this.activeSessions_.clear();
   this.offlineSessionIds_ = [];
   this.config_ = null;
   this.onError_ = null;
   this.playerInterface_ = null;
-
-  return Promise.all(async);
 };
 
 
@@ -454,7 +485,7 @@ shaka.media.DrmEngine.prototype.attach = function(video) {
   let setServerCertificate = this.setServerCertificate();
 
   return Promise.all([setMediaKeys, setServerCertificate]).then(() => {
-    if (this.destroyed_) return Promise.reject();
+    if (this.isDestroying_) { return Promise.reject(); }
 
     this.createOrLoad();
     if (!this.currentDrmInfo_.initData.length &&
@@ -466,7 +497,7 @@ shaka.media.DrmEngine.prototype.attach = function(video) {
       this.eventManager_.listen(this.video_, 'encrypted', cb);
     }
   }).catch((error) => {
-    if (this.destroyed_) return Promise.resolve();  // Ignore destruction errors
+    if (this.isDestroying_) { return; }
     return Promise.reject(error);
   });
 };
@@ -836,7 +867,7 @@ shaka.media.DrmEngine.prototype.queryMediaKeys_ = function(configsByKeySystem) {
       if (hasLicenseServer != shouldHaveLicenseServer) return;
 
       p = p.catch(function() {
-        if (this.destroyed_) return Promise.reject();
+        if (this.isDestroying_) { return; }
         return navigator.requestMediaKeySystemAccess(keySystem, [config]);
       }.bind(this));
     });
@@ -850,7 +881,7 @@ shaka.media.DrmEngine.prototype.queryMediaKeys_ = function(configsByKeySystem) {
   });
 
   p = p.then(function(mediaKeySystemAccess) {
-    if (this.destroyed_) return Promise.reject();
+    if (this.isDestroying_) { return Promise.reject(); }
 
     // Get the set of supported content types from the audio and video
     // capabilities. Avoid duplicates so that it is easier to read what is
@@ -886,14 +917,14 @@ shaka.media.DrmEngine.prototype.queryMediaKeys_ = function(configsByKeySystem) {
 
     return mediaKeySystemAccess.createMediaKeys();
   }.bind(this)).then(function(mediaKeys) {
-    if (this.destroyed_) return Promise.reject();
+    if (this.isDestroying_) { return Promise.reject(); }
     shaka.log.info('Created MediaKeys object for key system',
                    this.currentDrmInfo_.keySystem);
 
     this.mediaKeys_ = mediaKeys;
     this.initialized_ = true;
   }.bind(this)).catch(function(exception) {
-    if (this.destroyed_) return Promise.resolve();  // Ignore destruction errors
+    if (this.isDestroying_) { return; }
 
     // Don't rewrap a shaka.util.Error from earlier in the chain:
     this.currentDrmInfo_ = null;
@@ -1003,7 +1034,7 @@ shaka.media.DrmEngine.prototype.loadOfflineSession_ = function(sessionId) {
   this.activeSessions_.set(session, metadata);
 
   return session.load(sessionId).then(function(present) {
-    if (this.destroyed_) return;
+    if (this.isDestroying_) { return Promise.reject(); }
     shaka.log.v2('Loaded offline session', sessionId, present);
 
     if (!present) {
@@ -1025,7 +1056,7 @@ shaka.media.DrmEngine.prototype.loadOfflineSession_ = function(sessionId) {
 
     return session;
   }.bind(this), function(error) {
-    if (this.destroyed_) return;
+    if (this.isDestroying_) { return; }
 
     this.activeSessions_.delete(session);
 
@@ -1078,7 +1109,7 @@ shaka.media.DrmEngine.prototype.createTemporarySession_ =
   this.activeSessions_.set(session, metadata);
 
   session.generateRequest(initDataType, initData.buffer).catch((error) => {
-    if (this.destroyed_) return;
+    if (this.isDestroying_) { return; }
 
     this.activeSessions_.delete(session);
 
@@ -1157,7 +1188,7 @@ shaka.media.DrmEngine.prototype.sendLicenseRequest_ = function(event) {
 
   this.playerInterface_.netEngine.request(requestType, request).promise
       .then(function(response) {
-        if (this.destroyed_) return Promise.reject();
+        if (this.isDestroying_) { return Promise.reject(); }
 
         // Request succeeded, now pass the response to the CDM.
         return session.update(response.data).then(function() {
@@ -1183,7 +1214,7 @@ shaka.media.DrmEngine.prototype.sendLicenseRequest_ = function(event) {
         }.bind(this));
       }.bind(this), function(error) {
         // Ignore destruction errors
-        if (this.destroyed_) return Promise.resolve();
+        if (this.isDestroying_) { return; }
 
         // Request failed!
         goog.asserts.assert(error instanceof shaka.util.Error,
@@ -1199,7 +1230,7 @@ shaka.media.DrmEngine.prototype.sendLicenseRequest_ = function(event) {
         }
       }.bind(this)).catch(function(error) {
         // Ignore destruction errors
-        if (this.destroyed_) return Promise.resolve();
+        if (this.isDestroying_) { return; }
 
         // Session update failed!
         let shakaErr = new shaka.util.Error(
@@ -1889,6 +1920,35 @@ shaka.media.DrmEngine.fillInDrmInfoDefaults_ = function(
 };
 
 
+/**
+ * Close a drm session while accounting for a bug in Chrome. Sometimes the
+ * Promise returned by close() never resolves.
+ *
+ * See issue #1093 and https://crbug.com/690583.
+ *
+ * @param {!MediaKeySession} session
+ * @return {!Promise}
+ * @private
+ */
+shaka.media.DrmEngine.closeSession_ = async function(session) {
+  const DrmEngine = shaka.media.DrmEngine;
+
+  /** @type {!Promise.<boolean>} */
+  const close = session.close().then(() => true);
+
+  /** @type {!Promise.<boolean>} */
+  const timeout =
+      DrmEngine.timeout_(DrmEngine.CLOSE_TIMEOUT_).then(() => false);
+
+  /** @type {boolean} */
+  const wasSessionClosed = await Promise.race([close, timeout]);
+
+  if (!wasSessionClosed) {
+    shaka.log.warning('Timeout waiting for session close');
+  }
+};
+
+
 /**
  * The amount of time, in seconds, we wait to consider a session closed.
  * This allows us to work around Chrome bug https://crbug.com/690583.