Ensure that OCSPIOLoop is associated with right thread if restarted.

net/ocsp/nss_ocsp.cc

 // Copyright (c) 2012 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 41 matching lines @@
 
 class OCSPRequestSession;
 
 class OCSPIOLoop {
  public:
   void StartUsing() {
     base::AutoLock autolock(lock_);
     used_ = true;
     io_loop_ = MessageLoopForIO::current();
     DCHECK(io_loop_);
+    // In tests |g_ocsp_io_loop| may be shut down and restarted. Ensure that
+    // the thread checker is associated with the correct thread after a restart
+    // to avoid DCHECK's going off later.
+    if (shutdown_) {
+      thread_checker_.DetachFromThread();
+      thread_checker_.CalledOnValidThread();
+      shutdown_ = false;
+    }

[Ryan Sleevi, 2012/11/05 14:43:31]

Perhaps moving this into an explicit Reset() method to signal the intent to
re-use this (see comments below re: |used|)

[blundell, 2012/11/05 15:19:02]

I thought about factoring this out into its own method, and maybe even adding
that method to the public interface so that the re-use would have to be enabled
via explicit client invocation rather than happening beyond the scenes (and
could make clear via comments/method naming that the re-use should be enabled
only when testing). Do you like that idea?

On 2012/11/05 14:43:31, Ryan Sleevi wrote:

[Ryan Sleevi, 2012/11/06 15:43:47]

IMO, yeah. Makes it easier to separate out bugs-in-tests and bugs-in-impl, and
the default behaviour (requiring callers to call ResetForTest[ing] ) would allow
the presubmit scripts to say "O hai, you're calling a testing method from a
non-unittest file"

   }
 
   // Called on IO loop.
   void Shutdown();
 
   bool used() const {
     base::AutoLock autolock(lock_);
     return used_;
   }
 
@@ skipping 843 matching lines @@
 
 }  // anonymous namespace
 
 void SetMessageLoopForNSSHttpIO() {
   // 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);

[Ryan Sleevi, 2012/11/05 14:43:31]

What about the fact that this is not reset?

Attempting to call this in a subsequent test will trigger an issue

[blundell, 2012/11/05 15:19:02]

used_ is set to false in Shutdown() (where shutdown_ is set to true).

On 2012/11/05 14:43:31, Ryan Sleevi wrote:

+  // Ensure that |g_ocsp_io_loop| is associated with the current message loop.
+  g_ocsp_io_loop.Get().StartUsing();

[Ryan Sleevi, 2012/11/05 14:43:31]

I'm not sure I understand the motivation here for this change?

Why would someone not call EnsureNSSHttpIOInit? (Granted, perhaps I'm confused
myself on the distinction between the two)

[blundell, 2012/11/05 15:19:02]

|SetMessageLoopForNSSHttpIO| is called in |IOThread::Init()|. My thought was
that by adding the call to |StartUsing|, the re-use could occur transparently in
the case where multiple IO threads are being created sequentially for testing.
Of course, this goes directly against the idea of making the re-use explicit and
targeted...I was torn on which direction to go.

On 2012/11/05 14:43:31, Ryan Sleevi wrote:

[Ryan Sleevi, 2012/11/06 15:43:47]

I would prefer not to tightly couple these two (eg: don't call StartUsing).
Otherwise, it remains wholly unclear the distinction between this and
EnsureNSSHttpIOInit

 }
 
 void EnsureNSSHttpIOInit() {
   g_ocsp_io_loop.Get().StartUsing();
   g_ocsp_nss_initialization.Get();
 }
 
 void ShutdownNSSHttpIO() {
   g_ocsp_io_loop.Get().Shutdown();
 }
 
 // This function would be called before NSS initialization.
 void SetURLRequestContextForNSSHttpIO(URLRequestContext* request_context) {
   pthread_mutex_lock(&g_request_context_lock);
   if (request_context) {
     DCHECK(!g_request_context);
   }
   g_request_context = request_context;
   pthread_mutex_unlock(&g_request_context_lock);
 }
 
 }  // namespace net