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

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc

 // Copyright 2014 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/chromeos/ownership/owner_settings_service_chromeos.h"
 
 #include <keyhi.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 47 matching lines @@
           ::switches::kTestType) ||
       !CrosSettings::IsInitialized()) {
     return false;
   }
   const base::Value* value = CrosSettings::Get()->GetPref(kDeviceOwner);
   if (!value || value->GetType() != base::Value::Type::STRING)
     return false;
   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
   // available then. FindPrivateKeyInSlot internally checks for a null slot if
   // needbe.
   //
   // TODO(davidben): The null check should be in the caller rather than
   // internally in the OwnerKeyUtil implementation. The tests currently get a
   // null private_slot and expect the mock OwnerKeyUtil to still be called.
   scoped_refptr<PrivateKey> private_key(
       new PrivateKey(owner_key_util->FindPrivateKeyInSlot(public_key->data(),
                                                           private_slot.get())));
   if (!private_key->key()) {
     private_key = new PrivateKey(owner_key_util->FindPrivateKeyInSlot(
         public_key->data(), public_slot.get()));
   }
   BrowserThread::PostTask(BrowserThread::UI,

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

Seems a bit weird this posts explicitly to the UI thread, whereas the rest of
the code doesn't make any guarantees about what the owning thread is. (Also
applies at the other place that posts to UI.)

(I realize this is pre-existing.. just wanted to point that out.)

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

Maybe we can pass TaskRunner around to avoid being explicit about the UI thread,
but since I'm alien to this code I want to avoid refactoring...

                           FROM_HERE,
                           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,
     const base::Callback<void(const scoped_refptr<PublicKey>& public_key,
                               const scoped_refptr<PrivateKey>& private_key)>&
         callback) {
   std::vector<uint8_t> public_key_data;
   scoped_refptr<PublicKey> public_key;
   if (!owner_key_util->ImportPublicKey(&public_key_data)) {
     scoped_refptr<PrivateKey> private_key;
@@ skipping 540 matching lines @@
   const bool is_owner = IsOwner() || IsOwnerInTests(user_id_);
   if (is_owner && device_settings_service_)
     device_settings_service_->InitOwner(user_id_, weak_factory_.GetWeakPtr());
 
   has_pending_fixups_ = true;
 }
 
 void OwnerSettingsServiceChromeOS::ReloadKeypairImpl(const base::Callback<
     void(const scoped_refptr<PublicKey>& public_key,
          const scoped_refptr<PrivateKey>& private_key)>& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());

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

What thread does this end up on? UI?

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

IIUC this object lives on the UI thread.

 
   if (waiting_for_profile_creation_ || waiting_for_tpm_token_)
     return;
   scoped_refptr<base::TaskRunner> task_runner =
       BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
           base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   task_runner->PostTask(
       FROM_HERE,
       base::Bind(&LoadPrivateKey,
                  owner_key_util_,
@@ skipping 57 matching lines @@
 
 void OwnerSettingsServiceChromeOS::ReportStatusAndContinueStoring(
     bool success) {
   store_settings_factory_.InvalidateWeakPtrs();
   for (auto& observer : observers_)
     observer.OnSignedPolicyStored(success);
   StorePendingChanges();
 }
 
 }  // namespace chromeos