Stop using DestructionObserver in OCSP code. Explicilty cancel all URLRequests.

BUG=59630
TEST=none

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

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

2 files changed, 160 insertions, 127 deletions

diff --git a/net/base/net_test_suite.h b/net/base/net_test_suite.h
index eab3a53..00d9844 100644
--- a/net/base/net_test_suite.h
+++ b/net/base/net_test_suite.h
@@ -9,7 +9,9 @@
 #include "base/message_loop.h"
 #include "base/ref_counted.h"
 #include "base/test/test_suite.h"
+#include "build/build_config.h"
 #include "net/base/mock_host_resolver.h"
+#include "net/ocsp/nss_ocsp.h"
 
 class NetTestSuite : public base::TestSuite {
  public:
@@ -39,6 +41,10 @@ class NetTestSuite : public base::TestSuite {
   }
 
   virtual void Shutdown() {
+#if defined(OS_LINUX)
+    net::ShutdownOCSP();
+#endif  // defined(OS_LINUX)
+
     // We want to destroy this here before the TestSuite continues to tear down
     // the environment.
     message_loop_.reset();
diff --git a/net/ocsp/nss_ocsp.cc b/net/ocsp/nss_ocsp.cc
index b7fcb65..fafaa68 100644
--- a/net/ocsp/nss_ocsp.cc
+++ b/net/ocsp/nss_ocsp.cc
@@ -22,9 +22,10 @@
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "base/stl_util-inl.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
-#include "base/thread.h"
+#include "base/thread_checker.h"
 #include "base/time.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/io_buffer.h"
@@ -40,16 +41,16 @@ namespace {
 pthread_mutex_t g_request_context_lock = PTHREAD_MUTEX_INITIALIZER;
 static URLRequestContext* g_request_context = NULL;
 
-class OCSPIOLoop : public MessageLoop::DestructionObserver {
- public:
-  // MessageLoop::DestructionObserver:
-  virtual void WillDestroyCurrentMessageLoop() { Shutdown(); }
+class OCSPRequestSession;
 
+class OCSPIOLoop {
+ public:
   void StartUsing() {
     AutoLock autolock(lock_);
     used_ = true;
   }
 
+  // Called on IO loop.
   void Shutdown();
 
   bool used() const {
@@ -62,69 +63,28 @@ class OCSPIOLoop : public MessageLoop::DestructionObserver {
 
   void EnsureIOLoop();
 
+  void AddRequest(OCSPRequestSession* request);
+  void RemoveRequest(OCSPRequestSession* request);
+
  private:
   friend struct base::DefaultLazyInstanceTraits<OCSPIOLoop>;
 
   OCSPIOLoop();
   ~OCSPIOLoop();
 
+  void CancelAllRequests();
+
   mutable Lock lock_;
+  bool shutdown_;  // Protected by |lock_|.
+  std::set<OCSPRequestSession*> requests_;  // Protected by |lock_|.
   bool used_;  // Protected by |lock_|.
   // This should not be modified after |used_|.
   MessageLoopForIO* io_loop_;  // Protected by |lock_|.
+  ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(OCSPIOLoop);
 };
 
-OCSPIOLoop::OCSPIOLoop()
-    : used_(false),
-      io_loop_(MessageLoopForIO::current()) {
-  DCHECK(io_loop_);
-  io_loop_->AddDestructionObserver(this);
-}
-
-OCSPIOLoop::~OCSPIOLoop() {
-  // IO thread was already deleted before the singleton is deleted
-  // in AtExitManager.
-  {
-    AutoLock autolock(lock_);
-    DCHECK(!io_loop_);
-    DCHECK(!used_);
-  }
-
-  pthread_mutex_lock(&g_request_context_lock);
-  DCHECK(!g_request_context);
-  pthread_mutex_unlock(&g_request_context_lock);
-}
-
-void OCSPIOLoop::Shutdown() {
-  MessageLoopForIO::current()->RemoveDestructionObserver(this);
-
-  // Prevent the worker thread from trying to access |io_loop_|.
-  {
-    AutoLock autolock(lock_);
-    DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
-    io_loop_ = NULL;
-    used_ = false;
-  }
-
-  pthread_mutex_lock(&g_request_context_lock);
-  g_request_context = NULL;
-  pthread_mutex_unlock(&g_request_context_lock);
-}
-
-void OCSPIOLoop::PostTaskToIOLoop(
-    const tracked_objects::Location& from_here, Task* task) {
-  AutoLock autolock(lock_);
-  if (io_loop_)
-    io_loop_->PostTask(from_here, task);
-}
-
-void OCSPIOLoop::EnsureIOLoop() {
-  AutoLock autolock(lock_);
-  DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
-}
-
 base::LazyInstance<OCSPIOLoop> g_ocsp_io_loop(base::LINKER_INITIALIZED);
 
 const int kRecvBufferSize = 4096;
@@ -175,44 +135,6 @@ class OCSPNSSInitialization {
   DISALLOW_COPY_AND_ASSIGN(OCSPNSSInitialization);
 };
 
-OCSPNSSInitialization::OCSPNSSInitialization() {
-  // NSS calls the functions in the function table to download certificates
-  // or CRLs or talk to OCSP responders over HTTP.  These functions must
-  // set an NSS/NSPR error code when they fail.  Otherwise NSS will get the
-  // residual error code from an earlier failed function call.
-  client_fcn_.version = 1;
-  SEC_HttpClientFcnV1Struct *ft = &client_fcn_.fcnTable.ftable1;
-  ft->createSessionFcn = OCSPCreateSession;
-  ft->keepAliveSessionFcn = OCSPKeepAliveSession;
-  ft->freeSessionFcn = OCSPFreeSession;
-  ft->createFcn = OCSPCreate;
-  ft->setPostDataFcn = OCSPSetPostData;
-  ft->addHeaderFcn = OCSPAddHeader;
-  ft->trySendAndReceiveFcn = OCSPTrySendAndReceive;
-  ft->cancelFcn = NULL;
-  ft->freeFcn = OCSPFree;
-  SECStatus status = SEC_RegisterDefaultHttpClient(&client_fcn_);
-  if (status != SECSuccess) {
-    NOTREACHED() << "Error initializing OCSP: " << PR_GetError();
-  }
-
-  // Work around NSS bugs 524013 and 564334.  NSS incorrectly thinks the
-  // CRLs for Network Solutions Certificate Authority have bad signatures,
-  // which causes certificates issued by that CA to be reported as revoked.
-  // By using OCSP for those certificates, which don't have AIA extensions,
-  // we can work around these bugs.  See http://crbug.com/41730.
-  CERT_StringFromCertFcn old_callback = NULL;
-  status = CERT_RegisterAlternateOCSPAIAInfoCallBack(
-      GetAlternateOCSPAIAInfo, &old_callback);
-  if (status == SECSuccess) {
-    DCHECK(!old_callback);
-  } else {
-    NOTREACHED() << "Error initializing OCSP: " << PR_GetError();
-  }
-}
-
-OCSPNSSInitialization::~OCSPNSSInitialization() {}
-
 base::LazyInstance<OCSPNSSInitialization> g_ocsp_nss_initialization(
     base::LINKER_INITIALIZED);
 
@@ -223,8 +145,7 @@ base::LazyInstance<OCSPNSSInitialization> g_ocsp_nss_initialization(
 // on IO thread.
 class OCSPRequestSession
     : public base::RefCountedThreadSafe<OCSPRequestSession>,
-      public URLRequest::Delegate,
-      public MessageLoop::DestructionObserver {
+      public URLRequest::Delegate {
  public:
   OCSPRequestSession(const GURL& url,
                      const char* http_request_method,
@@ -353,7 +274,7 @@ class OCSPRequestSession
     if (!request_->status().is_io_pending()) {
       delete request_;
       request_ = NULL;
-      io_loop_->RemoveDestructionObserver(this);
+      g_ocsp_io_loop.Get().RemoveRequest(this);
       {
         AutoLock autolock(lock_);
         finished_ = true;
@@ -364,13 +285,28 @@ class OCSPRequestSession
     }
   }
 
-  virtual void WillDestroyCurrentMessageLoop() {
-    DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
+  // Must be called on the IO loop thread.
+  void CancelURLRequest() {
+#ifndef NDEBUG
     {
       AutoLock autolock(lock_);
-      io_loop_ = NULL;
+      if (io_loop_)
+        DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
+    }
+#endif
+    if (request_) {
+      request_->Cancel();
+      delete request_;
+      request_ = NULL;
+      g_ocsp_io_loop.Get().RemoveRequest(this);
+      {
+        AutoLock autolock(lock_);
+        finished_ = true;
+        io_loop_ = NULL;
+      }
+      cv_.Signal();
+      Release();  // Balanced with StartURLRequest().
     }
-    CancelURLRequest();
   }
 
  private:
@@ -409,7 +345,7 @@ class OCSPRequestSession
       AutoLock autolock(lock_);
       DCHECK(!io_loop_);
       io_loop_ = MessageLoopForIO::current();
-      io_loop_->AddDestructionObserver(this);
+      g_ocsp_io_loop.Get().AddRequest(this);
     }
 
     request_ = new URLRequest(url_, this);
@@ -436,32 +372,6 @@ class OCSPRequestSession
     AddRef();  // Release after |request_| deleted.
   }
 
-  void CancelURLRequest() {
-#ifndef NDEBUG
-    {
-      AutoLock autolock(lock_);
-      if (io_loop_)
-        DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
-    }
-#endif
-    if (request_) {
-      request_->Cancel();
-      delete request_;
-      request_ = NULL;
-      // |io_loop_| may be NULL here if it called from
-      // WillDestroyCurrentMessageLoop().
-      if (io_loop_)
-        io_loop_->RemoveDestructionObserver(this);
-      {
-        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
@@ -527,6 +437,123 @@ class OCSPServerSession {
   DISALLOW_COPY_AND_ASSIGN(OCSPServerSession);
 };
 
+OCSPIOLoop::OCSPIOLoop()
+    : shutdown_(false),
+      used_(false),
+      io_loop_(MessageLoopForIO::current()) {
+  DCHECK(io_loop_);
+}
+
+OCSPIOLoop::~OCSPIOLoop() {
+  // IO thread was already deleted before the singleton is deleted
+  // in AtExitManager.
+  {
+    AutoLock autolock(lock_);
+    DCHECK(!io_loop_);
+    DCHECK(!used_);
+    DCHECK(shutdown_);
+  }
+
+  pthread_mutex_lock(&g_request_context_lock);
+  DCHECK(!g_request_context);
+  pthread_mutex_unlock(&g_request_context_lock);
+}
+
+void OCSPIOLoop::Shutdown() {
+  // Safe to read outside lock since we only write on IO thread anyway.
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Prevent the worker thread from trying to access |io_loop_|.
+  {
+    AutoLock autolock(lock_);
+    io_loop_ = NULL;
+    used_ = false;
+    shutdown_ = true;
+  }
+
+  CancelAllRequests();
+
+  pthread_mutex_lock(&g_request_context_lock);
+  g_request_context = NULL;
+  pthread_mutex_unlock(&g_request_context_lock);
+}
+
+void OCSPIOLoop::PostTaskToIOLoop(
+    const tracked_objects::Location& from_here, Task* task) {
+  AutoLock autolock(lock_);
+  if (io_loop_)
+    io_loop_->PostTask(from_here, task);
+}
+
+void OCSPIOLoop::EnsureIOLoop() {
+  AutoLock autolock(lock_);
+  DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
+}
+
+void OCSPIOLoop::AddRequest(OCSPRequestSession* request) {
+  DCHECK(!ContainsKey(requests_, request));
+  requests_.insert(request);
+}
+
+void OCSPIOLoop::RemoveRequest(OCSPRequestSession* request) {
+  {
+    // Ignore if we've already shutdown.
+    AutoLock auto_lock(lock_);
+    if (shutdown_)
+      return;
+  }
+
+  DCHECK(ContainsKey(requests_, request));
+  requests_.erase(request);
+}
+
+void OCSPIOLoop::CancelAllRequests() {
+  std::set<OCSPRequestSession*> requests;
+  requests.swap(requests_);
+
+  for (std::set<OCSPRequestSession*>::iterator it = requests.begin();
+       it != requests.end(); ++it)
+    (*it)->CancelURLRequest();
+}
+
+OCSPNSSInitialization::OCSPNSSInitialization() {
+  // NSS calls the functions in the function table to download certificates
+  // or CRLs or talk to OCSP responders over HTTP.  These functions must
+  // set an NSS/NSPR error code when they fail.  Otherwise NSS will get the
+  // residual error code from an earlier failed function call.
+  client_fcn_.version = 1;
+  SEC_HttpClientFcnV1Struct *ft = &client_fcn_.fcnTable.ftable1;
+  ft->createSessionFcn = OCSPCreateSession;
+  ft->keepAliveSessionFcn = OCSPKeepAliveSession;
+  ft->freeSessionFcn = OCSPFreeSession;
+  ft->createFcn = OCSPCreate;
+  ft->setPostDataFcn = OCSPSetPostData;
+  ft->addHeaderFcn = OCSPAddHeader;
+  ft->trySendAndReceiveFcn = OCSPTrySendAndReceive;
+  ft->cancelFcn = NULL;
+  ft->freeFcn = OCSPFree;
+  SECStatus status = SEC_RegisterDefaultHttpClient(&client_fcn_);
+  if (status != SECSuccess) {
+    NOTREACHED() << "Error initializing OCSP: " << PR_GetError();
+  }
+
+  // Work around NSS bugs 524013 and 564334.  NSS incorrectly thinks the
+  // CRLs for Network Solutions Certificate Authority have bad signatures,
+  // which causes certificates issued by that CA to be reported as revoked.
+  // By using OCSP for those certificates, which don't have AIA extensions,
+  // we can work around these bugs.  See http://crbug.com/41730.
+  CERT_StringFromCertFcn old_callback = NULL;
+  status = CERT_RegisterAlternateOCSPAIAInfoCallBack(
+      GetAlternateOCSPAIAInfo, &old_callback);
+  if (status == SECSuccess) {
+    DCHECK(!old_callback);
+  } else {
+    NOTREACHED() << "Error initializing OCSP: " << PR_GetError();
+  }
+}
+
+OCSPNSSInitialization::~OCSPNSSInitialization() {}
+
 
 // OCSP Http Client functions.
 // Our Http Client functions operate in blocking mode.
@@ -878,7 +905,7 @@ URLRequestContext* GetURLRequestContextForOCSP() {
   pthread_mutex_lock(&g_request_context_lock);
   URLRequestContext* request_context = g_request_context;
   pthread_mutex_unlock(&g_request_context_lock);
-  DCHECK(request_context->is_main());
+  DCHECK(!request_context || request_context->is_main());
   return request_context;
 }