chromeos: Remove SystemSaltGetter::GetSystemSaltSync

Make CryptohomeClient::GetSystemSalt asynchronous

BUG=141009

Review URL: https://codereview.chromium.org/43503003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@231286 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 85 insertions, 63 deletions

diff --git a/chromeos/cryptohome/system_salt_getter.cc b/chromeos/cryptohome/system_salt_getter.cc
index 1fa25a3..2a26db7 100644
--- a/chromeos/cryptohome/system_salt_getter.cc
+++ b/chromeos/cryptohome/system_salt_getter.cc
@@ -27,35 +27,43 @@ SystemSaltGetter::~SystemSaltGetter() {
 
 void SystemSaltGetter::GetSystemSalt(
     const GetSystemSaltCallback& callback) {
+  if (!system_salt_.empty()) {
+    base::MessageLoopProxy::current()->PostTask(
+        FROM_HERE, base::Bind(callback, system_salt_));
+    return;
+  }
+
   DBusThreadManager::Get()->GetCryptohomeClient()->WaitForServiceToBeAvailable(
-      base::Bind(&SystemSaltGetter::GetSystemSaltInternal,
+      base::Bind(&SystemSaltGetter::DidWaitForServiceToBeAvailable,
                  weak_ptr_factory_.GetWeakPtr(),
                  callback));
 }
 
-std::string SystemSaltGetter::GetSystemSaltSync() {
-  LoadSystemSalt();  // no-op if it's already loaded.
-  return system_salt_;
-}
-
-void SystemSaltGetter::GetSystemSaltInternal(
+void SystemSaltGetter::DidWaitForServiceToBeAvailable(
     const GetSystemSaltCallback& callback,
     bool service_is_available) {
-  LOG_IF(ERROR, !service_is_available) << "WaitForServiceToBeAvailable failed.";
-  // TODO(hashimoto): Stop using GetSystemSaltSync(). crbug.com/141009
-  callback.Run(GetSystemSaltSync());
+  if (!service_is_available) {
+    LOG(ERROR) << "WaitForServiceToBeAvailable failed.";
+    callback.Run(std::string());
+    return;
+  }
+  DBusThreadManager::Get()->GetCryptohomeClient()->GetSystemSalt(
+      base::Bind(&SystemSaltGetter::DidGetSystemSalt,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback));
 }
 
-void SystemSaltGetter::LoadSystemSalt() {
-  if (!system_salt_.empty())
-    return;
-  std::vector<uint8> salt;
-  DBusThreadManager::Get()->GetCryptohomeClient()->GetSystemSalt(&salt);
-  if (salt.empty() || salt.size() % 2 != 0U) {
+void SystemSaltGetter::DidGetSystemSalt(const GetSystemSaltCallback& callback,
+                                        DBusMethodCallStatus call_status,
+                                        const std::vector<uint8>& system_salt) {
+  if (call_status == DBUS_METHOD_CALL_SUCCESS &&
+      !system_salt.empty() &&
+      system_salt.size() % 2 == 0U)
+    system_salt_ = ConvertRawSaltToHexString(system_salt);
+  else
     LOG(WARNING) << "System salt not available";
-    return;
-  }
-  system_salt_ = ConvertRawSaltToHexString(salt);
+
+  callback.Run(system_salt_);
 }
 
 // static
diff --git a/chromeos/cryptohome/system_salt_getter.h b/chromeos/cryptohome/system_salt_getter.h
index 1e4cf81..ee33133 100644
--- a/chromeos/cryptohome/system_salt_getter.h
+++ b/chromeos/cryptohome/system_salt_getter.h
@@ -12,6 +12,7 @@
 #include "base/callback_forward.h"
 #include "base/memory/weak_ptr.h"
 #include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
 
 namespace chromeos {
 
@@ -31,26 +32,20 @@ class CHROMEOS_EXPORT SystemSaltGetter {
   static std::string ConvertRawSaltToHexString(const std::vector<uint8>& salt);
 
   // Returns system hash in hex encoded ascii format. Note: this may return
-  // an empty string (e.g. if cryptohome is not running). It is up to the
-  // calling function to try again after a delay if desired.
+  // an empty string (e.g. errors in D-Bus layer)
   void GetSystemSalt(const GetSystemSaltCallback& callback);
 
-  // Synchronous version of GetSystemSalt().
-  // Blocks the UI thread until the Cryptohome service returns the result.
-  // DEPRECATED: DO NOT USE.
-  std::string GetSystemSaltSync();
-
  protected:
   SystemSaltGetter();
   ~SystemSaltGetter();
 
  private:
   // Used to implement GetSystemSalt().
-  void GetSystemSaltInternal(const GetSystemSaltCallback& callback,
-                             bool service_is_available);
-
-  // Loads the system salt from cryptohome and caches it.
-  void LoadSystemSalt();
+  void DidWaitForServiceToBeAvailable(const GetSystemSaltCallback& callback,
+                                      bool service_is_available);
+  void DidGetSystemSalt(const GetSystemSaltCallback& callback,
+                        DBusMethodCallStatus call_status,
+                        const std::vector<uint8>& system_salt);
 
   std::string system_salt_;
 
diff --git a/chromeos/cryptohome/system_salt_getter_unittest.cc b/chromeos/cryptohome/system_salt_getter_unittest.cc
index 84403e4..07883a0 100644
--- a/chromeos/cryptohome/system_salt_getter_unittest.cc
+++ b/chromeos/cryptohome/system_salt_getter_unittest.cc
@@ -55,6 +55,7 @@ TEST_F(SystemSaltGetterTest, GetSystemSalt) {
 
   // Service becomes available.
   fake_cryptohome_client_->SetServiceIsAvailable(true);
+  base::RunLoop().RunUntilIdle();
   const std::string expected_system_salt =
       SystemSaltGetter::ConvertRawSaltToHexString(
           FakeCryptohomeClient::GetStubSystemSalt());
diff --git a/chromeos/dbus/cryptohome_client.cc b/chromeos/dbus/cryptohome_client.cc
index ca2d48a..77848d3 100644
--- a/chromeos/dbus/cryptohome_client.cc
+++ b/chromeos/dbus/cryptohome_client.cc
@@ -110,20 +110,13 @@ class CryptohomeClientImpl : public CryptohomeClient {
   }
 
   // CryptohomeClient override.
-  virtual bool GetSystemSalt(std::vector<uint8>* salt) OVERRIDE {
+  virtual void GetSystemSalt(const GetSystemSaltCallback& callback) OVERRIDE {
     dbus::MethodCall method_call(cryptohome::kCryptohomeInterface,
                                  cryptohome::kCryptohomeGetSystemSalt);
-    scoped_ptr<dbus::Response> response(
-        blocking_method_caller_->CallMethodAndBlock(&method_call));
-    if (!response.get())
-      return false;
-    dbus::MessageReader reader(response.get());
-    uint8* bytes = NULL;
-    size_t length = 0;
-    if (!reader.PopArrayOfBytes(&bytes, &length))
-      return false;
-    salt->assign(bytes, bytes + length);
-    return true;
+    proxy_->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                       base::Bind(&CryptohomeClientImpl::OnGetSystemSalt,
+                                  weak_ptr_factory_.GetWeakPtr(),
+                                  callback));
   }
 
   // CryptohomeClient override,
@@ -720,6 +713,24 @@ class CryptohomeClientImpl : public CryptohomeClient {
     callback.Run(async_id);
   }
 
+  // Handles the result of GetSystemSalt().
+  void OnGetSystemSalt(const GetSystemSaltCallback& callback,
+                       dbus::Response* response) {
+    if (!response) {
+      callback.Run(DBUS_METHOD_CALL_FAILURE, std::vector<uint8>());
+      return;
+    }
+    dbus::MessageReader reader(response);
+    uint8* bytes = NULL;
+    size_t length = 0;
+    if (!reader.PopArrayOfBytes(&bytes, &length)) {
+      callback.Run(DBUS_METHOD_CALL_FAILURE, std::vector<uint8>());
+      return;
+    }
+    callback.Run(DBUS_METHOD_CALL_SUCCESS,
+                 std::vector<uint8>(bytes, bytes + length));
+  }
+
   // Calls a method without result values.
   void CallVoidMethod(dbus::MethodCall* method_call,
                       const VoidDBusMethodCallback& callback) {
diff --git a/chromeos/dbus/cryptohome_client.h b/chromeos/dbus/cryptohome_client.h
index 036cf64..87457a7 100644
--- a/chromeos/dbus/cryptohome_client.h
+++ b/chromeos/dbus/cryptohome_client.h
@@ -35,6 +35,10 @@ class CHROMEOS_EXPORT CryptohomeClient : public DBusClient {
       AsyncCallStatusWithDataHandler;
   // A callback to handle responses of AsyncXXX methods.
   typedef base::Callback<void(int async_id)> AsyncMethodCallback;
+  // A callback for GetSystemSalt().
+  typedef base::Callback<void(
+      DBusMethodCallStatus call_status,
+      const std::vector<uint8>& system_salt)> GetSystemSaltCallback;
   // A callback for WaitForServiceToBeAvailable().
   typedef base::Callback<void(bool service_is_ready)>
       WaitForServiceToBeAvailableCallback;
@@ -105,9 +109,9 @@ class CHROMEOS_EXPORT CryptohomeClient : public DBusClient {
   virtual void AsyncRemove(const std::string& username,
                            const AsyncMethodCallback& callback) = 0;
 
-  // Calls GetSystemSalt method.  This method blocks until the call returns.
-  // The original content of |salt| is lost.
-  virtual bool GetSystemSalt(std::vector<uint8>* salt) = 0;
+  // Calls GetSystemSalt method.  |callback| is called after the method call
+  // succeeds.
+  virtual void GetSystemSalt(const GetSystemSaltCallback& callback) = 0;
 
   // Calls GetSanitizedUsername method.  |callback| is called after the method
   // call succeeds.
diff --git a/chromeos/dbus/fake_cryptohome_client.cc b/chromeos/dbus/fake_cryptohome_client.cc
index 861a1c5..50ae793 100644
--- a/chromeos/dbus/fake_cryptohome_client.cc
+++ b/chromeos/dbus/fake_cryptohome_client.cc
@@ -80,9 +80,11 @@ void FakeCryptohomeClient::AsyncRemove(
   ReturnAsyncMethodResult(callback, false);
 }
 
-bool FakeCryptohomeClient::GetSystemSalt(std::vector<uint8>* salt) {
-  *salt = system_salt_;
-  return true;
+void FakeCryptohomeClient::GetSystemSalt(
+    const GetSystemSaltCallback& callback) {
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(callback, DBUS_METHOD_CALL_SUCCESS, system_salt_));
 }
 
 void FakeCryptohomeClient::GetSanitizedUsername(
diff --git a/chromeos/dbus/fake_cryptohome_client.h b/chromeos/dbus/fake_cryptohome_client.h
index 450678b..fd482b9 100644
--- a/chromeos/dbus/fake_cryptohome_client.h
+++ b/chromeos/dbus/fake_cryptohome_client.h
@@ -36,7 +36,7 @@ class CHROMEOS_EXPORT FakeCryptohomeClient : public CryptohomeClient {
                                const AsyncMethodCallback& callback) OVERRIDE;
   virtual void AsyncRemove(const std::string& username,
                            const AsyncMethodCallback& callback) OVERRIDE;
-  virtual bool GetSystemSalt(std::vector<uint8>* salt) OVERRIDE;
+  virtual void GetSystemSalt(const GetSystemSaltCallback& callback) OVERRIDE;
   virtual void GetSanitizedUsername(
       const std::string& username,
       const StringDBusMethodCallback& callback) OVERRIDE;
diff --git a/chromeos/dbus/mock_cryptohome_client.h b/chromeos/dbus/mock_cryptohome_client.h
index cdb1ad8..c8232da 100644
--- a/chromeos/dbus/mock_cryptohome_client.h
+++ b/chromeos/dbus/mock_cryptohome_client.h
@@ -37,7 +37,7 @@ class MockCryptohomeClient : public CryptohomeClient {
                     const AsyncMethodCallback& callback));
   MOCK_METHOD2(AsyncRemove, void(const std::string& username,
                                  const AsyncMethodCallback& callback));
-  MOCK_METHOD1(GetSystemSalt, bool(std::vector<uint8>* salt));
+  MOCK_METHOD1(GetSystemSalt, void(const GetSystemSaltCallback& callback));
   MOCK_METHOD2(GetSanitizedUsername,
                void(const std::string& username,
                     const StringDBusMethodCallback& callback));
diff --git a/chromeos/dbus/mock_dbus_thread_manager.cc b/chromeos/dbus/mock_dbus_thread_manager.cc
index 8183d3e..95901b15 100644
--- a/chromeos/dbus/mock_dbus_thread_manager.cc
+++ b/chromeos/dbus/mock_dbus_thread_manager.cc
@@ -4,6 +4,7 @@
 
 #include "chromeos/dbus/mock_dbus_thread_manager.h"
 
+#include "base/message_loop/message_loop.h"
 #include "chromeos/dbus/dbus_thread_manager_observer.h"
 #include "chromeos/dbus/fake_bluetooth_adapter_client.h"
 #include "chromeos/dbus/fake_bluetooth_agent_manager_client.h"
@@ -26,24 +27,25 @@
 #include "chromeos/dbus/power_policy_controller.h"
 
 using ::testing::AnyNumber;
+using ::testing::Invoke;
 using ::testing::Return;
 using ::testing::ReturnNull;
-using ::testing::SetArgumentPointee;
 using ::testing::_;
 
 namespace chromeos {
 
 namespace {
 
-std::vector<uint8>* GetMockSystemSalt() {
-  static std::vector<uint8>* s_system_salt = NULL;
-  if (!s_system_salt) {
-    const char kStubSystemSalt[] = "stub_system_salt";
-    s_system_salt = new std::vector<uint8>();
-    s_system_salt->assign(kStubSystemSalt,
-                          kStubSystemSalt + arraysize(kStubSystemSalt) - 1);
-  }
-  return s_system_salt;
+void GetMockSystemSalt(
+    const CryptohomeClient::GetSystemSaltCallback& callback) {
+  const char kStubSystemSalt[] = "stub_system_salt";
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(callback,
+                 DBUS_METHOD_CALL_SUCCESS,
+                 std::vector<uint8>(
+                     kStubSystemSalt,
+                     kStubSystemSalt + arraysize(kStubSystemSalt) - 1)));
 }
 
 }  // namespace
@@ -117,8 +119,7 @@ MockDBusThreadManager::MockDBusThreadManager()
       .Times(AnyNumber());
   // Called from various locations.
   EXPECT_CALL(*mock_cryptohome_client_.get(), GetSystemSalt(_))
-      .WillRepeatedly(DoAll(SetArgumentPointee<0>(*GetMockSystemSalt()),
-                            Return(true)));
+      .WillRepeatedly(Invoke(&GetMockSystemSalt));
   EXPECT_CALL(*mock_cryptohome_client_.get(), TpmIsEnabled(_))
       .Times(AnyNumber());