ash: Fix a screen-locking crash.

ash/wm/lock_state_controller.cc

Index: ash/wm/lock_state_controller.cc
diff --git a/ash/wm/lock_state_controller.cc b/ash/wm/lock_state_controller.cc
index 09a46afa2a1436404485a642a920205f69a4cad8..754d4b68eacd050d43832215092f849f1288359c 100644
--- a/ash/wm/lock_state_controller.cc
+++ b/ash/wm/lock_state_controller.cc
@@ -16,6 +16,7 @@
 #include "ash/wm/session_state_animator.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
+#include "base/strings/string_util.h"
 #include "base/timer/timer.h"
 #include "ui/aura/window_tree_host.h"
 #include "ui/compositor/layer_animation_sequence.h"
@@ -584,9 +585,12 @@ void LockStateController::LockAnimationCancelled() {
 }
 
 void LockStateController::PreLockAnimationFinished(bool request_lock) {
-  can_cancel_lock_animation_ = false;
   VLOG(1) << "PreLockAnimationFinished";
+  can_cancel_lock_animation_ = false;
+
   if (request_lock) {
+    DCHECK(!system_is_locked_) << "Got request to lock already-locked system "
+                               << "at completion of pre-lock animation";
     Shell::GetInstance()->metrics()->RecordUserMetricsAction(
         shutdown_after_lock_ ?
         UMA_ACCEL_LOCK_SCREEN_POWER_BUTTON :
@@ -594,24 +598,23 @@ void LockStateController::PreLockAnimationFinished(bool request_lock) {
     delegate_->RequestLockScreen();

[oshima, 2014/08/01 23:29:58]

Just for my understanding. Where do we request locking screen when this crash
was happening?

[Daniel Erat, 2014/08/01 23:52:06]

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. :-(

   }
 
-  int lock_timeout = kLockFailTimeoutMs;
-
+  // Don't start the lock-fail timer if the screen was already locked while the
+  // animation was going.
+  if (!system_is_locked_) {

[oshima, 2014/08/01 23:29:58]

nit: early exit?

[Daniel Erat, 2014/08/01 23:52:06]

Done.

+    base::TimeDelta timeout =
+        base::TimeDelta::FromMilliseconds(kLockFailTimeoutMs);
 #if defined(OS_CHROMEOS)
-  std::string board = base::SysInfo::GetLsbReleaseBoard();
-
-  // Increase lock timeout for slower hardware, see http://crbug.com/350628
-  if (board == "x86-mario" ||
-      board.substr(0, 8) == "x86-alex" ||
-      board.substr(0, 7) == "x86-zgb") {
-    lock_timeout *= 2;
-  }
+    // Increase lock timeout for slower hardware, see http://crbug.com/350628
+    const std::string board = base::SysInfo::GetLsbReleaseBoard();
+    if (board == "x86-mario" ||
+        StartsWithASCII(board, "x86-alex", true) ||
+        StartsWithASCII(board, "x86-zgb", true)) {

[oshima, 2014/08/01 23:29:58]

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?

[Daniel Erat, 2014/08/01 23:52:06]

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).

[oshima, 2014/08/02 00:27:53]

This can happen with shift-ctrl-l without suspending, no?
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.

[Daniel Erat, 2014/08/02 00:38:13]

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
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.

+      timeout *= 2;
+    }
 #endif
-
-  lock_fail_timer_.Start(
-      FROM_HERE,
-      base::TimeDelta::FromMilliseconds(lock_timeout),
-      this,
-      &LockStateController::OnLockFailTimeout);
+    lock_fail_timer_.Start(
+        FROM_HERE, timeout, this, &LockStateController::OnLockFailTimeout);
+  }
 }
 
 void LockStateController::PostLockAnimationFinished() {