Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
435083002:
ash: Fix a screen-locking crash. (Closed)
|
Created:
6 years, 4 months ago by Daniel Erat Modified:
6 years, 4 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org, Nikita (slow), Denis Kuznetsov (DE-MUC), Chris Masone Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionash: Fix a screen-locking crash.
LockStateController starts its 350 ms pre-lock animation
when the screen is locked via the system tray. When the
animation completes, the controller was incorrectly starting
the lock-fail timer. If the lock screen had already become
ready before this point (possible on a fast system), the
timer would crash the process eight seconds later.
Change LockStateController to only start the timer when
actually requesting that the screen be locked.
BUG=349083
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287187
Patch Set 1 #
Total comments: 1
Patch Set 2 : start timer as long as screen isn't already locked #
Total comments: 10
Patch Set 3 : re-upload with diff against correct branch #
Total comments: 4
Patch Set 4 : apply review feedback #Patch Set 5 : diff against correct branch again #Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/435083002/diff/1/ash/wm/lock_state_controller.cc File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/435083002/diff/1/ash/wm/lock_state_controller... ash/wm/lock_state_controller.cc:593: if (system_is_locked_) { another option would be structuring the body of this method like this: if (request_lock) // request lock if (!system_is_locked_) // start timer otherwise, we're not running the timer at all when locking using means other than the power button. there's a bit more risk that doing that may not fix the crashes, though (i.e. if the pre-lock animation is run sometimes without a corresponding lock request). this class isn't well-tested, so maybe it's safest to just stick with what i have here.
lgtm w/nits https://codereview.chromium.org/435083002/diff/40001/ash/wm/lock_state_contro... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/435083002/diff/40001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:603: if (!system_is_locked_) { nit: Early exit https://codereview.chromium.org/435083002/diff/40001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:611: StartsWithASCII(board, "x86-zgb", true)) { nit: what is "true"?
lgtm with nits and question. https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:598: delegate_->RequestLockScreen(); Just for my understanding. Where do we request locking screen when this crash was happening? https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:603: if (!system_is_locked_) { nit: early exit? https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:611: StartsWithASCII(board, "x86-zgb", true)) { don't have to be in this CL, but there are a lot of timeout on arm. Do you think we should increase the timeout on arm as well? https://codereview.chromium.org/435083002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc (right): https://codereview.chromium.org/435083002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc:45: const user_manager::User* user = manager ? manager->GetActiveUser() : NULL; can you update the CL description?
https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:598: delegate_->RequestLockScreen(); On 2014/08/01 23:29:58, oshima wrote: > Just for my understanding. Where do we request locking screen when this crash > was happening? this is a maze of similarly-named methods, but i think that this is how it works: [user clicks lock button in system tray] -> DateDefaultView::ButtonPressed -> SystemTrayDelegateChromeOS::RequestLockScreen -> SessionManagerClient::RequestLockScreen -> [d-bus request is sent to session_manager] -> [session_manager asks chrome to display the lock screen via d-bus] -> ScreenLockServiceProvider::LockScreen -> ScreenLocker::HandleLockScreenRequest -> LockStateController::OnStartingLock -> LockStateController::StartImmediatePreLockAnimation i am still amazed that it used to be even more complicated. :-( https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:603: if (!system_is_locked_) { On 2014/08/01 23:29:58, oshima wrote: > nit: early exit? Done. https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:611: StartsWithASCII(board, "x86-zgb", true)) { On 2014/08/01 23:29:58, oshima wrote: > don't have to be in this CL, but there are a lot of timeout on arm. > Do you think we should increase the timeout on arm as well? i think that powerd only gives chrome ten seconds to say it's ready before suspending, so i'm not sure that doubling the timeout does much good. even if we're unable to make chrome display the lock screen in less than eight seconds (which still seems weird to me), maybe chrome could get into a state where the desktop is protected more quickly. that would probably be a better approach than adding more boards to this list (and if we were going to add to the list, i'd say we should add a USE flag that's used to pass a flag to chrome instead). https://codereview.chromium.org/435083002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc (right): https://codereview.chromium.org/435083002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc:45: const user_manager::User* user = manager ? manager->GetActiveUser() : NULL; On 2014/08/01 23:29:58, oshima wrote: > can you update the CL description? sorry, didn't mean for this to creep in. re-uploaded the change https://codereview.chromium.org/435083002/diff/40001/ash/wm/lock_state_contro... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/435083002/diff/40001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:603: if (!system_is_locked_) { On 2014/08/01 23:23:56, stevenjb wrote: > nit: Early exit Done. https://codereview.chromium.org/435083002/diff/40001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:611: StartsWithASCII(board, "x86-zgb", true)) { On 2014/08/01 23:23:56, stevenjb wrote: > nit: what is "true"? done (but just because it still fits without wrapping :-P)
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/435083002/80001
slgtm https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:611: StartsWithASCII(board, "x86-zgb", true)) { On 2014/08/01 23:52:06, Daniel Erat wrote: > On 2014/08/01 23:29:58, oshima wrote: > > don't have to be in this CL, but there are a lot of timeout on arm. > > Do you think we should increase the timeout on arm as well? > > i think that powerd only gives chrome ten seconds to say it's ready before > suspending, so i'm not sure that doubling the timeout does much good. This can happen with shift-ctrl-l without suspending, no? > even if > we're unable to make chrome display the lock screen in less than eight seconds > (which still seems weird to me), maybe chrome could get into a state where the > desktop is protected more quickly. that would probably be a better approach than > adding more boards to this list (and if we were going to add to the list, i'd > say we should add a USE flag that's used to pass a flag to chrome instead). I may be wrong since It's been really long time since last time I looked into screen lock, but IIRC, we accept password input only if the lock is successful, and this is security feature, so it's better to exit if it's not working as expected.
+cmasone since he likes screen-lock timeouts https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... File ash/wm/lock_state_controller.cc (right): https://codereview.chromium.org/435083002/diff/20001/ash/wm/lock_state_contro... ash/wm/lock_state_controller.cc:611: StartsWithASCII(board, "x86-zgb", true)) { On 2014/08/02 00:27:53, oshima wrote: > On 2014/08/01 23:52:06, Daniel Erat wrote: > > On 2014/08/01 23:29:58, oshima wrote: > > > don't have to be in this CL, but there are a lot of timeout on arm. > > > Do you think we should increase the timeout on arm as well? > > > > i think that powerd only gives chrome ten seconds to say it's ready before > > suspending, so i'm not sure that doubling the timeout does much good. > > This can happen with shift-ctrl-l without suspending, no? yes, there are a bunch of ways the screen can be locked without suspending: - ctrl-shift-L - system tray icon - user inactivity (powerd locks 10 seconds after turning the screen off by default iirc; a different delay can be set via enterprise policy) - maybe more that i'm forgetting > > even if > > we're unable to make chrome display the lock screen in less than eight seconds > > (which still seems weird to me), maybe chrome could get into a state where the > > desktop is protected more quickly. that would probably be a better approach > than > > adding more boards to this list (and if we were going to add to the list, i'd > > say we should add a USE flag that's used to pass a flag to chrome instead). > > I may be wrong since It's been really long time since last time I looked into > screen lock, > but IIRC, we accept password input only if the lock is successful, and this is > security feature, so it's better to exit if it's not working as expected. sure, but i think that the main reason we crash is to make sure that we don't leave the system in an unlocked state. if the timer instead just checked that the desktop was hidden without waiting for the webui lock screen to appear, we could still be secure without needing to have these long timeouts on slower systems.
Message was sent while issue was closed.
Change committed as 287187 |
