Properly handle asynchronous lookups from the ServerBoundCertStore.

r216233 introduced an issue where, upon successful asynchronous completion
of a lookup in the ServerBoundCertStore, the ServerBoundCertService would
return the existing domain-bound cert for the domain to the caller,
then generate a new domain-bound cert for that domain.
Fix it, by returning without generating a new cert upon successful
asynchronous store lookup, and add unit tests to prevent this in the future.

Additionally, r216233 introduced debug checks on shutdown in the asynchronous
store tests, which this fixes.

BUG=259097

Review URL: https://chromiumcodereview.appspot.com/22603004

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

3 files changed, 22 insertions, 15 deletions

diff --git a/net/ssl/server_bound_cert_service.cc b/net/ssl/server_bound_cert_service.cc
index 434f8ad..4bc82ed 100644
--- a/net/ssl/server_bound_cert_service.cc
+++ b/net/ssl/server_bound_cert_service.cc
@@ -366,7 +366,8 @@ ServerBoundCertService::ServerBoundCertService(
       weak_ptr_factory_(this),
       requests_(0),
       cert_store_hits_(0),
-      inflight_joins_(0) {
+      inflight_joins_(0),
+      workers_created_(0) {
   base::Time start = base::Time::Now();
   base::Time end = start + base::TimeDelta::FromDays(
       kValidityPeriodInDays + kSystemTimeValidityBufferInDays);
@@ -455,6 +456,7 @@ int ServerBoundCertService::GetDomainBoundCert(
 
   if (err == ERR_FILE_NOT_FOUND) {
     // Sync lookup did not find a valid cert.  Start generating a new one.
+    workers_created_++;
     ServerBoundCertServiceWorker* worker = new ServerBoundCertServiceWorker(
         domain,
         base::Bind(&ServerBoundCertService::GeneratedServerBoundCert,
@@ -507,8 +509,10 @@ void ServerBoundCertService::GotServerBoundCert(
     cert_store_hits_++;
     // ServerBoundCertServiceRequest::Post will do the histograms and stuff.
     HandleResult(OK, server_identifier, key, cert);
+    return;
   }
   // Async lookup did not find a valid cert. Start generating a new one.
+  workers_created_++;
   ServerBoundCertServiceWorker* worker = new ServerBoundCertServiceWorker(
       server_identifier,
       base::Bind(&ServerBoundCertService::GeneratedServerBoundCert,
diff --git a/net/ssl/server_bound_cert_service.h b/net/ssl/server_bound_cert_service.h
index 5734a4c..d931ec8 100644
--- a/net/ssl/server_bound_cert_service.h
+++ b/net/ssl/server_bound_cert_service.h
@@ -121,6 +121,7 @@ class NET_EXPORT ServerBoundCertService
   uint64 requests() const { return requests_; }
   uint64 cert_store_hits() const { return cert_store_hits_; }
   uint64 inflight_joins() const { return inflight_joins_; }
+  uint64 workers_created() const { return workers_created_; }
 
  private:
   // Cancels the specified request. |req| is the handle stored by
@@ -153,6 +154,7 @@ class NET_EXPORT ServerBoundCertService
   uint64 requests_;
   uint64 cert_store_hits_;
   uint64 inflight_joins_;
+  uint64 workers_created_;
 
   bool is_system_time_valid_;
 
diff --git a/net/ssl/server_bound_cert_service_unittest.cc b/net/ssl/server_bound_cert_service_unittest.cc
index e0ee18f..0063f49 100644
--- a/net/ssl/server_bound_cert_service_unittest.cc
+++ b/net/ssl/server_bound_cert_service_unittest.cc
@@ -510,10 +510,8 @@ TEST_F(ServerBoundCertServiceTest, Expiration) {
 TEST_F(ServerBoundCertServiceTest, AsyncStoreGetNoCertsInStore) {
   MockServerBoundCertStoreWithAsyncGet* mock_store =
       new MockServerBoundCertStoreWithAsyncGet();
-  scoped_refptr<base::SequencedWorkerPool> sequenced_worker_pool(
-      new base::SequencedWorkerPool(3, "ServerBoundCertServiceTest"));
-  scoped_ptr<ServerBoundCertService> service(
-      new ServerBoundCertService(mock_store, sequenced_worker_pool));
+  service_ = scoped_ptr<ServerBoundCertService>(
+      new ServerBoundCertService(mock_store, sequenced_worker_pool_));
 
   std::string host("encrypted.google.com");
 
@@ -523,8 +521,8 @@ TEST_F(ServerBoundCertServiceTest, AsyncStoreGetNoCertsInStore) {
 
   // Asynchronous completion with no certs in the store.
   std::string private_key_info, der_cert;
-  EXPECT_EQ(0, service->cert_count());
-  error = service->GetDomainBoundCert(
+  EXPECT_EQ(0, service_->cert_count());
+  error = service_->GetDomainBoundCert(
       host, &private_key_info, &der_cert, callback.callback(), &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
   EXPECT_TRUE(request_handle.is_active());
@@ -534,7 +532,7 @@ TEST_F(ServerBoundCertServiceTest, AsyncStoreGetNoCertsInStore) {
 
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
-  EXPECT_EQ(1, service->cert_count());
+  EXPECT_EQ(1, service_->cert_count());
   EXPECT_FALSE(private_key_info.empty());
   EXPECT_FALSE(der_cert.empty());
   EXPECT_FALSE(request_handle.is_active());
@@ -543,10 +541,8 @@ TEST_F(ServerBoundCertServiceTest, AsyncStoreGetNoCertsInStore) {
 TEST_F(ServerBoundCertServiceTest, AsyncStoreGetOneCertInStore) {
   MockServerBoundCertStoreWithAsyncGet* mock_store =
       new MockServerBoundCertStoreWithAsyncGet();
-  scoped_refptr<base::SequencedWorkerPool> sequenced_worker_pool(
-      new base::SequencedWorkerPool(3, "ServerBoundCertServiceTest"));
-  scoped_ptr<ServerBoundCertService> service(
-      new ServerBoundCertService(mock_store, sequenced_worker_pool));
+  service_ = scoped_ptr<ServerBoundCertService>(
+      new ServerBoundCertService(mock_store, sequenced_worker_pool_));
 
   std::string host("encrypted.google.com");
 
@@ -556,8 +552,8 @@ TEST_F(ServerBoundCertServiceTest, AsyncStoreGetOneCertInStore) {
 
   // Asynchronous completion with a cert in the store.
   std::string private_key_info, der_cert;
-  EXPECT_EQ(0, service->cert_count());
-  error = service->GetDomainBoundCert(
+  EXPECT_EQ(0, service_->cert_count());
+  error = service_->GetDomainBoundCert(
       host, &private_key_info, &der_cert, callback.callback(), &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
   EXPECT_TRUE(request_handle.is_active());
@@ -567,7 +563,12 @@ TEST_F(ServerBoundCertServiceTest, AsyncStoreGetOneCertInStore) {
 
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
-  EXPECT_EQ(1, service->cert_count());
+  EXPECT_EQ(1, service_->cert_count());
+  EXPECT_EQ(1u, service_->requests());
+  EXPECT_EQ(1u, service_->cert_store_hits());
+  // Because the cert was found in the store, no new workers should have been
+  // created.
+  EXPECT_EQ(0u, service_->workers_created());
   EXPECT_STREQ("ab", private_key_info.c_str());
   EXPECT_STREQ("cd", der_cert.c_str());
   EXPECT_FALSE(request_handle.is_active());