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..66cfab6cbb3edbdb744782e5a688e23917ed8d57 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,9 @@ DeviceOAuth2TokenServiceFactory* g_factory = NULL;
 }  // namespace
 
 DeviceOAuth2TokenServiceFactory::DeviceOAuth2TokenServiceFactory()
-    : token_service_(new DeviceOAuth2TokenService(
-        g_browser_process->system_request_context(),
-        g_browser_process->local_state(),
-        new CryptohomeTokenEncryptor)) {
+    : initialized_(false),
+      token_service_(NULL),
+      weak_ptr_factory_(this) {
 }
 
 DeviceOAuth2TokenServiceFactory::~DeviceOAuth2TokenServiceFactory() {
@@ -33,30 +35,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 +52,7 @@ void DeviceOAuth2TokenServiceFactory::Initialize() {
 
   DCHECK(!g_factory);
   g_factory = new DeviceOAuth2TokenServiceFactory;
+  g_factory->CreateTokenService();
 }
 
 // static
@@ -77,4 +65,56 @@ 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_);
+
+  if (system_salt.empty()) {
+    LOG(ERROR) << "Failed to get the system salt";
+  } else {
+    token_service_= new DeviceOAuth2TokenService(
+        g_browser_process->system_request_context(),
+        g_browser_process->local_state(),
+        new CryptohomeTokenEncryptor(system_salt));
+  }
+
+  // Run callbacks regardless of whether token_service_ is created or not,
+  // but don't run callbacks immediately. This is tricky, but running
+  // callbacks directly here can introduce a subtle bug, as these callbacks
+  // may call Get(), which in turn appends new callbacks to
+  // pending_callbacks_, while the vector is being accessed here.
+  for (size_t i = 0; i < pending_callbacks_.size(); ++i) {

[pastarmovj, 2013/10/24 09:43:20]

You can create a local empty vector here and swap() it against
pending_callbacks_ then you will achieve "atomicity" of the vector regardless of
what happens inside the callbacks or am I missing something here?

[satorux1, 2013/10/24 10:23:14]

I considered, but it's still complicated, as we'll clear() the vector after the
loop.

[satorux1, 2013/10/24 10:53:01]

oops. I misunderstood your comment. If we are to swap() before the loop,
pending_callbacks_ is empty when it's swapped, but the new callbacks will be
added to pending_callbacks_, if Get() is called from Get(). Then, the callbacks
in pending_callbacks_ won't be run.

[pastarmovj, 2013/10/24 11:28:49]

How are they going to be run with the current setup?

[satorux1, 2013/10/24 13:15:52]

New callbacks are posted to MessageLoop in RunAsync(), rather than queued in
peding_callbacks_, because initialize_ is true, by the time they are added.

BTW, each callback would trigger an interesting action, so running these
callbacks consecutively may be expensive and potentially  dangerous, so I think
it's nicer to post tasks here, rather than run them consecutively.

[pastarmovj, 2013/10/24 13:21:30]

This makes sense. Thanks for clarifying this! :)

[satorux1, 2013/10/24 13:22:00]

Let me drop "by the time they are added"...

[pneubeck (no reviews), 2013/10/24 19:44:13]

nit: use vector<>::const_iterator instead of an index.

[satorux1, 2013/10/25 02:36:02]

IMHO that's verbose for little benefit. :) but I changed this to std::queue
that's semantically more sound, and don't have the subtle issues which could be
caused by std::vector (i.e. with std:queue, it's safer to run callbacks
immediately here, if we want).

+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(pending_callbacks_[i], token_service_));
+  }
+  pending_callbacks_.clear();
+  // Also mark that the factory is initialized.
+  initialized_ = true;

[pneubeck (no reviews), 2013/10/24 19:44:13]

you could move this "initialized_=true" to the more logical place directly
behind "token_service_ =.." above
and instead of duplicating the PostTask
"base::MessageLoop::current()->PostTask(...", just call
"RunAsync(pending_callbacks_[i])".

If you keep the "DCHECK(pending_callbacks_.empty());" below, then you have to
add a local vector and swap. But I think, you can as well remove that check.

[satorux1, 2013/10/25 02:36:02]

Thank you for the suggestion. Moved initialized=true and removed DCHECK below. I
didn't change it to "RunAsync(pending_callbacks_[i])" as the extra call stack
was a bit hard to follow. :)

+}
+
+void DeviceOAuth2TokenServiceFactory::RunAsync(const GetCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (initialized_) {
+    // No pending callbacks should be left.
+    DCHECK(pending_callbacks_.empty());
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, token_service_));
+    return;
+  }
+
+  pending_callbacks_.push_back(callback);
+}
+
 }  // namespace chromeos