Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796303005:
cros: Call GetPrivateSlotForChromeOSUser properly (Closed)
Descriptioncros: Call GetPrivateSlotForChromeOSUser properly
OwnerSettingsServiceChromeOS calls GetPrivateSlotForChromeOSUser
without using the callback, assuming the private slot is available
after profile load and tpm token ready. The CL changes to use
the callback arg of GetPrivateSlotForChromeOSUser so that the code
does not depend on the assumption and can properly wait for the
slot initialization.
BUG=708823
Review-Url: https://codereview.chromium.org/2796303005
Cr-Commit-Position: refs/heads/master@{#462561}
Committed: https://chromium.googlesource.com/chromium/src/+/bb330d1b8f803ccb942c7ca2e7e2510c612c1828
Patch Set 1 #
Total comments: 3
Messages
Total messages: 15 (8 generated)
The CQ bit was checked by xiyuan@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...
xiyuan@chromium.org changed reviewers: + mattm@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2796303005/diff/1/chrome/browser/chromeos/own... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2796303005/diff/1/chrome/browser/chromeos/own... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:129: void LoadPrivateKeyOnIOThread(const scoped_refptr<OwnerKeyUtil>& owner_key_util, Is it possible for this function to get called multiple times? It seems like there might be the possibility of multiple callbacks waiting on the private slot at the same time. I don't know how likely that is or if it would cause problems.
https://codereview.chromium.org/2796303005/diff/1/chrome/browser/chromeos/own... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2796303005/diff/1/chrome/browser/chromeos/own... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:129: void LoadPrivateKeyOnIOThread(const scoped_refptr<OwnerKeyUtil>& owner_key_util, On 2017/04/06 18:00:52, mattm wrote: > Is it possible for this function to get called multiple times? It seems like > there might be the possibility of multiple callbacks waiting on the private slot > at the same time. I don't know how likely that is or if it would cause problems. Yes, it is possible. I have seen two overlapping calls during ownership establishment. One triggered from OnTPMTokenReady and one from OwnerKeySet. This happens on the existing code. And I am not aware problems with that. If you want to change that, do you think we should protect against this? Should we ignore subsequent loads where there is a load going on? Or should we cancel previous loads when new load request comes in?
lgtm https://codereview.chromium.org/2796303005/diff/1/chrome/browser/chromeos/own... File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc (right): https://codereview.chromium.org/2796303005/diff/1/chrome/browser/chromeos/own... chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:129: void LoadPrivateKeyOnIOThread(const scoped_refptr<OwnerKeyUtil>& owner_key_util, On 2017/04/06 18:14:25, xiyuan wrote: > On 2017/04/06 18:00:52, mattm wrote: > > Is it possible for this function to get called multiple times? It seems like > > there might be the possibility of multiple callbacks waiting on the private > slot > > at the same time. I don't know how likely that is or if it would cause > problems. > > Yes, it is possible. I have seen two overlapping calls during ownership > establishment. One triggered from OnTPMTokenReady and one from OwnerKeySet. This > happens on the existing code. And I am not aware problems with that. > > If you want to change that, do you think we should protect against this? Should > we ignore subsequent loads where there is a load going on? Or should we cancel > previous loads when new load request comes in? If it's already happening and the code is fine with it, I don't think there's any need to change it for this CL.
On 2017/04/06 18:23:27, mattm wrote: > lgtm > > https://codereview.chromium.org/2796303005/diff/1/chrome/browser/chromeos/own... > File chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc > (right): > > https://codereview.chromium.org/2796303005/diff/1/chrome/browser/chromeos/own... > chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc:129: void > LoadPrivateKeyOnIOThread(const scoped_refptr<OwnerKeyUtil>& owner_key_util, > On 2017/04/06 18:14:25, xiyuan wrote: > > On 2017/04/06 18:00:52, mattm wrote: > > > Is it possible for this function to get called multiple times? It seems like > > > there might be the possibility of multiple callbacks waiting on the private > > slot > > > at the same time. I don't know how likely that is or if it would cause > > problems. > > > > Yes, it is possible. I have seen two overlapping calls during ownership > > establishment. One triggered from OnTPMTokenReady and one from OwnerKeySet. > This > > happens on the existing code. And I am not aware problems with that. > > > > If you want to change that, do you think we should protect against this? > Should > > we ignore subsequent loads where there is a load going on? Or should we cancel > > previous loads when new load request comes in? > > If it's already happening and the code is fine with it, I don't think there's > any need to change it for this CL. Cool. Thanks for the quick review.
The CQ bit was checked by xiyuan@chromium.org
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": 1, "attempt_start_ts": 1491503132547430, "parent_rev":
"9d291fab2615e3fcefba25e352dfab6e4fb0355f", "commit_rev":
"bb330d1b8f803ccb942c7ca2e7e2510c612c1828"}
Message was sent while issue was closed.
Description was changed from ========== cros: Call GetPrivateSlotForChromeOSUser properly OwnerSettingsServiceChromeOS calls GetPrivateSlotForChromeOSUser without using the callback, assuming the private slot is available after profile load and tpm token ready. The CL changes to use the callback arg of GetPrivateSlotForChromeOSUser so that the code does not depend on the assumption and can properly wait for the slot initialization. BUG=708823 ========== to ========== cros: Call GetPrivateSlotForChromeOSUser properly OwnerSettingsServiceChromeOS calls GetPrivateSlotForChromeOSUser without using the callback, assuming the private slot is available after profile load and tpm token ready. The CL changes to use the callback arg of GetPrivateSlotForChromeOSUser so that the code does not depend on the assumption and can properly wait for the slot initialization. BUG=708823 Review-Url: https://codereview.chromium.org/2796303005 Cr-Commit-Position: refs/heads/master@{#462561} Committed: https://chromium.googlesource.com/chromium/src/+/bb330d1b8f803ccb942c7ca2e7e2... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/bb330d1b8f803ccb942c7ca2e7e2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
