Clear Key CDM should resolve the promise if session not found

media/cdm/proxy_decryptor.cc

Index: media/cdm/proxy_decryptor.cc
diff --git a/media/cdm/proxy_decryptor.cc b/media/cdm/proxy_decryptor.cc
index 28143f9fba16e26f7b58ff0792d254f1b3525bb6..bbae6af934ea5b74f28f71b0e2cbc689ee1805dd 100644
--- a/media/cdm/proxy_decryptor.cc
+++ b/media/cdm/proxy_decryptor.cc
@@ -133,13 +133,14 @@ void ProxyDecryptor::GenerateKeyRequest(EmeInitDataType init_data_type,
 
 // 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);
 }
 
@@ -160,9 +161,12 @@ void ProxyDecryptor::GenerateKeyRequestInternal(
 
   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,
@@ -171,18 +175,13 @@ void ProxyDecryptor::GenerateKeyRequestInternal(
 
   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;
   }
 
@@ -390,7 +389,16 @@ void ProxyDecryptor::OnLegacySessionError(const std::string& session_id,
 }
 
 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;