Make NSSCertDatabase::ListCerts work async on a worker thread.

net/cert/nss_cert_database.h

Index: net/cert/nss_cert_database.h
diff --git a/net/cert/nss_cert_database.h b/net/cert/nss_cert_database.h
index 0707b8d8c619c3bc3ba088414341cee3a39acc42..5bfc70cd03269335ebe5a6ad2786369b42eacad9 100644
--- a/net/cert/nss_cert_database.h
+++ b/net/cert/nss_cert_database.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string16.h"
 #include "crypto/scoped_nss_types.h"
@@ -18,9 +19,19 @@
 
 namespace base {
 template <typename T> struct DefaultLazyInstanceTraits;
+class TaskRunner;
 }
 template <class ObserverType> class ObserverListThreadSafe;
 
+#if defined(OS_CHROMEOS)
+namespace chromeos {
+class CertLoaderTest;
+class ClientCertResolverTest;
+class NetworkCertMigratorTest;
+class NetworkConnectionHandlerTest;
+}
+#endif

[Ryan Sleevi, 2014/01/29 02:24:20]

Ok, good example of where ForTest() was cleaner :)

Feel free to push back more in the future

[tbarzic, 2014/01/29 20:50:52]

removed :)

+
 namespace net {
 
 class CryptoModule;
@@ -96,7 +107,13 @@ class NET_EXPORT NSSCertDatabase {
 
   // Get a list of unique certificates in the certificate database (one
   // instance of all certificates).

[Ryan Sleevi, 2014/01/29 02:24:20]

Is this deprecated? It would seem it should be.

[tbarzic, 2014/01/29 20:50:52]

yep

-  virtual void ListCerts(CertificateList* certs);
+  virtual void ListCertsSync(CertificateList* certs);
+
+  // Asynchronously get a list of unique certificates in the certificate
+  // database (one instance of all certificates).
+  typedef base::Callback<void(scoped_ptr<CertificateList> certs)>
+      ListCertsCallback;

[Ryan Sleevi, 2014/01/29 02:24:20]

Move typedef to line 104 (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
)

Design: Why does this API receive a scoped_ptr<CertificateList>? Why not just
have it be const CertificateList&?

The motivation (perhaps misguided) of having ListCertsSync was to avoid a
reallocation of a vector, but that's unavoidable here with the async ListCerts,
so the API doesn't make sense there.

[tbarzic, 2014/01/29 20:50:52]

The motivation was to avoid copying the vector while passing it between threads
(by passing around a pointer), and not having to wrap the callback passed to
|ListCerts| with a method that would call it with a reference instead of a
pointer.

Though, having a wrapper around |callback| would be useful for handling case
where the databasae is gone by the time callback triggers, which could happen
for the NSSCertDatabaseChromeOS.

btw. what do you think is the right thing to do in the case the callback is
scheduled after the database is deleted?

[Ryan Sleevi, 2014/01/30 04:16:30]

"Don't allow that"?

Otherwise, use a WeakPtrFactory as an internal implementation detail, although
that creates thread affinity issues.

You're not calling back into this class though, nor its members, so I'm not sure
what the problem would be.

[tbarzic, 2014/02/03 23:42:04]

ah, sorry for a bit vague question. There's no problem in NSSCertDatabase per
se. I was more concerned about caller assuming that the database is still around
when the callback runs.. something like:

database->ListCerts(base::Bind(&OnCertList, database));

void OnCertList(database, cert_list) {
  // do something with database
}

What I meant to ask is whether you'd prefer not to invoke the callback if the
database is gone instead of relying on ListCert users to do the right thing. The
thing I don't like about that approach is exactly what you mentioned: stricter
threading constraints due to use of weak ptrs (i.e. ListCert could not be called
from the UI thread).

[Ryan Sleevi, 2014/02/04 02:50:20]

Yeah, that's "caller's responsibility, so don't do that if it's dangerous"

+  virtual void ListCerts(const ListCertsCallback& callback);
 
   // Get the default slot for public key data.
   virtual crypto::ScopedPK11Slot GetPublicSlot() const;
@@ -213,14 +230,41 @@ class NET_EXPORT NSSCertDatabase {
   NSSCertDatabase();
   virtual ~NSSCertDatabase();
 
+  // Certificate listing implementation used by |ListCerts| and |ListCertsSync|.
+  // Static so it may safelly be used on the worker thread.
+  static void ListCertsImpl(CertificateList* certs);
+
+  // Gets task runner that should be use for slow tasks like certificate
+  // listing. Defaults to a base::WorkerPool runner, but may be overriden
+  // in tests (see SetSlowTaskRunnerForTest).
+  scoped_refptr<base::TaskRunner> GetSlowTaskRunner() const;
+
  private:
   friend struct base::DefaultLazyInstanceTraits<NSSCertDatabase>;
 
+  // Friend test that use |SetSlowTaskRunnerForTest|.
+  friend class NSSCertDatabaseChromeOSTest;
+  FRIEND_TEST_ALL_PREFIXES(CertDatabaseNSSTest, ListCerts);
+
+#if defined(OS_CHROMEOS)
+  friend class chromeos::CertLoaderTest;
+  friend class chromeos::ClientCertResolverTest;
+  friend class chromeos::NetworkCertMigratorTest;
+  friend class chromeos::NetworkConnectionHandlerTest;
+#endif
+
   // Broadcasts notifications to all registered observers.
   void NotifyObserversOfCertAdded(const X509Certificate* cert);
   void NotifyObserversOfCertRemoved(const X509Certificate* cert);
   void NotifyObserversOfCACertChanged(const X509Certificate* cert);
 
+  // Overrides task runner that's used for running slow tasks.
+  void SetSlowTaskRunnerForTest(
+      const scoped_refptr<base::TaskRunner>& task_runner);
+
+  // Task runner that should be used in tests if set.
+  scoped_refptr<base::TaskRunner> slow_task_runner_for_test_;
+
   const scoped_refptr<ObserverListThreadSafe<Observer> > observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(NSSCertDatabase);