Update keygen to use correct NSS slot on ChromeOS multiprofile.

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 7c0d34282ffff2e7f4c9992a9ef32cdcd8f788ea..cf58eae9c61df335aeb1ad453a13ec232fca52e2 100644
--- a/chrome/browser/profiles/profile_io_data.cc
+++ b/chrome/browser/profiles/profile_io_data.cc
@@ -62,6 +62,7 @@
 #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"
@@ -103,7 +104,7 @@
 #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;
@@ -826,7 +827,7 @@ ProfileIOData::ResourceContext::CreateClientCertStore() {
   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>();
@@ -838,6 +839,36 @@ ProfileIOData::ResourceContext::CreateClientCertStore() {
 #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);
 }