Call crypto::InitializeTPMToken on the IO thread

chromeos/network/cert_loader.cc

Index: chromeos/network/cert_loader.cc
diff --git a/chromeos/network/cert_loader.cc b/chromeos/network/cert_loader.cc
index f75f8bb959b6f2863e1254336eb9d77bb1ee5bde..a5caa8a0ad020fabb21c21454a6df99f19928928 100644
--- a/chromeos/network/cert_loader.cc
+++ b/chromeos/network/cert_loader.cc
@@ -7,7 +7,9 @@
 #include <algorithm>
 
 #include "base/chromeos/chromeos_version.h"
+#include "base/message_loop/message_loop_proxy.h"
 #include "base/observer_list.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/threading/worker_pool.h"
@@ -46,6 +48,16 @@ void LoadNSSCertificates(net::CertificateList* cert_list) {
     net::NSSCertDatabase::GetInstance()->ListCerts(cert_list);
 }
 
+void CallOpenPersistentNSSDB() {
+  // Called from crypto_task_runner_.
+  VLOG(1) << "CallOpenPersistentNSSDB";
+
+  // Ensure we've opened the user's key/certificate database.
+  crypto::OpenPersistentNSSDB();
+  if (base::chromeos::IsRunningOnChromeOS())
+    crypto::EnableTPMTokenForNSS();
+}
+
 }  // namespace
 
 CertLoader::CertLoader()
@@ -58,6 +70,11 @@ CertLoader::CertLoader()
           base::TimeDelta::FromMilliseconds(kInitialRequestDelayMs)),
       initialize_token_factory_(this),
       update_certificates_factory_(this) {
+}
+
+void CertLoader::Init(
+    const scoped_refptr<base::SequencedTaskRunner>& crypto_task_runner) {
+  crypto_task_runner_ = crypto_task_runner;
   net::CertDatabase::GetInstance()->AddObserver(this);
   if (LoginState::IsInitialized())
     LoginState::Get()->AddObserver(this);
@@ -96,26 +113,36 @@ void CertLoader::RequestCertificates() {
 
   certificates_requested_ = true;
 
-  // Ensure we've opened the user's key/certificate database.
-  crypto::OpenPersistentNSSDB();
-  if (base::chromeos::IsRunningOnChromeOS())
-    crypto::EnableTPMTokenForNSS();
-
-  // This is the entry point to the TPM token initialization process, which we
-  // should do at most once.
-  DCHECK(!initialize_token_factory_.HasWeakPtrs());
+  // This is the entry point to the TPM token initialization process,
+  // which we should do at most once.
+  DCHECK_EQ(tpm_token_state_, TPM_STATE_UNKNOWN);
   InitializeTokenAndLoadCertificates();
 }
 
 void CertLoader::InitializeTokenAndLoadCertificates() {
   CHECK(thread_checker_.CalledOnValidThread());
-  VLOG(1) << "InitializeTokenAndLoadCertificates";
+  VLOG(1) << "InitializeTokenAndLoadCertificates: " << tpm_token_state_;
 
   switch (tpm_token_state_) {
     case TPM_STATE_UNKNOWN: {
+      if (crypto_task_runner_->PostTaskAndReply(
+              FROM_HERE,
+              base::Bind(&CallOpenPersistentNSSDB),
+              base::Bind(&CertLoader::OnPersistentNSSDBOpened,
+                         initialize_token_factory_.GetWeakPtr()))) {
+        return;
+      }
+      // The task was not posted, e.g. because a test has not started the
+      // thread supplied to Init(). Call the crypto methonds on the current

[Ryan Sleevi, 2013/06/07 23:12:43]

typo: s/methonds/methods

+      // thread which should be safe, just not asynchronous.

[Ryan Sleevi, 2013/06/07 23:12:43]

I don't think this should be necessary. Your synchronous tests should just
supply the current TaskRunner.

[stevenjb, 2013/06/08 02:06:51]

I did some investigating and discovered that we were unnecessarily and
incorrectly calling RequestCertificates() from Init(), before the IO thread was
actually loaded. Hopefully fixing that will fix the tests.

+      CallOpenPersistentNSSDB();
+      tpm_token_state_ = TPM_DB_OPENED;
+      // FALL_THROUGH_INTENDED
+    }
+    case TPM_DB_OPENED: {
       DBusThreadManager::Get()->GetCryptohomeClient()->TpmIsEnabled(
-      base::Bind(&CertLoader::OnTpmIsEnabled,
-                 initialize_token_factory_.GetWeakPtr()));
+          base::Bind(&CertLoader::OnTpmIsEnabled,
+                     initialize_token_factory_.GetWeakPtr()));
       return;
     }
     case TPM_DISABLED: {
@@ -138,10 +165,23 @@ void CertLoader::InitializeTokenAndLoadCertificates() {
       return;
     }
     case TPM_TOKEN_INFO_RECEIVED: {
-      InitializeNSSForTPMToken();
-      return;
+      if (base::chromeos::IsRunningOnChromeOS()) {
+        if (base::PostTaskAndReplyWithResult(
+                crypto_task_runner_.get(),
+                FROM_HERE,
+                base::Bind(&crypto::InitializeTPMToken,
+                           tpm_token_name_, tpm_user_pin_),
+                base::Bind(&CertLoader::OnTPMTokenInitialized,
+                           initialize_token_factory_.GetWeakPtr()))) {
+          return;
+        }
+        // No task posted, call InitializeTPMToken directly.
+        crypto::InitializeTPMToken(tpm_token_name_, tpm_user_pin_);

[Ryan Sleevi, 2013/06/07 23:12:43]

Same here

+      }
+      tpm_token_state_ = TPM_TOKEN_INITIALIZED;
+      // FALL_THROUGH_INTENDED
     }
-    case TPM_TOKEN_NSS_INITIALIZED: {
+    case TPM_TOKEN_INITIALIZED: {
       StartLoadCertificates();
       return;
     }
@@ -149,6 +189,7 @@ void CertLoader::InitializeTokenAndLoadCertificates() {
 }
 
 void CertLoader::RetryTokenInitializationLater() {
+  CHECK(thread_checker_.CalledOnValidThread());
   LOG(WARNING) << "Re-Requesting Certificates later.";
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
@@ -158,6 +199,12 @@ void CertLoader::RetryTokenInitializationLater() {
   tpm_request_delay_ = GetNextRequestDelayMs(tpm_request_delay_);
 }
 
+void CertLoader::OnPersistentNSSDBOpened() {
+  VLOG(1) << "PersistentNSSDBOpened";
+  tpm_token_state_ = TPM_DB_OPENED;
+  InitializeTokenAndLoadCertificates();
+}
+
 // For background see this discussion on dev-tech-crypto.lists.mozilla.org:
 // http://web.archiveorange.com/archive/v/6JJW7E40sypfZGtbkzxX
 //
@@ -234,21 +281,19 @@ void CertLoader::OnPkcs11GetTpmTokenInfo(DBusMethodCallStatus call_status,
   InitializeTokenAndLoadCertificates();
 }
 
-void CertLoader::InitializeNSSForTPMToken() {
-  VLOG(1) << "InitializeNSSForTPMToken";
-
-  if (base::chromeos::IsRunningOnChromeOS() &&
-      !crypto::InitializeTPMToken(tpm_token_name_, tpm_user_pin_)) {
+void CertLoader::OnTPMTokenInitialized(bool success) {
+  VLOG(1) << "OnTPMTokenInitialized: " << success;
+  if (!success) {
     RetryTokenInitializationLater();
     return;
   }
-
-  tpm_token_state_ = TPM_TOKEN_NSS_INITIALIZED;
+  tpm_token_state_ = TPM_TOKEN_INITIALIZED;
   InitializeTokenAndLoadCertificates();
 }
 
 void CertLoader::StartLoadCertificates() {
-  VLOG(1) << "StartLoadCertificates";
+  CHECK(thread_checker_.CalledOnValidThread());
+  VLOG(1) << "StartLoadCertificates: " << certificates_update_running_;
 
   if (certificates_update_running_) {
     certificates_update_required_ = true;
@@ -303,7 +348,8 @@ void CertLoader::OnCertRemoved(const net::X509Certificate* cert) {
 
 void CertLoader::LoggedInStateChanged(LoginState::LoggedInState state) {
   VLOG(1) << "LoggedInStateChanged: " << state;
-  RequestCertificates();
+  if (state == LoginState::LOGGED_IN_ACTIVE)
+    RequestCertificates();
 }
 
 }  // namespace chromeos