Call OwnerKeyUtil::FindPrivateKeyInSlot() in the blocking pool.

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc

Index: chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
index 29b91ffb7be7255fee5d61f82100c73c5a2433b0..093ac647f504038d58ab1e490b5ac386104cb556 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc
@@ -65,18 +65,15 @@ bool IsOwnerInTests(const std::string& user_id) {
   return static_cast<const base::Value*>(value)->GetString() == user_id;
 }
 
-void LoadPrivateKeyByPublicKey(
+void LoadPrivateKeyByPublicKeyOnWorkerThread(
     const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     scoped_refptr<PublicKey> public_key,
-    const std::string& username_hash,
+    crypto::ScopedPK11Slot public_slot,
+    crypto::ScopedPK11Slot private_slot,
     const base::Callback<void(const scoped_refptr<PublicKey>& public_key,
                               const scoped_refptr<PrivateKey>& private_key)>&
         callback) {
-  crypto::EnsureNSSInit();
-  crypto::ScopedPK11Slot public_slot =
-      crypto::GetPublicSlotForChromeOSUser(username_hash);
-  crypto::ScopedPK11Slot private_slot = crypto::GetPrivateSlotForChromeOSUser(
-      username_hash, base::Callback<void(crypto::ScopedPK11Slot)>());
+  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
 
   // If private slot is already available, this will check it. If not, we'll get
   // called again later when the TPM Token is ready, and the slot will be
@@ -98,6 +95,32 @@ void LoadPrivateKeyByPublicKey(
                           base::Bind(callback, public_key, private_key));
 }
 
+void LoadPrivateKeyByPublicKey(
+    const scoped_refptr<OwnerKeyUtil>& owner_key_util,
+    scoped_refptr<PublicKey> public_key,
+    const std::string& username_hash,
+    const base::Callback<void(const scoped_refptr<PublicKey>& public_key,
+                              const scoped_refptr<PrivateKey>& private_key)>&
+        callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);

[davidben, 2017/04/03 21:07:49]

I was going to suggest that all of this move to the IO thread, since you're
doing:
UI -> IO (do some tiny amount of work) -> worker thread (heavier work) -> UI

which seems a little silly.

However, I see GetPrivateSlotForChromeOSUser has a ThreadChecker, which is kind
of disturbing, and calls into question whether even this change is safe to do.
It looks like there's a whole lot of technical debt around the CrOS TPM mess
that was never paid off.

That was added in https://codereview.chromium.org/64723006 which cites
https://crbug.com/125848.

Moving the review to mattm or rsleevi. Hopefully one of you knows what's going
on here.

[mattm, 2017/04/03 22:28:21]

(Did you mean to say worker thread there, rather than IO?)
It looks like it's actually UI->Worker->IO->Worker->UI

LoadPrivateKey is already on the worker thread when it posts to IO for this. So
it could be shortened to UI->IO->Worker->UI if the slots were looked up first.

 
I believe the ThreadChecker was to protect the NSSInitSingleton initialization &
member variables, once you have the slot using it on a different thread is fine.

[Shuhei Takahashi, 2017/04/04 05:53:29]

OK, then your suggestion is to make it "UI->IO->Worker->UI"?

[Shuhei Takahashi, 2017/04/05 11:07:36]

Updated the patch to avoid unnecessary thread switches.

+
+  crypto::EnsureNSSInit();
+  crypto::ScopedPK11Slot public_slot =
+      crypto::GetPublicSlotForChromeOSUser(username_hash);
+  crypto::ScopedPK11Slot private_slot = crypto::GetPrivateSlotForChromeOSUser(
+      username_hash, base::Callback<void(crypto::ScopedPK11Slot)>());

[mattm, 2017/04/03 22:28:21]

This isn't specifying a callback or checking the private_slot result. Is there
any reason to be sure that the slot for that user is already initialized at this
point? It didn't seem obvious that it is, but if there is, there should be a
comment about why this is okay. Otherwise it should handle that.

[Shuhei Takahashi, 2017/04/04 05:53:29]

Sorry I don't know. I just mechanically moved the operation off to worker
threads.

+
+  // This task interacts with the TPM, so use the blocking pool.
+  scoped_refptr<base::TaskRunner> task_runner =
+      BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
+          base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+  task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&LoadPrivateKeyByPublicKeyOnWorkerThread, owner_key_util,
+                 public_key, base::Passed(std::move(public_slot)),
+                 base::Passed(std::move(private_slot)), callback));
+}
+
 void LoadPrivateKey(
     const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     const std::string username_hash,