Remove default key slot from KeygenHandler.

chrome/browser/profiles/profile_io_data.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 "chrome/browser/profiles/profile_io_data.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 23 matching lines @@
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/extensions/extension_resource_protocols.h"
 #include "chrome/browser/io_thread.h"
 #include "chrome/browser/media/media_device_id_salt.h"
 #include "chrome/browser/net/about_protocol_handler.h"
 #include "chrome/browser/net/chrome_fraudulent_certificate_reporter.h"
 #include "chrome/browser/net/chrome_http_user_agent_settings.h"
 #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"
 #include "chrome/browser/signin/signin_names_io_thread.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "components/startup_metric_utils/startup_metric_utils.h"
 #include "components/sync_driver/pref_names.h"
@@ skipping 256 matching lines @@
       StartTPMSlotInitializationOnIOThread(username, username_hash);
     } else {
       DVLOG(1) << "Waiting for tpm ready ...";
     }
   } else {
     crypto::InitializePrivateSoftwareSlotForChromeOSUser(username_hash);
   }
 }
 #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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   PrefService* pref_service = profile->GetPrefs();
   PrefService* local_state_pref_service = g_browser_process->local_state();
 
   scoped_ptr<ProfileParams> params(new ProfileParams);
   params->path = profile->GetPath();
 
@@ skipping 571 matching lines @@
 void ProfileIOData::ResourceContext::CreateKeygenHandler(
     uint32 key_size_in_bits,
     const std::string& challenge_string,
     const GURL& url,
     const base::Callback<void(scoped_ptr<net::KeygenHandler>)>& callback) {
   DCHECK(!callback.is_null());
 #if defined(USE_NSS)
   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::Callback<void(crypto::ScopedPK11Slot)> get_slot_callback =
+      base::Bind(&CreateKeygenHandlerWithSlot,
+                 url,
+                 callback,
+                 base::Passed(&keygen_handler));
 
-  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();
+  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)));
 #endif
 }
 
 bool ProfileIOData::ResourceContext::AllowMicAccess(const GURL& origin) {
   return AllowContentAccess(origin, CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC);
 }
 
@@ skipping 315 matching lines @@
 void ProfileIOData::SetCookieSettingsForTesting(
     CookieSettings* cookie_settings) {
   DCHECK(!cookie_settings_.get());
   cookie_settings_ = cookie_settings;
 }
 
 void ProfileIOData::set_signin_names_for_testing(
     SigninNamesOnIOThread* signin_names) {
   signin_names_.reset(signin_names);
 }