Call crypto::InitializeTPMToken on the IO thread (Take 2)

This is identical to https://chromiumcodereview.appspot.com/15649018
with the following addition:

Call DeviceSettingsService::EnsureReload once certs are loaded

BUG=244455
R=mnissler@chromium.org, nkostylev@chromium.org, rsleevi@chromium.org, xiyuan@chromium.org

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

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

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -278,6 +278,7 @@ class DBusServices {
 
     LoginState::Initialize();
     CryptohomeLibrary::Initialize();
+    CertLoader::Initialize();
 
     // This function and SystemKeyEventListener use InputMethodManager.
     chromeos::input_method::Initialize(
@@ -372,8 +373,6 @@ ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos() {
   if (KioskModeSettings::Get()->IsKioskModeEnabled())
     ShutdownKioskModeScreensaver();
 
-  dbus_services_.reset();
-
   // To be precise, logout (browser shutdown) is not yet done, but the
   // remaining work is negligible, hence we say LogoutDone here.
   BootTimesLoader::Get()->AddLogoutTimeMarker("LogoutDone", false);
@@ -449,6 +448,11 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopStart() {
 // Threads are initialized between MainMessageLoopStart and MainMessageLoopRun.
 // about_flags settings are applied in ChromeBrowserMainParts::PreCreateThreads.
 void ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() {
+  // Set the crypto thread after the IO thread has been created/started.
+  CertLoader::Get()->SetCryptoTaskRunner(
+      content::BrowserThread::GetMessageLoopProxyForThread(
+          content::BrowserThread::IO));
+
   if (ash::switches::UseNewAudioHandler()) {
     CrasAudioHandler::Initialize(
         AudioDevicesPrefHandler::Create(g_browser_process->local_state()));
@@ -827,7 +831,11 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() {
 }
 
 void ChromeBrowserMainPartsChromeos::PostDestroyThreads() {
+  // Destroy DBus services immediately after threads are stopped.
+  dbus_services_.reset();
+
   ChromeBrowserMainPartsLinux::PostDestroyThreads();
+
   // Destroy DeviceSettingsService after g_browser_process.
   DeviceSettingsService::Shutdown();
 }

chrome/browser/chromeos/cros/cert_library.cc

@@ -121,11 +121,11 @@ bool CertLibrary::IsInitialized() {
 }
 
 CertLibrary::CertLibrary() {
-  NetworkHandler::Get()->cert_loader()->AddObserver(this);
+  CertLoader::Get()->AddObserver(this);
 }
 
 CertLibrary::~CertLibrary() {
-  NetworkHandler::Get()->cert_loader()->RemoveObserver(this);
+  CertLoader::Get()->RemoveObserver(this);
 }
 
 void CertLibrary::AddObserver(CertLibrary::Observer* observer) {
@@ -137,15 +137,15 @@ void CertLibrary::RemoveObserver(CertLibrary::Observer* observer) {
 }
 
 bool CertLibrary::CertificatesLoading() const {
-  return NetworkHandler::Get()->cert_loader()->CertificatesLoading();
+  return CertLoader::Get()->CertificatesLoading();
 }
 
 bool CertLibrary::CertificatesLoaded() const {
-  return NetworkHandler::Get()->cert_loader()->certificates_loaded();
+  return CertLoader::Get()->certificates_loaded();
 }
 
 bool CertLibrary::IsHardwareBacked() const {
-  return NetworkHandler::Get()->cert_loader()->IsHardwareBacked();
+  return CertLoader::Get()->IsHardwareBacked();
 }
 
 int CertLibrary::NumCertificates(CertType type) const {
@@ -169,13 +169,13 @@ std::string CertLibrary::GetCertPkcs11IdAt(CertType type, int index) const {
 }
 
 bool CertLibrary::IsCertHardwareBackedAt(CertType type, int index) const {
-  if (!NetworkHandler::Get()->cert_loader()->IsHardwareBacked())
+  if (!CertLoader::Get()->IsHardwareBacked())
     return false;
   net::X509Certificate* cert = GetCertificateAt(type, index);
   std::string cert_token_name =
       x509_certificate_model::GetTokenName(cert->os_cert_handle());
   return cert_token_name ==
-      NetworkHandler::Get()->cert_loader()->tpm_token_name();
+      CertLoader::Get()->tpm_token_name();
 }
 
 int CertLibrary::GetCertIndexByPEM(CertType type,

chrome/browser/chromeos/cros/cert_library.h

@@ -8,7 +8,7 @@
 #include <string>
 
 #include "base/strings/string16.h"
-#include "chromeos/network/cert_loader.h"
+#include "chromeos/cert_loader.h"
 #include "net/cert/x509_certificate.h"
 
 namespace chromeos {

chrome/browser/chromeos/cros/network_library.cc

@@ -16,7 +16,6 @@
 #include "chrome/browser/chromeos/cros/network_library_impl_cros.h"
 #include "chrome/browser/chromeos/cros/network_library_impl_stub.h"
 #include "chrome/common/net/x509_certificate_model.h"
-#include "chromeos/network/cert_loader.h"
 #include "chromeos/network/certificate_pattern.h"
 #include "chromeos/network/certificate_pattern_matcher.h"
 #include "chromeos/network/cros_network_functions.h"

chrome/browser/chromeos/settings/device_settings_service.cc

@@ -75,10 +75,14 @@ DeviceSettingsService::DeviceSettingsService()
       weak_factory_(this),
       store_status_(STORE_SUCCESS),
       load_retries_left_(kMaxLoadRetries) {
+  if (CertLoader::IsInitialized())
+    CertLoader::Get()->AddObserver(this);
 }
 
 DeviceSettingsService::~DeviceSettingsService() {
   DCHECK(pending_operations_.empty());
+  if (CertLoader::IsInitialized())
+    CertLoader::Get()->RemoveObserver(this);
 }
 
 void DeviceSettingsService::SetSessionManager(
@@ -208,6 +212,14 @@ void DeviceSettingsService::PropertyChangeComplete(bool success) {
   EnsureReload(false);
 }
 
+void DeviceSettingsService::OnCertificatesLoaded(
+    const net::CertificateList& cert_list,
+    bool initial_load) {
+  // CertLoader initializes the TPM and NSS database which is necessary to
+  // determine ownership. Force a reload once we know these are initialized.
+  EnsureReload(true);
+}
+
 void DeviceSettingsService::Enqueue(SessionManagerOperation* operation) {
   pending_operations_.push_back(operation);
   if (pending_operations_.front() == operation)

chrome/browser/chromeos/settings/device_settings_service.h

@@ -16,6 +16,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "chrome/browser/policy/cloud/cloud_policy_validator.h"
+#include "chromeos/cert_loader.h"
 #include "chromeos/dbus/session_manager_client.h"
 
 namespace crypto {
@@ -71,7 +72,8 @@ class OwnerKey : public base::RefCountedThreadSafe<OwnerKey> {
 //
 // DeviceSettingsService generates notifications for key and policy update
 // events so interested parties can reload state as appropriate.
-class DeviceSettingsService : public SessionManagerClient::Observer {
+class DeviceSettingsService : public SessionManagerClient::Observer,
+                              public CertLoader::Observer {
  public:
   // Indicates ownership status of the device.
   enum OwnershipStatus {
@@ -188,6 +190,10 @@ class DeviceSettingsService : public SessionManagerClient::Observer {
   virtual void OwnerKeySet(bool success) OVERRIDE;
   virtual void PropertyChangeComplete(bool success) OVERRIDE;
 
+  // CertLoader::Observer:
+  virtual void OnCertificatesLoaded(const net::CertificateList& cert_list,
+                                    bool initial_load) OVERRIDE;
+
  private:
   // Enqueues a new operation. Takes ownership of |operation| and starts it
   // right away if there is no active operation currently.

chrome/browser/ui/webui/options/certificate_manager_browsertest.cc

@@ -56,6 +56,7 @@ class CertificateManagerBrowserTest : public options::OptionsBrowserTest {
         network_configuration_updater()->SetUserPolicyService(
             true, "", connector->policy_service());
 #endif
+    content::RunAllPendingInMessageLoop();
   }
 
 #if defined(OS_CHROMEOS)

chromeos/network/cert_loader.cc → chromeos/cert_loader.cc

@@ -2,12 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chromeos/network/cert_loader.h"
+#include "chromeos/cert_loader.h"
 
 #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,8 +48,45 @@ 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
 
+static CertLoader* g_cert_loader = NULL;
+// static
+void CertLoader::Initialize() {
+  CHECK(!g_cert_loader);
+  g_cert_loader = new CertLoader();
+  g_cert_loader->Init();
+}
+
+// static
+void CertLoader::Shutdown() {
+  CHECK(g_cert_loader);
+  delete g_cert_loader;
+  g_cert_loader = NULL;
+}
+
+// static
+CertLoader* CertLoader::Get() {
+  CHECK(g_cert_loader)
+      << "CertLoader::Get() called before Initialize()";
+  return g_cert_loader;
+}
+
+// static
+bool CertLoader::IsInitialized() {
+  return g_cert_loader;
+}
+
 CertLoader::CertLoader()
     : certificates_requested_(false),
       certificates_loaded_(false),
@@ -58,10 +97,18 @@ CertLoader::CertLoader()
           base::TimeDelta::FromMilliseconds(kInitialRequestDelayMs)),
       initialize_token_factory_(this),
       update_certificates_factory_(this) {
+}
+
+void CertLoader::Init() {
   net::CertDatabase::GetInstance()->AddObserver(this);
   if (LoginState::IsInitialized())
     LoginState::Get()->AddObserver(this);
-  RequestCertificates();
+}
+
+void CertLoader::SetCryptoTaskRunner(
+    const scoped_refptr<base::SequencedTaskRunner>& crypto_task_runner) {
+  crypto_task_runner_ = crypto_task_runner;
+  MaybeRequestCertificates();
 }
 
 CertLoader::~CertLoader() {
@@ -86,36 +133,44 @@ bool CertLoader::IsHardwareBacked() const {
   return !tpm_token_name_.empty();
 }
 
-void CertLoader::RequestCertificates() {
+void CertLoader::MaybeRequestCertificates() {
   CHECK(thread_checker_.CalledOnValidThread());
+
+  // This is the entry point to the TPM token initialization process,
+  // which we should do at most once.
+  if (certificates_requested_ || !crypto_task_runner_.get())
+    return;
+
   const bool logged_in = LoginState::IsInitialized() ?
       LoginState::Get()->IsUserLoggedIn() : false;
   VLOG(1) << "RequestCertificates: " << logged_in;
-  if (certificates_requested_ || !logged_in)
+  if (!logged_in)
     return;
 
   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());
+  // Ensure we only initialize the TPM token 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: {
+      crypto_task_runner_->PostTaskAndReply(
+          FROM_HERE,
+          base::Bind(&CallOpenPersistentNSSDB),
+          base::Bind(&CertLoader::OnPersistentNSSDBOpened,
+                     initialize_token_factory_.GetWeakPtr()));
+      return;
+    }
+    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,17 +193,28 @@ void CertLoader::InitializeTokenAndLoadCertificates() {
       return;
     }
     case TPM_TOKEN_INFO_RECEIVED: {
-      InitializeNSSForTPMToken();
-      return;
+      if (base::chromeos::IsRunningOnChromeOS()) {
+        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;
+      }
+      tpm_token_state_ = TPM_TOKEN_INITIALIZED;
+      // FALL_THROUGH_INTENDED
     }
-    case TPM_TOKEN_NSS_INITIALIZED: {
+    case TPM_TOKEN_INITIALIZED: {
       StartLoadCertificates();
       return;
     }
   }
 }
 
 void CertLoader::RetryTokenInitializationLater() {
+  CHECK(thread_checker_.CalledOnValidThread());
   LOG(WARNING) << "Re-Requesting Certificates later.";
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
@@ -158,6 +224,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 +306,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 +373,7 @@ void CertLoader::OnCertRemoved(const net::X509Certificate* cert) {
 
 void CertLoader::LoggedInStateChanged(LoginState::LoggedInState state) {
   VLOG(1) << "LoggedInStateChanged: " << state;
-  RequestCertificates();
+  MaybeRequestCertificates();
 }
 
 }  // namespace chromeos