Move grace period check from RebootNotificationsScheduler

go/cros-reboot-command-dd

Move grace period check to DeviceScheduledRebootHandler and make it
configurable.

The RebootNotificationsScheduler is about to be used for reboot remote
command which does not have grace time. Only the DeviceScheduledReboot
policy needs the grace period check.

Bug: b:225913691
Test: unit_tests --gtest_filter=DeviceScheduledRebootHandlerTest.*
Test: unit_tests --gtest_filter=ScheduledRebootTimerFailureTest.*
Test: unit_tests --gtest_filter=ScheduledRebootDelayedServiceTest.*
Change-Id: I2fafc36cfdabeca6775568a6e417f4d60ad07f8b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4132848
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Sanja Perisic <sanjaperisic@chromium.org>
Reviewed-by: Ben Franz <bfranz@chromium.org>
Commit-Queue: Artem Sumaneev <asumaneev@google.com>
Cr-Commit-Position: refs/heads/main@{#1108274}

chrome/browser/ash/policy/scheduled_task_handler/device_scheduled_reboot_handler.cc

@@ -15,9 +15,8 @@
 #include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
-#include "base/location.h"
 #include "base/logging.h"
-#include "base/memory/ptr_util.h"
+#include "base/system/sys_info.h"
 #include "base/values.h"
 #include "chrome/browser/ash/policy/scheduled_task_handler/scheduled_task_util.h"
 #include "chrome/browser/ash/policy/scheduled_task_handler/scoped_wake_lock.h"
@@ -44,6 +43,10 @@ constexpr char kWakeLockReason[] = "DeviceScheduledRebootHandler";
 // Task name used for parsing ScheduledTaskData.
 constexpr char kTaskTimeFieldName[] = "reboot_time";
 
+base::Time GetBootTime() {
+  return base::Time::Now() - base::SysInfo::Uptime();
+}
+
 }  // namespace
 
 constexpr char DeviceScheduledRebootHandler::kRebootTimerTag[];
@@ -52,14 +55,27 @@ DeviceScheduledRebootHandler::DeviceScheduledRebootHandler(
     ash::CrosSettings* cros_settings,
     std::unique_ptr<ScheduledTaskExecutor> scheduled_task_executor,
     std::unique_ptr<RebootNotificationsScheduler> notifications_scheduler)
+    : DeviceScheduledRebootHandler(cros_settings,
+                                   std::move(scheduled_task_executor),
+                                   std::move(notifications_scheduler),
+                                   base::BindRepeating(GetBootTime)) {}
+
+DeviceScheduledRebootHandler::DeviceScheduledRebootHandler(
+    ash::CrosSettings* cros_settings,
+    std::unique_ptr<ScheduledTaskExecutor> scheduled_task_executor,
+    std::unique_ptr<RebootNotificationsScheduler> notifications_scheduler,
+    GetBootTimeCallback get_boot_time_callback)
     : cros_settings_(cros_settings),
       cros_settings_subscription_(cros_settings_->AddSettingsObserver(
           ash::kDeviceScheduledReboot,
           base::BindRepeating(
               &DeviceScheduledRebootHandler::OnScheduledRebootDataChanged,
               base::Unretained(this)))),
       scheduled_task_executor_(std::move(scheduled_task_executor)),
-      notifications_scheduler_(std::move(notifications_scheduler)) {
+      notifications_scheduler_(std::move(notifications_scheduler)),
+      get_boot_time_callback_(std::move(get_boot_time_callback)) {
+  DCHECK(get_boot_time_callback_);
+
   ash::system::TimezoneSettings::GetInstance()->AddObserver(this);
   auto* power_manager_client = chromeos::PowerManagerClient::Get();
   if (power_manager_client) {
@@ -205,8 +221,9 @@ void DeviceScheduledRebootHandler::OnRebootTimerStartResult(
     scheduled_reboot_data_ = absl::nullopt;
     return;
   }
-  // Set |skip_reboot_| flag if the grace time should be applied.
-  skip_reboot_ = notifications_scheduler_->ShouldApplyGraceTime(
+
+  skip_reboot_ = scheduled_task_util::ShouldSkipRebootDueToGracePeriod(
+      get_boot_time_callback_.Run(),
       scheduled_task_executor_->GetScheduledTaskTime());
 
   // If the flag is enabled, schedule reboot notification and dialog.

chrome/browser/ash/policy/scheduled_task_handler/device_scheduled_reboot_handler.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/memory/raw_ptr.h"
 #include "base/scoped_observation.h"
 #include "base/time/time.h"
 #include "chrome/browser/ash/policy/scheduled_task_handler/reboot_notifications_scheduler.h"
@@ -60,6 +61,15 @@ class DeviceScheduledRebootHandler
   bool IsRebootSkippedForTest() const;
 
  protected:
+  using GetBootTimeCallback = base::RepeatingCallback<base::Time()>;
+
+  // Extended constructor for testing purposes.
+  DeviceScheduledRebootHandler(
+      ash::CrosSettings* cros_settings,
+      std::unique_ptr<ScheduledTaskExecutor> scheduled_task_executor,
+      std::unique_ptr<RebootNotificationsScheduler> notifications_scheduler,
+      GetBootTimeCallback get_boot_time_callback);
+
   // Called when scheduled timer fires. Triggers a reboot and
   // schedules the next reboot based on |scheduled_reboot_data_|.
   virtual void OnRebootTimerExpired();
@@ -91,7 +101,7 @@ class DeviceScheduledRebootHandler
   void RebootDevice(const std::string& reboot_description) const;
 
   // Used to retrieve Chrome OS settings. Not owned.
-  ash::CrosSettings* const cros_settings_;
+  const base::raw_ptr<ash::CrosSettings> cros_settings_;
 
   // Subscription for callback when settings change.
   base::CallbackListSubscription cros_settings_subscription_;
@@ -112,6 +122,11 @@ class DeviceScheduledRebootHandler
   // Indicating if the reboot should be skipped.
   bool skip_reboot_ = false;
 
+  // Returns device's boot timestamp. The functor is used because the boot time
+  // is not constant and can change at runtime, e.g. because of the time
+  // sync.
+  GetBootTimeCallback get_boot_time_callback_;
+
   // Observation of chromeos::PowerManagerClient.
   base::ScopedObservation<chromeos::PowerManagerClient,
                           chromeos::PowerManagerClient::Observer>

chrome/browser/ash/policy/scheduled_task_handler/reboot_notifications_scheduler.cc

@@ -5,7 +5,6 @@
 #include "chrome/browser/ash/policy/scheduled_task_handler/reboot_notifications_scheduler.h"
 
 #include "ash/constants/ash_pref_names.h"
-#include "base/system/sys_info.h"
 #include "base/time/default_clock.h"
 #include "base/time/default_tick_clock.h"
 #include "chrome/browser/ash/app_restore/full_restore_service_factory.h"
@@ -20,7 +19,6 @@ namespace policy {
 namespace {
 constexpr base::TimeDelta kNotificationDelay = base::Hours(1);
 constexpr base::TimeDelta kDialogDelay = base::Minutes(5);
-constexpr base::TimeDelta kGraceTime = base::Hours(1);
 }  // namespace
 
 RebootNotificationsScheduler* RebootNotificationsScheduler::instance = nullptr;
@@ -32,7 +30,9 @@ RebootNotificationsScheduler::RebootNotificationsScheduler()
 RebootNotificationsScheduler::RebootNotificationsScheduler(
     const base::Clock* clock,
     const base::TickClock* tick_clock)
-    : notification_timer_(clock, tick_clock), dialog_timer_(clock, tick_clock) {
+    : notification_timer_(clock, tick_clock),
+      dialog_timer_(clock, tick_clock),
+      clock_(clock) {
   DCHECK(!RebootNotificationsScheduler::Get());
   RebootNotificationsScheduler::SetInstance(this);
   if (session_manager::SessionManager::Get())
@@ -68,9 +68,6 @@ void RebootNotificationsScheduler::SchedulePendingRebootNotifications(
     base::OnceClosure reboot_callback,
     const base::Time& reboot_time) {
   ResetState();
-  if (ShouldApplyGraceTime(reboot_time)) {
-    return;
-  }
 
   reboot_time_ = reboot_time;
   reboot_callback_ = std::move(reboot_callback);
@@ -139,12 +136,6 @@ void RebootNotificationsScheduler::ResetState() {
   reboot_callback_.Reset();
 }
 
-bool RebootNotificationsScheduler::ShouldApplyGraceTime(
-    const base::Time& reboot_time) const {
-  base::TimeDelta delay = GetRebootDelay(reboot_time);
-  return ((delay + GetSystemUptime()) <= kGraceTime);
-}
-
 void RebootNotificationsScheduler::MaybeShowPendingRebootNotification() {
   notification_controller_.MaybeShowPendingRebootNotification(
       reboot_time_,
@@ -176,17 +167,9 @@ void RebootNotificationsScheduler::SetInstance(
   RebootNotificationsScheduler::instance = reboot_notifications_scheduler;
 }
 
-const base::Time RebootNotificationsScheduler::GetCurrentTime() const {
-  return base::Time::Now();
-}
-
-const base::TimeDelta RebootNotificationsScheduler::GetSystemUptime() const {
-  return base::SysInfo::Uptime();
-}
-
 base::TimeDelta RebootNotificationsScheduler::GetRebootDelay(
     const base::Time& reboot_time) const {
-  return (reboot_time - GetCurrentTime());
+  return reboot_time - clock_->Now();
 }
 
 void RebootNotificationsScheduler::CloseNotifications() {
@@ -206,4 +189,4 @@ bool RebootNotificationsScheduler::IsPostRebootPrefSet(PrefService* prefs) {
   return prefs->GetBoolean(ash::prefs::kShowPostRebootNotification);
 }
 
-}  // namespace policy
+}  // namespace policy

chrome/browser/ash/policy/scheduled_task_handler/reboot_notifications_scheduler.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_ASH_POLICY_SCHEDULED_TASK_HANDLER_REBOOT_NOTIFICATIONS_SCHEDULER_H_
 
 #include "base/functional/callback.h"
+#include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observation.h"
 #include "base/time/time.h"
@@ -17,6 +18,11 @@
 #include "components/session_manager/core/session_manager.h"
 #include "components/session_manager/core/session_manager_observer.h"
 
+namespace base {
+class Clock;
+class TickClock;
+}  // namespace base
+
 namespace policy {
 
 // This class schedules timers for showing pending reboot notification and
@@ -57,10 +63,6 @@ class RebootNotificationsScheduler
   // Resets timers and closes notification and dialog if open.
   void ResetState();
 
-  // Grace time applies if the reboot is scheduled in less then an hour from the
-  // last device reboot.
-  bool ShouldApplyGraceTime(const base::Time& reboot_time) const;
-
   // SessionManagerObserver:
   void OnUserSessionStarted(bool is_primary_user) override;
 
@@ -87,12 +89,6 @@ class RebootNotificationsScheduler
   // Returns prefs for active profile or nullptr.
   virtual PrefService* GetPrefsForActiveProfile() const;
 
-  // Returns current time.
-  virtual const base::Time GetCurrentTime() const;
-
-  // Returns time since last reboot.
-  virtual const base::TimeDelta GetSystemUptime() const;
-
   // Returns delay from now until |reboot_time|.
   base::TimeDelta GetRebootDelay(const base::Time& reboot_time) const;
 
@@ -123,9 +119,11 @@ class RebootNotificationsScheduler
                           session_manager::SessionManagerObserver>
       observation_{this};
 
+  base::raw_ptr<const base::Clock> clock_;
+
   base::WeakPtrFactory<RebootNotificationsScheduler> weak_ptr_factory_{this};
 };
 
 }  // namespace policy
 
-#endif  // CHROME_BROWSER_ASH_POLICY_SCHEDULED_TASK_HANDLER_REBOOT_NOTIFICATIONS_SCHEDULER_H_
+#endif  // CHROME_BROWSER_ASH_POLICY_SCHEDULED_TASK_HANDLER_REBOOT_NOTIFICATIONS_SCHEDULER_H_

chrome/browser/ash/policy/scheduled_task_handler/scheduled_task_util.cc

@@ -18,6 +18,8 @@
 namespace policy {
 namespace {
 
+constexpr base::TimeDelta kDefaultGracePeriod = base::Hours(1);
+
 ScheduledTaskExecutor::Frequency GetFrequency(const std::string& frequency) {
   if (frequency == "DAILY")
     return ScheduledTaskExecutor::Frequency::kDaily;
@@ -267,5 +269,12 @@ base::TimeDelta GenerateRandomDelay(int max_delay_in_seconds) {
   return base::Milliseconds(random_delay);
 }
 
+bool ShouldSkipRebootDueToGracePeriod(base::Time boot_time,
+                                      base::Time reboot_time) {
+  // Skip reboot if reboot scheduled within [boot time, boot time + grace time]
+  // interval.
+  return boot_time + kDefaultGracePeriod >= reboot_time;
+}
+
 }  // namespace scheduled_task_util
 }  // namespace policy

chrome/browser/ash/policy/scheduled_task_handler/scheduled_task_util.h

@@ -57,6 +57,10 @@ std::unique_ptr<icu::Calendar> CalculateNextScheduledTimeAfter(
 // max_delay_in_seconds).
 base::TimeDelta GenerateRandomDelay(int max_delay_in_seconds);
 
+// Returns true if `reboot_time` is within grace time period.
+bool ShouldSkipRebootDueToGracePeriod(base::Time boot_time,
+                                      base::Time reboot_time);
+
 }  // namespace scheduled_task_util
 
 }  // namespace policy