EME: Make sure sessions are closed before they are destroyed

media/blink/webcontentdecryptionmodulesession_impl.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 "webcontentdecryptionmodulesession_impl.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "media/base/cdm_key_information.h"
 #include "media/base/cdm_promise.h"
 #include "media/base/key_system_names.h"
 #include "media/base/key_systems.h"
 #include "media/base/limits.h"
 #include "media/base/media_keys.h"
 #include "media/blink/cdm_result_promise.h"
@@ skipping 197 matching lines @@
     std::string sanitized_data = GenerateJWKSet(keys, session_type);
     sanitized_response->assign(sanitized_data.begin(), sanitized_data.end());
     return true;
   }
 
   // TODO(jrummell): Verify responses for Widevine.
   sanitized_response->assign(response, response + response_length);
   return true;
 }
 
+// If we need to close the session while destroying this object, we need a
+// dummy promise that won't call back into this object (or try to pass
+// something back to blink).
+class IgnoreResponsePromise : public SimpleCdmPromise {

[xhwang, 2016/11/11 18:52:46]

move this to an anonymous namespace, maybe together with all the static
functions above.

[jrummell, 2016/11/11 21:14:23]

Done.

+ public:
+  IgnoreResponsePromise() {}
+  ~IgnoreResponsePromise() override {}
+
+  // SimpleCdmPromise implementation.
+  void resolve() final { MarkPromiseSettled(); }
+  void reject(CdmPromise::Exception exception_code,
+              uint32_t system_code,
+              const std::string& error_message) final {
+    MarkPromiseSettled();
+  }
+};
+
 WebContentDecryptionModuleSessionImpl::WebContentDecryptionModuleSessionImpl(
     const scoped_refptr<CdmSessionAdapter>& adapter)
-    : adapter_(adapter), is_closed_(false), weak_ptr_factory_(this) {
-}
+    : adapter_(adapter),
+      is_closed_(false),
+      has_close_been_called_(false),
+      weak_ptr_factory_(this) {}
 
 WebContentDecryptionModuleSessionImpl::
     ~WebContentDecryptionModuleSessionImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!session_id_.empty())
+
+  if (!session_id_.empty()) {
     adapter_->UnregisterSession(session_id_);
+
+    // From http://w3c.github.io/encrypted-media/#mediakeysession-interface
+    // "If a MediaKeySession object is not closed when it becomes inaccessible
+    // to the page, the CDM shall close the key session associated with the
+    // object."
+    //
+    // This object is destroyed when the corresponding blink object is no
+    // longer needed (which may be due to it becoming inaccessible to the
+    // page), so if the session is not closed and CloseSession() has not yet
+    // been called, call CloseSession() now. Since this object is being
+    // destroyed, there is no need for the promise to do anything as this
+    // session will be gone.
+    if (!is_closed_ && !has_close_been_called_) {

[xhwang, 2016/11/11 18:52:46]

There's a corner case where the CDM may self-close the session but |this| is not
notified yet. CDMs should handle this case. But this seems a general issue and
not related to this CL.

[jrummell, 2016/11/11 21:14:23]

Acknowledged.

+      adapter_->CloseSession(session_id_,
+                             base::MakeUnique<IgnoreResponsePromise>());
+    }
+  }
 }
 
 void WebContentDecryptionModuleSessionImpl::setClientInterface(Client* client) {
   client_ = client;
 }
 
 blink::WebString WebContentDecryptionModuleSessionImpl::sessionId() const {
   return blink::WebString::fromUTF8(session_id_);
 }
 
@@ skipping 139 matching lines @@
 
   adapter_->UpdateSession(
       session_id_, sanitized_response,
       std::unique_ptr<SimpleCdmPromise>(new CdmResultPromise<>(
           result, adapter_->GetKeySystemUMAPrefix() + kUpdateSessionUMAName)));
 }
 
 void WebContentDecryptionModuleSessionImpl::close(
     blink::WebContentDecryptionModuleResult result) {
   DCHECK(!session_id_.empty());
   DCHECK(thread_checker_.CalledOnValidThread());

[xhwang, 2016/11/11 18:52:46]

DCHECK(!has_close_been_called_) since we can't close() twice? Or can we?

Also, can we call close() if |is_closed_| is true?

[jrummell, 2016/11/11 21:14:23]

Done. We shouldn't call close() if |is_closed_| is true, but since close() is
async there is a small window where close() was called just before the closed
event arrives.

+
+  has_close_been_called_ = true;
   adapter_->CloseSession(
       session_id_,
       std::unique_ptr<SimpleCdmPromise>(new CdmResultPromise<>(
           result, adapter_->GetKeySystemUMAPrefix() + kCloseSessionUMAName)));
 }
 
 void WebContentDecryptionModuleSessionImpl::remove(
     blink::WebContentDecryptionModuleResult result) {
   DCHECK(!session_id_.empty());
   DCHECK(thread_checker_.CalledOnValidThread());
@@ skipping 57 matching lines @@
 
   DCHECK(session_id_.empty()) << "Session ID may not be changed once set.";
   session_id_ = session_id;
   *status =
       adapter_->RegisterSession(session_id_, weak_ptr_factory_.GetWeakPtr())
           ? SessionInitStatus::NEW_SESSION
           : SessionInitStatus::SESSION_ALREADY_EXISTS;
 }
 
 }  // namespace media