EME: Make sure sessions are closed before they are destroyed

media/blink/webcontentdecryptionmodulesession_impl.cc

Index: media/blink/webcontentdecryptionmodulesession_impl.cc
diff --git a/media/blink/webcontentdecryptionmodulesession_impl.cc b/media/blink/webcontentdecryptionmodulesession_impl.cc
index ab138e7447d95a9786b94b88895939907fe4ab60..0c431f8cb17f4291076aec3a10b08a21c8c96603 100644
--- a/media/blink/webcontentdecryptionmodulesession_impl.cc
+++ b/media/blink/webcontentdecryptionmodulesession_impl.cc
@@ -7,6 +7,7 @@
 #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"
@@ -224,16 +225,53 @@ static bool SanitizeResponse(const std::string& key_system,
   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) {
@@ -394,6 +432,8 @@ 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<>(