net: add OCSP tests.

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 1eb0b21d41a916d296a17d8beea7f0db97a9d1d0..5600f60ea0dc60b21e975c9d4ea9a7f04bc93e79 100644
--- a/net/ocsp/nss_ocsp.cc
+++ b/net/ocsp/nss_ocsp.cc
@@ -49,6 +49,8 @@ class OCSPIOLoop {
   void StartUsing() {
     base::AutoLock autolock(lock_);
     used_ = true;
+    io_loop_ = MessageLoopForIO::current();
+    DCHECK(io_loop_);
   }
 
   // Called on IO loop.
@@ -456,8 +458,7 @@ class OCSPServerSession {
 OCSPIOLoop::OCSPIOLoop()
     : shutdown_(false),
       used_(false),
-      io_loop_(MessageLoopForIO::current()) {
-  DCHECK(io_loop_);
+      io_loop_(NULL) {
 }
 
 OCSPIOLoop::~OCSPIOLoop() {
@@ -512,13 +513,6 @@ void OCSPIOLoop::AddRequest(OCSPRequestSession* request) {
 }
 
 void OCSPIOLoop::RemoveRequest(OCSPRequestSession* request) {
-  {
-    // Ignore if we've already shutdown.
-    base::AutoLock auto_lock(lock_);
-    if (shutdown_)
-      return;
-  }
-
   DCHECK(ContainsKey(requests_, request));
   requests_.erase(request);
 }
@@ -527,9 +521,11 @@ void OCSPIOLoop::CancelAllRequests() {
   std::set<OCSPRequestSession*> requests;
   requests.swap(requests_);
 
-  for (std::set<OCSPRequestSession*>::iterator it = requests.begin();
-       it != requests.end(); ++it)
-    (*it)->CancelURLRequest();
+  // We can't use an iterator here because CancelURLRequest will call back into
+  // us with RemoveRequest and mutate |requests_|.
+  while (!requests_.empty()) {

[Ryan Sleevi, 2012/03/13 23:06:39]

This is surely wrong - the swap on line 522 should have requests_.empty() always
be true.

Further, I'm not sure how the original code was bugged, since mutating
|requests_| would be fine, since we swapped on line 528 (original) and only
iterate the stack copy (line 530).

[agl, 2012/03/13 23:44:03]

Yes, sorry. That was a slip with git add -p, it shouldn't have been staged. This
was me misunderstanding what was going wrong. The rest of the changes in this
file are the bug fix. In short:

1) We would grab the MessageLoop at construction time, which broke if we did an
Init/Shutdown several times with a different MessageLoop.

2) The code in RemoveRequest to ignore removals at shutdown time would mean that
they were left in requests_ and triggered a use-after-free when we shut it down
again.

[Ryan Sleevi, 2012/03/13 23:50:20]

For unit tests that are affected by such globals, you're supposed to construct a
base::ShadowingAtExitManager within the test (fixture or individual test).

This should allow you to avoid most of the changes, mod this one.
Is this unnecessary if using a ShadowingAtExitManager?

I'm just trying to figure out where the root bug is.

+    (*requests.begin())->CancelURLRequest();
+  }
 }
 
 OCSPNSSInitialization::OCSPNSSInitialization() {