Fix GCMKeyStore interleaved request handling

components/gcm_driver/crypto/gcm_key_store.cc

 // Copyright 2015 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 "components/gcm_driver/crypto/gcm_key_store.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 95 matching lines @@
   EncryptionData encryption_data;
   encryption_data.set_app_id(app_id);
   encryption_data.set_auth_secret(auth_secret);
 
   KeyPair* pair = encryption_data.add_keys();
   pair->set_type(KeyPair::ECDH_P256);
   pair->set_private_key(private_key);
   pair->set_public_key_x509(public_key_x509);
   pair->set_public_key(public_key);
 
+  // Write them immediately to our cache, so subsequent calls to
+  // {Get/Create/Remove}Keys can see them.
+  key_pairs_[app_id] = *pair;
+  auth_secrets_[app_id] = auth_secret;
+
   using EntryVectorType =
       leveldb_proto::ProtoDatabase<EncryptionData>::KeyEntryVector;
 
   std::unique_ptr<EntryVectorType> entries_to_save(new EntryVectorType());
   std::unique_ptr<std::vector<std::string>> keys_to_remove(
       new std::vector<std::string>());
 
   entries_to_save->push_back(std::make_pair(app_id, encryption_data));
 
   database_->UpdateEntries(
       std::move(entries_to_save), std::move(keys_to_remove),
-      base::Bind(&GCMKeyStore::DidStoreKeys, weak_factory_.GetWeakPtr(), app_id,
-                 *pair, auth_secret, callback));
+      base::Bind(&GCMKeyStore::DidStoreKeys, weak_factory_.GetWeakPtr(), *pair,
+                 auth_secret, callback));
 }
 
-void GCMKeyStore::DidStoreKeys(const std::string& app_id,
-                               const KeyPair& pair,
+void GCMKeyStore::DidStoreKeys(const KeyPair& pair,
                                const std::string& auth_secret,
                                const KeysCallback& callback,
                                bool success) {
   UMA_HISTOGRAM_BOOLEAN("GCM.Crypto.CreateKeySuccessRate", success);
-  DCHECK_EQ(0u, key_pairs_.count(app_id));
 
   if (!success) {
-    DVLOG(1) << "Unable to store the created key in the GCM Key Store.";
+    LOG(ERROR) << "Unable to store the created key in the GCM Key Store.";
+    // Our cache is now inconsistent. Reject requests until Chrome is restarted.

[Peter Beverloo, 2016/05/05 15:29:35]

nit (1): Blank line above
nit (2): Do not mention Chrome here please

dito re: line 191

[johnme, 2016/05/05 17:32:46]

Done.

+    state_ = State::FAILED;
     callback.Run(KeyPair(), std::string() /* auth_secret */);
     return;
   }
 
-  key_pairs_[app_id] = pair;
-  auth_secrets_[app_id] = auth_secret;
-
-  callback.Run(key_pairs_[app_id], auth_secret);
+  callback.Run(pair, auth_secret);
 }
 
 void GCMKeyStore::RemoveKeys(const std::string& app_id,
                              const base::Closure& callback) {
   LazyInitialize(base::Bind(&GCMKeyStore::RemoveKeysAfterInitialize,
                             weak_factory_.GetWeakPtr(), app_id, callback));
 }
 
 void GCMKeyStore::RemoveKeysAfterInitialize(const std::string& app_id,
                                             const base::Closure& callback) {
   DCHECK(state_ == State::INITIALIZED || state_ == State::FAILED);
   const auto iter = key_pairs_.find(app_id);
   if (iter == key_pairs_.end() || state_ != State::INITIALIZED) {
     callback.Run();
     return;
   }
 
+  // Clear them immediately from our cache, so subsequent calls to
+  // {Get/Create/Remove}Keys don't see them.
+  key_pairs_.erase(app_id);
+  auth_secrets_.erase(app_id);
+
   using EntryVectorType =
       leveldb_proto::ProtoDatabase<EncryptionData>::KeyEntryVector;
 
   std::unique_ptr<EntryVectorType> entries_to_save(new EntryVectorType());
   std::unique_ptr<std::vector<std::string>> keys_to_remove(
       new std::vector<std::string>(1, app_id));
 
-  database_->UpdateEntries(
-      std::move(entries_to_save), std::move(keys_to_remove),
-      base::Bind(&GCMKeyStore::DidRemoveKeys, weak_factory_.GetWeakPtr(),
-                 app_id, callback));
+  database_->UpdateEntries(std::move(entries_to_save),
+                           std::move(keys_to_remove),
+                           base::Bind(&GCMKeyStore::DidRemoveKeys,
+                                      weak_factory_.GetWeakPtr(), callback));
 }
 
-void GCMKeyStore::DidRemoveKeys(const std::string& app_id,
-                                const base::Closure& callback,
-                                bool success) {
+void GCMKeyStore::DidRemoveKeys(const base::Closure& callback, bool success) {
   UMA_HISTOGRAM_BOOLEAN("GCM.Crypto.RemoveKeySuccessRate", success);
 
-  if (success) {
-    key_pairs_.erase(app_id);
-    auth_secrets_.erase(app_id);
-  } else {
-    DVLOG(1) << "Unable to delete a key from the GCM Key Store.";
+  if (!success) {
+    LOG(ERROR) << "Unable to delete a key from the GCM Key Store.";
+    // Our cache is now inconsistent. Reject requests until Chrome is restarted.
+    state_ = State::FAILED;
   }
 
   callback.Run();
 }
 
 void GCMKeyStore::LazyInitialize(const base::Closure& done_closure) {
   if (delayed_task_controller_.CanRunTaskWithoutDelay()) {
     done_closure.Run();
     return;
   }
@@ skipping 44 matching lines @@
     key_pairs_[entry.app_id()] = entry.keys(0);
     auth_secrets_[entry.app_id()] = entry.auth_secret();
   }
 
   state_ = State::INITIALIZED;
 
   delayed_task_controller_.SetReady();
 }
 
 }  // namespace gcm