Don't call RemoveDestructionObserver on non-IO thread.

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>
 #include <nss.h>
 #include <secerr.h>
 
 #include <string>
 
 #include "base/compiler_specific.h"
 #include "base/condition_variable.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
+#include "base/singleton.h"
 #include "base/thread.h"
 #include "base/time.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 
 namespace {
 
-static const int kRecvBufferSize = 4096;
+const int kRecvBufferSize = 4096;
 
 // All OCSP handlers should be called in the context of
 // CertVerifier's thread (i.e. worker pool, not on the I/O thread).
 // It supports blocking mode only.
 
 class OCSPInitSingleton : public MessageLoop::DestructionObserver {
  public:
+  // Called on IO thread.
   virtual void WillDestroyCurrentMessageLoop() {
+    AutoLock autolock(lock_);
+    DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
     io_loop_ = NULL;
+    request_context_ = NULL;
   };
 
-  MessageLoop* io_thread() const {
-    return io_loop_;
+  // Called from worker thread.
+  void PostTaskToIOLoop(
+      const tracked_objects::Location& from_here, Task* task) {
+    AutoLock autolock(lock_);
+    if (io_loop_)
+      io_loop_->PostTask(from_here, task);
   }
 
   // This is static method because it is called before NSS initialization,
   // that is, before OCSPInitSingleton is initialized.
   static void set_url_request_context(URLRequestContext* request_context) {
     request_context_ = request_context;
   }
   static URLRequestContext* url_request_context() {
     return request_context_;
   }
 
  private:
   friend struct DefaultSingletonTraits<OCSPInitSingleton>;
+
   OCSPInitSingleton();
   virtual ~OCSPInitSingleton() {
-    if (io_loop_)
-      io_loop_->RemoveDestructionObserver(this);
-    request_context_ = NULL;
+    // IO thread was already deleted before the singleton is deleted
+    // in AtExitManager.
+    AutoLock autolock(lock_);
+    DCHECK(!io_loop_);
+    DCHECK(!request_context_);
   }
 
   SEC_HttpClientFcn client_fcn_;
 
+  // |lock_| protects |io_loop_|.
+  Lock lock_;
   // I/O thread.
   MessageLoop* io_loop_;  // I/O thread
-
   // URLRequestContext for OCSP handlers.
   static URLRequestContext* request_context_;
 
   DISALLOW_COPY_AND_ASSIGN(OCSPInitSingleton);
 };
 
 URLRequestContext* OCSPInitSingleton::request_context_ = NULL;
 
 // Concrete class for SEC_HTTP_REQUEST_SESSION.
 // Public methods except virtual methods of URLRequest::Delegate (On* methods)
 // run on certificate verifier thread (worker thread).
 // Virtual methods of URLRequest::Delegate and private methods run
 // on IO thread.
 class OCSPRequestSession
     : public base::RefCountedThreadSafe<OCSPRequestSession>,
       public URLRequest::Delegate,
       public MessageLoop::DestructionObserver {
  public:
   OCSPRequestSession(const GURL& url,
                      const char* http_request_method,
                      base::TimeDelta timeout)
       : url_(url),
         http_request_method_(http_request_method),
         timeout_(timeout),
         request_(NULL),
         buffer_(new net::IOBuffer(kRecvBufferSize)),
         response_code_(-1),
         cv_(&lock_),
-        io_loop_(Singleton<OCSPInitSingleton>::get()->io_thread()),
+        io_loop_(NULL),
         finished_(false) {}
 
   void SetPostData(const char* http_data, PRUint32 http_data_len,
                    const char* http_content_type) {
     upload_content_.assign(http_data, http_data_len);
     upload_content_type_.assign(http_content_type);
   }
 
   void AddHeader(const char* http_header_name, const char* http_header_value) {
     if (!extra_request_headers_.empty())
       extra_request_headers_ += "\r\n";
     StringAppendF(&extra_request_headers_,
                   "%s: %s", http_header_name, http_header_value);
   }
 
   void Start() {
-    // 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::StartURLRequest));
-    }
+    // At this point, it runs on worker thread.
+    // |io_loop_| was initialized to be NULL in constructor, and
+    // set only in StartURLRequest, so no need to lock |lock_| here.
+    DCHECK(!io_loop_);
+    Singleton<OCSPInitSingleton>()->PostTaskToIOLoop(
+        FROM_HERE,
+        NewRunnableMethod(this, &OCSPRequestSession::StartURLRequest));
   }
 
   bool Started() const {
     return request_ != NULL;
   }
 
   void Cancel() {
     // IO thread may set |io_loop_| to NULL, so protect by |lock_|.
     AutoLock autolock(lock_);
     CancelLocked();
@@ skipping 99 matching lines @@
       AutoLock autolock(lock_);
       io_loop_ = NULL;
     }
     CancelURLRequest();
   }
 
  private:
   friend class base::RefCountedThreadSafe<OCSPRequestSession>;
 
   virtual ~OCSPRequestSession() {
+    // When this destructor is called, there should be only one thread that has
+    // a reference to this object, and so that thread doesn't need to lock
+    // |lock_| here.
     DCHECK(!request_);
-    if (io_loop_)
-      io_loop_->RemoveDestructionObserver(this);
+    DCHECK(!io_loop_);
   }
 
+  // Must call this method while holding |lock_|.
   void CancelLocked() {
+    lock_.AssertAcquired();
     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);
+    URLRequestContext* url_request_context =
+        OCSPInitSingleton::url_request_context();
+    if (url_request_context == NULL)
+      return;
+
+    {
+      AutoLock autolock(lock_);
+      DCHECK(!io_loop_);
+      io_loop_ = MessageLoopForIO::current();
+      io_loop_->AddDestructionObserver(this);
+    }
 
     request_ = new URLRequest(url_, this);
-    request_->set_context(
-        Singleton<OCSPInitSingleton>::get()->url_request_context());
+    request_->set_context(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_);
+#ifndef NDEBUG
+    {
+      AutoLock autolock(lock_);
+      if (io_loop_)
+        DCHECK_EQ(MessageLoopForIO::current(), io_loop_);
+    }
+#endif
     if (request_) {
       request_->Cancel();
       delete request_;
       request_ = NULL;
       {
         AutoLock autolock(lock_);
         finished_ = true;
         io_loop_ = NULL;
       }
       cv_.Signal();
@@ skipping 64 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(OCSPServerSession);
 };
 
 
 // OCSP Http Client functions.
 // Our Http Client functions operate in blocking mode.
 SECStatus OCSPCreateSession(const char* host, PRUint16 portnum,
                             SEC_HTTP_SERVER_SESSION* pSession) {
   LOG(INFO) << "OCSP create session: host=" << host << " port=" << portnum;
   DCHECK(!MessageLoop::current());
-  if (Singleton<OCSPInitSingleton>::get()->url_request_context() == NULL) {
+  if (OCSPInitSingleton::url_request_context() == NULL) {
     LOG(ERROR) << "No URLRequestContext for OCSP handler.";
     return SECFailure;
   }
   *pSession = new OCSPServerSession(host, portnum);
   return SECSuccess;
 }
 
 SECStatus OCSPKeepAliveSession(SEC_HTTP_SERVER_SESSION session,
                                PRPollDesc **pPollDesc) {
   LOG(INFO) << "OCSP keep alive";
@@ skipping 147 matching lines @@
   LOG(INFO) << "OCSP free";
   DCHECK(!MessageLoop::current());
   OCSPRequestSession* req = reinterpret_cast<OCSPRequestSession*>(request);
   req->Cancel();
   req->Release();
   return SECSuccess;
 }
 
 OCSPInitSingleton::OCSPInitSingleton()
     : io_loop_(MessageLoopForIO::current()) {
+  DCHECK(io_loop_);
   io_loop_->AddDestructionObserver(this);
   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;
@@ skipping 16 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