Migrate NSS client cert store from WorkerPool to TaskScheduler.

WorkerPool is being deprecated in favor of TaskScheduler.

NSS calls may acquire the NSS lock or reenter Chrome code via
extension hooks (such as smart card UI). To ensure threads are not
starved or deadlocked, base::ScopedBlockingCall instances
increment the thread pool capacity when too much time is spent
in a scope where an NSS call is made.

Bug: 659191
Change-Id: Iaf088d84521753a3399083047a4e5733d44c472f
Reviewed-on: https://chromium-review.googlesource.com/648218Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Commit-Queue: Francois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501248}

chrome/browser/chromeos/net/client_cert_store_chromeos.cc

@@ -14,8 +14,8 @@
 #include "base/callback.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
-#include "base/task_runner_util.h"
-#include "base/threading/worker_pool.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
 #include "crypto/nss_crypto_module_delegate.h"
 #include "net/ssl/ssl_cert_request_info.h"
@@ -64,18 +64,13 @@ void ClientCertStoreChromeOS::GotAdditionalCerts(
   scoped_refptr<crypto::CryptoModuleBlockingPasswordDelegate> password_delegate;
   if (!password_delegate_factory_.is_null())
     password_delegate = password_delegate_factory_.Run(request->host_and_port);
-  if (base::PostTaskAndReplyWithResult(
-          base::WorkerPool::GetTaskRunner(true /* task_is_slow */).get(),
-          FROM_HERE,
-          base::Bind(&ClientCertStoreChromeOS::GetAndFilterCertsOnWorkerThread,
-                     base::Unretained(this), password_delegate,
-                     base::Unretained(request),
-                     base::Passed(&additional_certs)),
-          callback)) {
-    return;
-  }
-  // If the task could not be posted, behave as if there were no certificates.
-  callback.Run(net::ClientCertIdentityList());
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      base::Bind(&ClientCertStoreChromeOS::GetAndFilterCertsOnWorkerThread,
+                 base::Unretained(this), password_delegate,
+                 base::Unretained(request), base::Passed(&additional_certs)),
+      callback);
 }
 
 net::ClientCertIdentityList
@@ -84,6 +79,12 @@ ClientCertStoreChromeOS::GetAndFilterCertsOnWorkerThread(
         password_delegate,
     const net::SSLCertRequestInfo* request,
     net::ClientCertIdentityList additional_certs) {
+  // This method may acquire the NSS lock or reenter this code via extension
+  // hooks (such as smart card UI). To ensure threads are not starved or
+  // deadlocked, the base::ScopedBlockingCall below increments the thread pool
+  // capacity if this method takes too much time to run.
+  base::ScopedBlockingCall scoped_blocking_call(base::BlockingType::MAY_BLOCK);
+
   net::ClientCertIdentityList client_certs;
   net::ClientCertStoreNSS::GetPlatformCertsOnWorkerThread(
       std::move(password_delegate),

chrome/browser/chromeos/net/client_cert_store_chromeos_unittest.cc

@@ -12,9 +12,9 @@
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
-#include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
 #include "crypto/scoped_test_nss_db.h"
@@ -87,7 +87,9 @@ void SaveIdentitiesAndQuitCallback(net::ClientCertIdentityList* out_identities,
 
 class ClientCertStoreChromeOSTest : public ::testing::Test {
  public:
-  ClientCertStoreChromeOSTest() : message_loop_(new base::MessageLoopForIO()) {}
+  ClientCertStoreChromeOSTest()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::IO) {}
 
   scoped_refptr<net::X509Certificate> ImportCertToSlot(
       const std::string& cert_filename,
@@ -100,7 +102,7 @@ class ClientCertStoreChromeOSTest : public ::testing::Test {
   }
 
  private:
-  std::unique_ptr<base::MessageLoop> message_loop_;
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
 };
 
 // Ensure that cert requests, that are started before the filter is initialized,

net/ssl/client_cert_store_nss.cc

@@ -18,8 +18,8 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_piece.h"
-#include "base/task_runner_util.h"
-#include "base/threading/worker_pool.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "crypto/nss_crypto_module_delegate.h"
 #include "net/cert/scoped_nss_types.h"
 #include "net/cert/x509_util_nss.h"
@@ -50,17 +50,13 @@ class ClientCertIdentityNSS : public ClientCertIdentity {
           private_key_callback) override {
     // Caller is responsible for keeping the ClientCertIdentity alive until
     // the |private_key_callback| is run, so it's safe to use Unretained here.
-    if (base::PostTaskAndReplyWithResult(
-            base::WorkerPool::GetTaskRunner(true /* task_is_slow */).get(),
-            FROM_HERE,
-            base::Bind(&FetchClientCertPrivateKey,
-                       base::Unretained(certificate()), cert_certificate_.get(),
-                       base::Unretained(password_delegate_.get())),
-            private_key_callback)) {
-      return;
-    }
-    // If the task could not be posted, behave as if there was no key.
-    private_key_callback.Run(nullptr);
+    base::PostTaskWithTraitsAndReplyWithResult(
+        FROM_HERE,
+        {base::MayBlock(), base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+        base::Bind(&FetchClientCertPrivateKey, base::Unretained(certificate()),
+                   cert_certificate_.get(),
+                   base::Unretained(password_delegate_.get())),
+        private_key_callback);
   }
 
  private:
@@ -83,19 +79,15 @@ void ClientCertStoreNSS::GetClientCerts(
   scoped_refptr<crypto::CryptoModuleBlockingPasswordDelegate> password_delegate;
   if (!password_delegate_factory_.is_null())
     password_delegate = password_delegate_factory_.Run(request.host_and_port);
-  if (base::PostTaskAndReplyWithResult(
-          base::WorkerPool::GetTaskRunner(true /* task_is_slow */).get(),
-          FROM_HERE,
-          base::Bind(&ClientCertStoreNSS::GetAndFilterCertsOnWorkerThread,
-                     // Caller is responsible for keeping the ClientCertStore
-                     // alive until the callback is run.
-                     base::Unretained(this), std::move(password_delegate),
-                     base::Unretained(&request)),
-          callback)) {
-    return;
-  }
-  // If the task could not be posted, behave as if there were no certificates.
-  callback.Run(ClientCertIdentityList());
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      base::Bind(&ClientCertStoreNSS::GetAndFilterCertsOnWorkerThread,
+                 // Caller is responsible for keeping the ClientCertStore
+                 // alive until the callback is run.
+                 base::Unretained(this), std::move(password_delegate),
+                 base::Unretained(&request)),
+      callback);
 }
 
 // static
@@ -166,6 +158,11 @@ ClientCertIdentityList ClientCertStoreNSS::GetAndFilterCertsOnWorkerThread(
     scoped_refptr<crypto::CryptoModuleBlockingPasswordDelegate>
         password_delegate,
     const SSLCertRequestInfo* request) {
+  // This method may acquire the NSS lock or reenter this code via extension
+  // hooks (such as smart card UI). To ensure threads are not starved or
+  // deadlocked, the base::ScopedBlockingCall below increments the thread pool
+  // capacity if this method takes too much time to run.
+  base::ScopedBlockingCall scoped_blocking_call(base::BlockingType::MAY_BLOCK);
   ClientCertIdentityList selected_identities;
   GetPlatformCertsOnWorkerThread(std::move(password_delegate), CertFilter(),
                                  &selected_identities);

net/ssl/ssl_platform_key_nss.cc

@@ -14,6 +14,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "crypto/nss_crypto_module_delegate.h"
 #include "crypto/scoped_nss_types.h"
 #include "net/cert/x509_certificate.h"
@@ -157,6 +158,12 @@ scoped_refptr<SSLPrivateKey> FetchClientCertPrivateKey(
     const X509Certificate* certificate,
     CERTCertificate* cert_certificate,
     crypto::CryptoModuleBlockingPasswordDelegate* password_delegate) {
+  // This function may acquire the NSS lock or reenter this code via extension
+  // hooks (such as smart card UI). To ensure threads are not starved or
+  // deadlocked, the base::ScopedBlockingCall below increments the thread pool
+  // capacity if this method takes too much time to run.
+  base::ScopedBlockingCall scoped_blocking_call(base::BlockingType::MAY_BLOCK);
+
   void* wincx = password_delegate ? password_delegate->wincx() : nullptr;
   crypto::ScopedSECKEYPrivateKey key(
       PK11_FindKeyByAnyCert(cert_certificate, wincx));