Remove the deprecated NSSCertDatabase::GetInstance() .

The NSSCertDatabase singleton for Linux is now maintained by nss_context_linux.cc .

BUG=329735

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

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

chrome/browser/net/nss_context.h

@@ -40,7 +40,7 @@ crypto::ScopedPK11Slot GetPrivateNSSKeySlotForResourceContext(
 // |callback| will be run once the DB is initialized. Ownership is not
 // transferred, but the caller may save the pointer, which will remain valid for
 // the lifetime of the ResourceContext.
-// Should be called only on the IO thread.
+// Must be called only on the IO thread.
 net::NSSCertDatabase* GetNSSCertDatabaseForResourceContext(
     content::ResourceContext* context,
     const base::Callback<void(net::NSSCertDatabase*)>& callback)

chrome/browser/net/nss_context_linux.cc

@@ -8,6 +8,10 @@
 #include "crypto/nss_util_internal.h"
 #include "net/cert/nss_cert_database.h"
 
+namespace {
+net::NSSCertDatabase* g_nss_cert_database = NULL;
+}  // namespace
+
 crypto::ScopedPK11Slot GetPublicNSSKeySlotForResourceContext(
     content::ResourceContext* context) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
@@ -24,6 +28,18 @@ crypto::ScopedPK11Slot GetPrivateNSSKeySlotForResourceContext(
 net::NSSCertDatabase* GetNSSCertDatabaseForResourceContext(
     content::ResourceContext* context,
     const base::Callback<void(net::NSSCertDatabase*)>& callback) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
-  return net::NSSCertDatabase::GetInstance();
+  // This initialization is not thread safe. This CHECK ensures that this code
+  // is only run on a single thread.
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  if (!g_nss_cert_database) {
+    // Linux has only a single persistent slot compared to ChromeOS's separate
+    // public and private slot.
+    // Redirect any slot usage to this persistent slot on Linux.
+    g_nss_cert_database = new net::NSSCertDatabase(
+        crypto::ScopedPK11Slot(
+            crypto::GetPersistentNSSKeySlot()) /* public slot */,
+        crypto::ScopedPK11Slot(
+            crypto::GetPersistentNSSKeySlot()) /* private slot */);
+  }
+  return g_nss_cert_database;
 }

chrome/common/net/x509_certificate_model_unittest.cc

@@ -11,6 +11,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(USE_NSS)
+#include "crypto/nss_util_internal.h"
 #include "net/cert/nss_cert_database.h"
 #endif
 
@@ -223,9 +224,16 @@ TEST(X509CertificateModelTest, GetTypeCA) {
   EXPECT_EQ(net::CA_CERT,
             x509_certificate_model::GetType(cert->os_cert_handle()));
 
+  // Additional parantheses required to disambiguate from function declaration.
+  net::NSSCertDatabase db(
+      (crypto::ScopedPK11Slot(
+          crypto::GetPersistentNSSKeySlot())) /* public slot */,
+      crypto::ScopedPK11Slot(
+          crypto::GetPersistentNSSKeySlot()) /* private lot */);
+
   // Test that explicitly distrusted CA certs are still returned as CA_CERT
   // type. See http://crbug.com/96654.
-  EXPECT_TRUE(net::NSSCertDatabase::GetInstance()->SetCertTrust(
+  EXPECT_TRUE(db.SetCertTrust(
       cert.get(), net::CA_CERT, net::NSSCertDatabase::DISTRUSTED_SSL));
 
   EXPECT_EQ(net::CA_CERT,
@@ -251,16 +259,22 @@ TEST(X509CertificateModelTest, GetTypeServer) {
   EXPECT_EQ(net::OTHER_CERT,
             x509_certificate_model::GetType(cert->os_cert_handle()));
 
-  net::NSSCertDatabase* cert_db = net::NSSCertDatabase::GetInstance();
+  // Additional parantheses required to disambiguate from function declaration.
+  net::NSSCertDatabase db(
+      (crypto::ScopedPK11Slot(
+          crypto::GetPersistentNSSKeySlot())) /* public slot */,
+      crypto::ScopedPK11Slot(
+          crypto::GetPersistentNSSKeySlot()) /* private lot */);
+
   // Test GetCertType with server certs and explicit trust.
-  EXPECT_TRUE(cert_db->SetCertTrust(
+  EXPECT_TRUE(db.SetCertTrust(
       cert.get(), net::SERVER_CERT, net::NSSCertDatabase::TRUSTED_SSL));
 
   EXPECT_EQ(net::SERVER_CERT,
             x509_certificate_model::GetType(cert->os_cert_handle()));
 
   // Test GetCertType with server certs and explicit distrust.
-  EXPECT_TRUE(cert_db->SetCertTrust(
+  EXPECT_TRUE(db.SetCertTrust(
       cert.get(), net::SERVER_CERT, net::NSSCertDatabase::DISTRUSTED_SSL));
 
   EXPECT_EQ(net::SERVER_CERT,

chromeos/cert_loader.cc

@@ -88,8 +88,7 @@ int CertLoader::TPMTokenSlotID() const {
   if (!database_)
     return -1;
   crypto::ScopedPK11Slot slot(database_->GetPrivateSlot());
-  if (!slot)
-    return -1;
+  DCHECK(slot);
   return static_cast<int>(PK11_GetSlotID(slot.get()));
 }
 
@@ -99,8 +98,7 @@ bool CertLoader::IsHardwareBacked() const {
   if (!database_)
     return false;
   crypto::ScopedPK11Slot slot(database_->GetPrivateSlot());
-  if (!slot)
-    return false;
+  DCHECK(slot);
   return PK11_IsHW(slot.get());
 }
 

chromeos/cert_loader_unittest.cc

@@ -305,19 +305,5 @@ TEST_F(CertLoaderTest, UpdatedOnCACertTrustChange) {
   EXPECT_EQ(1U, GetAndResetCertificatesLoadedEventsCount());
 }
 
-TEST_F(CertLoaderTest, DatabaseWithUnsetSlots) {
-  primary_db_.reset(new net::NSSCertDatabaseChromeOS(crypto::ScopedPK11Slot(),
-                                                     crypto::ScopedPK11Slot()));
-  primary_db_->SetSlowTaskRunnerForTest(message_loop_.message_loop_proxy());
-  cert_loader_->StartWithNSSDB(primary_db_.get());
-
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1u, GetAndResetCertificatesLoadedEventsCount());
-
-  EXPECT_TRUE(cert_loader_->certificates_loaded());
-  EXPECT_EQ(-1, cert_loader_->TPMTokenSlotID());
-  EXPECT_FALSE(cert_loader_->IsHardwareBacked());
-}
-
 }  // namespace
 }  // namespace chromeos

net/cert/nss_cert_database.cc

@@ -12,15 +12,12 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list_threadsafe.h"
 #include "base/task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/threading/worker_pool.h"
-#include "crypto/nss_util.h"
-#include "crypto/nss_util_internal.h"
 #include "crypto/scoped_nss_types.h"
 #include "net/base/crypto_module.h"
 #include "net/base/net_errors.h"
@@ -42,6 +39,8 @@ namespace net {
 
 namespace {
 
+// TODO(pneubeck): Move this class out of NSSCertDatabase and to the caller of
+// the c'tor of NSSCertDatabase, see https://crbug.com/395983 .
 // Helper that observes events from the NSSCertDatabase and forwards them to
 // the given CertDatabase.
 class CertNotificationForwarder : public NSSCertDatabase::Observer {
@@ -70,9 +69,6 @@ class CertNotificationForwarder : public NSSCertDatabase::Observer {
   DISALLOW_COPY_AND_ASSIGN(CertNotificationForwarder);
 };
 
-base::LazyInstance<NSSCertDatabase>::Leaky
-    g_nss_cert_database = LAZY_INSTANCE_INITIALIZER;
-
 }  // namespace
 
 NSSCertDatabase::ImportCertFailure::ImportCertFailure(
@@ -82,20 +78,15 @@ NSSCertDatabase::ImportCertFailure::ImportCertFailure(
 
 NSSCertDatabase::ImportCertFailure::~ImportCertFailure() {}
 
-// static
-NSSCertDatabase* NSSCertDatabase::GetInstance() {
-  // TODO(mattm): Remove this ifdef guard once the linux impl of
-  // GetNSSCertDatabaseForResourceContext does not call GetInstance.
-#if defined(OS_CHROMEOS)
-  LOG(ERROR) << "NSSCertDatabase::GetInstance() is deprecated."
-             << "See http://crbug.com/329735.";
-#endif
-  return &g_nss_cert_database.Get();
-}
-
-NSSCertDatabase::NSSCertDatabase()
-    : observer_list_(new ObserverListThreadSafe<Observer>),
+NSSCertDatabase::NSSCertDatabase(crypto::ScopedPK11Slot public_slot,
+                                 crypto::ScopedPK11Slot private_slot)
+    : public_slot_(public_slot.Pass()),
+      private_slot_(private_slot.Pass()),
+      observer_list_(new ObserverListThreadSafe<Observer>),
       weak_factory_(this) {
+  DCHECK(public_slot_);
+  DCHECK(private_slot_);
+
   // This also makes sure that NSS has been initialized.
   CertDatabase* cert_db = CertDatabase::GetInstance();
   cert_notification_forwarder_.reset(new CertNotificationForwarder(cert_db));
@@ -140,11 +131,11 @@ void NSSCertDatabase::ListCertsInSlot(const ListCertsCallback& callback,
 }
 
 crypto::ScopedPK11Slot NSSCertDatabase::GetPublicSlot() const {
-  return crypto::ScopedPK11Slot(crypto::GetPersistentNSSKeySlot());
+  return crypto::ScopedPK11Slot(PK11_ReferenceSlot(public_slot_.get()));
 }
 
 crypto::ScopedPK11Slot NSSCertDatabase::GetPrivateSlot() const {
-  return crypto::ScopedPK11Slot(crypto::GetPersistentNSSKeySlot());
+  return crypto::ScopedPK11Slot(PK11_ReferenceSlot(private_slot_.get()));
 }
 
 CryptoModule* NSSCertDatabase::GetPublicModule() const {

net/cert/nss_cert_database.h

@@ -20,7 +20,6 @@
 #include "net/cert/x509_certificate.h"
 
 namespace base {
-template <typename T> struct DefaultLazyInstanceTraits;
 class TaskRunner;
 }
 template <class ObserverType> class ObserverListThreadSafe;
@@ -35,7 +34,6 @@ typedef std::vector<scoped_refptr<CryptoModule> > CryptoModuleList;
 // singleton.
 class NET_EXPORT NSSCertDatabase {
  public:
-
   class NET_EXPORT Observer {
    public:
     virtual ~Observer() {}
@@ -102,8 +100,17 @@ class NET_EXPORT NSSCertDatabase {
 
   typedef base::Callback<void(bool)> DeleteCertCallback;
 
-  // DEPRECATED: See http://crbug.com/329735.
-  static NSSCertDatabase* GetInstance();
+  // Creates a NSSCertDatabase that will store public information (such as
+  // certificates and trust records) in |public_slot|, and private information
+  // (such as keys) in |private_slot|.
+  // In general, code should avoid creating an NSSCertDatabase directly,
+  // as doing so requires making opinionated decisions about where to store
+  // data, and instead prefer to be passed an existing NSSCertDatabase
+  // instance.
+  // Both slots must not be NULL but can be identical.
+  NSSCertDatabase(crypto::ScopedPK11Slot public_slot,
+                  crypto::ScopedPK11Slot private_slot);
+  virtual ~NSSCertDatabase();
 
   // Get a list of unique certificates in the certificate database (one
   // instance of all certificates).
@@ -124,10 +131,10 @@ class NET_EXPORT NSSCertDatabase {
                                PK11SlotInfo* slot);
 
   // Get the default slot for public key data.
-  virtual crypto::ScopedPK11Slot GetPublicSlot() const;
+  crypto::ScopedPK11Slot GetPublicSlot() const;
 
   // Get the default slot for private key or mixed private/public key data.
-  virtual crypto::ScopedPK11Slot GetPrivateSlot() const;
+  crypto::ScopedPK11Slot GetPrivateSlot() const;
 
   // Get the default module for public key data.
   // The returned pointer must be stored in a scoped_refptr<CryptoModule>.
@@ -232,9 +239,6 @@ class NET_EXPORT NSSCertDatabase {
       const scoped_refptr<base::TaskRunner>& task_runner);
 
  protected:
-  NSSCertDatabase();
-  virtual ~NSSCertDatabase();
-
   // Certificate listing implementation used by |ListCerts*| and
   // |ListCertsSync|. Static so it may safely be used on the worker thread.
   // If |slot| is NULL, obtains the certs of all slots, otherwise only of
@@ -248,8 +252,6 @@ class NET_EXPORT NSSCertDatabase {
   scoped_refptr<base::TaskRunner> GetSlowTaskRunner() const;
 
  private:
-  friend struct base::DefaultLazyInstanceTraits<NSSCertDatabase>;
-
   // Registers |observer| to receive notifications of certificate changes.  The
   // thread on which this is called is the thread on which |observer| will be
   // called back with notifications.
@@ -277,6 +279,9 @@ class NET_EXPORT NSSCertDatabase {
   // it may safely be used on the worker thread.
   static bool DeleteCertAndKeyImpl(scoped_refptr<X509Certificate> cert);
 
+  crypto::ScopedPK11Slot public_slot_;
+  crypto::ScopedPK11Slot private_slot_;
+
   // A helper observer that forwards events from this database to CertDatabase.
   scoped_ptr<Observer> cert_notification_forwarder_;
 

net/cert/nss_cert_database_chromeos.cc

@@ -21,8 +21,7 @@ namespace net {
 NSSCertDatabaseChromeOS::NSSCertDatabaseChromeOS(
     crypto::ScopedPK11Slot public_slot,
     crypto::ScopedPK11Slot private_slot)
-    : public_slot_(public_slot.Pass()),
-      private_slot_(private_slot.Pass()) {
+    : NSSCertDatabase(public_slot.Pass(), private_slot.Pass()) {
   profile_filter_.Init(GetPublicSlot(), GetPrivateSlot());
 }
 
@@ -46,16 +45,6 @@ void NSSCertDatabaseChromeOS::ListCerts(
       base::Bind(callback, base::Passed(&certs)));
 }
 
-crypto::ScopedPK11Slot NSSCertDatabaseChromeOS::GetPublicSlot() const {
-  return crypto::ScopedPK11Slot(
-      public_slot_ ? PK11_ReferenceSlot(public_slot_.get()) : NULL);
-}
-
-crypto::ScopedPK11Slot NSSCertDatabaseChromeOS::GetPrivateSlot() const {
-  return crypto::ScopedPK11Slot(
-      private_slot_ ? PK11_ReferenceSlot(private_slot_.get()) : NULL);
-}
-
 void NSSCertDatabaseChromeOS::ListModules(CryptoModuleList* modules,
                                           bool need_rw) const {
   NSSCertDatabase::ListModules(modules, need_rw);

net/cert/nss_cert_database_chromeos.h

@@ -24,8 +24,6 @@ class NET_EXPORT NSSCertDatabaseChromeOS : public NSSCertDatabase {
   virtual void ListCertsSync(CertificateList* certs) OVERRIDE;
   virtual void ListCerts(const NSSCertDatabase::ListCertsCallback& callback)
       OVERRIDE;
-  virtual crypto::ScopedPK11Slot GetPublicSlot() const OVERRIDE;
-  virtual crypto::ScopedPK11Slot GetPrivateSlot() const OVERRIDE;
   virtual void ListModules(CryptoModuleList* modules, bool need_rw) const
       OVERRIDE;
 
@@ -41,8 +39,6 @@ class NET_EXPORT NSSCertDatabaseChromeOS : public NSSCertDatabase {
   static void ListCertsImpl(const NSSProfileFilterChromeOS& profile_filter,
                             CertificateList* certs);
 
-  crypto::ScopedPK11Slot public_slot_;
-  crypto::ScopedPK11Slot private_slot_;
   NSSProfileFilterChromeOS profile_filter_;
 
   DISALLOW_COPY_AND_ASSIGN(NSSCertDatabaseChromeOS);