The CertVerifierJob destructor should delete canceled requests.

Add a job to inflight_ only after the job's worker has started
successfully.

R=agl
BUG=63357,67289
TEST=net_unittests --gtest_filter=CertVerifierTest.CancelRequestThenQuit
should not leak a CertVerifierRequest object under valgrind.
Review URL: http://codereview.chromium.org/5973004

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

2 files changed, 42 insertions, 4 deletions

diff --git a/net/base/cert_verifier.cc b/net/base/cert_verifier.cc
index 044524f..a3755aa 100644
--- a/net/base/cert_verifier.cc
+++ b/net/base/cert_verifier.cc
@@ -109,6 +109,8 @@ class CertVerifierRequest {
     delete this;
   }
 
+  bool canceled() const { return !callback_; }
+
  private:
   CompletionCallback* callback_;
   CertVerifyResult* verify_result_;
@@ -236,8 +238,10 @@ class CertVerifierJob {
   }
 
   ~CertVerifierJob() {
-    if (worker_)
+    if (worker_) {
       worker_->Cancel();
+      DeleteAllCanceled();
+    }
   }
 
   void AddRequest(CertVerifierRequest* request) {
@@ -261,6 +265,17 @@ class CertVerifierJob {
     }
   }
 
+  void DeleteAllCanceled() {
+    for (std::vector<CertVerifierRequest*>::iterator
+         i = requests_.begin(); i != requests_.end(); i++) {
+      if ((*i)->canceled()) {
+        delete *i;
+      } else {
+        LOG(DFATAL) << "CertVerifierRequest leaked!";
+      }
+    }
+  }
+
   std::vector<CertVerifierRequest*> requests_;
   CertVerifierWorker* worker_;
 };
@@ -328,14 +343,15 @@ int CertVerifier::Verify(X509Certificate* cert,
     CertVerifierWorker* worker = new CertVerifierWorker(cert, hostname, flags,
                                                         this);
     job = new CertVerifierJob(worker);
-    inflight_.insert(std::make_pair(key, job));
     if (!worker->Start()) {
-      inflight_.erase(key);
       delete job;
       delete worker;
       *out_req = NULL;
-      return ERR_FAILED;  // TODO(wtc): Log an error message.
+      // TODO(wtc): log to the NetLog.
+      LOG(ERROR) << "CertVerifierWorker couldn't be started.";
+      return ERR_INSUFFICIENT_RESOURCES;  // Just a guess.
     }
+    inflight_.insert(std::make_pair(key, job));
   }
 
   CertVerifierRequest* request =
diff --git a/net/base/cert_verifier_unittest.cc b/net/base/cert_verifier_unittest.cc
index ca5e1f4..c49a5f4 100644
--- a/net/base/cert_verifier_unittest.cc
+++ b/net/base/cert_verifier_unittest.cc
@@ -257,4 +257,26 @@ TEST_F(CertVerifierTest, CancelRequest) {
   }
 }
 
+// Tests that a canceled request is not leaked.
+TEST_F(CertVerifierTest, CancelRequestThenQuit) {
+  CertVerifier verifier;
+
+  FilePath certs_dir = GetTestCertsDirectory();
+  scoped_refptr<X509Certificate> google_cert(
+      ImportCertFromFile(certs_dir, "google.single.der"));
+  ASSERT_NE(static_cast<X509Certificate*>(NULL), google_cert);
+
+  int error;
+  CertVerifyResult verify_result;
+  TestCompletionCallback callback;
+  CertVerifier::RequestHandle request_handle;
+
+  error = verifier.Verify(google_cert, "www.example.com", 0, &verify_result,
+                          &callback, &request_handle);
+  ASSERT_EQ(ERR_IO_PENDING, error);
+  ASSERT_TRUE(request_handle != NULL);
+  verifier.CancelRequest(request_handle);
+  // Destroy |verifier| by going out of scope.
+}
+
 }  // namespace net