Remove default key slot from KeygenHandler.

chrome/browser/profiles/profile_io_data.cc

Index: chrome/browser/profiles/profile_io_data.cc
diff --git a/chrome/browser/profiles/profile_io_data.cc b/chrome/browser/profiles/profile_io_data.cc
index dcf3bcf9d593e80c22e8e293bff942b704ac5f2a..69a2bbd627c32fae9c28f634f1dfe5c02314e6fb 100644
--- a/chrome/browser/profiles/profile_io_data.cc
+++ b/chrome/browser/profiles/profile_io_data.cc
@@ -41,6 +41,7 @@
 #include "chrome/browser/net/chrome_net_log.h"
 #include "chrome/browser/net/chrome_network_delegate.h"
 #include "chrome/browser/net/cookie_store_util.h"
+#include "chrome/browser/net/nss_context.h"
 #include "chrome/browser/net/proxy_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -317,6 +318,22 @@ void StartNSSInitOnIOThread(const std::string& username,
 }
 #endif  // defined(OS_CHROMEOS)
 
+#if defined(USE_NSS)
+void CreateKeygenHandlerWithSlot(
+    const GURL& url,
+    const base::Callback<void(scoped_ptr<net::KeygenHandler>)>& callback,
+    scoped_ptr<net::KeygenHandler> keygen_handler,
+    crypto::ScopedPK11Slot slot) {
+  scoped_ptr<crypto::CryptoModuleBlockingPasswordDelegate> delegate(
+      new ChromeNSSCryptoModuleDelegate(chrome::kCryptoModulePasswordKeygen,
+                                        net::HostPortPair::FromURL(url)));
+  keygen_handler->set_crypto_module_delegate(delegate.Pass());
+  keygen_handler->set_key_slot(slot.get());

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

BUG?: This may potentially close |slot|. Should we update the KeygenHandler to
make it clear it's taking a ScopedPK11Slot (and thus keeping it alive?)

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

I didn't see a point in passing ownership of a PK11Slot:
1) Anybody can increase the ref-count to the slot
2) Someone sets up and will destroy the slot. That someone should have the real
ownership (or pass it on, together with the responsibility to destroy). Passing
around multiple scoped ptrs to the same slot obscures that
ownership/responsibility. Following this argument, there should be exactly one
ScopedPK11Slot at a time and all other users have a PK11SlotInfo*. But maybe I'm
missing an important part of NSS memory management.

I agree that relying on 1) isn't ideal because it introduces dependencies on
PK11_ReferenceSlot all over the place. And in this function it's confusing that
CreateKeygenHandlerWithSlot has ownership but doesn't pass it on, because I
didn't touch GetPrivateNSSKeySlotForResourceContext.


What's your usual strategy of passing PK11Slots around?

[Ryan Sleevi, 2014/07/15 14:34:21]

We treat it like scoped_refptr, because the objects are reference counted.
(Scoped_ptr is appropriate, and how Chrome generally handles reference counted
types that are external, because few systems give insight into the actual count
to permit sane releasing strategies)

That's why I requested (and still request) scopedPtr, since it forces you to
increment the ref count beforehand (PK11_ReferenceSlot) and then pass ownership
of that reference through to the class, which will ensure the slot is references
for the duration of the class.

+
+  callback.Run(keygen_handler.Pass());
+}
+#endif  // defined(USE_NSS)
+
 }  // namespace
 
 void ProfileIOData::InitializeOnUIThread(Profile* profile) {
@@ -908,21 +925,22 @@ void ProfileIOData::ResourceContext::CreateKeygenHandler(
   scoped_ptr<net::KeygenHandler> keygen_handler(
       new net::KeygenHandler(key_size_in_bits, challenge_string, url));
 
-  scoped_ptr<ChromeNSSCryptoModuleDelegate> delegate(
-      new ChromeNSSCryptoModuleDelegate(chrome::kCryptoModulePasswordKeygen,
-                                        net::HostPortPair::FromURL(url)));
-  ChromeNSSCryptoModuleDelegate* delegate_ptr = delegate.get();
-  keygen_handler->set_crypto_module_delegate(
-      delegate.PassAs<crypto::NSSCryptoModuleDelegate>());
-
-  base::Closure bound_callback =
-      base::Bind(callback, base::Passed(&keygen_handler));
-  if (delegate_ptr->InitializeSlot(this, bound_callback)) {
-    // Initialization complete, run the callback synchronously.
-    bound_callback.Run();
+  base::Callback<void(crypto::ScopedPK11Slot)> get_slot_callback =
+      base::Bind(&CreateKeygenHandlerWithSlot,
+                 url,
+                 callback,
+                 base::Passed(&keygen_handler));
+
+  crypto::ScopedPK11Slot slot =
+      GetPrivateNSSKeySlotForResourceContext(this, get_slot_callback);
+
+  if (slot) {
+    // Obtained the slot synchronously, run the callback synchronously.
+    get_slot_callback.Run(slot.Pass());
     return;
   }
-  // Otherwise, the InitializeSlot will run the callback asynchronously.
+  // Otherwise, GetPrivateNSSKeySlotForResourceContext will run the callback
+  // asynchronously.
 #else
   callback.Run(make_scoped_ptr(
       new net::KeygenHandler(key_size_in_bits, challenge_string, url)));