[eme] Handle multiple calls to MediaKeySession.close()

media/cdm/aes_decryptor.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/cdm/aes_decryptor.h"
 
 #include <stddef.h>
 #include <list>
 #include <utility>
 #include <vector>
@@ skipping 311 matching lines @@
   // that do not support loadSession. See http://crbug.com/342481
   promise->reject(CdmPromise::NOT_SUPPORTED_ERROR, 0,
                   "LoadSession() is not supported.");
 }
 
 void AesDecryptor::UpdateSession(const std::string& session_id,
                                  const std::vector<uint8_t>& response,
                                  std::unique_ptr<SimpleCdmPromise> promise) {
   CHECK(!response.empty());
 
   // TODO(jrummell): Convert back to a DCHECK once prefixed EME is removed.

[xhwang, 2016/12/07 06:51:42]

Can we do this now?

[jrummell, 2016/12/07 22:26:50]

Nope (due to this being called asynchronously). Added comment and link to EME
spec bug.

   if (valid_sessions_.find(session_id) == valid_sessions_.end()) {
     promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
                     "Session does not exist.");
     return;
   }
 
   std::string key_string(response.begin(), response.end());
 
   KeyIdAndKeyPairs keys;
   SessionType session_type = MediaKeys::TEMPORARY_SESSION;
@@ skipping 55 matching lines @@
       }
     }
   }
 
   session_keys_change_cb_.Run(session_id, key_added, std::move(keys_info));
 }
 
 // Runs the parallel steps from https://w3c.github.io/encrypted-media/#close.
 void AesDecryptor::CloseSession(const std::string& session_id,
                                 std::unique_ptr<SimpleCdmPromise> promise) {
-  // Validate that this is a reference to an active session and then forget it.
+  // Validate that this is a reference to an active session. close() shouldn't
+  // be called if the session is already closed. However, the operation is
+  // asynchronous, so there is a window where close() was called a second time
+  // just before the closed event arrives. As a result it is possible that the
+  // session is already closed, so assume that the session is closed if it
+  // doesn't exist.
+  //
+  // close() is called from a MediaKeySession object, so it is unlikely that
+  // this method will be called with a previously unseen |session_id|.
   std::set<std::string>::iterator it = valid_sessions_.find(session_id);
-  DCHECK(it != valid_sessions_.end());
+  if (it == valid_sessions_.end()) {

[xhwang, 2016/12/07 06:51:42]

Now we cannot detect an invalid sesion_id... Since the spec actually doesn't say
removing the session, shall we keep a list of closed_sessions_?

Also valid_sessions_ seems ambiguous. It really should be open_sessions_.

[jrummell, 2016/12/07 22:26:50]

I don't think there is any way to get a invalid session_id, unless due to a bug
somewhere. So it's not necessary to bother with closed session IDs. Worst case
is we indicate that the invalid session ID is closed, which I guess is
technically true as it doesn't exist.
Done.

+    promise->resolve();
+    return;
+  }
 
   // 5.1. Let cdm be the CDM instance represented by session's cdm instance
   //      value.
   // 5.2. Use cdm to close the session associated with session.
   valid_sessions_.erase(it);
   DeleteKeysForSession(session_id);
 
   // 5.3. Queue a task to run the following steps:
   // 5.3.1. Run the Session Closed algorithm on the session.
   session_closed_cb_.Run(session_id);
@@ skipping 181 matching lines @@
 bool AesDecryptor::DecryptionKey::Init() {
   CHECK(!secret_.empty());
   decryption_key_ =
       crypto::SymmetricKey::Import(crypto::SymmetricKey::AES, secret_);
   if (!decryption_key_)
     return false;
   return true;
 }
 
 }  // namespace media