Ensure that OCSPIOLoop is associated with right thread if restarted.

net/ocsp/nss_ocsp.cc

Index: net/ocsp/nss_ocsp.cc
diff --git a/net/ocsp/nss_ocsp.cc b/net/ocsp/nss_ocsp.cc
index cabda31dd046c86ee5ce96dd1e7f2b28cae1673c..763502101d393eb227c06dd5db0e5a12d0ababe0 100644
--- a/net/ocsp/nss_ocsp.cc
+++ b/net/ocsp/nss_ocsp.cc
@@ -59,6 +59,14 @@ class OCSPIOLoop {
     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.
@@ -923,6 +931,8 @@ void SetMessageLoopForNSSHttpIO() {
 
   // 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() {