Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817933003:
Wait for Cryptohome service to be available on login. (Closed)
|
Created:
3 years, 8 months ago by Sergey Poromov Modified:
3 years, 8 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWait for Cryptohome service to be available on login.
It could happen that cryptohome service is not yet available
during login in ExistingUserController, especially for auto-launched
public sessions or ARC Kiosk. In that case mounting will fail and session
will exit.
It's safer to use CryptohomeClient::WaitForServiceToBeAvailable to ensure.
BUG=702045
Review-Url: https://codereview.chromium.org/2817933003
Cr-Commit-Position: refs/heads/master@{#465641}
Committed: https://chromium.googlesource.com/chromium/src/+/acba6ce3ba4f318e84b772ce094cc8990bdf2022
Patch Set 1 #
Total comments: 2
Patch Set 2 : better error handling + fix ExistingUserControllerActiveDirectoryTest #
Total comments: 5
Patch Set 3 : use OnceClosure for ContinueLoginWhenCryptohomeAvailable #
Total comments: 4
Patch Set 4 : remove useless const and std::move #
Total comments: 4
Patch Set 5 : nits #
Messages
Total messages: 37 (21 generated)
poromov@chromium.org changed reviewers: + xiyuan@chromium.org
Hey Xiyuan, please review the solution. While I don't have Samus device right now to check whether this actually fixes the original issue, I'd like to get your feedback on the solution during the weekend.
The CQ bit was checked by poromov@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...
Think the approach should fix the issue. Just one nit. https://codereview.chromium.org/2817933003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/2817933003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/existing_user_controller.cc:1245: login_display_->SetUIEnabled(true); How about do LOG(ERROR) << "Cryptohome service is not available"; OnAuthFailure(AuthFailure::COULD_NOT_MOUNT_CRYPTOHOME); so that we get it in log and user gets some visual feedback?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by poromov@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...
poromov@chromium.org changed reviewers: + rsorokin@chromium.org
https://codereview.chromium.org/2817933003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/2817933003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/existing_user_controller.cc:1245: login_display_->SetUIEnabled(true); On 2017/04/13 18:07:31, xiyuan wrote: > How about do > > LOG(ERROR) << "Cryptohome service is not available"; > OnAuthFailure(AuthFailure::COULD_NOT_MOUNT_CRYPTOHOME); > > so that we get it in log and user gets some visual feedback? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.h:255: void ContinueLoginWhenCryptohomeAvailable(const base::Closure& continuation, Why not OnceClosure?
https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.h:255: void ContinueLoginWhenCryptohomeAvailable(const base::Closure& continuation, On 2017/04/18 13:30:42, Roman Sorokin (fast) wrote: > Why not OnceClosure? Because it's a callback from ContinueLoginIfDeviceNotDisabled and it can't be single-owned since it's sometimes is passed as part of a callback into cros_settings_->PrepareTrustedValues()
lgtm https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.h:255: void ContinueLoginWhenCryptohomeAvailable(const base::Closure& continuation, On 2017/04/18 14:16:39, Sergey Poromov wrote: > On 2017/04/18 13:30:42, Roman Sorokin (fast) wrote: > > Why not OnceClosure? > > Because it's a callback from ContinueLoginIfDeviceNotDisabled and it can't be > single-owned since it's sometimes is passed as part of a callback into > cros_settings_->PrepareTrustedValues() I guess passing it to PrepareTrustedValues is not a problem. But yeah, this should not be part of this CL. https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:787: // Overriden from ExistingUserControllerTest: nit: Remove the comment
https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/2817933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.h:255: void ContinueLoginWhenCryptohomeAvailable(const base::Closure& continuation, On 2017/04/18 14:16:39, Sergey Poromov wrote: > On 2017/04/18 13:30:42, Roman Sorokin (fast) wrote: > > Why not OnceClosure? > > Because it's a callback from ContinueLoginIfDeviceNotDisabled and it can't be > single-owned since it's sometimes is passed as part of a callback into > cros_settings_->PrepareTrustedValues() But on the other hand it could be left as (Repeated)Closure in ContinueLoginIfDeviceNotDisabled, but converted to OnceClosure as soon as passed to ContinueLoginWhenCryptohomeAvailable - thanks! Done.
The CQ bit was checked by poromov@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.
https://codereview.chromium.org/2817933003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/2817933003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.h:256: const base::OnceClosure continuation, This could not be "const" since we would std::move() it, right? At least, it should be consistent with cc file.
https://codereview.chromium.org/2817933003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/2817933003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.cc:1301: weak_factory_.GetWeakPtr(), std::move(continuation))); std::move on const & does nothing AFAIK. For this CL probably does not make sense to bother with OnceCallback
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
https://codereview.chromium.org/2817933003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/2817933003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.cc:1301: weak_factory_.GetWeakPtr(), std::move(continuation))); On 2017/04/19 09:00:09, Roman Sorokin (fast) wrote: > std::move on const & does nothing AFAIK. For this CL probably does not make > sense to bother with OnceCallback Yep, std::move is useless here. About OnceCallback - guidelines say that new code should use either RepeatedCallback or OnceCallback. So, I prefer to explicitly use OnceCallback in the new function, just for semantics. https://codereview.chromium.org/2817933003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/2817933003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.h:256: const base::OnceClosure continuation, On 2017/04/18 16:38:00, xiyuan wrote: > This could not be "const" since we would std::move() it, right? At least, it > should be consistent with cc file. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2817933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/2817933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.h:254: // Invokes |continuation| after verifying that cryptohome service is available nit: dot at the end. https://codereview.chromium.org/2817933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/2817933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:787: // Overriden from ExistingUserControllerTest: nit: Remove comment
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2817933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.h (right): https://codereview.chromium.org/2817933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.h:254: // Invokes |continuation| after verifying that cryptohome service is available On 2017/04/19 11:26:02, Roman Sorokin (fast) wrote: > nit: dot at the end. Done. https://codereview.chromium.org/2817933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc (right): https://codereview.chromium.org/2817933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:787: // Overriden from ExistingUserControllerTest: On 2017/04/19 11:26:02, Roman Sorokin (fast) wrote: > nit: Remove comment Done.
lgtm
lgtm
The CQ bit was checked by poromov@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": 80001, "attempt_start_ts": 1492619492454940,
"parent_rev": "5d308f5270f55633e074bdaa2b2e06d303057d53", "commit_rev":
"acba6ce3ba4f318e84b772ce094cc8990bdf2022"}
Message was sent while issue was closed.
Description was changed from ========== Wait for Cryptohome service to be available on login. It could happen that cryptohome service is not yet available during login in ExistingUserController, especially for auto-launched public sessions or ARC Kiosk. In that case mounting will fail and session will exit. It's safer to use CryptohomeClient::WaitForServiceToBeAvailable to ensure. BUG=702045 ========== to ========== Wait for Cryptohome service to be available on login. It could happen that cryptohome service is not yet available during login in ExistingUserController, especially for auto-launched public sessions or ARC Kiosk. In that case mounting will fail and session will exit. It's safer to use CryptohomeClient::WaitForServiceToBeAvailable to ensure. BUG=702045 Review-Url: https://codereview.chromium.org/2817933003 Cr-Commit-Position: refs/heads/master@{#465641} Committed: https://chromium.googlesource.com/chromium/src/+/acba6ce3ba4f318e84b772ce094c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/acba6ce3ba4f318e84b772ce094c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
