Revert 60739 (still leaks on ChromiumOS!) - Reland r59972: Eagerly set the IO loop used for OCSP.

ChromeOS will create a special Profile for login. Previously, OCSP initialization was done for the "default" ChromeURLRequestContext for each Profile. Since we can have multiple profiles, this causes the initialization (and uninitialization) to happen multiple times, which causes problems for OCSP since we use statics. The solution is to identify the "main" Profile. We create said Profile in BrowserMain. I add an "is_main_" variable to URLRequestContextGetter and URLRequestContext, so that only the "main" URLRequestContext will initialize OCSP.

The only change from r59972 is the location that I set_is_main().  I set it when we set the default request context.  This is because, by the time I had called set_is_main() in BrowserMain, the ChromeURLRequestContextGetter was already created (it gets created during the CreateProfile() function).  So I was too late.

BUG=55940
TEST=Manual

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

TBR=willchan@chromium.org
Review URL: http://codereview.chromium.org/3421039

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

10 files changed, 102 insertions, 225 deletions

diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index d66a2a4..3712a06 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -50,8 +50,8 @@
 #include "chrome/browser/metrics/metrics_log.h"
 #include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/browser/net/blob_url_request_job_factory.h"
-#include "chrome/browser/net/metadata_url_request.h"
 #include "chrome/browser/net/predictor_api.h"
+#include "chrome/browser/net/metadata_url_request.h"
 #include "chrome/browser/net/sdch_dictionary_fetcher.h"
 #include "chrome/browser/net/websocket_experiment/websocket_experiment_runner.h"
 #include "chrome/browser/plugin_service.h"
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index 1df6dcf..42a8a17 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -25,9 +25,6 @@
 #include "net/base/net_util.h"
 #include "net/http/http_auth_filter.h"
 #include "net/http/http_auth_handler_factory.h"
-#if defined(USE_NSS)
-#include "net/ocsp/nss_ocsp.h"
-#endif  // defined(USE_NSS)
 
 namespace {
 
@@ -161,12 +158,6 @@ void IOThread::ChangedToOnTheRecord() {
 void IOThread::Init() {
   BrowserProcessSubThread::Init();
 
-  DCHECK_EQ(MessageLoop::TYPE_IO, message_loop()->type());
-
-#if defined(USE_NSS)
-  net::SetMessageLoopForOCSP();
-#endif // defined(USE_NSS)
-
   DCHECK(!globals_);
   globals_ = new Globals;
 
diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc
index 1322f99..7cc4a91 100644
--- a/chrome/browser/net/chrome_url_request_context.cc
+++ b/chrome/browser/net/chrome_url_request_context.cc
@@ -313,6 +313,11 @@ ChromeURLRequestContext* FactoryForOriginal::Create() {
 
   appcache_service_->set_request_context(context);
 
+#if defined(USE_NSS)
+  // TODO(ukai): find a better way to set the URLRequestContext for OCSP.
+  net::SetURLRequestContextForOCSP(context);
+#endif
+
   context->set_net_log(io_thread_globals->net_log.get());
   return context;
 }
@@ -552,14 +557,6 @@ URLRequestContext* ChromeURLRequestContextGetter::GetURLRequestContext() {
   if (!url_request_context_) {
     DCHECK(factory_.get());
     url_request_context_ = factory_->Create();
-    if (is_main()) {
-      url_request_context_->set_is_main(true);
-#if defined(USE_NSS)
-      // TODO(ukai): find a better way to set the URLRequestContext for OCSP.
-      net::SetURLRequestContextForOCSP(url_request_context_);
-#endif
-    }
-
     factory_.reset();
   }
 
@@ -774,8 +771,7 @@ ChromeURLRequestContext::~ChromeURLRequestContext() {
   }
 
 #if defined(USE_NSS)
-  if (is_main()) {
-    DCHECK_EQ(this, net::GetURLRequestContextForOCSP());
+  if (this == net::GetURLRequestContextForOCSP()) {
     // We are releasing the URLRequestContext used by OCSP handlers.
     net::SetURLRequestContextForOCSP(NULL);
   }
diff --git a/chrome/browser/net/chrome_url_request_context.h b/chrome/browser/net/chrome_url_request_context.h
index fa58bcc..ebae377 100644
--- a/chrome/browser/net/chrome_url_request_context.h
+++ b/chrome/browser/net/chrome_url_request_context.h
@@ -278,6 +278,7 @@ class ChromeURLRequestContext : public URLRequestContext {
   bool is_off_the_record_;
 
  private:
+
   DISALLOW_COPY_AND_ASSIGN(ChromeURLRequestContext);
 };
 
diff --git a/chrome/browser/profile_impl.cc b/chrome/browser/profile_impl.cc
index 8203739c..5a61e1c 100644
--- a/chrome/browser/profile_impl.cc
+++ b/chrome/browser/profile_impl.cc
@@ -722,7 +722,6 @@ URLRequestContextGetter* ProfileImpl::GetRequestContext() {
     // created first.
     if (!default_request_context_) {
       default_request_context_ = request_context_;
-      request_context_->set_is_main(true);
       // TODO(eroman): this isn't terribly useful anymore now that the
       // URLRequestContext is constructed by the IO thread...
       NotificationService::current()->Notify(
diff --git a/chrome/common/net/url_request_context_getter.h b/chrome/common/net/url_request_context_getter.h
index 8b51a0b..34aa668 100644
--- a/chrome/common/net/url_request_context_getter.h
+++ b/chrome/common/net/url_request_context_getter.h
@@ -35,32 +35,15 @@ class URLRequestContextGetter
   // may be used).
   virtual scoped_refptr<base::MessageLoopProxy> GetIOMessageLoopProxy() = 0;
 
-  // Controls whether or not the URLRequestContextGetter considers itself to be
-  // the the "main" URLRequestContextGetter.  Note that each Profile will have a
-  // "default" URLRequestContextGetter.  Therefore, "is_main" refers to the
-  // default URLRequestContextGetter for the "main" Profile.
-  // TODO(willchan): Move this code to ChromeURLRequestContextGetter, since this
-  // ia a browser process specific concept.
-  void set_is_main(bool is_main) { is_main_ = is_main; }
-
  protected:
   friend class DeleteTask<URLRequestContextGetter>;
   friend struct URLRequestContextGetterTraits;
 
-  URLRequestContextGetter() : is_main_(false) {}
-
   virtual ~URLRequestContextGetter() {}
-
-  bool is_main() const { return is_main_; }
-
  private:
   // OnDestruct is meant to ensure deletion on the thread on which the request
   // IO happens.
   void OnDestruct();
-
-  // Indicates whether or not this is the default URLRequestContextGetter for
-  // the main Profile.
-  bool is_main_;
 };
 
 struct URLRequestContextGetterTraits {
@@ -70,3 +53,4 @@ struct URLRequestContextGetterTraits {
 };
 
 #endif  // CHROME_COMMON_NET_URL_REQUEST_CONTEXT_GETTER_H_
+
diff --git a/net/ocsp/nss_ocsp.cc b/net/ocsp/nss_ocsp.cc
index b60c146..2368df0 100644
--- a/net/ocsp/nss_ocsp.cc
+++ b/net/ocsp/nss_ocsp.cc
@@ -9,19 +9,16 @@
 #include <ocsp.h>
 #include <nspr.h>
 #include <nss.h>
-#include <pthread.h>
 #include <secerr.h>
 
 #include <string>
 
-#include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/condition_variable.h"
 #include "base/histogram.h"
-#include "base/lazy_instance.h"
-#include "base/lock.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
+#include "base/singleton.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/thread.h"
@@ -36,93 +33,6 @@
 
 namespace {
 
-// Protects |g_request_context|.
-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();
-
-  void StartUsing() {
-    AutoLock autolock(lock_);
-    used_ = true;
-  }
-
-  bool used() const {
-    AutoLock autolock(lock_);
-    return used_;
-  }
-
-  // Called from worker thread.
-  void PostTaskToIOLoop(const tracked_objects::Location& from_here, Task* task);
-
-  void EnsureIOLoop();
-
- private:
-  friend struct base::DefaultLazyInstanceTraits<OCSPIOLoop>;
-
-  OCSPIOLoop();
-  ~OCSPIOLoop();
-
-  mutable Lock lock_;
-  bool used_;  // Protected by |lock_|.
-  // This should not be modified after |used_|.
-  MessageLoopForIO* io_loop_;  // Protected by |lock_|.
-
-  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::WillDestroyCurrentMessageLoop() {
-  // 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;
 
 // All OCSP handlers should be called in the context of
@@ -159,58 +69,97 @@ SECStatus OCSPFree(SEC_HTTP_REQUEST_SESSION request);
 
 char* GetAlternateOCSPAIAInfo(CERTCertificate *cert);
 
-class OCSPNSSInitialization {
- private:
-  friend struct base::DefaultLazyInstanceTraits<OCSPNSSInitialization>;
+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;
+  };
 
-  OCSPNSSInitialization();
-  ~OCSPNSSInitialization();
+  // 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);
+  }
 
-  SEC_HttpClientFcn client_fcn_;
+  // 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_;
+  }
 
-  DISALLOW_COPY_AND_ASSIGN(OCSPNSSInitialization);
-};
+ private:
+  friend struct DefaultSingletonTraits<OCSPInitSingleton>;
+
+  OCSPInitSingleton()
+      : io_loop_(MessageLoopForIO::current()) {
+    DCHECK(io_loop_);
+    io_loop_->AddDestructionObserver(this);
+
+    // 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();
+    }
 
-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();
+    // 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() {}
+  virtual ~OCSPInitSingleton() {
+    // 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_;
 
-base::LazyInstance<OCSPNSSInitialization> g_ocsp_nss_initialization(
-    base::LINKER_INITIALIZED);
+  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)
@@ -251,7 +200,7 @@ class OCSPRequestSession
     // |io_loop_| was initialized to be NULL in constructor, and
     // set only in StartURLRequest, so no need to lock |lock_| here.
     DCHECK(!io_loop_);
-    g_ocsp_io_loop.Get().PostTaskToIOLoop(
+    Singleton<OCSPInitSingleton>()->PostTaskToIOLoop(
         FROM_HERE,
         NewRunnableMethod(this, &OCSPRequestSession::StartURLRequest));
   }
@@ -390,14 +339,11 @@ class OCSPRequestSession
     }
   }
 
-  // Runs on |g_ocsp_io_loop|'s IO loop.
   void StartURLRequest() {
     DCHECK(!request_);
 
-    pthread_mutex_lock(&g_request_context_lock);
-    URLRequestContext* url_request_context = g_request_context;
-    pthread_mutex_unlock(&g_request_context_lock);
-
+    URLRequestContext* url_request_context =
+        OCSPInitSingleton::url_request_context();
     if (url_request_context == NULL)
       return;
 
@@ -530,10 +476,7 @@ SECStatus OCSPCreateSession(const char* host, PRUint16 portnum,
                             SEC_HTTP_SERVER_SESSION* pSession) {
   LOG(INFO) << "OCSP create session: host=" << host << " port=" << portnum;
   DCHECK(!MessageLoop::current());
-  pthread_mutex_lock(&g_request_context_lock);
-  URLRequestContext* request_context = g_request_context;
-  pthread_mutex_unlock(&g_request_context_lock);
-  if (request_context == NULL) {
+  if (OCSPInitSingleton::url_request_context() == NULL) {
     LOG(ERROR) << "No URLRequestContext for OCSP handler.";
     // The application failed to call SetURLRequestContextForOCSP, so we
     // can't create and use URLRequest.  PR_NOT_IMPLEMENTED_ERROR is not an
@@ -840,40 +783,17 @@ char* GetAlternateOCSPAIAInfo(CERTCertificate *cert) {
 
 namespace net {
 
-void SetMessageLoopForOCSP() {
-  // Must have a MessageLoopForIO.
-  DCHECK(MessageLoopForIO::current());
-
-  bool used = g_ocsp_io_loop.Get().used();
-
-  // Should not be called when g_ocsp_io_loop has already been used.
-  DCHECK(!used);
-}
-
 void EnsureOCSPInit() {
-  g_ocsp_io_loop.Get().StartUsing();
-  g_ocsp_nss_initialization.Get();
+  Singleton<OCSPInitSingleton>::get();
 }
 
 // This function would be called before NSS initialization.
 void SetURLRequestContextForOCSP(URLRequestContext* request_context) {
-  pthread_mutex_lock(&g_request_context_lock);
-  if (request_context) {
-    DCHECK(request_context->is_main());
-    DCHECK(!g_request_context);
-  } else {
-    DCHECK(g_request_context);
-  }
-  g_request_context = request_context;
-  pthread_mutex_unlock(&g_request_context_lock);
+  OCSPInitSingleton::set_url_request_context(request_context);
 }
 
 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());
-  return request_context;
+  return OCSPInitSingleton::url_request_context();
 }
 
 }  // namespace net
diff --git a/net/ocsp/nss_ocsp.h b/net/ocsp/nss_ocsp.h
index 97d69b9..a31d025 100644
--- a/net/ocsp/nss_ocsp.h
+++ b/net/ocsp/nss_ocsp.h
@@ -10,11 +10,6 @@ class URLRequestContext;
 
 namespace net {
 
-// Sets the MessageLoop for OCSP to the current message loop.
-// This should be called before EnsureOCSPInit() if you want to
-// control the message loop for OCSP.
-void SetMessageLoopForOCSP();
-
 // Initializes OCSP handlers for NSS.  This must be called before any
 // certificate verification functions.  This function is thread-safe, and OCSP
 // handlers will only ever be initialized once.
diff --git a/net/url_request/url_request_context.cc b/net/url_request/url_request_context.cc
index 5c50bbb..0ea8477 100644
--- a/net/url_request/url_request_context.cc
+++ b/net/url_request/url_request_context.cc
@@ -15,8 +15,7 @@ URLRequestContext::URLRequestContext()
       http_auth_handler_factory_(NULL),
       network_delegate_(NULL),
       cookie_policy_(NULL),
-      transport_security_state_(NULL),
-      is_main_(false) {
+      transport_security_state_(NULL) {
 }
 
 const std::string& URLRequestContext::GetUserAgent(const GURL& url) const {
diff --git a/net/url_request/url_request_context.h b/net/url_request/url_request_context.h
index 548d0c6..6ddc940 100644
--- a/net/url_request/url_request_context.h
+++ b/net/url_request/url_request_context.h
@@ -104,11 +104,6 @@ class URLRequestContext
     referrer_charset_ = charset;
   }
 
-  // Controls whether or not the URLRequestContext considers itself to be the
-  // "main" URLRequestContext.
-  bool is_main() const { return is_main_; }
-  void set_is_main(bool is_main) { is_main_ = is_main; }
-
  protected:
   friend class base::RefCountedThreadSafe<URLRequestContext>;
 
@@ -136,9 +131,6 @@ class URLRequestContext
   std::string referrer_charset_;
 
  private:
-  // Indicates whether or not this is the main URLRequestContext.
-  bool is_main_;
-
   DISALLOW_COPY_AND_ASSIGN(URLRequestContext);
 };