AddRef() / Release() while URLRequest is alive in OCSPRequestSession

net/ocsp/nss_ocsp.cc

 // Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ocsp/nss_ocsp.h"
 
 #include <certt.h>
 #include <certdb.h>
 #include <ocsp.h>
 #include <nspr.h>
@@ skipping 110 matching lines @@
     }
   }
 
   bool Started() const {
     return request_ != NULL;
   }
 
   void Cancel() {
     // IO thread may set |io_loop_| to NULL, so protect by |lock_|.
     AutoLock autolock(lock_);
-    if (io_loop_) {
-      io_loop_->PostTask(
-          FROM_HERE,
-          NewRunnableMethod(this, &OCSPRequestSession::CancelURLRequest));
-    }
+    CancelLocked();
   }
 
   bool Finished() const {
     AutoLock autolock(lock_);
     return finished_;
   }
 
   bool Wait() {
     base::TimeDelta timeout = timeout_;
     AutoLock autolock(lock_);
     while (!finished_) {
       base::TimeTicks last_time = base::TimeTicks::Now();
       cv_.TimedWait(timeout);
       // Check elapsed time
       base::TimeDelta elapsed_time = base::TimeTicks::Now() - last_time;
       timeout -= elapsed_time;
       if (timeout < base::TimeDelta()) {
         LOG(INFO) << "OCSP Timed out";
         if (!finished_)
-          Cancel();
+          CancelLocked();
         break;
       }
     }
     return finished_;
   }
 
   const GURL& url() const {
     return url_;
   }
 
@@ skipping 52 matching lines @@
     if (!request_->status().is_io_pending()) {
       delete request_;
       request_ = NULL;
       io_loop_->RemoveDestructionObserver(this);
       {
         AutoLock autolock(lock_);
         finished_ = true;
         io_loop_ = NULL;
       }
       cv_.Signal();
+      Release();  // Balanced with StartURLRequest().
     }
   }
 
   virtual void WillDestroyCurrentMessageLoop() {

[wtc, 2009/12/08 01:44:01]

It's a shame that the WillDestroyCurrentMessageLoop method
may lock lock_ and set io_loop_ to NULL twice, once here,
and once (if request_ is not NULL) inside CancelURLRequest.
But I can't come up with a way to avoid this.

     DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
     {
       AutoLock autolock(lock_);
       io_loop_ = NULL;
     }
-    if (request_) {
-      request_->Cancel();
-      delete request_;
-      request_ = NULL;
-    }
+    CancelURLRequest();
   }
 
  private:
   friend class base::RefCountedThreadSafe<OCSPRequestSession>;
 
   virtual ~OCSPRequestSession() {
     DCHECK(!request_);
     if (io_loop_)
       io_loop_->RemoveDestructionObserver(this);
   }
 
+  void CancelLocked() {

[wtc, 2009/12/08 01:44:01]

Please add a comment to document that we must call this method
while holding lock_.

You can add this comment to your other CL at
http://codereview.chromium.org/460067

Thanks!

[ukai, 2009/12/08 06:02:05]

Sure. Added lock_.AssertAcquired() too.

+    if (io_loop_) {
+      io_loop_->PostTask(
+          FROM_HERE,
+          NewRunnableMethod(this, &OCSPRequestSession::CancelURLRequest));
+    }
+  }
+
   void StartURLRequest() {
     DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
     DCHECK(!request_);
 
     io_loop_->AddDestructionObserver(this);
 
     request_ = new URLRequest(url_, this);
     request_->set_context(
         Singleton<OCSPInitSingleton>::get()->url_request_context());
     // To meet the privacy requirements of off-the-record mode.
     request_->set_load_flags(
         net::LOAD_DISABLE_CACHE|net::LOAD_DO_NOT_SAVE_COOKIES);
 
     if (http_request_method_ == "POST") {
       DCHECK(!upload_content_.empty());
       DCHECK(!upload_content_type_.empty());
 
       request_->set_method("POST");
       if (!extra_request_headers_.empty())
         extra_request_headers_ += "\r\n";
       StringAppendF(&extra_request_headers_,
                     "Content-Type: %s", upload_content_type_.c_str());
       request_->AppendBytesToUpload(upload_content_.data(),
                                     static_cast<int>(upload_content_.size()));
     }
     if (!extra_request_headers_.empty())
       request_->SetExtraRequestHeaders(extra_request_headers_);
 
     request_->Start();
+    AddRef();  // Release after |request_| deleted.
   }
 
   void CancelURLRequest() {
     if (io_loop_)
       DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
     if (request_) {
       request_->Cancel();
       delete request_;
       request_ = NULL;
+      {
+        AutoLock autolock(lock_);
+        finished_ = true;
+        io_loop_ = NULL;
+      }
+      cv_.Signal();
+      Release();  // Balanced with StartURLRequest().
     }
   }
 
   GURL url_;                      // The URL we eventually wound up at
   std::string http_request_method_;
   base::TimeDelta timeout_;       // The timeout for OCSP
   URLRequest* request_;           // The actual request this wraps
   scoped_refptr<net::IOBuffer> buffer_;  // Read buffer
   std::string extra_request_headers_;  // Extra headers for the request, if any
   std::string upload_content_;    // HTTP POST payload
@@ skipping 257 matching lines @@
 // This function would be called before NSS initialization.
 void SetURLRequestContextForOCSP(URLRequestContext* request_context) {
   OCSPInitSingleton::set_url_request_context(request_context);
 }
 
 URLRequestContext* GetURLRequestContextForOCSP() {
   return OCSPInitSingleton::url_request_context();
 }
 
 }  // namespace net