Clear Key CDM should resolve the promise if session not found

media/cdm/proxy_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/proxy_decryptor.h"
 
 #include <cstring>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 115 matching lines @@
     pending_requests_.push_back(
         new PendingGenerateKeyRequestData(init_data_type, init_data_vector));
     return;
   }
 
   GenerateKeyRequestInternal(init_data_type, init_data_vector);
 }
 
 // Returns true if |data| is prefixed with |header| and has data after the
 // |header|.
-bool HasHeader(const std::vector<uint8>& data, const std::string& header) {
+static bool HasHeader(const std::vector<uint8>& data,
+                      const std::string& header) {
   return data.size() > header.size() &&
          std::equal(header.begin(), header.end(), data.begin());
 }
 
 // Removes the first |length| items from |data|.
-void StripHeader(std::vector<uint8>& data, size_t length) {
+static void StripHeader(std::vector<uint8>& data, size_t length) {
   data.erase(data.begin(), data.begin() + length);
 }
 
 void ProxyDecryptor::GenerateKeyRequestInternal(
     EmeInitDataType init_data_type,
     const std::vector<uint8>& init_data) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(!is_creating_cdm_);
 
   if (!media_keys_) {
     OnLegacySessionError(std::string(), MediaKeys::NOT_SUPPORTED_ERROR, 0,
                          "CDM creation failed.");
     return;
   }
 
   const char kPrefixedApiPersistentSessionHeader[] = "PERSISTENT|";
   const char kPrefixedApiLoadSessionHeader[] = "LOAD_SESSION|";
 
   SessionCreationType session_creation_type = TemporarySession;
   std::vector<uint8> stripped_init_data = init_data;
+  std::string possible_session_id;
   if (HasHeader(init_data, kPrefixedApiLoadSessionHeader)) {
     session_creation_type = LoadSession;
     StripHeader(stripped_init_data, strlen(kPrefixedApiLoadSessionHeader));
+    possible_session_id.assign(stripped_init_data.begin(),
+                               stripped_init_data.end());
   } else if (HasHeader(init_data, kPrefixedApiPersistentSessionHeader)) {
     session_creation_type = PersistentSession;
     StripHeader(stripped_init_data,
                 strlen(kPrefixedApiPersistentSessionHeader));
   }
 
   scoped_ptr<NewSessionCdmPromise> promise(new CdmCallbackPromise<std::string>(
       base::Bind(&ProxyDecryptor::SetSessionId, weak_ptr_factory_.GetWeakPtr(),
-                 session_creation_type),
+                 session_creation_type, possible_session_id),
       base::Bind(&ProxyDecryptor::OnLegacySessionError,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 std::string())));  // No session id until created.
+                 weak_ptr_factory_.GetWeakPtr(), possible_session_id)));

[ddorwin, 2015/07/08 21:50:13]

Correction: _This_ appears to be a change in behavior - reporting the requested
session ID instead of an empty string in the load case.

This callback should only apply to real errors and not the session-not-found
case, right?

[jrummell, 2015/07/08 23:15:08]

Correct. Reverted.

 
   if (session_creation_type == LoadSession) {
-    media_keys_->LoadSession(
-        MediaKeys::PERSISTENT_LICENSE_SESSION,
-        std::string(
-            reinterpret_cast<const char*>(vector_as_array(&stripped_init_data)),
-            stripped_init_data.size()),
-        promise.Pass());
+    media_keys_->LoadSession(MediaKeys::PERSISTENT_LICENSE_SESSION,
+                             possible_session_id, promise.Pass());

[ddorwin, 2015/07/08 20:32:13]

This appears to be a change in behavior.

[jrummell, 2015/07/08 20:58:17]

It shouldn't be. 2nd parameter was a std::string of stripped_init_data, and
possible_session_id is set to that on line 168 above. It's only set when
session_creation_type is set to LoadSession, and so it should be the same.

Passing it in the promise above could be considered a change in behavior, since
the session_id will get included in some errors from LoadSession() where it was
"" before, but I don't think that is an issue.

I could change line 168 to use the char* + size parameters if that would be
clearer. WDYT?

[ddorwin, 2015/07/08 21:50:13]

Oops, I commented on the wrong line. See my comment on the promise above (as you
suspected).

[jrummell, 2015/07/08 23:15:07]

Acknowledged.

     return;
   }
 
   MediaKeys::SessionType session_type =
       session_creation_type == PersistentSession
           ? MediaKeys::PERSISTENT_LICENSE_SESSION
           : MediaKeys::TEMPORARY_SESSION;
 
   // No permission required when AesDecryptor is used or when the key system is
   // external clear key.
@@ skipping 187 matching lines @@
     default:
       // This will include all other CDM4 errors and any error generated
       // by CDM5 or later.
       error_code = MediaKeys::kUnknownError;
       break;
   }
   key_error_cb_.Run(session_id, error_code, system_code);
 }
 
 void ProxyDecryptor::SetSessionId(SessionCreationType session_type,
+                                  const std::string& possible_session_id,

[ddorwin, 2015/07/08 20:32:13]

If keeping this, we should rename to requested_ or something like that.

                                   const std::string& session_id) {
+  // Load() returns empty |session_id| if the session is not found, so
+  // convert this into an error. |possible_session_id| is the session ID
+  // passed to load().
+  if (session_type == LoadSession && session_id.empty()) {
+    key_error_cb_.Run(possible_session_id, MediaKeys::kUnknownError, 0);

[ddorwin, 2015/07/08 20:32:13]

If the above is a change in behavior, perhaps we can avoid this mess by also
using string().

[jrummell, 2015/07/08 20:58:17]

Looking at this part again this is a change in behavior. Previously a failed
promise would have called OnLegacySessionError with session_id empty. This
"improves" things by including the actual session_id. I can revert it if you'd
prefer that it doesn't change.

[ddorwin, 2015/07/08 21:50:13]

Since we're going to disable this very soon, I don't think we should change
behavior, even if it might be an "improvement."

[jrummell, 2015/07/08 23:15:08]

Done.

+    return;
+  }
+
   // Loaded sessions are considered persistent.
   bool is_persistent =
       session_type == PersistentSession || session_type == LoadSession;
   active_sessions_.insert(std::make_pair(session_id, is_persistent));
 
   // For LoadSession(), generate the KeyAdded event.
   if (session_type == LoadSession)
     GenerateKeyAdded(session_id);
 }
 
 }  // namespace media