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..0847d0deb1e43d4a96e6759024378e20c741afaa 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"
@@ -58,6 +60,12 @@ 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) {
+  main_task_runner_ = base::MessageLoopProxy::current();
+  crypto_task_runner_ = crypto_task_runner;
   net::CertDatabase::GetInstance()->AddObserver(this);
   if (LoginState::IsInitialized())
     LoginState::Get()->AddObserver(this);
@@ -96,15 +104,27 @@ void CertLoader::RequestCertificates() {
 
   certificates_requested_ = true;
 
+  crypto_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&CertLoader::CallOpenPersistentNSSDB,
+                 initialize_token_factory_.GetWeakPtr()));
+}
+
+void CertLoader::CallOpenPersistentNSSDB() {
+  VLOG(1) << "CallOpenPersistentNSSDB";

[Ryan Sleevi, 2013/06/06 23:03:41]

Necessary?

[stevenjb, 2013/06/07 02:37:47]

This can be very helpful when debugging hardware that is failing for some
reason.

+
   // 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());
-  InitializeTokenAndLoadCertificates();
+  // This is the entry point to the TPM token initialization process,
+  // which we should do at most once.
+  DCHECK(tpm_token_state_ == TPM_STATE_UNKNOWN);

[Ryan Sleevi, 2013/06/06 23:03:41]

DCHECK_EQ

[stevenjb, 2013/06/07 02:37:47]

Done.

+  main_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&CertLoader::InitializeTokenAndLoadCertificates,
+                 initialize_token_factory_.GetWeakPtr()));
 }
 
 void CertLoader::InitializeTokenAndLoadCertificates() {
@@ -138,6 +158,17 @@ void CertLoader::InitializeTokenAndLoadCertificates() {
       return;
     }
     case TPM_TOKEN_INFO_RECEIVED: {
+      if (base::chromeos::IsRunningOnChromeOS()) {
+        crypto_task_runner_->PostTask(
+            FROM_HERE,
+            base::Bind(&CertLoader::CallInitializeTPMToken,
+                       initialize_token_factory_.GetWeakPtr()));
+        return;
+      }
+      tpm_token_state_ = TPM_TOKEN_INITIALIZED;
+      // FALLTHROUGH_INTENDED
+    }
+    case TPM_TOKEN_INITIALIZED: {
       InitializeNSSForTPMToken();
       return;
     }
@@ -234,11 +265,19 @@ void CertLoader::OnPkcs11GetTpmTokenInfo(DBusMethodCallStatus call_status,
   InitializeTokenAndLoadCertificates();
 }
 
+void CertLoader::CallInitializeTPMToken() {
+  if (crypto::InitializeTPMToken(tpm_token_name_, tpm_user_pin_))
+    tpm_token_state_ = TPM_TOKEN_INITIALIZED;

[Ryan Sleevi, 2013/06/06 23:03:41]

THREADING: You're mutating this object on another thread. Is that really safe?

[stevenjb, 2013/06/07 02:37:47]

Was that rhetorical? :) Fixed with the change suggested below.

+  // Call InitializeNSSForTPMToken() on the main (UI) thread.
+  main_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&CertLoader::CallInitializeTPMToken,
+                 initialize_token_factory_.GetWeakPtr()));

[Ryan Sleevi, 2013/06/06 23:03:41]

THREADING: You're passing the same WeakPtr() on two different threads. This is a
violation of the WeakPtr contract, which states that a WeakPtr may only be
dereferenced on one thread.

Design suggestion:
Use PostTaskAndReplyWithResult to marshal the threading

if (!PostTaskAndReplyWithResult(
        crypto_task_runner_.get(),
        FROM_HERE,
        base::Bind(&crypto::InitializeTPMToken, tpm_token_name_, tpm_user_pin_),
        base::Bind(&CertLoader::CallInitializeTPMToken,
initialize_token_factory_.GetWeakPtr())) {
  // failed to initialize (no tasks posted)
}

CertLoader::CallInitializeTPMToken(bool initialized) {
  // handle yo stuff
}

this also frees you from tracking main_task_runner_ (PostTaskAndReplyWithResult
implicitly handles this)

[stevenjb, 2013/06/07 02:37:47]

Yeah, that was an oversight. I like your suggestion.
Done.

+}
+
 void CertLoader::InitializeNSSForTPMToken() {
   VLOG(1) << "InitializeNSSForTPMToken";
-
-  if (base::chromeos::IsRunningOnChromeOS() &&
-      !crypto::InitializeTPMToken(tpm_token_name_, tpm_user_pin_)) {
+  if (tpm_token_state_ != TPM_TOKEN_INITIALIZED) {
     RetryTokenInitializationLater();
     return;
   }