settings: Add async system salt retrieval logic in DeviceOAuth2TokenServiceFactory

chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc

Index: chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
index 5f7fb306267a4372e837ea45a33e6e37620349e6..a7d7ee69887c0b14b299afd2ef087fa9b950b071 100644
--- a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
+++ b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc
@@ -4,11 +4,14 @@
 
 #include "chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h"
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/message_loop/message_loop.h"
 #include "base/tracked_objects.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/settings/device_oauth2_token_service.h"
 #include "chrome/browser/chromeos/settings/token_encryptor.h"
+#include "chromeos/cryptohome/system_salt_getter.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace chromeos {
@@ -19,10 +22,8 @@ DeviceOAuth2TokenServiceFactory* g_factory = NULL;
 }  // namespace
 
 DeviceOAuth2TokenServiceFactory::DeviceOAuth2TokenServiceFactory()
-    : token_service_(new DeviceOAuth2TokenService(
-        g_browser_process->system_request_context(),
-        g_browser_process->local_state(),
-        new CryptohomeTokenEncryptor)) {
+    : token_service_(NULL),
+      weak_ptr_factory_(this) {
 }
 
 DeviceOAuth2TokenServiceFactory::~DeviceOAuth2TokenServiceFactory() {
@@ -33,30 +34,15 @@ DeviceOAuth2TokenServiceFactory::~DeviceOAuth2TokenServiceFactory() {
 void DeviceOAuth2TokenServiceFactory::Get(const GetCallback& callback) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  DeviceOAuth2TokenService* token_service = NULL;
-  if (g_factory)
-    token_service = g_factory->token_service_;
-
-  // TODO(satorux): Implement async initialization logic for
-  // DeviceOAuth2TokenService. crbug.com/309959.
-  // Here's how that should work:
-  //
-  // if token_service is ready:
-  //   run callback asynchronously via MessageLoop
-  //   return
-  //
-  // add callback to the pending callback list
-  //
-  // if there is only one pending callback:
-  //   start getting the system salt asynchronously...
-  //
-  // upon receiving the system salt:
-  //   create CryptohomeTokenEncryptor with that key
-  //   create DeviceOAuth2TokenService
-  //   run all the pending callbacks
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(callback, token_service));
+  if (!g_factory) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback,
+                   static_cast<DeviceOAuth2TokenService*>(NULL)));
+    return;
+  }
+
+  g_factory->RunAsync(callback);
 }
 
 // static
@@ -65,6 +51,7 @@ void DeviceOAuth2TokenServiceFactory::Initialize() {
 
   DCHECK(!g_factory);
   g_factory = new DeviceOAuth2TokenServiceFactory;
+  g_factory->CreateTokenService();
 }
 
 // static
@@ -77,4 +64,41 @@ void DeviceOAuth2TokenServiceFactory::Shutdown() {
   }
 }
 
+void DeviceOAuth2TokenServiceFactory::CreateTokenService() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  SystemSaltGetter::Get()->GetSystemSalt(
+      base::Bind(&DeviceOAuth2TokenServiceFactory::DidGetSystemSalt,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DeviceOAuth2TokenServiceFactory::DidGetSystemSalt(
+    const std::string& system_salt) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(!token_service_);

[hashimoto, 2013/10/24 06:50:00]

Can we handle system_salt.empty() case or emit an error log when it happens?

[satorux1, 2013/10/24 07:33:08]

If it's empty CryptohomeTokenEncryptor won't work correctly hence
DeviceOAuth2TokenService also won't work correctly either. I guess it's better
to return NULL to callbacks rather than returning a broken
DeviceOAuth2TokenService. Changed the code and added tests accordingly.

+
+  token_service_= new DeviceOAuth2TokenService(
+      g_browser_process->system_request_context(),
+      g_browser_process->local_state(),
+      new CryptohomeTokenEncryptor(system_salt));
+
+  for (size_t i = 0; i < pending_callbacks_.size(); ++i)
+    pending_callbacks_[i].Run(token_service_);
+  pending_callbacks_.clear();
+}
+
+void DeviceOAuth2TokenServiceFactory::RunAsync(const GetCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (token_service_) {
+    DCHECK(pending_callbacks_.empty());

[hashimoto, 2013/10/24 06:50:00]

Does this DCHECK mean pending_callbacks_[i].Run should not result in calling
DeviceOAuth2TokenServiceFactory::Get()?

[satorux1, 2013/10/24 07:33:08]

This is to check no pending callbacks are left (they should be run in
DidGetSystemSalt(). Added some comment.

[hashimoto, 2013/10/24 07:49:01]

Sorry for being unclear, what I worried about was a case where one of
pending_callbacks_ eventually calls DeviceOAuth2TokenServiceFactory::Get().
In patch set #2, this results in hitting DCHECK(pending_callbakcs_.empty())
failure.
In the newest patch set, this may result in a crash because
pending_callbacks_.push_back(callback) can destruct the running callback
(pending_callbakcs_[i] running in DidGetSystemSalt()).

[satorux1, 2013/10/24 08:31:28]

Ah that's a very tricky case. I changed the loop to post tasks rather than run
callbacks immediately

+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, token_service_));
+    return;
+  }
+
+  pending_callbacks_.push_back(callback);
+}
+
 }  // namespace chromeos