Disable Demo app launch when device is in Dev mode

Wrote unit test for DemoModeDetector,  accounting for existing behavior,
and also the new behavior introduced in this CL.

R=michaelpg@chromium.org
TEST=tested manually on dev chromebook, and with demo_mode_detector_unittest.cc

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

chrome/browser/chromeos/BUILD.gn

@@ -2199,6 +2199,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,16 @@
 
 #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_(std::make_unique<base::OneShotTimer>(tick_clock)),
+      idle_callback_(on_idle_callback) {}
 
 IdleDetector::~IdleDetector() {
   ui::UserActivityDetector* user_activity_detector =
@@ -32,10 +34,10 @@ void IdleDetector::Start(const base::TimeDelta& timeout) {
 }
 
 void IdleDetector::ResetTimer() {
-  if (timer_.IsRunning())
-    timer_.Reset();
+  if (timer_->IsRunning())
+    timer_->Reset();
   else
-    timer_.Start(FROM_HERE, timeout_, idle_callback_);
+    timer_->Start(FROM_HERE, timeout_, idle_callback_);
 }
 
 }  // namespace chromeos

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,11 +16,15 @@ 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);
 
+  void SetTickClockForTest(const base::TickClock* test_clock);
+
  private:
   // ui::UserActivityObserver overrides:
   void OnUserActivity(const ui::Event* event) override;
@@ -28,9 +32,9 @@ class IdleDetector : public ui::UserActivityObserver {
   // Resets |timer_| to fire when we reach our idle timeout.
   void ResetTimer();
 
-  base::OneShotTimer timer_;
+  std::unique_ptr<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,20 +5,18 @@
 #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 {
 
@@ -27,12 +25,18 @@ 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 +48,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 +78,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 +120,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,16 @@ 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 constexpr base::TimeDelta kDerelictDetectionTimeout =
+      base::TimeDelta::FromHours(8);
+  static constexpr base::TimeDelta kDerelictIdleTimeout =
+      base::TimeDelta::FromMinutes(5);
+  static constexpr base::TimeDelta kOobeTimerUpdateInterval =
+      base::TimeDelta::FromMinutes(5);
+
  private:
   void StartIdleDetection();
   void StartOobeTimer();
@@ -57,6 +68,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));
 }