Remove default key slot from KeygenHandler.

net/base/keygen_handler_unittest.cc

Index: net/base/keygen_handler_unittest.cc
diff --git a/net/base/keygen_handler_unittest.cc b/net/base/keygen_handler_unittest.cc
index a868b2925c8745ee9362f8cf1a6dee84df208028..e4012dc90bbca073ffd75bea7a356aef337808c2 100644
--- a/net/base/keygen_handler_unittest.cc
+++ b/net/base/keygen_handler_unittest.cc
@@ -10,11 +10,13 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/threading/worker_pool.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/synchronization/waitable_event.h"
 #include "build/build_config.h"
 #include "crypto/nss_util.h"
+#include "crypto/nss_util_internal.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(USE_NSS)
@@ -30,10 +32,16 @@ class KeygenHandlerTest : public ::testing::Test {
   KeygenHandlerTest() {}
   virtual ~KeygenHandlerTest() {}
 
- private:
-#if defined(OS_CHROMEOS) && defined(USE_NSS)
+ protected:
+#if defined(USE_NSS)
+  crypto::ScopedPK11Slot GetKeySlot() {
+    return crypto::ScopedPK11Slot(crypto::GetPersistentNSSKeySlot());
+  }
+
+#if defined(OS_CHROMEOS)

[Ryan Sleevi, 2014/07/14 21:54:25]

Is there any reason you restrict this to ChromeOS?

Seems like we'd *always* want it in a test DB, with your changes.

[pneubeck (no reviews), 2014/07/15 13:22:22]

Ack. Not sure why the restriction was there before.

   crypto::ScopedTestNSSDB test_nss_db_;
 #endif
+#endif
 };
 
 // Assert that |result| is a valid output for KeygenHandler given challenge
@@ -74,6 +82,9 @@ void AssertValidSignedPublicKeyAndChallenge(const std::string& result,
 
 TEST_F(KeygenHandlerTest, SmokeTest) {
   KeygenHandler handler(768, "some challenge", GURL("http://www.example.com"));
+#if defined(USE_NSS)
+  handler.set_key_slot(GetKeySlot().get());
+#endif
   handler.set_stores_key(false);  // Don't leave the key-pair behind
   std::string result = handler.GenKeyAndSignChallenge();
   VLOG(1) << "KeygenHandler produced: " << result;
@@ -82,11 +93,17 @@ TEST_F(KeygenHandlerTest, SmokeTest) {
 
 void ConcurrencyTestCallback(base::WaitableEvent* event,
                              const std::string& challenge,
+#if defined(USE_NSS)
+                             PK11SlotInfo* slot,
+#endif

[Ryan Sleevi, 2014/07/14 21:54:25]

I'm not at all a fan of embedding #ifdefs# like this. Is there some way we can
abstract this out to the minimal bits necessary?

[pneubeck (no reviews), 2014/07/15 13:22:21]

Yeah, this was a rather quick hack to see whether the change will work.
Cleaned this up now.

                              std::string* result) {
   // We allow Singleton use on the worker thread here since we use a
   // WaitableEvent to synchronize, so it's safe.
   base::ThreadRestrictions::ScopedAllowSingleton scoped_allow_singleton;
   KeygenHandler handler(768, challenge, GURL("http://www.example.com"));
+#if defined(USE_NSS)
+  handler.set_key_slot(slot);
+#endif
   handler.set_stores_key(false);  // Don't leave the key-pair behind.
   *result = handler.GenKeyAndSignChallenge();
   event->Signal();
@@ -105,16 +122,23 @@ void ConcurrencyTestCallback(base::WaitableEvent* event,
 // We asynchronously generate the keys so as not to hang up the IO thread. This
 // test tries to catch concurrency problems in the keygen implementation.
 TEST_F(KeygenHandlerTest, ConcurrencyTest) {
+#if defined(USE_NSS)
+  crypto::ScopedPK11Slot slot = GetKeySlot();
+#endif
   const int NUM_HANDLERS = 5;
   base::WaitableEvent* events[NUM_HANDLERS] = { NULL };
   std::string results[NUM_HANDLERS];
   for (int i = 0; i < NUM_HANDLERS; i++) {
     events[i] = new base::WaitableEvent(false, false);
-    base::WorkerPool::PostTask(
-        FROM_HERE,
-        base::Bind(ConcurrencyTestCallback, events[i], "some challenge",
-                   &results[i]),
-        true);
+    base::WorkerPool::PostTask(FROM_HERE,
+                               base::Bind(ConcurrencyTestCallback,
+                                          events[i],
+                                          "some challenge",
+#if defined(USE_NSS)
+                                          slot.get(),
+#endif
+                                          &results[i]),

[Ryan Sleevi, 2014/07/14 21:54:25]

Perhaps wrapping this base::Bind()ing into a helper function that creates a
callback, as appropriate?

[pneubeck (no reviews), 2014/07/15 13:22:22]

The handler doesn't have to be constructed in on the worker thread. Creating and
setting it up before now.

+                               true);
   }
 
   for (int i = 0; i < NUM_HANDLERS; i++) {