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

Since the close() operation is asynchronous, there is a window where
close() can be called a second time before the session is actually
closed. Changes allow the second close() to succeed if it is called
while the first close() is being processed.

BUG=668312
TEST=new test passes

Review-Url: https://codereview.chromium.org/2545083004
Cr-Commit-Position: refs/heads/master@{#438591}

media/base/content_decryption_module.h

@@ -125,6 +125,8 @@ class MEDIA_EXPORT ContentDecryptionModule
   // reject the |promise| when the call has been processed. This may be before
   // the session is closed. Once the session is closed, a SessionClosedCB must
   // also be called.
+  // Note that the EME spec executes the close() action asynchronously, so
+  // CloseSession() may be called multiple times on the same session.
   virtual void CloseSession(const std::string& session_id,
                             std::unique_ptr<SimpleCdmPromise> promise) = 0;
 

media/blink/webcontentdecryptionmodulesession_impl.cc

@@ -435,15 +435,19 @@ void WebContentDecryptionModuleSessionImpl::update(
 
 void WebContentDecryptionModuleSessionImpl::close(
     blink::WebContentDecryptionModuleResult result) {
-  // close() shouldn't be called if the session is already closed. blink
-  // prevents a second call to close(), but since the operation is
-  // asynchronous, there is a window where close() was called just before
-  // the closed event arrives. The CDM should handle the case where
-  // close() is called after it has already closed the session.
   DCHECK(!session_id_.empty());
-  DCHECK(!has_close_been_called_);
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // close() shouldn't be called if the session is already closed. Since the
+  // operation is asynchronous, there is a window where close() was called
+  // just before the closed event arrives. The CDM should handle the case where
+  // close() is called after it has already closed the session. However, if
+  // we can tell the session is now closed, simply resolve the promise.
+  if (is_closed_) {
+    result.complete();
+    return;
+  }
+
   has_close_been_called_ = true;
   adapter_->CloseSession(
       session_id_,

media/cdm/aes_decryptor.cc

@@ -261,7 +261,7 @@ void AesDecryptor::CreateSessionAndGenerateRequest(
     const std::vector<uint8_t>& init_data,
     std::unique_ptr<NewSessionCdmPromise> promise) {
   std::string session_id(base::UintToString(next_session_id_++));
-  valid_sessions_.insert(session_id);
+  open_sessions_.insert(session_id);
 
   // For now, the AesDecryptor does not care about |session_type|.
   // TODO(jrummell): Validate |session_type|.
@@ -329,8 +329,11 @@ void AesDecryptor::UpdateSession(const std::string& session_id,
                                  std::unique_ptr<SimpleCdmPromise> promise) {
   CHECK(!response.empty());
 
-  // TODO(jrummell): Convert back to a DCHECK once prefixed EME is removed.
-  if (valid_sessions_.find(session_id) == valid_sessions_.end()) {
+  // Currently the EME spec has blink check for session closed synchronously,
+  // but then this is called asynchronously. So it is possible that update()
+  // could get called on a closed session.
+  // https://github.com/w3c/encrypted-media/issues/365
+  if (open_sessions_.find(session_id) == open_sessions_.end()) {
     promise->reject(CdmPromise::INVALID_ACCESS_ERROR, 0,
                     "Session does not exist.");
     return;
@@ -405,14 +408,25 @@ void AesDecryptor::UpdateSession(const std::string& session_id,
 // 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.
-  std::set<std::string>::iterator it = valid_sessions_.find(session_id);
-  DCHECK(it != valid_sessions_.end());
+  // Validate that this is a reference to an open 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. https://github.com/w3c/encrypted-media/issues/365.
+  //
+  // 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 = open_sessions_.find(session_id);
+  if (it == open_sessions_.end()) {
+    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);
+  open_sessions_.erase(it);
   DeleteKeysForSession(session_id);
 
   // 5.3. Queue a task to run the following steps:

media/cdm/aes_decryptor.h

@@ -146,8 +146,8 @@ class MEDIA_EXPORT AesDecryptor : public ContentDecryptionModule,
   KeyIdToSessionKeysMap key_map_;  // Protected by |key_map_lock_|.
   mutable base::Lock key_map_lock_;  // Protects the |key_map_|.
 
-  // Keeps track of current valid sessions.
-  std::set<std::string> valid_sessions_;
+  // Keeps track of current open sessions.
+  std::set<std::string> open_sessions_;
 
   // Make session ID unique per renderer by making it static. Session
   // IDs seen by the app will be "1", "2", etc.

third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-session-multiple-close.html

+<!DOCTYPE html>
+<html>
+    <head>
+        <title>Test multiple calls to MediaKeySession.close()</title>
+        <script src="encrypted-media-utils.js"></script>
+        <script src="../../resources/testharness.js"></script>
+        <script src="../../resources/testharnessreport.js"></script>
+    </head>
+    <body>
+        <script>
+            // This test verifies that Chrome handles multiple close() calls
+            // correctly. Currently the EME spec has the check for session
+            // already closed happen immediately when the method is called,
+            // but the action is performed asynchronously.
+            // https://github.com/w3c/encrypted-media/issues/365
+
+            async_test(function(test)
+            {
+                var initDataType;
+                var initData;
+                var mediaKeySession;
+
+                navigator.requestMediaKeySystemAccess('org.w3.clearkey', getSimpleConfiguration()).then(function(access) {
+                    initDataType = access.getConfiguration().initDataTypes[0];
+                    initData = getInitData(initDataType);
+                    return access.createMediaKeys();
+                }).then(function(mediaKeys) {
+                    mediaKeySession = mediaKeys.createSession();
+                    return mediaKeySession.generateRequest(initDataType, initData);
+                }).then(function() {
+                    // Call close() multiple times.
+                    return Promise.all([mediaKeySession.close(), mediaKeySession.close(), mediaKeySession.close()]);
+                }).then(function() {
+                    test.done();
+                }).catch(function(error) {
+                    forceTestFailureFromPromise(test, error);
+                });
+            }, 'Test multiple calls to MediaKeySession.close().');
+        </script>
+    </body>
+</html>