Finish removing NSSCertDatabase::ListCertsSync

It was only used by tests at this point.

Bug: 340460
Change-Id: I1a52c17d439bfb5a198a908aee0fe7600bb7845d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1846468Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Auto-Submit: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704739}

chrome/browser/chromeos/policy/policy_certs_browsertest.cc

@@ -362,14 +362,11 @@ bool HasSubjectCommonName(CERTCertificate* cert_handle,
   return result;
 }
 
-void IsCertInNSSDatabaseOnIOThreadWithCertDb(
+void IsCertInNSSDatabaseOnIOThreadWithCertList(
     const std::string& subject_common_name,
     bool* out_system_slot_available,
     base::OnceClosure done_closure,
-    net::NSSCertDatabase* cert_db) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  base::ScopedAllowBlockingForTesting scoped_allow_blocking_for_testing;
-  net::ScopedCERTCertificateList certs = cert_db->ListCertsSync();
+    net::ScopedCERTCertificateList certs) {
   for (const net::ScopedCERTCertificate& cert : certs) {
     if (HasSubjectCommonName(cert.get(), subject_common_name)) {
       *out_system_slot_available = true;
@@ -379,6 +376,17 @@ void IsCertInNSSDatabaseOnIOThreadWithCertDb(
   std::move(done_closure).Run();
 }
 
+void IsCertInNSSDatabaseOnIOThreadWithCertDb(
+    const std::string& subject_common_name,
+    bool* out_system_slot_available,
+    base::OnceClosure done_closure,
+    net::NSSCertDatabase* cert_db) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  cert_db->ListCerts(base::BindOnce(
+      &IsCertInNSSDatabaseOnIOThreadWithCertList, subject_common_name,
+      out_system_slot_available, std::move(done_closure)));
+}
+
 void IsCertInNSSDatabaseOnIOThread(content::ResourceContext* resource_context,
                                    const std::string& subject_common_name,
                                    bool* out_cert_found,

net/cert/nss_cert_database.cc

@@ -82,10 +82,6 @@ NSSCertDatabase::NSSCertDatabase(crypto::ScopedPK11Slot public_slot,
 
 NSSCertDatabase::~NSSCertDatabase() = default;
 
-ScopedCERTCertificateList NSSCertDatabase::ListCertsSync() {
-  return ListCertsImpl(crypto::ScopedPK11Slot());
-}
-
 void NSSCertDatabase::ListCerts(ListCertsCallback callback) {
   base::PostTaskAndReplyWithResult(
       FROM_HERE,

net/cert/nss_cert_database.h

@@ -106,11 +106,6 @@ class NET_EXPORT NSSCertDatabase {
                   crypto::ScopedPK11Slot private_slot);
   virtual ~NSSCertDatabase();
 
-  // Get a list of unique certificates in the certificate database (one
-  // instance of all certificates).
-  // DEPRECATED by |ListCerts|. See http://crbug.com/340460.
-  virtual ScopedCERTCertificateList ListCertsSync();
-
   // Asynchronously get a list of unique certificates in the certificate
   // database (one instance of all certificates). Note that the callback may be
   // run even after the database is deleted.
@@ -240,10 +235,9 @@ class NET_EXPORT NSSCertDatabase {
   bool IsHardwareBacked(const CERTCertificate* cert) const;
 
  protected:
-  // 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
-  // |slot|.
+  // Certificate listing implementation used by |ListCerts*|. Static so it may
+  // safely be used on the worker thread. If |slot| is nullptr, obtains the
+  // certs of all slots, otherwise only of |slot|.
   static ScopedCERTCertificateList ListCertsImpl(crypto::ScopedPK11Slot slot);
 
   // Broadcasts notifications to all registered observers.

net/cert/nss_cert_database_chromeos.cc

@@ -39,10 +39,6 @@ void NSSCertDatabaseChromeOS::SetSystemSlot(
   profile_filter_.Init(GetPublicSlot(), GetPrivateSlot(), GetSystemSlot());
 }
 
-ScopedCERTCertificateList NSSCertDatabaseChromeOS::ListCertsSync() {
-  return ListCertsImpl(profile_filter_);
-}
-
 void NSSCertDatabaseChromeOS::ListCerts(
     NSSCertDatabase::ListCertsCallback callback) {
   base::PostTaskAndReplyWithResult(

net/cert/nss_cert_database_chromeos.h

@@ -26,7 +26,6 @@ class NET_EXPORT NSSCertDatabaseChromeOS : public NSSCertDatabase {
   void SetSystemSlot(crypto::ScopedPK11Slot system_slot);
 
   // NSSCertDatabase implementation.
-  ScopedCERTCertificateList ListCertsSync() override;
   void ListCerts(NSSCertDatabase::ListCertsCallback callback) override;
   void ListModules(std::vector<crypto::ScopedPK11Slot>* modules,
                    bool need_rw) const override;
@@ -37,7 +36,7 @@ class NET_EXPORT NSSCertDatabaseChromeOS : public NSSCertDatabase {
   // TODO(mattm): handle trust setting correctly for certs in read-only slots.
 
  private:
-  // Certificate listing implementation used by |ListCerts| and |ListCertsSync|.
+  // Certificate listing implementation used by |ListCerts|.
   // The certificate list normally returned by NSSCertDatabase::ListCertsImpl
   // is additionally filtered by |profile_filter|.
   // Static so it may safely be used on the worker thread.

net/cert/nss_cert_database_chromeos_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/run_loop.h"
 #include "crypto/nss_util_internal.h"
 #include "crypto/scoped_test_nss_chromeos_user.h"
 #include "crypto/scoped_test_nss_db.h"
@@ -159,39 +160,24 @@ TEST_F(NSSCertDatabaseChromeOSTest, ImportCACerts) {
   EXPECT_EQ(0U, failed.size());
 
   // Get cert list for each user.
-  ScopedCERTCertificateList user_1_certlist = db_1_->ListCertsSync();
-  ScopedCERTCertificateList user_2_certlist = db_2_->ListCertsSync();
-
-  // Check that the imported certs only shows up in the list for the user that
-  // imported them.
-  EXPECT_TRUE(IsCertInCertificateList(certs_1[0].get(), user_1_certlist));
-  EXPECT_FALSE(IsCertInCertificateList(certs_1[0].get(), user_2_certlist));
-
-  EXPECT_TRUE(IsCertInCertificateList(certs_2[0].get(), user_2_certlist));
-  EXPECT_FALSE(IsCertInCertificateList(certs_2[0].get(), user_1_certlist));
-
-  // Run the message loop so the observer notifications get processed.
-  RunUntilIdle();
-  // Should have gotten two OnCertDBChanged notifications.
-  ASSERT_EQ(2, db_changed_count_);
-
-  // Tests that the new certs are loaded by async ListCerts method.
-  ScopedCERTCertificateList user_1_certlist_async;
-  ScopedCERTCertificateList user_2_certlist_async;
+  ScopedCERTCertificateList user_1_certlist;
+  ScopedCERTCertificateList user_2_certlist;
   db_1_->ListCerts(
-      base::BindOnce(&SwapCertLists, base::Unretained(&user_1_certlist_async)));
+      base::BindOnce(&SwapCertLists, base::Unretained(&user_1_certlist)));
   db_2_->ListCerts(
-      base::BindOnce(&SwapCertLists, base::Unretained(&user_2_certlist_async)));
+      base::BindOnce(&SwapCertLists, base::Unretained(&user_2_certlist)));
 
+  // Run the message loop so the observer notifications get processed and
+  // lookups are completed.
   RunUntilIdle();
+  // Should have gotten two OnCertDBChanged notifications.
+  ASSERT_EQ(2, db_changed_count_);
 
-  EXPECT_TRUE(IsCertInCertificateList(certs_1[0].get(), user_1_certlist_async));
-  EXPECT_FALSE(
-      IsCertInCertificateList(certs_1[0].get(), user_2_certlist_async));
+  EXPECT_TRUE(IsCertInCertificateList(certs_1[0].get(), user_1_certlist));
+  EXPECT_FALSE(IsCertInCertificateList(certs_1[0].get(), user_2_certlist));
 
-  EXPECT_TRUE(IsCertInCertificateList(certs_2[0].get(), user_2_certlist_async));
-  EXPECT_FALSE(
-      IsCertInCertificateList(certs_2[0].get(), user_1_certlist_async));
+  EXPECT_TRUE(IsCertInCertificateList(certs_2[0].get(), user_2_certlist));
+  EXPECT_FALSE(IsCertInCertificateList(certs_2[0].get(), user_1_certlist));
 }
 
 // Test that ImportServerCerts imports the cert to the correct slot, and that
@@ -219,40 +205,25 @@ TEST_F(NSSCertDatabaseChromeOSTest, ImportServerCert) {
   EXPECT_EQ(0U, failed.size());
 
   // Get cert list for each user.
-  ScopedCERTCertificateList user_1_certlist = db_1_->ListCertsSync();
-  ScopedCERTCertificateList user_2_certlist = db_2_->ListCertsSync();
-
-  // Check that the imported certs only shows up in the list for the user that
-  // imported them.
-  EXPECT_TRUE(IsCertInCertificateList(certs_1[0].get(), user_1_certlist));
-  EXPECT_FALSE(IsCertInCertificateList(certs_1[0].get(), user_2_certlist));
-
-  EXPECT_TRUE(IsCertInCertificateList(certs_2[0].get(), user_2_certlist));
-  EXPECT_FALSE(IsCertInCertificateList(certs_2[0].get(), user_1_certlist));
+  ScopedCERTCertificateList user_1_certlist;
+  ScopedCERTCertificateList user_2_certlist;
+  db_1_->ListCerts(
+      base::BindOnce(&SwapCertLists, base::Unretained(&user_1_certlist)));
+  db_2_->ListCerts(
+      base::BindOnce(&SwapCertLists, base::Unretained(&user_2_certlist)));
 
-  // Run the message loop so the observer notifications get processed.
+  // Run the message loop so the observer notifications get processed and
+  // lookups are completed.
   RunUntilIdle();
   // TODO(mattm): ImportServerCert doesn't actually cause any observers to
   // fire. Is that correct?
   EXPECT_EQ(0, db_changed_count_);
 
-  // Tests that the new certs are loaded by async ListCerts method.
-  ScopedCERTCertificateList user_1_certlist_async;
-  ScopedCERTCertificateList user_2_certlist_async;
-  db_1_->ListCerts(
-      base::BindOnce(&SwapCertLists, base::Unretained(&user_1_certlist_async)));
-  db_2_->ListCerts(
-      base::BindOnce(&SwapCertLists, base::Unretained(&user_2_certlist_async)));
-
-  RunUntilIdle();
-
-  EXPECT_TRUE(IsCertInCertificateList(certs_1[0].get(), user_1_certlist_async));
-  EXPECT_FALSE(
-      IsCertInCertificateList(certs_1[0].get(), user_2_certlist_async));
+  EXPECT_TRUE(IsCertInCertificateList(certs_1[0].get(), user_1_certlist));
+  EXPECT_FALSE(IsCertInCertificateList(certs_1[0].get(), user_2_certlist));
 
-  EXPECT_TRUE(IsCertInCertificateList(certs_2[0].get(), user_2_certlist_async));
-  EXPECT_FALSE(
-      IsCertInCertificateList(certs_2[0].get(), user_1_certlist_async));
+  EXPECT_TRUE(IsCertInCertificateList(certs_2[0].get(), user_2_certlist));
+  EXPECT_FALSE(IsCertInCertificateList(certs_2[0].get(), user_1_certlist));
 }
 
 // Tests that There is no crash if the database is deleted while ListCerts
@@ -281,7 +252,10 @@ TEST_F(NSSCertDatabaseChromeOSTest, ListCertsReadsSystemSlot) {
                                      "client_2.pem",
                                      "client_2.pk8",
                                      db_1_->GetSystemSlot().get()));
-  ScopedCERTCertificateList certs = db_1_->ListCertsSync();
+
+  ScopedCERTCertificateList certs;
+  db_1_->ListCerts(base::BindOnce(&SwapCertLists, base::Unretained(&certs)));
+  RunUntilIdle();
   EXPECT_TRUE(IsCertInCertificateList(cert_1.get(), certs));
   EXPECT_TRUE(IsCertInCertificateList(cert_2.get(), certs));
 }
@@ -298,7 +272,9 @@ TEST_F(NSSCertDatabaseChromeOSTest, ListCertsDoesNotCrossReadSystemSlot) {
                                      "client_2.pem",
                                      "client_2.pk8",
                                      system_db_.slot()));
-  ScopedCERTCertificateList certs = db_2_->ListCertsSync();
+  ScopedCERTCertificateList certs;
+  db_2_->ListCerts(base::BindOnce(&SwapCertLists, base::Unretained(&certs)));
+  RunUntilIdle();
   EXPECT_TRUE(IsCertInCertificateList(cert_1.get(), certs));
   EXPECT_FALSE(IsCertInCertificateList(cert_2.get(), certs));
 }

net/cert/nss_cert_database_unittest.cc

@@ -134,15 +134,6 @@ class CertDatabaseNSSTest : public TestWithTaskEnvironment {
   scoped_refptr<CRLSet> crl_set_;
 };
 
-TEST_F(CertDatabaseNSSTest, ListCertsSync) {
-  // This test isn't terribly useful, though it might help with memory
-  // leak tests.
-  ScopedCERTCertificateList certs = cert_db_->ListCertsSync();
-  // The test DB is empty, but let's assume there will always be something in
-  // the other slots.
-  EXPECT_LT(0U, certs.size());
-}
-
 TEST_F(CertDatabaseNSSTest, ListCerts) {
   // This test isn't terribly useful, though it might help with memory
   // leak tests.