Fix for SDPY not cancelling ServerBoundCertServiceRequest.

Makes ServerBoundCertService::RequestHandle a real class which handles cancellation on destruction, instead of being just a void* handle.

BUG=167705

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

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

5 files changed, 137 insertions, 53 deletions

diff --git a/net/base/server_bound_cert_service.cc b/net/base/server_bound_cert_service.cc
index 52e3d42..3194a66 100644
--- a/net/base/server_bound_cert_service.cc
+++ b/net/base/server_bound_cert_service.cc
@@ -349,6 +349,38 @@ class ServerBoundCertServiceJob {
 // static
 const char ServerBoundCertService::kEPKIPassword[] = "";
 
+ServerBoundCertService::RequestHandle::RequestHandle()
+    : service_(NULL),
+      request_(NULL) {}
+
+ServerBoundCertService::RequestHandle::~RequestHandle() {
+  Cancel();
+}
+
+void ServerBoundCertService::RequestHandle::Cancel() {
+  if (request_) {
+    service_->CancelRequest(request_);
+    request_ = NULL;
+    callback_.Reset();
+  }
+}
+
+void ServerBoundCertService::RequestHandle::RequestStarted(
+    ServerBoundCertService* service,
+    ServerBoundCertServiceRequest* request,
+    const CompletionCallback& callback) {
+  DCHECK(request_ == NULL);
+  service_ = service;
+  request_ = request;
+  callback_ = callback;
+}
+
+void ServerBoundCertService::RequestHandle::OnRequestComplete(int result) {
+  request_ = NULL;
+  callback_.Run(result);
+  callback_.Reset();
+}
+
 ServerBoundCertService::ServerBoundCertService(
     ServerBoundCertStore* server_bound_cert_store,
     const scoped_refptr<base::TaskRunner>& task_runner)
@@ -391,8 +423,6 @@ int ServerBoundCertService::GetDomainBoundCert(
   DCHECK(CalledOnValidThread());
   base::TimeTicks request_start = base::TimeTicks::Now();
 
-  *out_req = NULL;
-
   if (callback.is_null() || !private_key || !cert || origin.empty() ||
       requested_types.empty()) {
     RecordGetDomainBoundCertResult(INVALID_ARGUMENT);
@@ -491,9 +521,11 @@ int ServerBoundCertService::GetDomainBoundCert(
   }
 
   ServerBoundCertServiceRequest* request = new ServerBoundCertServiceRequest(
-      request_start, callback, type, private_key, cert);
+      request_start,
+      base::Bind(&RequestHandle::OnRequestComplete, base::Unretained(out_req)),
+      type, private_key, cert);
   job->AddRequest(request);
-  *out_req = request;
+  out_req->RequestStarted(this, request, callback);
   return ERR_IO_PENDING;
 }
 
@@ -501,11 +533,9 @@ ServerBoundCertStore* ServerBoundCertService::GetCertStore() {
   return server_bound_cert_store_.get();
 }
 
-void ServerBoundCertService::CancelRequest(RequestHandle req) {
+void ServerBoundCertService::CancelRequest(ServerBoundCertServiceRequest* req) {
   DCHECK(CalledOnValidThread());
-  ServerBoundCertServiceRequest* request =
-      reinterpret_cast<ServerBoundCertServiceRequest*>(req);
-  request->Cancel();
+  req->Cancel();
 }
 
 // HandleResult is called by ServerBoundCertServiceWorker on the origin message
diff --git a/net/base/server_bound_cert_service.h b/net/base/server_bound_cert_service.h
index e02fbb8..c8c9a05 100644
--- a/net/base/server_bound_cert_service.h
+++ b/net/base/server_bound_cert_service.h
@@ -26,6 +26,7 @@ class TaskRunner;
 namespace net {
 
 class ServerBoundCertServiceJob;
+class ServerBoundCertServiceRequest;
 class ServerBoundCertServiceWorker;
 
 // A class for creating and fetching server bound certs.
@@ -34,8 +35,30 @@ class ServerBoundCertServiceWorker;
 class NET_EXPORT ServerBoundCertService
     : NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
-  // Opaque type used to cancel a request.
-  typedef void* RequestHandle;
+  class NET_EXPORT RequestHandle {
+   public:
+    RequestHandle();
+    ~RequestHandle();
+
+    // Cancel the request.  Does nothing if the request finished or was already
+    // cancelled.
+    void Cancel();
+
+    bool is_active() const { return request_ != NULL; }
+
+   private:
+    friend class ServerBoundCertService;
+
+    void RequestStarted(ServerBoundCertService* service,
+                        ServerBoundCertServiceRequest* request,
+                        const CompletionCallback& callback);
+
+    void OnRequestComplete(int result);
+
+    ServerBoundCertService* service_;
+    ServerBoundCertServiceRequest* request_;
+    CompletionCallback callback_;
+  };
 
   // Password used on EncryptedPrivateKeyInfo data stored in EC private_key
   // values.  (This is not used to provide any security, but to workaround NSS
@@ -78,8 +101,9 @@ class NET_EXPORT ServerBoundCertService
   // could not be completed immediately, in which case the result code will
   // be passed to the callback when available.
   //
-  // |*out_req| will be filled with a handle to the async request. This handle
-  // is not valid after the request has completed.
+  // |*out_req| will be initialized with a handle to the async request. This
+  // RequestHandle object must be cancelled or destroyed before the
+  // ServerBoundCertService is destroyed.
   int GetDomainBoundCert(
       const std::string& origin,
       const std::vector<uint8>& requested_types,
@@ -89,11 +113,6 @@ class NET_EXPORT ServerBoundCertService
       const CompletionCallback& callback,
       RequestHandle* out_req);
 
-  // Cancels the specified request. |req| is the handle returned by
-  // GetDomainBoundCert(). After a request is canceled, its completion
-  // callback will not be called.
-  void CancelRequest(RequestHandle req);
-
   // Returns the backing ServerBoundCertStore.
   ServerBoundCertStore* GetCertStore();
 
@@ -104,6 +123,11 @@ class NET_EXPORT ServerBoundCertService
   uint64 inflight_joins() const { return inflight_joins_; }
 
  private:
+  // Cancels the specified request. |req| is the handle stored by
+  // GetDomainBoundCert(). After a request is canceled, its completion
+  // callback will not be called.
+  void CancelRequest(ServerBoundCertServiceRequest* req);
+
   void HandleResult(const std::string& server_identifier,
                     int error,
                     scoped_ptr<ServerBoundCertStore::ServerBoundCert> cert);
diff --git a/net/base/server_bound_cert_service_unittest.cc b/net/base/server_bound_cert_service_unittest.cc
index 211b4ba..9e505db 100644
--- a/net/base/server_bound_cert_service_unittest.cc
+++ b/net/base/server_bound_cert_service_unittest.cc
@@ -82,13 +82,14 @@ TEST_F(ServerBoundCertServiceTest, CacheHit) {
       origin, types, &type1, &private_key_info1, &der_cert1,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->cert_count());
   EXPECT_EQ(CLIENT_CERT_ECDSA_SIGN, type1);
   EXPECT_FALSE(private_key_info1.empty());
   EXPECT_FALSE(der_cert1.empty());
+  EXPECT_FALSE(request_handle.is_active());
 
   // Synchronous completion.
   SSLClientCertType type2;
@@ -96,7 +97,7 @@ TEST_F(ServerBoundCertServiceTest, CacheHit) {
   error = service_->GetDomainBoundCert(
       origin, types, &type2, &private_key_info2, &der_cert2,
       callback.callback(), &request_handle);
-  EXPECT_TRUE(request_handle == NULL);
+  EXPECT_FALSE(request_handle.is_active());
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->cert_count());
   EXPECT_EQ(CLIENT_CERT_ECDSA_SIGN, type2);
@@ -123,7 +124,7 @@ TEST_F(ServerBoundCertServiceTest, UnsupportedTypes) {
       origin, types, &type1, &private_key_info1, &der_cert1,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_INVALID_ARGUMENT, error);
-  EXPECT_TRUE(request_handle == NULL);
+  EXPECT_FALSE(request_handle.is_active());
 
   // No supported types in requested_types.
   types.push_back(CLIENT_CERT_RSA_SIGN);
@@ -133,7 +134,7 @@ TEST_F(ServerBoundCertServiceTest, UnsupportedTypes) {
       origin, types, &type1, &private_key_info1, &der_cert1,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_CLIENT_AUTH_CERT_TYPE_UNSUPPORTED, error);
-  EXPECT_TRUE(request_handle == NULL);
+  EXPECT_FALSE(request_handle.is_active());
 
   // Supported types after unsupported ones in requested_types.
   types.push_back(CLIENT_CERT_ECDSA_SIGN);
@@ -143,7 +144,7 @@ TEST_F(ServerBoundCertServiceTest, UnsupportedTypes) {
       origin, types, &type1, &private_key_info1, &der_cert1,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->cert_count());
@@ -161,7 +162,7 @@ TEST_F(ServerBoundCertServiceTest, UnsupportedTypes) {
       origin, types, &type2, &private_key_info2, &der_cert2,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_INVALID_ARGUMENT, error);
-  EXPECT_TRUE(request_handle == NULL);
+  EXPECT_FALSE(request_handle.is_active());
 
   // No supported types in requested_types.
   types.push_back(CLIENT_CERT_RSA_SIGN);
@@ -171,14 +172,14 @@ TEST_F(ServerBoundCertServiceTest, UnsupportedTypes) {
       origin, types, &type2, &private_key_info2, &der_cert2,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_CLIENT_AUTH_CERT_TYPE_UNSUPPORTED, error);
-  EXPECT_TRUE(request_handle == NULL);
+  EXPECT_FALSE(request_handle.is_active());
 
   // If we request EC, the cert we created before should still be there.
   types.push_back(CLIENT_CERT_ECDSA_SIGN);
   error = service_->GetDomainBoundCert(
       origin, types, &type2, &private_key_info2, &der_cert2,
       callback.callback(), &request_handle);
-  EXPECT_TRUE(request_handle == NULL);
+  EXPECT_FALSE(request_handle.is_active());
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->cert_count());
   EXPECT_EQ(CLIENT_CERT_ECDSA_SIGN, type2);
@@ -201,7 +202,7 @@ TEST_F(ServerBoundCertServiceTest, StoreCerts) {
       origin1, types, &type1, &private_key_info1, &der_cert1,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(1, service_->cert_count());
@@ -213,7 +214,7 @@ TEST_F(ServerBoundCertServiceTest, StoreCerts) {
       origin2, types, &type2, &private_key_info2, &der_cert2,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(2, service_->cert_count());
@@ -225,7 +226,7 @@ TEST_F(ServerBoundCertServiceTest, StoreCerts) {
       origin3, types, &type3, &private_key_info3, &der_cert3,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(3, service_->cert_count());
@@ -262,7 +263,7 @@ TEST_F(ServerBoundCertServiceTest, InflightJoin) {
       origin, types, &type1, &private_key_info1, &der_cert1,
       callback1.callback(), &request_handle1);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle1 != NULL);
+  EXPECT_TRUE(request_handle1.is_active());
   // If we request RSA and EC in the 2nd request, should still join with the
   // original request.
   types.insert(types.begin(), CLIENT_CERT_RSA_SIGN);
@@ -270,7 +271,7 @@ TEST_F(ServerBoundCertServiceTest, InflightJoin) {
       origin, types, &type2, &private_key_info2, &der_cert2,
       callback2.callback(), &request_handle2);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle2 != NULL);
+  EXPECT_TRUE(request_handle2.is_active());
 
   error = callback1.WaitForResult();
   EXPECT_EQ(OK, error);
@@ -298,7 +299,7 @@ TEST_F(ServerBoundCertServiceTest, ExtractValuesFromBytesEC) {
       origin, types, &type, &private_key_info, &der_cert, callback.callback(),
       &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
 
@@ -339,8 +340,42 @@ TEST_F(ServerBoundCertServiceTest, CancelRequest) {
                                       base::Bind(&FailTest),
                                       &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
-  service_->CancelRequest(request_handle);
+  EXPECT_TRUE(request_handle.is_active());
+  request_handle.Cancel();
+  EXPECT_FALSE(request_handle.is_active());
+
+  // Wait for generation to finish.
+  sequenced_worker_pool_->FlushForTesting();
+  // Wait for reply from ServerBoundCertServiceWorker to be posted back to the
+  // ServerBoundCertService.
+  MessageLoop::current()->RunUntilIdle();
+
+  // Even though the original request was cancelled, the service will still
+  // store the result, it just doesn't call the callback.
+  EXPECT_EQ(1, service_->cert_count());
+}
+
+// Tests that destructing the RequestHandle cancels the request.
+TEST_F(ServerBoundCertServiceTest, CancelRequestByHandleDestruction) {
+  std::string origin("https://encrypted.google.com:443");
+  SSLClientCertType type;
+  std::string private_key_info, der_cert;
+  int error;
+  std::vector<uint8> types;
+  types.push_back(CLIENT_CERT_ECDSA_SIGN);
+  {
+    ServerBoundCertService::RequestHandle request_handle;
+
+    error = service_->GetDomainBoundCert(origin,
+                                         types,
+                                         &type,
+                                         &private_key_info,
+                                         &der_cert,
+                                         base::Bind(&FailTest),
+                                         &request_handle);
+    EXPECT_EQ(ERR_IO_PENDING, error);
+    EXPECT_TRUE(request_handle.is_active());
+  }
 
   // Wait for generation to finish.
   sequenced_worker_pool_->FlushForTesting();
@@ -370,10 +405,10 @@ TEST_F(ServerBoundCertServiceTest, DestructionWithPendingRequest) {
                                       base::Bind(&FailTest),
                                       &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle.is_active());
 
   // Cancel request and destroy the ServerBoundCertService.
-  service_->CancelRequest(request_handle);
+  request_handle.Cancel();
   service_.reset();
 
   // Wait for generation to finish.
@@ -391,22 +426,24 @@ TEST_F(ServerBoundCertServiceTest, SimultaneousCreation) {
   int error;
   std::vector<uint8> types;
   types.push_back(CLIENT_CERT_ECDSA_SIGN);
-  ServerBoundCertService::RequestHandle request_handle;
 
   std::string origin1("https://encrypted.google.com:443");
   SSLClientCertType type1;
   std::string private_key_info1, der_cert1;
   TestCompletionCallback callback1;
+  ServerBoundCertService::RequestHandle request_handle1;
 
   std::string origin2("https://foo.com:443");
   SSLClientCertType type2;
   std::string private_key_info2, der_cert2;
   TestCompletionCallback callback2;
+  ServerBoundCertService::RequestHandle request_handle2;
 
   std::string origin3("https://bar.com:443");
   SSLClientCertType type3;
   std::string private_key_info3, der_cert3;
   TestCompletionCallback callback3;
+  ServerBoundCertService::RequestHandle request_handle3;
 
   error = service_->GetDomainBoundCert(origin1,
                                        types,
@@ -414,9 +451,9 @@ TEST_F(ServerBoundCertServiceTest, SimultaneousCreation) {
                                        &private_key_info1,
                                        &der_cert1,
                                        callback1.callback(),
-                                       &request_handle);
+                                       &request_handle1);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle1.is_active());
 
   error = service_->GetDomainBoundCert(origin2,
                                        types,
@@ -424,9 +461,9 @@ TEST_F(ServerBoundCertServiceTest, SimultaneousCreation) {
                                        &private_key_info2,
                                        &der_cert2,
                                        callback2.callback(),
-                                       &request_handle);
+                                       &request_handle2);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle2.is_active());
 
   error = service_->GetDomainBoundCert(origin3,
                                        types,
@@ -434,9 +471,9 @@ TEST_F(ServerBoundCertServiceTest, SimultaneousCreation) {
                                        &private_key_info3,
                                        &der_cert3,
                                        callback3.callback(),
-                                       &request_handle);
+                                       &request_handle3);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle3.is_active());
 
   error = callback1.WaitForResult();
   EXPECT_EQ(OK, error);
@@ -498,7 +535,7 @@ TEST_F(ServerBoundCertServiceTest, Expiration) {
       "https://good", types, &type1, &private_key_info1, &der_cert1,
       callback.callback(), &request_handle);
   EXPECT_EQ(OK, error);
-  EXPECT_TRUE(request_handle == NULL);
+  EXPECT_FALSE(request_handle.is_active());
   EXPECT_EQ(2, service_->cert_count());
   EXPECT_EQ(CLIENT_CERT_ECDSA_SIGN, type1);
   EXPECT_STREQ("a", private_key_info1.c_str());
@@ -511,7 +548,7 @@ TEST_F(ServerBoundCertServiceTest, Expiration) {
       "https://expired", types, &type2, &private_key_info2, &der_cert2,
       callback.callback(), &request_handle);
   EXPECT_EQ(ERR_IO_PENDING, error);
-  EXPECT_TRUE(request_handle != NULL);
+  EXPECT_TRUE(request_handle.is_active());
   error = callback.WaitForResult();
   EXPECT_EQ(OK, error);
   EXPECT_EQ(2, service_->cert_count());
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index a3c1b12..809dbc5 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -896,7 +896,6 @@ SSLClientSocketNSS::Core::Core(
       transport_(transport),
       weak_net_log_factory_(net_log),
       server_bound_cert_service_(server_bound_cert_service),
-      domain_bound_cert_request_handle_(NULL),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       nss_fd_(NULL),
@@ -1081,11 +1080,7 @@ void SSLClientSocketNSS::Core::Detach() {
 
   network_handshake_state_.Reset();
 
-  if (domain_bound_cert_request_handle_ != NULL) {
-    server_bound_cert_service_->CancelRequest(
-        domain_bound_cert_request_handle_);
-    domain_bound_cert_request_handle_ = NULL;
-  }
+  domain_bound_cert_request_handle_.Cancel();
 }
 
 int SSLClientSocketNSS::Core::Read(IOBuffer* buf, int buf_len,
@@ -2656,7 +2651,6 @@ void SSLClientSocketNSS::Core::OnGetDomainBoundCertComplete(int result) {
   DVLOG(1) << __FUNCTION__ << " " << result;
   DCHECK(OnNetworkTaskRunner());
 
-  domain_bound_cert_request_handle_ = NULL;
   OnHandshakeIOComplete(result);
 }
 
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index aeb370e..91928fe 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -73,8 +73,7 @@ SpdyStream::SpdyStream(SpdySession* session,
       net_log_(net_log),
       send_bytes_(0),
       recv_bytes_(0),
-      domain_bound_cert_type_(CLIENT_CERT_INVALID_TYPE),
-      domain_bound_cert_request_handle_(NULL) {
+      domain_bound_cert_type_(CLIENT_CERT_INVALID_TYPE) {
 }
 
 class SpdyStream::SpdyStreamIOBufferProducer