Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912763003:
cros: Prevent shut down on lid-close during migration. (Closed)
|
Created:
3 years, 6 months ago by dspaid Modified:
3 years, 6 months ago CC:
chromium-reviews, derat+watch_chromium.org, alemate+watch_chromium.org, achuith+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Prevent shut down on lid-close during migration.
In the login screen, the system will shut down on lid close.
Although we ask users not to close the lid during the migration, it
should be better to change the lid-close action to SUSPEND during
migration.
BUG=726229
TEST=out/Release/chromeos_unittests --gtest_filter=PowerPolicyControllerTest.*
Review-Url: https://codereview.chromium.org/2912763003
Cr-Commit-Position: refs/heads/master@{#476091}
Committed: https://chromium.googlesource.com/chromium/src/+/3769f83c5922880a027a1aaa6589d9a99c7b782f
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : update comment wording #
Total comments: 2
Patch Set 4 : Only update if changed #
Total comments: 2
Patch Set 5 : revert unrelated change #
Messages
Total messages: 23 (7 generated)
dspaid@chromium.org changed reviewers: + derat@chromium.org, fukino@chromium.org, xiyuan@chromium.org
ptal This is to take over from fukino's work at https://codereview.chromium.org/2907773002/
https://codereview.chromium.org/2912763003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc (right): https://codereview.chromium.org/2912763003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:289: if (PowerPolicyController::IsInitialize()) i don't think that this code should ever run before PowerPolicyController has been initialized. can you double-check the initialization order and remove these checks if possible? https://codereview.chromium.org/2912763003/diff/1/chromeos/dbus/power_policy_... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/2912763003/diff/1/chromeos/dbus/power_policy_... chromeos/dbus/power_policy_controller.cc:391: if (encryption_migration_active_ && thinking about this a bit more, i think i misspoke earlier, and this block should come immediately before the chrome_is_exiting_ block. that is, if the lid is closed while chrome is both migrating and exiting, we want to do nothing (so the exit can finish) instead of suspending. sorry, mind moving this up a bit? https://codereview.chromium.org/2912763003/diff/1/chromeos/dbus/power_policy_... File chromeos/dbus/power_policy_controller.h (right): https://codereview.chromium.org/2912763003/diff/1/chromeos/dbus/power_policy_... chromeos/dbus/power_policy_controller.h:115: void SetEncryptionMigrationActive(bool active); add a comment documenting what this does, e.g. // Adjusts policy when the migration of a user homedir to a new // encryption format starts or stops. While migration is active, // the lid-closed action is overridden to ensure the system // doesn't shut down.
ptal https://codereview.chromium.org/2912763003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc (right): https://codereview.chromium.org/2912763003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:289: if (PowerPolicyController::IsInitialize()) On 2017/05/30 13:39:42, Daniel Erat wrote: > i don't think that this code should ever run before PowerPolicyController has > been initialized. can you double-check the initialization order and remove these > checks if possible? Done. https://codereview.chromium.org/2912763003/diff/1/chromeos/dbus/power_policy_... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/2912763003/diff/1/chromeos/dbus/power_policy_... chromeos/dbus/power_policy_controller.cc:391: if (encryption_migration_active_ && On 2017/05/30 13:39:42, Daniel Erat wrote: > thinking about this a bit more, i think i misspoke earlier, and this block > should come immediately before the chrome_is_exiting_ block. that is, if the lid > is closed while chrome is both migrating and exiting, we want to do nothing (so > the exit can finish) instead of suspending. sorry, mind moving this up a bit? Went ahead and moved it up, though in this case I don't think it matters since if chrome_is_exiting_ is true then it will set the action to DO_NOTHING, and we only suspend if the current policy is not DO_NOTHING. https://codereview.chromium.org/2912763003/diff/1/chromeos/dbus/power_policy_... File chromeos/dbus/power_policy_controller.h (right): https://codereview.chromium.org/2912763003/diff/1/chromeos/dbus/power_policy_... chromeos/dbus/power_policy_controller.h:115: void SetEncryptionMigrationActive(bool active); On 2017/05/30 13:39:42, Daniel Erat wrote: > add a comment documenting what this does, e.g. > > // Adjusts policy when the migration of a user homedir to a new > // encryption format starts or stops. While migration is active, > // the lid-closed action is overridden to ensure the system > // doesn't shut down. Done.
lgtm https://codereview.chromium.org/2912763003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc (right): https://codereview.chromium.org/2912763003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:286: // We should block power save and not poweroff on lid close during migration. nit: s/poweroff/shut down/ https://codereview.chromium.org/2912763003/diff/20001/chromeos/dbus/power_pol... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/2912763003/diff/20001/chromeos/dbus/power_pol... chromeos/dbus/power_policy_controller.cc:380: power_manager::PowerManagementPolicy_Action_SUSPEND); just out of curiosity, are you currently seeing the system shut down when you close the lid while the migration is in-progress? we usually shut down while at the login screen and suspend after the user has logged in (although enterprise policies can override this), so it'd depend on how far chrome thinks we've gotten in the login process when the migration begins (which i'm not sure of).
https://codereview.chromium.org/2912763003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc (right): https://codereview.chromium.org/2912763003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:286: // We should block power save and not poweroff on lid close during migration. On 2017/05/30 23:41:47, Daniel Erat wrote: > nit: s/poweroff/shut down/ Done. https://codereview.chromium.org/2912763003/diff/20001/chromeos/dbus/power_pol... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/2912763003/diff/20001/chromeos/dbus/power_pol... chromeos/dbus/power_policy_controller.cc:380: power_manager::PowerManagementPolicy_Action_SUSPEND); On 2017/05/30 23:41:47, Daniel Erat wrote: > just out of curiosity, are you currently seeing the system shut down when you > close the lid while the migration is in-progress? we usually shut down while at > the login screen and suspend after the user has logged in (although enterprise > policies can override this), so it'd depend on how far chrome thinks we've > gotten in the login process when the migration begins (which i'm not sure of). Yes, we do observe shut down on lid close for stock profiles. We interrupt the login process very early on, so user and enterprise policies have not yet been applied.
Dan, thank you for taking over this! https://codereview.chromium.org/2912763003/diff/40001/chromeos/dbus/power_pol... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/2912763003/diff/40001/chromeos/dbus/power_pol... chromeos/dbus/power_policy_controller.cc:281: SendCurrentPolicy(); Maybe we should call SendCurrentPolicy() only when |encryption_migration_active_| is changed. In encryption_migration_screen_handler, SetEncryptionMigrationActive(false) can be called several times in a second.
https://codereview.chromium.org/2912763003/diff/40001/chromeos/dbus/power_pol... File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/2912763003/diff/40001/chromeos/dbus/power_pol... chromeos/dbus/power_policy_controller.cc:281: SendCurrentPolicy(); On 2017/05/31 05:57:22, fukino wrote: > Maybe we should call SendCurrentPolicy() only when > |encryption_migration_active_| is changed. > > In encryption_migration_screen_handler, SetEncryptionMigrationActive(false) can > be called several times in a second. Done.
Thanks! lgtm.
thanks, good catch! still lgtm
https://codereview.chromium.org/2912763003/diff/60001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2912763003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.cc:91: LOG(FATAL) << "unmount"; Is this intentional?
https://codereview.chromium.org/2912763003/diff/60001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2912763003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.cc:91: LOG(FATAL) << "unmount"; On 2017/05/31 15:00:53, xiyuan wrote: > Is this intentional? Oops, unrelated debugging work I was doing.
The CQ bit was checked by dspaid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, fukino@chromium.org Link to the patchset: https://codereview.chromium.org/2912763003/#ps80001 (title: "revert unrelated change")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by dspaid@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": 1496273410960960,
"parent_rev": "97d0b89dd82c920b62afd2f5a0660748c261fe06", "commit_rev":
"3769f83c5922880a027a1aaa6589d9a99c7b782f"}
Message was sent while issue was closed.
Description was changed from ========== cros: Prevent shut down on lid-close during migration. In the login screen, the system will shut down on lid close. Although we ask users not to close the lid during the migration, it should be better to change the lid-close action to SUSPEND during migration. BUG=726229 TEST=out/Release/chromeos_unittests --gtest_filter=PowerPolicyControllerTest.* ========== to ========== cros: Prevent shut down on lid-close during migration. In the login screen, the system will shut down on lid close. Although we ask users not to close the lid during the migration, it should be better to change the lid-close action to SUSPEND during migration. BUG=726229 TEST=out/Release/chromeos_unittests --gtest_filter=PowerPolicyControllerTest.* Review-Url: https://codereview.chromium.org/2912763003 Cr-Commit-Position: refs/heads/master@{#476091} Committed: https://chromium.googlesource.com/chromium/src/+/3769f83c5922880a027a1aaa6589... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3769f83c5922880a027a1aaa6589... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
