Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2761353003:
Call OwnerKeyUtil::FindPrivateKeyInSlot() in the blocking pool. (Closed)
|
Created:
3 years, 9 months ago by Shuhei Takahashi Modified:
3 years, 8 months ago Reviewers:
Daniel Erat, Ryan Sleevi, igorcov, Andrew T Wilson (Slow), davidben, Thiemo Nagel, mattm, tbarzic CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCall OwnerKeyUtil::FindPrivateKeyInSlot() in the blocking pool.
The function can block for long time and should not be called on the IO
thread. In fact, on Chrome OS, it might not return until TPM-related
initialization is finished.
BUG=chromium:704024
TEST=unit_tests
Review-Url: https://codereview.chromium.org/2761353003
Cr-Commit-Position: refs/heads/master@{#462299}
Committed: https://chromium.googlesource.com/chromium/src/+/b3d6e89884650aac1ad2bf9a51e8dc51f06d1f42
Patch Set 1 #
Total comments: 2
Patch Set 2 : DCHECK for worker thread. #
Total comments: 10
Patch Set 3 : Avoid unnecessary thread switches. #Messages
Total messages: 41 (25 generated)
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nya@chromium.org changed reviewers: + derat@chromium.org
derat: PTAL
lgtm as an owner, but i don't think i've looked at this code before. please also add someone who has more experience with it as a reviewer. https://codereview.chromium.org/2761353003/diff/1/chrome/browser/chromeos/own... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2761353003/diff/1/chrome/browser/chromeos/own... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:75: callback) { can you DCHECK that this is running on the right thread here?
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2761353003/diff/1/chrome/browser/chromeos/own... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2761353003/diff/1/chrome/browser/chromeos/own... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:75: callback) { On 2017/03/22 14:47:18, Daniel Erat wrote: > can you DCHECK that this is running on the right thread here? Done.
On 2017/03/22 14:47:19, Daniel Erat wrote: > lgtm as an owner, but i don't think i've looked at this code before. please also > add someone who has more experience with it as a reviewer. Sure, I'll ask atwilson@.
nya@chromium.org changed reviewers: + atwilson@chromium.org
atwilson: PTAL
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
atwilson@chromium.org changed reviewers: + igorcov@chromium.org, tnagel@chromium.org
I don't know this code - +thiemo/igor to take a look.
On 2017/03/29 09:08:28, Andrew T Wilson (Slow) wrote: > I don't know this code - +thiemo/igor to take a look. I have no knowledge of this.
nya@chromium.org changed reviewers: + davidben@chromium.org, tbarzic@chromium.org
tbarzic, davidben: I'm looking for a reviewer familiar with this file. If so could you take a look please? I looked at a blame list of the file to find reviewers but most of them seem inactive today. 594 ygorshenin@ [inactive] 40 cmasone@ [inactive] 23 tbarzic@ 17 dcheng@ [cleanup only] 17 davidben@
davidben@chromium.org changed reviewers: + mattm@chromium.org, rsleevi@chromium.org
https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:105: DCHECK_CURRENTLY_ON(BrowserThread::IO); 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.
https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:93: BrowserThread::PostTask(BrowserThread::UI, 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.) https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:105: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/04/03 21:07:49, davidben wrote: > I was going to suggest that all of this move to the IO thread, since you're > doing: (Did you mean to say worker thread there, rather than IO?) > UI -> IO (do some tiny amount of work) -> worker thread (heavier work) -> UI > > which seems a little silly. 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. > 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. 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. https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:111: username_hash, base::Callback<void(crypto::ScopedPK11Slot)>()); 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. https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:684: DCHECK(thread_checker_.CalledOnValidThread()); What thread does this end up on? UI?
https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:105: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/04/03 22:28:21, mattm wrote: > On 2017/04/03 21:07:49, davidben wrote: > > I was going to suggest that all of this move to the IO thread, since you're > > doing: > > (Did you mean to say worker thread there, rather than IO?) > > > UI -> IO (do some tiny amount of work) -> worker thread (heavier work) -> UI > > > > which seems a little silly. > > 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. > > > > 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. > > 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. OK, then your suggestion is to make it "UI->IO->Worker->UI"? https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:111: username_hash, base::Callback<void(crypto::ScopedPK11Slot)>()); On 2017/04/03 22:28:21, mattm wrote: > 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. Sorry I don't know. I just mechanically moved the operation off to worker threads.
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:93: BrowserThread::PostTask(BrowserThread::UI, On 2017/04/03 22:28:21, mattm wrote: > 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.) 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... https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:105: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/04/04 05:53:29, Shuhei Takahashi wrote: > On 2017/04/03 22:28:21, mattm wrote: > > On 2017/04/03 21:07:49, davidben wrote: > > > I was going to suggest that all of this move to the IO thread, since you're > > > doing: > > > > (Did you mean to say worker thread there, rather than IO?) > > > > > UI -> IO (do some tiny amount of work) -> worker thread (heavier work) -> UI > > > > > > which seems a little silly. > > > > 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. > > > > > > > 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. > > > > 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. > > OK, then your suggestion is to make it "UI->IO->Worker->UI"? Updated the patch to avoid unnecessary thread switches. https://codereview.chromium.org/2761353003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:684: DCHECK(thread_checker_.CalledOnValidThread()); On 2017/04/03 22:28:21, mattm wrote: > What thread does this end up on? UI? IIUC this object lives on the UI thread.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. I filed crbug.com/708823 for the GetPrivateSlotForChromeOSUser issue.
On 2017/04/05 23:40:06, mattm wrote: > lgtm. > > I filed crbug.com/708823 for the GetPrivateSlotForChromeOSUser issue. Thanks!
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org Link to the patchset: https://codereview.chromium.org/2761353003/#ps40001 (title: "Avoid unnecessary thread switches.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491439302732270,
"parent_rev": "b6633e1e1e2045a0a8c742445d75c87a3bc0e47b", "commit_rev":
"b3d6e89884650aac1ad2bf9a51e8dc51f06d1f42"}
Message was sent while issue was closed.
Description was changed from ========== Call OwnerKeyUtil::FindPrivateKeyInSlot() in the blocking pool. The function can block for long time and should not be called on the IO thread. In fact, on Chrome OS, it might not return until TPM-related initialization is finished. BUG=chromium:704024 TEST=unit_tests ========== to ========== Call OwnerKeyUtil::FindPrivateKeyInSlot() in the blocking pool. The function can block for long time and should not be called on the IO thread. In fact, on Chrome OS, it might not return until TPM-related initialization is finished. BUG=chromium:704024 TEST=unit_tests Review-Url: https://codereview.chromium.org/2761353003 Cr-Commit-Position: refs/heads/master@{#462299} Committed: https://chromium.googlesource.com/chromium/src/+/b3d6e89884650aac1ad2bf9a51e8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b3d6e89884650aac1ad2bf9a51e8... |
