Update keygen to use correct NSS slot on ChromeOS multiprofile.

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 44 matching lines @@
 #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 "content/public/browser/browser_thread.h"
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/resource_context.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/common/constants.h"
+#include "net/base/keygen_handler.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_monster.h"
 #include "net/http/http_transaction_factory.h"
 #include "net/http/http_util.h"
 #include "net/http/transport_security_persister.h"
 #include "net/proxy/proxy_config_service_fixed.h"
 #include "net/proxy/proxy_script_fetcher_impl.h"
 #include "net/proxy/proxy_service.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/ssl/client_cert_store_impl.h"
@@ skipping 21 matching lines @@
 #include "chrome/browser/chromeos/policy/policy_cert_verifier.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "crypto/nss_util.h"
 #include "crypto/nss_util_internal.h"
 #endif  // defined(OS_CHROMEOS)
 
 #if defined(USE_NSS)
-#include "chrome/browser/ui/crypto_module_password_dialog.h"
+#include "chrome/browser/ui/crypto_module_delegate_nss.h"
 #endif
 
 using content::BrowserContext;
 using content::BrowserThread;
 using content::ResourceContext;
 
 namespace {
 
 // ----------------------------------------------------------------------------
 // CookieMonster::Delegate implementation
@@ skipping 702 matching lines @@
   DCHECK(io_data_->initialized_);
   return request_context_;
 }
 
 scoped_ptr<net::ClientCertStore>
 ProfileIOData::ResourceContext::CreateClientCertStore() {
 #if !defined(USE_OPENSSL)
   scoped_ptr<net::ClientCertStoreImpl> store(new net::ClientCertStoreImpl());
 #if defined(USE_NSS)
   store->set_password_delegate_factory(
-      base::Bind(&chrome::NewCryptoModuleBlockingDialogDelegate,
+      base::Bind(&chrome::CreateCryptoModuleBlockingPasswordDelegate,
                  chrome::kCryptoModulePasswordClientAuth));
 #endif
   return store.PassAs<net::ClientCertStore>();
 #else  // defined(USE_OPENSSL)
   // OpenSSL does not use the ClientCertStore infrastructure. On Android client
   // cert matching is done by the OS as part of the call to show the cert
   // selection dialog.
   return scoped_ptr<net::ClientCertStore>();
 #endif
 }
 
+scoped_ptr<net::KeygenHandler>
+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());
+  scoped_ptr<net::KeygenHandler> keygen_handler(
+      new net::KeygenHandler(key_size_in_bits, challenge_string, url));
+
+#if defined(USE_NSS)
+  scoped_ptr<ChromeNSSCryptoModuleDelegate> delegate(
+      new ChromeNSSCryptoModuleDelegate(chrome::kCryptoModulePasswordKeygen,
+                                        url.host()));
+  ChromeNSSCryptoModuleDelegate* delegate_ptr = delegate.get();
+  keygen_handler->set_crypto_module_delegate(
+      delegate.PassAs<crypto::NSSCryptoModuleDelegate>());
+
+  if (delegate_ptr->InitializeSlot(this, base::Closure()))
+    return keygen_handler.Pass();
+
+  bool rv = delegate_ptr->InitializeSlot(
+      this, base::Bind(callback, base::Passed(&keygen_handler)));
+  DCHECK(!rv);

[Ryan Sleevi, 2013/12/05 00:23:19]

I still find the async setup here a little weird.

I understand you want to support both synchronous (already-initialized) and
async cases, but the "calling InitializeSlot twice" paradigm feels... wrong.

Can we not use the net/ pattern of "true / positive value = completed
synchronously, false / negative value = completing async"?

I'm also not sure I understand the line 866 below. Minimally, a comment
explaining why returning NULL (and yet initializing one asynchronously above) is
somehow OK.

[mattm, 2013/12/05 00:42:44]

Yeah, this is weird. The problem is the base::Passed(&keygen_handler) in the
callback. With only a single call to InitializeSlot, the keygen_handler is
already owned by the callback so can't be returned on synchronous completion.
In the async case, the keygen_handler is passed to the callback, so it returns
NULL on 866.


I guess I could address both by changing the signature to:
bool CreateKeygenHandler(
       uint32 key_size_in_bits,
       const std::string& challenge_string,
       const GURL& url,
       const base::Closure& callback,
       scoped_ptr<KeygenHandler>* keygen_handler);

Then it would always set the keygen_handler, and in the sync case return true,
and the async case return false and use the callback on completion. I guess I
found the current way a little safer since it never returns a partially
initialized KeygenHandler, but it does make the implementation here weirder.
WDYT?

[mattm, 2013/12/05 00:46:48]

Oh yeah, the problem with that approach is that then the caller
(RenderMessageFilter) still somehow needs to get the KeygenHandler into the
callback so that it can do the magic once it is initialized.  If the
CreateKeygenHandler always returns the keygen handler, then there isn't a good
way to get it into the callback that I see.

[Ryan Sleevi, 2013/12/05 01:05:54]

I feel like you can avoid this by avoiding the base::Passed(&keygen_handler) in
currying, since you don't "always" want it passed (sync vs async)

struct ISuckAtNaming {
  scoped_ptr<KeygenHandler> handler;
};

void MyHelper(const base::Callback<void(scoped_ptr<net::KeygenHandler>)>&
callback,
ISuckAtNaming* foo) {
  callback.Run(foo->handler.Pass());
};

ISuckAtNaming* foo = new ISuckAtNaming;
foo->handler = keygen_handler.Pass();

// Keep |cb| on the stack, since ownership of |foo| is transferred to |cb|.
// This allows |foo| to still be accessed if InitializeSlot returns
// with synchronous success.
base::Closure cb = base::Bind(MyHelper, callback, base::Owned(foo));
bool rv = delegate_ptr->InitializeSlot(this, cb);
if (rv)
  return foo->handler.Pass();
return scoped_ptr<net::KeygenHandler>();

[mattm, 2013/12/05 04:41:25]

Ah, yeah. I've done that (though just using Owned directly on a scoped_ptr*, did
you want the struct for readability?)

+  return scoped_ptr<net::KeygenHandler>();
+#else
+  return keygen_handler.Pass();
+#endif
+}
+
 bool ProfileIOData::ResourceContext::AllowMicAccess(const GURL& origin) {
   return AllowContentAccess(origin, CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC);
 }
 
 bool ProfileIOData::ResourceContext::AllowCameraAccess(const GURL& origin) {
   return AllowContentAccess(origin, CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
 }
 
 bool ProfileIOData::ResourceContext::AllowContentAccess(
     const GURL& origin, ContentSettingsType type) {
@@ skipping 267 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);
 }