Reland "Disable Demo app launch when device is in Dev mode"

The previous (now reverted) version of this CL failed to build in
debug mode (and therefore broke the continuous build), because
apparently static constexpr class members, which must be initialized
in the class definition, are then not available at link time when
is_debug=true.  I didn't catch this before landing the CL because I
was building with is_debug=false.

The fix involved declaring the members as static const (not constexpr),
and defining them in demo_mode_detector.cc.

Revert "Revert "Disable Demo app launch when device is in Dev mode""

This reverts commit 225590d4.

Bug: 877655
Change-Id: Id8c15012e4bfa33230feda38ec8a94ea5c809741
Reviewed-on: https://chromium-review.googlesource.com/c/1318741
Commit-Queue: Danan S <danan@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605950}

chrome/browser/chromeos/BUILD.gn

@@ -2200,6 +2200,7 @@ source_set("unit_tests") {
     "lock_screen_apps/state_controller_unittest.cc",
     "login/auth/cryptohome_authenticator_unittest.cc",
     "login/demo_mode/demo_extensions_external_loader_unittest.cc",
+    "login/demo_mode/demo_mode_detector_unittest.cc",
     "login/demo_mode/demo_mode_resources_remover_unittest.cc",
     "login/demo_mode/demo_session_unittest.cc",
     "login/demo_mode/demo_setup_controller_unittest.cc",

chrome/browser/chromeos/idle_detector.cc

@@ -4,14 +4,15 @@
 
 #include "chrome/browser/chromeos/idle_detector.h"
 
-#include "base/bind.h"
-#include "base/logging.h"
+#include "base/location.h"
+#include "base/time/default_tick_clock.h"
 #include "ui/base/user_activity/user_activity_detector.h"
 
 namespace chromeos {
 
-IdleDetector::IdleDetector(const base::Closure& on_idle_callback)
-    : idle_callback_(on_idle_callback) {}
+IdleDetector::IdleDetector(const base::RepeatingClosure& on_idle_callback,
+                           const base::TickClock* tick_clock)
+    : timer_(tick_clock), idle_callback_(on_idle_callback) {}
 
 IdleDetector::~IdleDetector() {
   ui::UserActivityDetector* user_activity_detector =

chrome/browser/chromeos/idle_detector.h

@@ -6,8 +6,8 @@
 #define CHROME_BROWSER_CHROMEOS_IDLE_DETECTOR_H_
 
 #include "base/callback.h"
-#include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "ui/base/user_activity/user_activity_observer.h"
@@ -16,7 +16,9 @@ namespace chromeos {
 
 class IdleDetector : public ui::UserActivityObserver {
  public:
-  explicit IdleDetector(const base::Closure& on_idle_callback);
+  IdleDetector(const base::RepeatingClosure& on_idle_callback,
+               const base::TickClock* tick_clock);
+
   ~IdleDetector() override;
 
   void Start(const base::TimeDelta& timeout);
@@ -30,7 +32,7 @@ class IdleDetector : public ui::UserActivityObserver {
 
   base::OneShotTimer timer_;
 
-  base::Closure idle_callback_;
+  base::RepeatingClosure idle_callback_;
 
   base::TimeDelta timeout_;
 

chrome/browser/chromeos/login/demo_mode/demo_mode_detector.cc

@@ -5,34 +5,45 @@
 #include "chrome/browser/chromeos/login/demo_mode/demo_mode_detector.h"
 
 #include "base/command_line.h"
+#include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/sys_info.h"
+#include "base/time/default_tick_clock.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/dbus_switches.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 
-namespace {
-const int kDerelectDetectionTimeoutSeconds = 8 * 60 * 60;  // 8 hours.
-const int kDerelectIdleTimeoutSeconds = 5 * 60;            // 5 minutes.
-const int kOobeTimerUpdateIntervalSeconds = 5 * 60;        // 5 minutes.
-}  // namespace
 
 namespace chromeos {
 
+const base::TimeDelta DemoModeDetector::kDerelictDetectionTimeout =
+    base::TimeDelta::FromHours(8);
+const base::TimeDelta DemoModeDetector::kDerelictIdleTimeout =
+    base::TimeDelta::FromMinutes(5);
+const base::TimeDelta DemoModeDetector::kOobeTimerUpdateInterval =
+    base::TimeDelta::FromMinutes(5);
+
 // static
 void DemoModeDetector::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterInt64Pref(prefs::kTimeOnOobe, 0);
 }
 
-DemoModeDetector::DemoModeDetector() : weak_ptr_factory_(this) {
+DemoModeDetector::DemoModeDetector()
+    : tick_clock_(base::DefaultTickClock::GetInstance()),
+      weak_ptr_factory_(this) {
   SetupTimeouts();
 }
 
 DemoModeDetector::~DemoModeDetector() {}
 
+void DemoModeDetector::SetTickClockForTest(const base::TickClock* test_clock) {
+  tick_clock_ = test_clock;
+}
+
 void DemoModeDetector::InitDetection() {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableDemoMode))
@@ -44,6 +55,14 @@ void DemoModeDetector::InitDetection() {
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDerelictIdleTimeout);
 
+  // Devices in retail won't be in dev mode, and DUTs (devices under test) often
+  // sit unused at OOBE for a while.
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSystemDevMode) &&
+      !has_derelict_switch) {
+    return;
+  }
+
   if (base::SysInfo::IsRunningOnChromeOS() && !has_derelict_switch) {
     std::string track;
     // We're running on an actual device; if we cannot find our release track
@@ -66,8 +85,9 @@ void DemoModeDetector::StopDetection() {
 
 void DemoModeDetector::StartIdleDetection() {
   if (!idle_detector_) {
-    idle_detector_.reset(new IdleDetector(
-        base::Bind(&DemoModeDetector::OnIdle, weak_ptr_factory_.GetWeakPtr())));
+    auto callback = base::BindRepeating(&DemoModeDetector::OnIdle,
+                                        weak_ptr_factory_.GetWeakPtr());
+    idle_detector_ = std::make_unique<IdleDetector>(callback, tick_clock_);
   }
   idle_detector_->Start(derelict_idle_timeout_);
 }
@@ -107,33 +127,37 @@ void DemoModeDetector::SetupTimeouts() {
       base::TimeDelta::FromSeconds(prefs->GetInt64(prefs::kTimeOnOobe));
 
   int derelict_detection_timeout;
-  if (!cmdline->HasSwitch(switches::kDerelictDetectionTimeout) ||
-      !base::StringToInt(
+  if (cmdline->HasSwitch(switches::kDerelictDetectionTimeout) &&
+      base::StringToInt(
           cmdline->GetSwitchValueASCII(switches::kDerelictDetectionTimeout),
           &derelict_detection_timeout)) {
-    derelict_detection_timeout = kDerelectDetectionTimeoutSeconds;
+    derelict_detection_timeout_ =
+        base::TimeDelta::FromSeconds(derelict_detection_timeout);
+  } else {
+    derelict_detection_timeout_ = kDerelictDetectionTimeout;
   }
-  derelict_detection_timeout_ =
-      base::TimeDelta::FromSeconds(derelict_detection_timeout);
 
   int derelict_idle_timeout;
-  if (!cmdline->HasSwitch(switches::kDerelictIdleTimeout) ||
-      !base::StringToInt(
+  if (cmdline->HasSwitch(switches::kDerelictIdleTimeout) &&
+      base::StringToInt(
           cmdline->GetSwitchValueASCII(switches::kDerelictIdleTimeout),
           &derelict_idle_timeout)) {
-    derelict_idle_timeout = kDerelectIdleTimeoutSeconds;
+    derelict_idle_timeout_ =
+        base::TimeDelta::FromSeconds(derelict_idle_timeout);
+  } else {
+    derelict_idle_timeout_ = kDerelictIdleTimeout;
   }
-  derelict_idle_timeout_ = base::TimeDelta::FromSeconds(derelict_idle_timeout);
 
   int oobe_timer_update_interval;
-  if (!cmdline->HasSwitch(switches::kOobeTimerInterval) ||
-      !base::StringToInt(
+  if (cmdline->HasSwitch(switches::kOobeTimerInterval) &&
+      base::StringToInt(
           cmdline->GetSwitchValueASCII(switches::kOobeTimerInterval),
           &oobe_timer_update_interval)) {
-    oobe_timer_update_interval = kOobeTimerUpdateIntervalSeconds;
+    oobe_timer_update_interval_ =
+        base::TimeDelta::FromSeconds(oobe_timer_update_interval);
+  } else {
+    oobe_timer_update_interval_ = kOobeTimerUpdateInterval;
   }
-  oobe_timer_update_interval_ =
-      base::TimeDelta::FromSeconds(oobe_timer_update_interval);
 
   // In case we'd be derelict before our timer is set to trigger, reduce
   // the interval so we check again when we're scheduled to go derelict.

chrome/browser/chromeos/login/demo_mode/demo_mode_detector.h

@@ -10,6 +10,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/chromeos/idle_detector.h"
@@ -31,6 +32,13 @@ class DemoModeDetector {
   // Registers the preference for derelict state.
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
+  // Sets an alternative clock for testing purposes.
+  void SetTickClockForTest(const base::TickClock* test_clock);
+
+  static const base::TimeDelta kDerelictDetectionTimeout;
+  static const base::TimeDelta kDerelictIdleTimeout;
+  static const base::TimeDelta kOobeTimerUpdateInterval;
+
  private:
   void StartIdleDetection();
   void StartOobeTimer();
@@ -57,6 +65,8 @@ class DemoModeDetector {
 
   bool demo_launched_ = false;
 
+  const base::TickClock* tick_clock_;
+
   base::WeakPtrFactory<DemoModeDetector> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DemoModeDetector);

chrome/browser/chromeos/login/demo_mode/demo_mode_detector_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/login/demo_mode/demo_mode_detector.h"
+
+#include <memory>
+
+#include "base/memory/ref_counted.h"
+#include "base/test/scoped_command_line.h"
+#include "base/test/test_mock_time_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/values.h"
+#include "chrome/browser/chromeos/login/ui/mock_login_display_host.h"
+#include "chrome/browser/chromeos/scoped_set_running_on_chromeos_for_testing.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/dbus_switches.h"
+#include "components/prefs/testing_pref_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/user_activity/user_activity_detector.h"
+#include "ui/events/base_event_utils.h"
+
+namespace chromeos {
+
+class DemoModeDetectorTest : public testing::Test {
+ protected:
+  DemoModeDetectorTest();
+  ~DemoModeDetectorTest() override;
+
+  void StartDemoModeDetection();
+  void ExpectDemoModeWillLaunch();
+  void ExpectDemoModeWillNotLaunch();
+  void SetTimeOnOobePref(base::TimeDelta time_on_oobe);
+  base::TimeDelta GetTimeOnOobePref();
+  void DestroyDemoModeDetector();
+  void SimulateUserActivity();
+  scoped_refptr<base::TestMockTimeTaskRunner> runner_;
+
+ private:
+  TestingPrefServiceSimple local_state_;
+  MockLoginDisplayHost login_display_host_;
+  ui::UserActivityDetector user_activity_detector_;
+  std::unique_ptr<DemoModeDetector> demo_mode_detector_;
+  std::unique_ptr<base::ThreadTaskRunnerHandle> runner_handle_;
+};
+
+DemoModeDetectorTest::DemoModeDetectorTest() {
+  TestingBrowserProcess::GetGlobal()->SetLocalState(&local_state_);
+  DemoModeDetector::RegisterPrefs(local_state_.registry());
+  runner_ = base::MakeRefCounted<base::TestMockTimeTaskRunner>();
+  // If we don't register the TestMockTimeTaskRunner with a
+  // ThreadTaskRunnerHandle, the timers in the test fail
+  // to initialize, saying they need a SequencedContext.
+  runner_handle_ = std::make_unique<base::ThreadTaskRunnerHandle>(runner_);
+}
+
+DemoModeDetectorTest::~DemoModeDetectorTest() {
+  demo_mode_detector_.reset();
+  runner_handle_.reset();
+  TestingBrowserProcess::GetGlobal()->SetLocalState(nullptr);
+}
+
+void DemoModeDetectorTest::ExpectDemoModeWillLaunch() {
+  EXPECT_CALL(login_display_host_, StartDemoAppLaunch());
+}
+
+void DemoModeDetectorTest::ExpectDemoModeWillNotLaunch() {
+  EXPECT_CALL(login_display_host_, StartDemoAppLaunch()).Times(0);
+}
+
+void DemoModeDetectorTest::StartDemoModeDetection() {
+  demo_mode_detector_ = std::make_unique<DemoModeDetector>();
+  demo_mode_detector_->SetTickClockForTest(runner_->GetMockTickClock());
+  demo_mode_detector_->InitDetection();
+}
+
+void DemoModeDetectorTest::SetTimeOnOobePref(base::TimeDelta time_on_oobe) {
+  local_state_.SetUserPref(prefs::kTimeOnOobe,
+                           std::make_unique<base::Value>(
+                               base::Int64ToString(time_on_oobe.InSeconds())));
+}
+
+base::TimeDelta DemoModeDetectorTest::GetTimeOnOobePref() {
+  return base::TimeDelta::FromSeconds(
+      local_state_.GetInt64(prefs::kTimeOnOobe));
+}
+
+void DemoModeDetectorTest::SimulateUserActivity() {
+  user_activity_detector_.HandleExternalUserActivity();
+}
+
+// Tests follow.
+
+// Test to ensure that Demo mode isn't launched before the detector
+// has entered the derelict state.
+TEST_F(DemoModeDetectorTest, DemoModeWillNotLaunchBeforeDerelict) {
+  ExpectDemoModeWillNotLaunch();
+
+  StartDemoModeDetection();
+  // Run for half the timeout.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictDetectionTimeout -
+                         DemoModeDetector::kDerelictDetectionTimeout / 2);
+}
+
+// Test to ensure that Demo mode isn't launched after the detector
+// has entered the derelict state but before the idle timeout.
+TEST_F(DemoModeDetectorTest,
+       DemoModeWillNotLaunchAfterDerelictAndBeforeIdleTimeout) {
+  ExpectDemoModeWillNotLaunch();
+
+  StartDemoModeDetection();
+  // Run through the derelict threshold.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictDetectionTimeout);
+  // Run for 1 minute less than the idle threshold.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictIdleTimeout -
+                         base::TimeDelta::FromMinutes(1));
+}
+
+// Test to ensure that Demo mode isn't launched after the detector
+// has entered the derelict state but user activity is preventing the idle
+// timeout.
+TEST_F(DemoModeDetectorTest,
+       DemoModeWillNotLaunchAfterDerelictWithUserActivity) {
+  ExpectDemoModeWillNotLaunch();
+
+  StartDemoModeDetection();
+
+  // Run for through the derelict threshold.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictIdleTimeout);
+
+  // Run for 2 more minutes (less than the idle threshold).
+  runner_->FastForwardBy(base::TimeDelta::FromMinutes(2));
+
+  // Simulate a user activity event.
+  SimulateUserActivity();
+
+  // Run for 3 more minutes (more than the idle threshold).
+  runner_->FastForwardBy(base::TimeDelta::FromMinutes(3));
+
+  // Simulate a user activity event.
+  SimulateUserActivity();
+}
+
+// Test to ensure that Demo mode is launched after the detector
+// has entered the derelict state and after the idle timeout.
+TEST_F(DemoModeDetectorTest, DemoModeWillLaunchAfterDerelictAndIdleTimeout) {
+  ExpectDemoModeWillLaunch();
+
+  StartDemoModeDetection();
+  // Run for long enough for all thresholds to be exceeded.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictDetectionTimeout +
+                         DemoModeDetector::kDerelictIdleTimeout);
+}
+
+// Test to ensure that a device in dev mode disables the demo mode.
+TEST_F(DemoModeDetectorTest, DemoModeWillNotLaunchInDevMode) {
+  // Set the command line dev mode switch.
+  auto command_line_ = std::make_unique<base::test::ScopedCommandLine>();
+  command_line_->GetProcessCommandLine()->AppendSwitch(
+      switches::kSystemDevMode);
+
+  ExpectDemoModeWillNotLaunch();
+
+  StartDemoModeDetection();
+
+  // Run through the derelict threshold.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictDetectionTimeout);
+}
+
+// Test to ensure that the --disable-demo-mode switch disables demo mode.
+TEST_F(DemoModeDetectorTest, DemoModeWillNotLaunchWhenDisabledBySwitch) {
+  // Set the command line dev mode switch.
+  auto command_line_ = std::make_unique<base::test::ScopedCommandLine>();
+  command_line_->GetProcessCommandLine()->AppendSwitch(
+      switches::kDisableDemoMode);
+
+  ExpectDemoModeWillNotLaunch();
+
+  StartDemoModeDetection();
+  // Run through the derelict threshold.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictDetectionTimeout);
+}
+
+// Test to ensure that demo mode is disabled on "testimage" LsbRelease
+// CHROMEOS_RELEASE_TRACK values.
+TEST_F(DemoModeDetectorTest, DemoModeWillNotLaunchWhenTestimageInLsbRelease) {
+  std::string lsb_release =
+      "CHROMEOS_RELEASE_NAME=Chromium OS\n"
+      "CHROMEOS_RELEASE_TRACK=testimage\n";
+
+  base::Time release_time;
+  EXPECT_TRUE(
+      base::Time::FromString("Wed, 24 Oct 2018 12:00:00 PDT", &release_time));
+
+  ScopedSetRunningOnChromeOSForTesting version_info(lsb_release, release_time);
+
+  ExpectDemoModeWillNotLaunch();
+
+  StartDemoModeDetection();
+  // Run through the derelict threshold.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictDetectionTimeout);
+}
+
+// Test to ensure that Demo mode is launched after the detector
+// has resumed (i.e. after shutdown/reboot).
+TEST_F(DemoModeDetectorTest,
+       DemoModeWillLaunchAfterResumedAndDerelictAndIdleTimeout) {
+  ExpectDemoModeWillLaunch();
+
+  // Simulate 1 hour less than the threshold elapsed by setting pref.
+  const auto elapsed_time = DemoModeDetector::kDerelictDetectionTimeout -
+                            base::TimeDelta::FromHours(1);
+  SetTimeOnOobePref(elapsed_time);
+  EXPECT_EQ(GetTimeOnOobePref(), elapsed_time);
+
+  StartDemoModeDetection();
+
+  // Run another hour to hit the threshold.
+  runner_->FastForwardBy(base::TimeDelta::FromHours(1));
+
+  // Run through the idle timeout.
+  runner_->FastForwardBy(DemoModeDetector::kDerelictIdleTimeout);
+}
+
+}  // namespace chromeos

chrome/browser/chromeos/login/ui/preloaded_web_view.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/login/ui/preloaded_web_view.h"
 
 #include "base/callback_helpers.h"
+#include "base/time/default_tick_clock.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/idle_detector.h"
 #include "chrome/browser/profiles/profile.h"
@@ -35,7 +36,9 @@ PreloadedWebView::~PreloadedWebView() {}
 void PreloadedWebView::PreloadOnIdle(PreloadCallback preload) {
   preload_function_ = std::move(preload);
   idle_detector_ = std::make_unique<chromeos::IdleDetector>(
-      base::Bind(&PreloadedWebView::RunPreloader, weak_factory_.GetWeakPtr()));
+      base::BindRepeating(&PreloadedWebView::RunPreloader,
+                          weak_factory_.GetWeakPtr()),
+      base::DefaultTickClock::GetInstance());
   idle_detector_->Start(
       base::TimeDelta::FromSeconds(kIdleSecondsBeforePreloadingLockScreen));
 }