Make ONCCertificateImporter async.

2nd Reland of https://codereview.chromium.org/547553005/
after fixing memory issues.

This prepares for the new CertDatabase keyed service, which will have stricter threading restrictions. https://codereview.chromium.org/419013003/

Before, ONCCertificateImporter accessed the NSSCertDatabase from the UI thread and blocked on certificate store operations.

Now, the import itself is asychronous and calls back on completion.

While there, this also removes the GMock of the importer.

BUG=413219
TBR=eroman@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#296405}

chrome/browser/chromeos/policy/user_network_configuration_updater.cc

@@ -15,6 +15,7 @@
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/onc/onc_certificate_importer_impl.h"
 #include "components/user_manager/user.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_source.h"
 #include "net/cert/x509_certificate.h"
 #include "policy/policy_constants.h"
@@ -84,6 +85,15 @@ void UserNetworkConfigurationUpdater::GetWebTrustedCertificates(
   *certs = web_trust_certs_;
 }
 
+void UserNetworkConfigurationUpdater::OnCertificatesImported(
+    bool /* unused success */,
+    const net::CertificateList& onc_trusted_certificates) {
+  web_trust_certs_.clear();
+  if (allow_trusted_certificates_from_policy_)
+    web_trust_certs_ = onc_trusted_certificates;
+  NotifyTrustAnchorsChanged();
+}
+
 void UserNetworkConfigurationUpdater::ImportCertificates(
     const base::ListValue& certificates_onc) {
   // If certificate importer is not yet set, cache the certificate onc. It will
@@ -93,13 +103,11 @@ void UserNetworkConfigurationUpdater::ImportCertificates(
     return;
   }
 
-  web_trust_certs_.clear();
   certificate_importer_->ImportCertificates(
       certificates_onc,
       onc_source_,
-      allow_trusted_certificates_from_policy_ ? &web_trust_certs_ : NULL);
-
-  NotifyTrustAnchorsChanged();
+      base::Bind(&UserNetworkConfigurationUpdater::OnCertificatesImported,
+                 base::Unretained(this)));
 }
 
 void UserNetworkConfigurationUpdater::ApplyNetworkPolicy(
@@ -132,7 +140,10 @@ void UserNetworkConfigurationUpdater::CreateAndSetCertificateImporter(
     net::NSSCertDatabase* database) {
   DCHECK(database);
   SetCertificateImporter(scoped_ptr<chromeos::onc::CertificateImporter>(
-      new chromeos::onc::CertificateImporterImpl(database)));
+      new chromeos::onc::CertificateImporterImpl(
+          content::BrowserThread::GetMessageLoopProxyForThread(
+              content::BrowserThread::IO),
+          database)));
 }
 
 void UserNetworkConfigurationUpdater::SetCertificateImporter(

chrome/browser/chromeos/policy/user_network_configuration_updater.h

@@ -95,6 +95,11 @@ class UserNetworkConfigurationUpdater : public NetworkConfigurationUpdater,
       PolicyService* policy_service,
       chromeos::ManagedNetworkConfigurationHandler* network_config_handler);
 
+  // Called by the CertificateImporter when an import finished.
+  void OnCertificatesImported(
+      bool success,
+      const net::CertificateList& onc_trusted_certificates);
+
   // NetworkConfigurationUpdater:
   virtual void ImportCertificates(
       const base::ListValue& certificates_onc) OVERRIDE;

chrome/browser/ui/webui/net_internals/net_internals_ui.cc

@@ -257,6 +257,13 @@ class NetInternalsMessageHandler
   void ImportONCFileToNSSDB(const std::string& onc_blob,
                             const std::string& passcode,
                             net::NSSCertDatabase* nssdb);
+
+  // Called back by the CertificateImporter when a certificate import finished.
+  // |previous_error| contains earlier errors during this import.
+  void OnCertificatesImported(
+      const std::string& previous_error,
+      bool success,
+      const net::CertificateList& onc_trusted_certificates);
 #endif
 
  private:
@@ -1407,38 +1414,52 @@ void NetInternalsMessageHandler::ImportONCFileToNSSDB(
     const std::string& onc_blob,
     const std::string& passcode,
     net::NSSCertDatabase* nssdb) {
-  std::string error;
   user_manager::User* user = chromeos::ProfileHelper::Get()->GetUserByProfile(
       Profile::FromWebUI(web_ui()));
 
-  if (user) {
-    onc::ONCSource onc_source = onc::ONC_SOURCE_USER_IMPORT;
-
-    base::ListValue network_configs;
-    base::DictionaryValue global_network_config;
-    base::ListValue certificates;
-    if (!chromeos::onc::ParseAndValidateOncForImport(onc_blob,
-                                                     onc_source,
-                                                     passcode,
-                                                     &network_configs,
-                                                     &global_network_config,
-                                                     &certificates)) {
-      error = "Errors occurred during the ONC parsing. ";
-    }
-
-    chromeos::onc::CertificateImporterImpl cert_importer(nssdb);
-    if (!cert_importer.ImportCertificates(certificates, onc_source, NULL))
-      error += "Some certificates couldn't be imported. ";
+  if (!user) {
+    std::string error = "User not found.";
+    SendJavascriptCommand("receivedONCFileParse", new base::StringValue(error));
+    return;
+  }
 
-    std::string network_error;
-    chromeos::onc::ImportNetworksForUser(user, network_configs, &network_error);
-    if (!network_error.empty())
-      error += network_error;
-  } else {
-    error = "User not found.";
+  std::string error;
+  onc::ONCSource onc_source = onc::ONC_SOURCE_USER_IMPORT;
+  base::ListValue network_configs;
+  base::DictionaryValue global_network_config;
+  base::ListValue certificates;
+  if (!chromeos::onc::ParseAndValidateOncForImport(onc_blob,
+                                                   onc_source,
+                                                   passcode,
+                                                   &network_configs,
+                                                   &global_network_config,
+                                                   &certificates)) {
+    error = "Errors occurred during the ONC parsing. ";
   }
 
-  LOG_IF(ERROR, !error.empty()) << error;
+  std::string network_error;
+  chromeos::onc::ImportNetworksForUser(user, network_configs, &network_error);
+  if (!network_error.empty())
+    error += network_error;
+
+  chromeos::onc::CertificateImporterImpl cert_importer(
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO), nssdb);
+  cert_importer.ImportCertificates(
+      certificates,
+      onc_source,
+      base::Bind(&NetInternalsMessageHandler::OnCertificatesImported,
+                 AsWeakPtr(),
+                 error));
+}
+
+void NetInternalsMessageHandler::OnCertificatesImported(
+    const std::string& previous_error,
+    bool success,
+    const net::CertificateList& /* unused onc_trusted_certificates */) {
+  std::string error = previous_error;
+  if (!success)
+    error += "Some certificates couldn't be imported. ";
+
   SendJavascriptCommand("receivedONCFileParse", new base::StringValue(error));
 }
 

chromeos/chromeos.gyp

@@ -475,8 +475,6 @@
         'network/fake_network_device_handler.h',
         'network/mock_managed_network_configuration_handler.cc',
         'network/mock_managed_network_configuration_handler.h',
-        'network/onc/mock_certificate_importer.cc',
-        'network/onc/mock_certificate_importer.h',
         'network/onc/onc_test_utils.cc',
         'network/onc/onc_test_utils.h',
         'system/mock_statistics_provider.cc',

chromeos/network/onc/mock_certificate_importer.cc

-// Copyright 2013 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chromeos/network/onc/mock_certificate_importer.h"
-
-namespace chromeos {
-namespace onc {
-
-MockCertificateImporter::MockCertificateImporter() {
-}
-
-MockCertificateImporter::~MockCertificateImporter() {
-}
-
-}  // namespace onc
-}  // namespace chromeos

chromeos/network/onc/mock_certificate_importer.h

-// Copyright 2013 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROMEOS_NETWORK_ONC_MOCK_CERTIFICATE_IMPORTER_H_
-#define CHROMEOS_NETWORK_ONC_MOCK_CERTIFICATE_IMPORTER_H_
-
-#include "base/basictypes.h"
-#include "base/values.h"
-#include "chromeos/chromeos_export.h"
-#include "chromeos/network/onc/onc_certificate_importer.h"
-#include "testing/gmock/include/gmock/gmock.h"
-
-namespace chromeos {
-namespace onc {
-
-class CHROMEOS_EXPORT MockCertificateImporter : public CertificateImporter {
- public:
-  MockCertificateImporter();
-  virtual ~MockCertificateImporter();
-  MOCK_METHOD3(ImportCertificates, bool(const base::ListValue&,
-                                        ::onc::ONCSource,
-                                        net::CertificateList*));
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockCertificateImporter);
-};
-
-}  // namespace onc
-}  // namespace chromeos
-
-#endif  // CHROMEOS_NETWORK_ONC_MOCK_CERTIFICATE_IMPORTER_H_

chromeos/network/onc/onc_certificate_importer.h

@@ -6,6 +6,7 @@
 #define CHROMEOS_NETWORK_ONC_ONC_CERTIFICATE_IMPORTER_H_
 
 #include "base/basictypes.h"
+#include "base/callback_forward.h"
 #include "chromeos/chromeos_export.h"
 #include "components/onc/onc_constants.h"
 #include "net/cert/x509_certificate.h"
@@ -19,20 +20,25 @@ namespace onc {
 
 class CHROMEOS_EXPORT CertificateImporter {
  public:
+  typedef base::Callback<
+      void(bool success, const net::CertificateList& onc_trusted_certificates)>
+      DoneCallback;
+
   CertificateImporter() {}
   virtual ~CertificateImporter() {}
 
-  // Import the |certificates|, which must be a list of ONC Certificate objects.
-  // Certificates are only imported with web trust for user imports. If
-  // |onc_trusted_certificates| is not NULL, it will be filled with the list
-  // of certificates that requested the TrustBit "Web". If the "Remove" field of
-  // a certificate is enabled, then removes the certificate from the store
-  // instead of importing. Returns true if all certificates were imported
-  // successfully.
-  virtual bool ImportCertificates(
-      const base::ListValue& certificates,
-      ::onc::ONCSource source,
-      net::CertificateList* onc_trusted_certificates) = 0;
+  // Import |certificates|, which must be a list of ONC Certificate objects.
+  // Certificates are only imported with web trust for user imports. If the
+  // "Remove" field of a certificate is enabled, then removes the certificate
+  // from the store instead of importing.
+  // When the import is completed, |done_callback| will be called with |success|
+  // equal to true if all certificates were imported successfully.
+  // |onc_trusted_certificates| will contain the list of certificates that
+  // were imported and requested the TrustBit "Web".
+  // Never calls |done_callback| after this importer is destructed.
+  virtual void ImportCertificates(const base::ListValue& certificates,
+                                  ::onc::ONCSource source,
+                                  const DoneCallback& done_callback) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CertificateImporter);

chromeos/network/onc/onc_certificate_importer_impl.h

@@ -12,6 +12,7 @@
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/network/onc/onc_certificate_importer.h"
 #include "components/onc/onc_constants.h"
@@ -19,6 +20,8 @@
 namespace base {
 class DictionaryValue;
 class ListValue;
+class SequencedTaskRunner;
+class SingleThreadTaskRunner;
 }
 
 namespace net {
@@ -31,34 +34,36 @@ namespace chromeos {
 namespace onc {
 
 // This class handles certificate imports from ONC (both policy and user
-// imports) into the certificate store. The GUID of Client certificates is
-// stored together with the certificate as Nickname. In contrast, Server and CA
+// imports) into a certificate store. The GUID of Client certificates is stored
+// together with the certificate as Nickname. In contrast, Server and CA
 // certificates are identified by their PEM and not by GUID.
 // TODO(pneubeck): Replace Nickname by PEM for Client
 // certificates. http://crbug.com/252119
 class CHROMEOS_EXPORT CertificateImporterImpl : public CertificateImporter {
  public:
-  typedef std::map<std::string, scoped_refptr<net::X509Certificate> >
-      CertsByGUID;
-
-  explicit CertificateImporterImpl(net::NSSCertDatabase* target_nssdb_);
+  // |io_task_runner| will be used for NSSCertDatabase accesses.
+  CertificateImporterImpl(
+      const scoped_refptr<base::SequencedTaskRunner>& io_task_runner,
+      net::NSSCertDatabase* target_nssdb_);
+  virtual ~CertificateImporterImpl();
 
   // CertificateImporter overrides
-  virtual bool ImportCertificates(
-      const base::ListValue& certificates,
-      ::onc::ONCSource source,
-      net::CertificateList* onc_trusted_certificates) OVERRIDE;
-
-  // This implements ImportCertificates. Additionally, if
-  // |imported_server_and_ca_certs| is not NULL, it will be filled with the
-  // (GUID, Certificate) pairs of all succesfully imported Server and CA
-  // certificates.
-  bool ParseAndStoreCertificates(bool allow_trust_imports,
-                                 const base::ListValue& onc_certificates,
-                                 net::CertificateList* onc_trusted_certificates,
-                                 CertsByGUID* imported_server_and_ca_certs);
+  virtual void ImportCertificates(const base::ListValue& certificates,
+                                  ::onc::ONCSource source,
+                                  const DoneCallback& done_callback) OVERRIDE;
 
  private:
+  void RunDoneCallback(const CertificateImporter::DoneCallback& callback,
+                       bool success,
+                       const net::CertificateList& onc_trusted_certificates);
+
+  // This is the synchronous implementation of ImportCertificates. It is
+  // executed on the given |io_task_runner_|.
+  static void ParseAndStoreCertificates(::onc::ONCSource source,
+                                        const DoneCallback& done_callback,
+                                        base::ListValue* certificates,
+                                        net::NSSCertDatabase* nssdb);
+
   // Lists the certificates that have the string |label| as their certificate
   // nickname (exact match).
   static void ListCertsWithNickname(const std::string& label,
@@ -72,30 +77,36 @@ class CHROMEOS_EXPORT CertificateImporterImpl : public CertificateImporter {
 
   // Parses and stores/removes |certificate| in/from the certificate
   // store. Returns true if the operation succeeded.
-  bool ParseAndStoreCertificate(
+  static bool ParseAndStoreCertificate(
       bool allow_trust_imports,
       const base::DictionaryValue& certificate,
-      net::CertificateList* onc_trusted_certificates,
-      CertsByGUID* imported_server_and_ca_certs);
+      net::NSSCertDatabase* nssdb,
+      net::CertificateList* onc_trusted_certificates);
 
   // Imports the Server or CA certificate |certificate|. Web trust is only
   // applied if the certificate requests the TrustBits attribute "Web" and if
   // the |allow_trust_imports| permission is granted, otherwise the attribute is
   // ignored.
-  bool ParseServerOrCaCertificate(
+  static bool ParseServerOrCaCertificate(
       bool allow_trust_imports,
       const std::string& cert_type,
       const std::string& guid,
       const base::DictionaryValue& certificate,
-      net::CertificateList* onc_trusted_certificates,
-      CertsByGUID* imported_server_and_ca_certs);
+      net::NSSCertDatabase* nssdb,
+      net::CertificateList* onc_trusted_certificates);
 
-  bool ParseClientCertificate(const std::string& guid,
-                              const base::DictionaryValue& certificate);
+  static bool ParseClientCertificate(const std::string& guid,
+                                     const base::DictionaryValue& certificate,
+                                     net::NSSCertDatabase* nssdb);
+
+  // The task runner to use for NSSCertDatabase accesses.
+  scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
 
   // The certificate database to which certificates are imported.
   net::NSSCertDatabase* target_nssdb_;
 
+  base::WeakPtrFactory<CertificateImporterImpl> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(CertificateImporterImpl);
 };
 

chromeos/network/onc/onc_certificate_importer_impl_unittest.cc

@@ -13,11 +13,12 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chromeos/network/onc/onc_test_utils.h"
 #include "components/onc/onc_constants.h"
-#include "crypto/nss_util_internal.h"
-#include "crypto/scoped_test_nss_chromeos_user.h"
+#include "crypto/scoped_test_nss_db.h"
 #include "net/base/crypto_module.h"
 #include "net/cert/cert_type.h"
 #include "net/cert/nss_cert_database_chromeos.h"
@@ -63,30 +64,39 @@ net::CertType GetCertType(net::X509Certificate::OSCertHandle cert) {
 
 class ONCCertificateImporterImplTest : public testing::Test {
  public:
-  ONCCertificateImporterImplTest() : user_("username_hash"),
-                                     private_user_("private_user_hash") {}
-
-  virtual void SetUp() {
-    ASSERT_TRUE(user_.constructed_successfully());
-    ASSERT_TRUE(private_user_.constructed_successfully());
-
-    // By default test user will have the same public and private slot.
-    // Unfortunatelly, ONC importer should care about which slot certificates
-    // get imported to. To work around this, we create another NSS user whose
-    // public slot will act as the private slot.
-    // TODO(tbarzic): See if there's a better way to achieve this.
+  ONCCertificateImporterImplTest() {}
+  virtual ~ONCCertificateImporterImplTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(public_nssdb_.is_open());
+    ASSERT_TRUE(private_nssdb_.is_open());
+
+    task_runner_ = new base::TestSimpleTaskRunner();
+    thread_task_runner_handle_.reset(
+        new base::ThreadTaskRunnerHandle(task_runner_));
+
     test_nssdb_.reset(new net::NSSCertDatabaseChromeOS(
-        crypto::GetPublicSlotForChromeOSUser(user_.username_hash()),
-        crypto::GetPublicSlotForChromeOSUser(private_user_.username_hash())));
+        crypto::ScopedPK11Slot(PK11_ReferenceSlot(public_nssdb_.slot())),
+        crypto::ScopedPK11Slot(PK11_ReferenceSlot(private_nssdb_.slot()))));
 
     // Test db should be empty at start of test.
     EXPECT_TRUE(ListCertsInPublicSlot().empty());
     EXPECT_TRUE(ListCertsInPrivateSlot().empty());
   }
 
-  virtual ~ONCCertificateImporterImplTest() {}
+  virtual void TearDown() OVERRIDE {
+    thread_task_runner_handle_.reset();
+    task_runner_ = NULL;
+  }
 
  protected:
+  void OnImportCompleted(bool expected_success,
+                         bool success,
+                         const net::CertificateList& onc_trusted_certificates) {
+    EXPECT_EQ(expected_success, success);
+    web_trust_certificates_ = onc_trusted_certificates;
+  }
+
   void AddCertificatesFromFile(std::string filename, bool expected_success) {
     scoped_ptr<base::DictionaryValue> onc =
         test_utils::ReadTestDictionary(filename);
@@ -98,14 +108,15 @@ class ONCCertificateImporterImplTest : public testing::Test {
     onc_certificates_.reset(certificates);
 
     web_trust_certificates_.clear();
-    imported_server_and_ca_certs_.clear();
-    CertificateImporterImpl importer(test_nssdb_.get());
-    EXPECT_EQ(
-        expected_success,
-        importer.ParseAndStoreCertificates(true,  // allow web trust
-                                           *certificates,
-                                           &web_trust_certificates_,
-                                           &imported_server_and_ca_certs_));
+    CertificateImporterImpl importer(task_runner_, test_nssdb_.get());
+    importer.ImportCertificates(
+        *certificates,
+        ::onc::ONC_SOURCE_USER_IMPORT,  // allow web trust
+        base::Bind(&ONCCertificateImporterImplTest::OnImportCompleted,
+                   base::Unretained(this),
+                   expected_success));
+
+    task_runner_->RunUntilIdle();
 
     public_list_ = ListCertsInPublicSlot();
     private_list_ = ListCertsInPrivateSlot();
@@ -119,25 +130,29 @@ class ONCCertificateImporterImplTest : public testing::Test {
       guid = &guid_temporary;
 
     AddCertificatesFromFile(filename, true);
-    ASSERT_EQ(1ul, public_list_.size() + private_list_.size());
-    if (!public_list_.empty())
+
+    if (expected_type == net::SERVER_CERT || expected_type == net::CA_CERT) {
+      ASSERT_EQ(1u, public_list_.size());
       EXPECT_EQ(expected_type, GetCertType(public_list_[0]->os_cert_handle()));
-    if (!private_list_.empty())
+      EXPECT_TRUE(private_list_.empty());
+    } else {  // net::USER_CERT
+      EXPECT_TRUE(public_list_.empty());
+      ASSERT_EQ(1u, private_list_.size());
       EXPECT_EQ(expected_type, GetCertType(private_list_[0]->os_cert_handle()));
+    }
 
     base::DictionaryValue* certificate = NULL;
     onc_certificates_->GetDictionary(0, &certificate);
     certificate->GetStringWithoutPathExpansion(::onc::certificate::kGUID, guid);
-
-    if (expected_type == net::SERVER_CERT || expected_type == net::CA_CERT) {
-      EXPECT_EQ(1u, imported_server_and_ca_certs_.size());
-      EXPECT_TRUE(
-          imported_server_and_ca_certs_[*guid]->Equals(public_list_[0].get()));
-    } else {  // net::USER_CERT
-      EXPECT_TRUE(imported_server_and_ca_certs_.empty());
-    }
   }
 
+  // Certificates and the NSSCertDatabase depend on these test DBs. Destroy them
+  // last.
+  crypto::ScopedTestNSSDB public_nssdb_;
+  crypto::ScopedTestNSSDB private_nssdb_;
+
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  scoped_ptr<base::ThreadTaskRunnerHandle> thread_task_runner_handle_;
   scoped_ptr<net::NSSCertDatabaseChromeOS> test_nssdb_;
   scoped_ptr<base::ListValue> onc_certificates_;
   // List of certs in the nssdb's public slot.
@@ -145,15 +160,14 @@ class ONCCertificateImporterImplTest : public testing::Test {
   // List of certs in the nssdb's "private" slot.
   net::CertificateList private_list_;
   net::CertificateList web_trust_certificates_;
-  CertificateImporterImpl::CertsByGUID imported_server_and_ca_certs_;
 
  private:
   net::CertificateList ListCertsInPublicSlot() {
-    return ListCertsInSlot(test_nssdb_->GetPublicSlot().get());
+    return ListCertsInSlot(public_nssdb_.slot());
   }
 
   net::CertificateList ListCertsInPrivateSlot() {
-    return ListCertsInSlot(test_nssdb_->GetPrivateSlot().get());
+    return ListCertsInSlot(private_nssdb_.slot());
   }
 
   net::CertificateList ListCertsInSlot(PK11SlotInfo* slot) {
@@ -171,16 +185,13 @@ class ONCCertificateImporterImplTest : public testing::Test {
     std::sort(result.begin(), result.end(), net::X509Certificate::LessThan());
     return result;
   }
-
-  crypto::ScopedTestNSSChromeOSUser user_;
-  crypto::ScopedTestNSSChromeOSUser private_user_;
 };
 
 TEST_F(ONCCertificateImporterImplTest, MultipleCertificates) {
   AddCertificatesFromFile("managed_toplevel2.onc", true);
   EXPECT_EQ(onc_certificates_->GetSize(), public_list_.size());
   EXPECT_TRUE(private_list_.empty());
-  EXPECT_EQ(2ul, imported_server_and_ca_certs_.size());
+  EXPECT_EQ(2ul, public_list_.size());
 }
 
 TEST_F(ONCCertificateImporterImplTest, MultipleCertificatesWithFailures) {
@@ -188,7 +199,6 @@ TEST_F(ONCCertificateImporterImplTest, MultipleCertificatesWithFailures) {
   EXPECT_EQ(3ul, onc_certificates_->GetSize());
   EXPECT_EQ(1ul, private_list_.size());
   EXPECT_TRUE(public_list_.empty());
-  EXPECT_TRUE(imported_server_and_ca_certs_.empty());
 }
 
 TEST_F(ONCCertificateImporterImplTest, AddClientCertificate) {
@@ -199,7 +209,7 @@ TEST_F(ONCCertificateImporterImplTest, AddClientCertificate) {
   EXPECT_TRUE(public_list_.empty());
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
   EXPECT_TRUE(privkey_list);
   if (privkey_list) {
     SECKEYPrivateKeyListNode* node = PRIVKEY_LIST_HEAD(privkey_list);
@@ -216,7 +226,7 @@ TEST_F(ONCCertificateImporterImplTest, AddClientCertificate) {
   }
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
+      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
   EXPECT_TRUE(pubkey_list);
   if (pubkey_list) {
     SECKEYPublicKeyListNode* node = PUBKEY_LIST_HEAD(pubkey_list);
@@ -234,11 +244,11 @@ TEST_F(ONCCertificateImporterImplTest, AddServerCertificateWithWebTrust) {
   AddCertificateFromFile("certificate-server.onc", net::SERVER_CERT, NULL);
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
+      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
   EXPECT_FALSE(pubkey_list);
 
   ASSERT_EQ(1u, web_trust_certificates_.size());
@@ -252,11 +262,11 @@ TEST_F(ONCCertificateImporterImplTest, AddWebAuthorityCertificateWithWebTrust) {
   AddCertificateFromFile("certificate-web-authority.onc", net::CA_CERT, NULL);
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
+      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
   EXPECT_FALSE(pubkey_list);
 
   ASSERT_EQ(1u, web_trust_certificates_.size());
@@ -271,11 +281,11 @@ TEST_F(ONCCertificateImporterImplTest, AddAuthorityCertificateWithoutWebTrust) {
   EXPECT_TRUE(web_trust_certificates_.empty());
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
+      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
   EXPECT_FALSE(pubkey_list);
 }