Try to fix crash in OCSP handlers.

Make sure OCSPRequestSession::Core is cancelled when OCSPRequestSession
is deleted as URLFetcher does.

Revert http://src.chromium.org/viewvc/chrome?view=rev&revision=23575


BUG=18907,10911
TEST=none

Review URL: http://codereview.chromium.org/165362

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

2 files changed, 21 insertions, 24 deletions

diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index ff40deb..9a8b70c 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -344,10 +344,6 @@ SECStatus PKIXVerifyCert(X509Certificate::OSCertHandle cert_handle,
                          const SECOidTag* policy_oids,
                          int num_policy_oids,
                          CERTValOutParam* cvout) {
-  // TODO(wtc): Disable OCSP until we track down the crash in OCSP code.
-  // See http://crbug.com/18907.
-  bool use_ocsp = false;
-
   PRUint64 revocation_method_flags =
       CERT_REV_M_TEST_USING_THIS_METHOD |
       CERT_REV_M_ALLOW_NETWORK_FETCHING |
@@ -361,8 +357,6 @@ SECStatus PKIXVerifyCert(X509Certificate::OSCertHandle cert_handle,
     // revoked if we don't have revocation info.
     // TODO(wtc): Add a bool parameter to expressly specify we're doing EV
     // verification or we want strict revocation flags.
-    if (!use_ocsp)
-      return SECFailure;  // No OCSP, no EV.
     revocation_method_flags |= CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE;
     revocation_method_independent_flags |=
         CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE;
@@ -375,19 +369,12 @@ SECStatus PKIXVerifyCert(X509Certificate::OSCertHandle cert_handle,
   method_flags[cert_revocation_method_crl] = revocation_method_flags;
   method_flags[cert_revocation_method_ocsp] = revocation_method_flags;
 
-  int number_of_defined_methods;
   CERTRevocationMethodIndex preferred_revocation_methods[1];
-  if (use_ocsp) {
-    number_of_defined_methods = arraysize(method_flags);
-    preferred_revocation_methods[0] = cert_revocation_method_ocsp;
-  } else {
-    number_of_defined_methods = arraysize(method_flags) - 1;
-    preferred_revocation_methods[0] = cert_revocation_method_crl;
-  }
+  preferred_revocation_methods[0] = cert_revocation_method_ocsp;
 
   CERTRevocationFlags revocation_flags;
   revocation_flags.leafTests.number_of_defined_methods =
-      number_of_defined_methods;
+      arraysize(method_flags);
   revocation_flags.leafTests.cert_rev_flags_per_method = method_flags;
   revocation_flags.leafTests.number_of_preferred_methods =
       arraysize(preferred_revocation_methods);
@@ -396,7 +383,7 @@ SECStatus PKIXVerifyCert(X509Certificate::OSCertHandle cert_handle,
       revocation_method_independent_flags;
 
   revocation_flags.chainTests.number_of_defined_methods =
-      number_of_defined_methods;
+      arraysize(method_flags);
   revocation_flags.chainTests.cert_rev_flags_per_method = method_flags;
   revocation_flags.chainTests.number_of_preferred_methods =
       arraysize(preferred_revocation_methods);
diff --git a/net/ocsp/nss_ocsp.cc b/net/ocsp/nss_ocsp.cc
index da584b2..396349a 100644
--- a/net/ocsp/nss_ocsp.cc
+++ b/net/ocsp/nss_ocsp.cc
@@ -80,7 +80,7 @@ class OCSPRequestSession {
   OCSPRequestSession(const GURL& url,
                      const char* http_request_method,
                      base::TimeDelta timeout);
-  ~OCSPRequestSession() {}
+  ~OCSPRequestSession();
 
   void SetPostData(const char* http_data,
                    const PRUint32 http_data_len,
@@ -137,15 +137,18 @@ class OCSPRequestSession::Core
       public URLRequest::Delegate {
  public:
   explicit Core(OCSPRequestSession* req)
-      : ocsp_req_(req),
-        url_(ocsp_req_->url()),
+      : url_(req->url()),
+        http_request_method_(req->http_request_method()),
+        timeout_(req->timeout()),
         io_loop_(Singleton<OCSPInitSingleton>::get()->io_thread()),
         request_(NULL),
         buffer_(new net::IOBuffer(kRecvBufferSize)),
         response_code_(-1),
         cv_(&lock_),
         finished_(false) {}
-  virtual ~Core() {}
+  virtual ~Core() {
+    DCHECK(!request_);
+  }
 
   void SetPostData(const char* http_data, PRUint32 http_data_len,
                    const char* http_content_type) {
@@ -181,7 +184,7 @@ class OCSPRequestSession::Core
   }
 
   bool Wait() {
-    base::TimeDelta timeout = ocsp_req_->timeout();
+    base::TimeDelta timeout = timeout_;
     AutoLock autolock(lock_);
     while (!finished_) {
       base::TimeTicks last_time = base::TimeTicks::Now();
@@ -226,6 +229,7 @@ class OCSPRequestSession::Core
   virtual void OnResponseStarted(URLRequest* request) {
     DCHECK(request == request_);
     DCHECK(MessageLoopForIO::current() == io_loop_);
+
     int bytes_read = 0;
     if (request->status().is_success()) {
       response_code_ = request_->GetResponseCode();
@@ -269,7 +273,7 @@ class OCSPRequestSession::Core
     request_->set_load_flags(
         net::LOAD_DISABLE_CACHE|net::LOAD_DO_NOT_SAVE_COOKIES);
 
-    if (ocsp_req_->http_request_method() == "POST") {
+    if (http_request_method_ == "POST") {
       DCHECK(!upload_content_.empty());
       DCHECK(!upload_content_type_.empty());
 
@@ -290,13 +294,15 @@ class OCSPRequestSession::Core
   void CancelURLRequest() {
     DCHECK(MessageLoopForIO::current() == io_loop_);
     if (request_) {
+      request_->Cancel();
       delete request_;
       request_ = NULL;
     }
   }
 
-  OCSPRequestSession* ocsp_req_;  // corresponding OCSP session
   GURL url_;                      // The URL we eventually wound up at
+  std::string http_request_method_;
+  base::TimeDelta timeout_;       // The timeout for OCSP
   MessageLoop* io_loop_;          // Message loop of the IO thread
   URLRequest* request_;           // The actual request this wraps
   scoped_refptr<net::IOBuffer> buffer_;  // Read buffer
@@ -326,6 +332,10 @@ OCSPRequestSession::OCSPRequestSession(const GURL& url,
       ALLOW_THIS_IN_INITIALIZER_LIST(core_(new Core(this))) {
 }
 
+OCSPRequestSession::~OCSPRequestSession() {
+  core_->Cancel();
+}
+
 void OCSPRequestSession::SetPostData(const char* http_data,
                                      const PRUint32 http_data_len,
                                      const char* http_content_type) {
@@ -508,7 +518,7 @@ bool OCSPSetResponse(OCSPRequestSession* req,
       return false;
     }
   }
-  LOG(INFO) << "OSCP response "
+  LOG(INFO) << "OCSP response "
             << " response_code=" << req->http_response_code()
             << " content_type=" << req->http_response_content_type()
             << " header=" << req->http_response_headers()