Decouple ServerBoundCertServiceWorker from ServerBoundCertService, remove explicit cancellation.

BUG=none


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

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

2 files changed, 108 insertions, 195 deletions

diff --git a/net/base/server_bound_cert_service.cc b/net/base/server_bound_cert_service.cc
index 86c1c57..52e3d42 100644
--- a/net/base/server_bound_cert_service.cc
+++ b/net/base/server_bound_cert_service.cc
@@ -14,7 +14,7 @@
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
@@ -23,7 +23,6 @@
 #include "googleurl/src/gurl.h"
 #include "net/base/net_errors.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
-#include "net/base/server_bound_cert_store.h"
 #include "net/base/x509_certificate.h"
 #include "net/base/x509_util.h"
 
@@ -89,6 +88,69 @@ void RecordGetCertTime(base::TimeDelta request_time) {
                              50);
 }
 
+// On success, returns a ServerBoundCert object and sets |*error| to OK.
+// Otherwise, returns NULL, and |*error| will be set to a net error code.
+// |serial_number| is passed in because base::RandInt cannot be called from an
+// unjoined thread, due to relying on a non-leaked LazyInstance
+scoped_ptr<ServerBoundCertStore::ServerBoundCert> GenerateCert(
+    const std::string& server_identifier,
+    SSLClientCertType type,
+    uint32 serial_number,
+    int* error) {
+  scoped_ptr<ServerBoundCertStore::ServerBoundCert> result;
+
+  base::TimeTicks start = base::TimeTicks::Now();
+  base::Time not_valid_before = base::Time::Now();
+  base::Time not_valid_after =
+      not_valid_before + base::TimeDelta::FromDays(kValidityPeriodInDays);
+  std::string der_cert;
+  std::vector<uint8> private_key_info;
+  switch (type) {
+    case CLIENT_CERT_ECDSA_SIGN: {
+      scoped_ptr<crypto::ECPrivateKey> key(crypto::ECPrivateKey::Create());
+      if (!key.get()) {
+        DLOG(ERROR) << "Unable to create key pair for client";
+        *error = ERR_KEY_GENERATION_FAILED;
+        return result.Pass();
+      }
+      if (!x509_util::CreateDomainBoundCertEC(key.get(), server_identifier,
+                                              serial_number, not_valid_before,
+                                              not_valid_after, &der_cert)) {
+        DLOG(ERROR) << "Unable to create x509 cert for client";
+        *error = ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED;
+        return result.Pass();
+      }
+
+      if (!key->ExportEncryptedPrivateKey(ServerBoundCertService::kEPKIPassword,
+                                          1, &private_key_info)) {
+        DLOG(ERROR) << "Unable to export private key";
+        *error = ERR_PRIVATE_KEY_EXPORT_FAILED;
+        return result.Pass();
+      }
+      break;
+    }
+    default:
+      NOTREACHED();
+      *error = ERR_INVALID_ARGUMENT;
+      return result.Pass();
+  }
+
+  // TODO(rkn): Perhaps ExportPrivateKey should be changed to output a
+  // std::string* to prevent this copying.
+  std::string key_out(private_key_info.begin(), private_key_info.end());
+
+  result.reset(new ServerBoundCertStore::ServerBoundCert(
+      server_identifier, type, not_valid_before, not_valid_after, key_out,
+      der_cert));
+  UMA_HISTOGRAM_CUSTOM_TIMES("DomainBoundCerts.GenerateCertTime",
+                             base::TimeTicks::Now() - start,
+                             base::TimeDelta::FromMilliseconds(1),
+                             base::TimeDelta::FromMinutes(5),
+                             50);
+  *error = OK;
+  return result.Pass();
+}
+
 }  // namespace
 
 // Represents the output and result callback of a request.
@@ -166,52 +228,42 @@ class ServerBoundCertServiceRequest {
 };
 
 // ServerBoundCertServiceWorker runs on a worker thread and takes care of the
-// blocking process of performing key generation. Deletes itself eventually
-// if Start() succeeds.
-// TODO(mattm): don't use locking and explicit cancellation.
+// blocking process of performing key generation. Will take care of deleting
+// itself once Start() is called.
 class ServerBoundCertServiceWorker {
  public:
+  typedef base::Callback<void(
+      const std::string&,
+      int,
+      scoped_ptr<ServerBoundCertStore::ServerBoundCert>)> WorkerDoneCallback;
+
   ServerBoundCertServiceWorker(
       const std::string& server_identifier,
       SSLClientCertType type,
-      ServerBoundCertService* server_bound_cert_service)
+      const WorkerDoneCallback& callback)
       : server_identifier_(server_identifier),
         type_(type),
         serial_number_(base::RandInt(0, std::numeric_limits<int>::max())),
-        origin_loop_(MessageLoop::current()),
-        server_bound_cert_service_(server_bound_cert_service),
-        canceled_(false),
-        error_(ERR_FAILED) {
+        origin_loop_(base::MessageLoopProxy::current()),
+        callback_(callback) {
   }
 
   bool Start(const scoped_refptr<base::TaskRunner>& task_runner) {
-    DCHECK_EQ(MessageLoop::current(), origin_loop_);
+    DCHECK(origin_loop_->RunsTasksOnCurrentThread());
 
     return task_runner->PostTask(
         FROM_HERE,
-        base::Bind(&ServerBoundCertServiceWorker::Run, base::Unretained(this)));
-  }
-
-  // Cancel is called from the origin loop when the ServerBoundCertService is
-  // getting deleted.
-  void Cancel() {
-    DCHECK_EQ(MessageLoop::current(), origin_loop_);
-    base::AutoLock locked(lock_);
-    canceled_ = true;
+        base::Bind(&ServerBoundCertServiceWorker::Run, base::Owned(this)));
   }
 
  private:
   void Run() {
     // Runs on a worker thread.
-    error_ = ServerBoundCertService::GenerateCert(server_identifier_,
-                                                  type_,
-                                                  serial_number_,
-                                                  &creation_time_,
-                                                  &expiration_time_,
-                                                  &private_key_,
-                                                  &cert_);
+    int error = ERR_FAILED;
+    scoped_ptr<ServerBoundCertStore::ServerBoundCert> cert =
+        GenerateCert(server_identifier_, type_, serial_number_, &error);
     DVLOG(1) << "GenerateCert " << server_identifier_ << " " << type_
-             << " returned " << error_;
+             << " returned " << error;
 #if defined(USE_NSS)
     // Detach the thread from NSPR.
     // Calling NSS functions attaches the thread to NSPR, which stores
@@ -222,73 +274,18 @@ class ServerBoundCertServiceWorker {
     // destructors run.
     PR_DetachThread();
 #endif
-    Finish();
-  }
-
-  // DoReply runs on the origin thread.
-  void DoReply() {
-    DCHECK_EQ(MessageLoop::current(), origin_loop_);
-    {
-      // We lock here because the worker thread could still be in Finished,
-      // after the PostTask, but before unlocking |lock_|. If we do not lock in
-      // this case, we will end up deleting a locked Lock, which can lead to
-      // memory leaks or worse errors.
-      base::AutoLock locked(lock_);
-      if (!canceled_) {
-        server_bound_cert_service_->HandleResult(
-            server_identifier_, error_, type_, creation_time_, expiration_time_,
-            private_key_, cert_);
-      }
-    }
-    delete this;
-  }
-
-  void Finish() {
-    // Runs on the worker thread.
-    // We assume that the origin loop outlives the ServerBoundCertService. If
-    // the ServerBoundCertService is deleted, it will call Cancel on us. If it
-    // does so before the Acquire, we'll delete ourselves and return. If it's
-    // trying to do so concurrently, then it'll block on the lock and we'll
-    // call PostTask while the ServerBoundCertService (and therefore the
-    // MessageLoop) is still alive. If it does so after this function, we
-    // assume that the MessageLoop will process pending tasks. In which case
-    // we'll notice the |canceled_| flag in DoReply.
-
-    bool canceled;
-    {
-      base::AutoLock locked(lock_);
-      canceled = canceled_;
-      if (!canceled) {
-        origin_loop_->PostTask(
-            FROM_HERE, base::Bind(&ServerBoundCertServiceWorker::DoReply,
-                                  base::Unretained(this)));
-      }
-    }
-    if (canceled)
-      delete this;
+    origin_loop_->PostTask(FROM_HERE,
+                           base::Bind(callback_, server_identifier_, error,
+                                      base::Passed(&cert)));
   }
 
   const std::string server_identifier_;
   const SSLClientCertType type_;
   // Note that serial_number_ must be initialized on a non-worker thread
-  // (see documentation for ServerBoundCertService::GenerateCert).
+  // (see documentation for GenerateCert).
   uint32 serial_number_;
-  MessageLoop* const origin_loop_;
-  ServerBoundCertService* const server_bound_cert_service_;
-
-  // lock_ protects canceled_.
-  base::Lock lock_;
-
-  // If canceled_ is true,
-  // * origin_loop_ cannot be accessed by the worker thread,
-  // * server_bound_cert_service_ cannot be accessed by any thread.
-  bool canceled_;
-
-  int error_;
-  base::Time creation_time_;
-  base::Time expiration_time_;
-  std::string private_key_;
-  std::string cert_;
+  scoped_refptr<base::SequencedTaskRunner> origin_loop_;
+  WorkerDoneCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ServerBoundCertServiceWorker);
 };
@@ -298,16 +295,12 @@ class ServerBoundCertServiceWorker {
 // origin message loop.
 class ServerBoundCertServiceJob {
  public:
-  ServerBoundCertServiceJob(ServerBoundCertServiceWorker* worker,
-                            SSLClientCertType type)
-      : worker_(worker), type_(type) {
+  ServerBoundCertServiceJob(SSLClientCertType type) : type_(type) {
   }
 
   ~ServerBoundCertServiceJob() {
-    if (worker_) {
-      worker_->Cancel();
+    if (!requests_.empty())
       DeleteAllCanceled();
-    }
   }
 
   SSLClientCertType type() const { return type_; }
@@ -320,7 +313,6 @@ class ServerBoundCertServiceJob {
                     SSLClientCertType type,
                     const std::string& private_key,
                     const std::string& cert) {
-    worker_ = NULL;
     PostAll(error, type, private_key, cert);
   }
 
@@ -351,7 +343,6 @@ class ServerBoundCertServiceJob {
   }
 
   std::vector<ServerBoundCertServiceRequest*> requests_;
-  ServerBoundCertServiceWorker* worker_;
   SSLClientCertType type_;
 };
 
@@ -363,6 +354,7 @@ ServerBoundCertService::ServerBoundCertService(
     const scoped_refptr<base::TaskRunner>& task_runner)
     : server_bound_cert_store_(server_bound_cert_store),
       task_runner_(task_runner),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
       requests_(0),
       cert_store_hits_(0),
       inflight_joins_(0) {
@@ -486,16 +478,15 @@ int ServerBoundCertService::GetDomainBoundCert(
     ServerBoundCertServiceWorker* worker = new ServerBoundCertServiceWorker(
             domain,
             preferred_type,
-            this);
-    job = new ServerBoundCertServiceJob(worker, preferred_type);
+            base::Bind(&ServerBoundCertService::HandleResult,
+                       weak_ptr_factory_.GetWeakPtr()));
     if (!worker->Start(task_runner_)) {
-      delete job;
-      delete worker;
       // TODO(rkn): Log to the NetLog.
       LOG(ERROR) << "ServerBoundCertServiceWorker couldn't be started.";
       RecordGetDomainBoundCertResult(WORKER_FAILURE);
       return ERR_INSUFFICIENT_RESOURCES;  // Just a guess.
     }
+    job = new ServerBoundCertServiceJob(preferred_type);
     inflight_[domain] = job;
   }
 
@@ -510,66 +501,6 @@ ServerBoundCertStore* ServerBoundCertService::GetCertStore() {
   return server_bound_cert_store_.get();
 }
 
-// static
-int ServerBoundCertService::GenerateCert(const std::string& server_identifier,
-                                         SSLClientCertType type,
-                                         uint32 serial_number,
-                                         base::Time* creation_time,
-                                         base::Time* expiration_time,
-                                         std::string* private_key,
-                                         std::string* cert) {
-  base::TimeTicks start = base::TimeTicks::Now();
-  base::Time not_valid_before = base::Time::Now();
-  base::Time not_valid_after =
-      not_valid_before + base::TimeDelta::FromDays(kValidityPeriodInDays);
-  std::string der_cert;
-  std::vector<uint8> private_key_info;
-  switch (type) {
-    case CLIENT_CERT_ECDSA_SIGN: {
-      scoped_ptr<crypto::ECPrivateKey> key(crypto::ECPrivateKey::Create());
-      if (!key.get()) {
-        DLOG(ERROR) << "Unable to create key pair for client";
-        return ERR_KEY_GENERATION_FAILED;
-      }
-      if (!x509_util::CreateDomainBoundCertEC(
-          key.get(),
-          server_identifier,
-          serial_number,
-          not_valid_before,
-          not_valid_after,
-          &der_cert)) {
-        DLOG(ERROR) << "Unable to create x509 cert for client";
-        return ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED;
-      }
-
-      if (!key->ExportEncryptedPrivateKey(
-          kEPKIPassword, 1, &private_key_info)) {
-        DLOG(ERROR) << "Unable to export private key";
-        return ERR_PRIVATE_KEY_EXPORT_FAILED;
-      }
-      break;
-    }
-    default:
-      NOTREACHED();
-      return ERR_INVALID_ARGUMENT;
-  }
-
-  // TODO(rkn): Perhaps ExportPrivateKey should be changed to output a
-  // std::string* to prevent this copying.
-  std::string key_out(private_key_info.begin(), private_key_info.end());
-
-  private_key->swap(key_out);
-  cert->swap(der_cert);
-  *creation_time = not_valid_before;
-  *expiration_time = not_valid_after;
-  UMA_HISTOGRAM_CUSTOM_TIMES("DomainBoundCerts.GenerateCertTime",
-                             base::TimeTicks::Now() - start,
-                             base::TimeDelta::FromMilliseconds(1),
-                             base::TimeDelta::FromMinutes(5),
-                             50);
-  return OK;
-}
-
 void ServerBoundCertService::CancelRequest(RequestHandle req) {
   DCHECK(CalledOnValidThread());
   ServerBoundCertServiceRequest* request =
@@ -579,19 +510,18 @@ void ServerBoundCertService::CancelRequest(RequestHandle req) {
 
 // HandleResult is called by ServerBoundCertServiceWorker on the origin message
 // loop. It deletes ServerBoundCertServiceJob.
-void ServerBoundCertService::HandleResult(const std::string& server_identifier,
-                                          int error,
-                                          SSLClientCertType type,
-                                          base::Time creation_time,
-                                          base::Time expiration_time,
-                                          const std::string& private_key,
-                                          const std::string& cert) {
+void ServerBoundCertService::HandleResult(
+    const std::string& server_identifier,
+    int error,
+    scoped_ptr<ServerBoundCertStore::ServerBoundCert> cert) {
   DCHECK(CalledOnValidThread());
 
   if (error == OK) {
+    // TODO(mattm): we should just Pass() the cert object to
+    // SetServerBoundCert().
     server_bound_cert_store_->SetServerBoundCert(
-        server_identifier, type, creation_time, expiration_time, private_key,
-        cert);
+        cert->server_identifier(), cert->type(), cert->creation_time(),
+        cert->expiration_time(), cert->private_key(), cert->cert());
   }
 
   std::map<std::string, ServerBoundCertServiceJob*>::iterator j;
@@ -603,7 +533,10 @@ void ServerBoundCertService::HandleResult(const std::string& server_identifier,
   ServerBoundCertServiceJob* job = j->second;
   inflight_.erase(j);
 
-  job->HandleResult(error, type, private_key, cert);
+  if (cert)
+    job->HandleResult(error, cert->type(), cert->private_key(), cert->cert());
+  else
+    job->HandleResult(error, CLIENT_CERT_INVALID_TYPE, "", "");
   delete job;
 }
 
diff --git a/net/base/server_bound_cert_service.h b/net/base/server_bound_cert_service.h
index 94ca21b..e02fbb8 100644
--- a/net/base/server_bound_cert_service.h
+++ b/net/base/server_bound_cert_service.h
@@ -11,10 +11,12 @@
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_export.h"
+#include "net/base/server_bound_cert_store.h"
 #include "net/base/ssl_client_cert_type.h"
 
 namespace base {
@@ -25,7 +27,6 @@ namespace net {
 
 class ServerBoundCertServiceJob;
 class ServerBoundCertServiceWorker;
-class ServerBoundCertStore;
 
 // A class for creating and fetching server bound certs.
 // Inherits from NonThreadSafe in order to use the function
@@ -103,31 +104,9 @@ class NET_EXPORT ServerBoundCertService
   uint64 inflight_joins() const { return inflight_joins_; }
 
  private:
-  friend class ServerBoundCertServiceWorker;  // Calls HandleResult.
-
-  // On success, |private_key| stores a DER-encoded PrivateKeyInfo
-  // struct, |cert| stores a DER-encoded certificate, |creation_time| stores the
-  // start of the validity period of the certificate and |expiration_time|
-  // stores the expiration time of the certificate. Returns OK if successful and
-  // an error code otherwise.
-  // |serial_number| is passed in because it is created with the function
-  // base::RandInt, which opens the file /dev/urandom. /dev/urandom is opened
-  // with a LazyInstance, which is not allowed on a worker thread.
-  static int GenerateCert(const std::string& server_identifier,
-                          SSLClientCertType type,
-                          uint32 serial_number,
-                          base::Time* creation_time,
-                          base::Time* expiration_time,
-                          std::string* private_key,
-                          std::string* cert);
-
   void HandleResult(const std::string& server_identifier,
                     int error,
-                    SSLClientCertType type,
-                    base::Time creation_time,
-                    base::Time expiration_time,
-                    const std::string& private_key,
-                    const std::string& cert);
+                    scoped_ptr<ServerBoundCertStore::ServerBoundCert> cert);
 
   scoped_ptr<ServerBoundCertStore> server_bound_cert_store_;
   scoped_refptr<base::TaskRunner> task_runner_;
@@ -135,6 +114,7 @@ class NET_EXPORT ServerBoundCertService
   // inflight_ maps from a server to an active generation which is taking
   // place.
   std::map<std::string, ServerBoundCertServiceJob*> inflight_;
+  base::WeakPtrFactory<ServerBoundCertService> weak_ptr_factory_;
 
   uint64 requests_;
   uint64 cert_store_hits_;