Move the core state machine of SSLClientSocketNSS into a thread-safe Core

net/socket/client_socket_factory.cc

Index: net/socket/client_socket_factory.cc
diff --git a/net/socket/client_socket_factory.cc b/net/socket/client_socket_factory.cc
index 42f6d4f20cebcf90c4d82d2c1be78d6d38ed099c..5c4037b07e65fbd32fccc5eb45fa3275266b7415 100644
--- a/net/socket/client_socket_factory.cc
+++ b/net/socket/client_socket_factory.cc
@@ -5,6 +5,7 @@
 #include "net/socket/client_socket_factory.h"
 
 #include "base/lazy_instance.h"
+#include "base/threading/thread.h"
 #include "build/build_config.h"
 #include "net/base/cert_database.h"
 #include "net/socket/client_socket_handle.h"
@@ -31,10 +32,24 @@ namespace {
 
 bool g_use_system_ssl = false;
 
+// ChromeOS uses a hardware TPM module that may cause NSS operations to
+// block for upwards of several seconds. To avoid blocking all network and
+// IPC activity, run NSS SSL functions on a dedicated thread.
+#if defined(OS_CHROMEOS)

[wtc, 2012/05/30 22:54:29]

It may be a good idea to do this on more platforms (at least
on Linux) as a way to exercise this code more.

[Ryan Sleevi, 2012/05/30 23:20:10]

I agree, but as a possible merge candidate, I wanted to keep the scope of the
change somewhat limited, even though that means less testing via the channels.

[wtc, 2012/05/31 01:23:42]

If you plan to merge this CL to the Chrome 20 branch, then
testing is especially important.

We can leave this part as is in this CL so that you can
simply use drover to merge.  On the trunk, after you
check in this CL, please create another CL to enable
this mode for Linux at least.  (I would enable it for
all platforms.)

[Ryan Sleevi, 2012/05/31 01:31:14]

Agreed, that's the goal :)

+bool g_use_dedicated_nss_thread = true;
+#else
+bool g_use_dedicated_nss_thread = false;
+#endif

[Ryan Sleevi, 2012/05/30 02:11:33]

Design context: I debated very heavily on where this belongs. One option was to
pass it through the SSLClientSocketContext, so that it goes all the way up from
the HttpNetworkParams to the IO thread (chrome/browser/io_thread.h/.cc).

The benefit of sticking it there is that we can reliably shut down the child
thread when the IO thread goes out of scope - currently, it leaks (by way of the
DefaultClientSocketFactory being leaky).

However, that seems a wrong layering, since we really want to ensure all tests
(as well as things like test_shell, content_shell, anything that depends on
net/) has the same /tested/ and reliable behaviour.

Another thought was to expose a global function to set the NSS thread. If it
wasn't set, simply default to MessageLoop::current(). This is similar to what
the OCSP/CRL/AIA code uses (net::SetMessgeLoopForNSSHttpIO). This as well is
something only called from the io_thread, so it means that some of the
multi-threaded behaviours are never tested.

It also seems the wrong sort of encapsulation, because it leaks the NSS-specific
knowledge from being within the DefaultClientSocketFactory all the way up to the
Chrome layer.

I'm not convinced that it's best to put it here - in fact, I have serious
reservations. Think of this part as "in flux"

[wtc, 2012/05/30 22:54:29]

I agree with the design decision of creating the NSS SSL thread
here.

+
 class DefaultClientSocketFactory : public ClientSocketFactory,
                                    public CertDatabase::Observer {
  public:
   DefaultClientSocketFactory() {
+    if (g_use_dedicated_nss_thread) {
+      nss_thread_.reset(new base::Thread("NSS SSL Thread"));
+      nss_thread_->Start();
+    }
+
     CertDatabase::AddObserver(this);
   }
 
@@ -76,25 +91,38 @@ class DefaultClientSocketFactory : public ClientSocketFactory,
       const SSLClientSocketContext& context) {
     scoped_ptr<SSLHostInfo> shi(ssl_host_info);
 
-#if defined(OS_WIN)
+    scoped_refptr<base::SingleThreadTaskRunner> network_task_runner(
+        base::MessageLoopProxy::current());
+    DCHECK(network_task_runner);
+
+    scoped_refptr<base::SingleThreadTaskRunner> nss_task_runner(
+        network_task_runner);
+
+    if (g_use_dedicated_nss_thread && nss_thread_->message_loop_proxy())

[wtc, 2012/05/30 22:54:29]

If g_use_dedicated_nss_thread is true, nss_thread_->message_loop_proxy()
cannot be NULL, right?

[Ryan Sleevi, 2012/05/30 23:20:10]

If the thread fails to start, it'll be NULL. With this check, rather than crash,
we'll block the IO thread, which at least offers reliable functionality, at the
cost of user experience.

+      nss_task_runner = nss_thread_->message_loop_proxy();

[Ryan Sleevi, 2012/05/30 02:14:33]

Further design context:

Note that I'm not checking |ssl_config.send_client_cert| or
|ssl_config.client_cert| here. Originally, we discussed only running in
multi-threaded mode when client certs are involved.

However, as implemented, both domain bound certs and /locating/ client certs may
cause blocking to occur, due to the module locks during C_Login, and the session
lock during C_SignFinal. This also applies to creating the peer cert, where it
may try to scan the module list again.

It's these latter points that had me move all I/O off, rather than just
client-auth handshakes.

[wtc, 2012/05/30 22:54:29]

I agree with the design decision of moving all NSS functions
to the NSS SSL thread, rather than just client-auth handshakes.

+
+#if defined(USE_OPENSSL)
+    return new SSLClientSocketOpenSSL(transport_socket, host_and_port,
+                                      ssl_config, context);
+#elif defined(USE_NSS)
+    return new SSLClientSocketNSS(network_task_runner, nss_task_runner,
+                                  transport_socket, host_and_port, ssl_config,
+                                  shi.release(), context);
+#elif defined(OS_WIN)
     if (g_use_system_ssl) {
       return new SSLClientSocketWin(transport_socket, host_and_port,
                                     ssl_config, context);
     }
-    return new SSLClientSocketNSS(transport_socket, host_and_port, ssl_config,
-                                  shi.release(), context);
-#elif defined(USE_OPENSSL)
-    return new SSLClientSocketOpenSSL(transport_socket, host_and_port,
-                                      ssl_config, context);
-#elif defined(USE_NSS)
-    return new SSLClientSocketNSS(transport_socket, host_and_port, ssl_config,
+    return new SSLClientSocketNSS(network_task_runner, nss_task_runner,
+                                  transport_socket, host_and_port, ssl_config,
                                   shi.release(), context);
 #elif defined(OS_MACOSX)
     if (g_use_system_ssl) {
       return new SSLClientSocketMac(transport_socket, host_and_port,
                                     ssl_config, context);
     }
-    return new SSLClientSocketNSS(transport_socket, host_and_port, ssl_config,
+    return new SSLClientSocketNSS(network_task_runner, nss_task_runner,
+                                  transport_socket, host_and_port, ssl_config,
                                   shi.release(), context);
 #else
     NOTIMPLEMENTED();
@@ -106,6 +134,8 @@ class DefaultClientSocketFactory : public ClientSocketFactory,
     SSLClientSocket::ClearSessionCache();
   }
 
+ private:
+  scoped_ptr<base::Thread> nss_thread_;
 };
 
 static base::LazyInstance<DefaultClientSocketFactory>