DeviceScheduledUpdateChecker: Refactor retry logic in a separate class

This change adds a separate class to do the dirty work of running and
retrying a task if it fails. This will be used for all tasks that would
need to be run on an update check.

BUG=924762
TEST=Unit tests.

Change-Id: I89cc65fc93631989857c3bd8dce6f53dca31fe2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1661972Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: Abhishek Bhardwaj <abhishekbh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671479}

chrome/browser/chromeos/BUILD.gn

@@ -1727,6 +1727,8 @@ source_set("chromeos") {
     "policy/status_uploader.h",
     "policy/system_log_uploader.cc",
     "policy/system_log_uploader.h",
+    "policy/task_executor_with_retries.cc",
+    "policy/task_executor_with_retries.h",
     "policy/tpm_auto_update_mode_policy_handler.cc",
     "policy/tpm_auto_update_mode_policy_handler.h",
     "policy/upload_job.h",

chrome/browser/chromeos/policy/device_scheduled_update_checker.cc

@@ -50,9 +50,9 @@ constexpr int kDaysInAWeek = 7;
 // The tag associated to register |update_check_timer_|.
 constexpr char kUpdateCheckTimerTag[] = "DeviceScheduledUpdateChecker";
 
-// The tag associated to register |start_update_check_retry_timer_|.
-constexpr char kStartUpdateCheckRetryTimerTag[] =
-    "DeviceScheduledUpdateCheckerRetry";
+// The tag associated to register |start_update_check_timer_task_executor_|.
+constexpr char kStartUpdateCheckTimerTaskRunnerTag[] =
+    "StartUpdateCheckTimerTaskRunner";
 
 DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::Frequency GetFrequency(
     const std::string& frequency) {
@@ -265,6 +265,8 @@ base::Optional<base::Time> CalculateNextUpdateCheckMonthlyTime(
 // |cros_settings_observer_| will be destroyed as part of this object
 // guaranteeing to not run |OnScheduledUpdateCheckDataChanged| after its
 // destruction. Therefore, it's safe to use "this" while adding this observer.
+// Similarly, |start_update_check_timer_task_executor_| will be destroyed as
+// part of this object, so it's safe to use "this" with any callbacks.
 DeviceScheduledUpdateChecker::DeviceScheduledUpdateChecker(
     chromeos::CrosSettings* cros_settings)
     : cros_settings_(cros_settings),
@@ -272,7 +274,13 @@ DeviceScheduledUpdateChecker::DeviceScheduledUpdateChecker(
           chromeos::kDeviceScheduledUpdateCheck,
           base::BindRepeating(
               &DeviceScheduledUpdateChecker::OnScheduledUpdateCheckDataChanged,
-              base::Unretained(this)))) {
+              base::Unretained(this)))),
+      start_update_check_timer_task_executor_(
+          kStartUpdateCheckTimerTaskRunnerTag,
+          base::BindRepeating(&DeviceScheduledUpdateChecker::GetTicksSinceBoot,
+                              base::Unretained(this)),
+          update_checker_internal::kMaxRetryUpdateCheckIterations,
+          update_checker_internal::kStartUpdateCheckTimerRetryTime) {
   // Check if policy already exists.
   OnScheduledUpdateCheckDataChanged();
 }
@@ -291,7 +299,15 @@ void DeviceScheduledUpdateChecker::UpdateCheck() {
   // If a policy exists, schedule the next update check timer.
   if (!scheduled_update_check_data_)
     return;
-  StartUpdateCheckTimer();
+
+  // |start_update_check_timer_task_executor_| will be destroyed as part of this
+  // object, so it's safe to use "this" with any callbacks.
+  start_update_check_timer_task_executor_.Start(
+      base::BindRepeating(&DeviceScheduledUpdateChecker::StartUpdateCheckTimer,
+                          base::Unretained(this)),
+      base::BindOnce(
+          &DeviceScheduledUpdateChecker::OnStartUpdateCheckTimerRetryFailure,
+          base::Unretained(this)));
 }
 
 void DeviceScheduledUpdateChecker::OnScheduledUpdateCheckDataChanged() {
@@ -314,7 +330,12 @@ void DeviceScheduledUpdateChecker::OnScheduledUpdateCheckDataChanged() {
   scheduled_update_check_data_ = std::move(scheduled_update_check_data);
 
   // Policy has been updated, calculate and set |update_check_timer_| again.
-  StartUpdateCheckTimer();
+  start_update_check_timer_task_executor_.Start(
+      base::BindRepeating(&DeviceScheduledUpdateChecker::StartUpdateCheckTimer,
+                          base::Unretained(this)),
+      base::BindOnce(
+          &DeviceScheduledUpdateChecker::OnStartUpdateCheckTimerRetryFailure,
+          base::Unretained(this)));
 }
 
 base::Optional<base::Time>
@@ -392,12 +413,6 @@ DeviceScheduledUpdateChecker::CalculateNextUpdateCheckTime(
 }
 
 void DeviceScheduledUpdateChecker::StartUpdateCheckTimer() {
-  // Cancel any pending calls to |StartUpdateCheckTimer| to avoid redundant
-  // work, one could be lingering due to a call to
-  // |RetryStartUpdateCheckTimer|. If an error occurs while starting the
-  // timer it will be retried again in this function.
-  start_update_check_retry_timer_.reset();
-
   // For accuracy of the next update check, capture current time as close to the
   // start of this function as possible.
   const base::TimeTicks cur_ticks = GetTicksSinceBoot();
@@ -410,7 +425,7 @@ void DeviceScheduledUpdateChecker::StartUpdateCheckTimer() {
   base::Optional<base::Time> update_check_time =
       CalculateNextUpdateCheckTime(cur_time);
   if (!update_check_time) {
-    RetryStartUpdateCheckTimer();
+    start_update_check_timer_task_executor_.ScheduleRetry();
     return;
   }
   scheduled_update_check_data_->next_update_check_time_ticks =
@@ -434,58 +449,26 @@ void DeviceScheduledUpdateChecker::StartUpdateCheckTimer() {
 }
 
 void DeviceScheduledUpdateChecker::OnTimerStartResult(bool result) {
+  // Schedule a retry if |update_check_timer_| failed to start.
   if (!result) {
     LOG(ERROR) << "Failed to start update check timer";
-    // This method runs either due to |update_check_timer_|'s start operation
-    // failing or |start_update_check_timer_|'s start operation failing. In both
-    // cases it's called by |NativeTimer| and it's best to schedule
-    // |RetryStartUpdateCheckTimer| as a separate task as it destroys the same
-    // |NativeTimer| object inside it.
-    base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::BindOnce(
-            &DeviceScheduledUpdateChecker::RetryStartUpdateCheckTimer,
-            weak_factory_.GetWeakPtr()));
+    start_update_check_timer_task_executor_.ScheduleRetry();
     return;
   }
 }
 
-void DeviceScheduledUpdateChecker::RetryStartUpdateCheckTimer() {
+void DeviceScheduledUpdateChecker::OnStartUpdateCheckTimerRetryFailure() {
   // Retrying has a limit. In the unlikely scenario this is met, reset all
   // state. Now an update check can only happen when a new policy comes in or
   // Chrome is restarted.
-  if (update_check_timer_start_attempts_ >=
-      update_checker_internal::kMaxRetryUpdateCheckIterations) {
-    LOG(ERROR) << "Aborting attempts to start update check timer";
+  LOG(ERROR) << "Failed to start update check timer after all retries";
   ResetState();
-    return;
-  }
-
-  // There can only be one pending call to |StartUpdateCheckTimer| at any given
-  // time. For easier state maintenance, instantiate fresh timers for each
-  // retry attempt. The old timer must be destroyed before creating a new timer
-  // with the same tag as per the semantics of |NativeTimer|. That's why using
-  // std::make_unique with the assignment operator would not have worked here.
-  update_check_timer_.reset();
-  ++update_check_timer_start_attempts_;
-  start_update_check_retry_timer_.reset();
-  start_update_check_retry_timer_ =
-      std::make_unique<chromeos::NativeTimer>(kStartUpdateCheckRetryTimerTag);
-  start_update_check_retry_timer_->Start(
-      GetTicksSinceBoot() +
-          update_checker_internal::kStartUpdateCheckTimerRetryTime,
-      base::BindOnce(&DeviceScheduledUpdateChecker::StartUpdateCheckTimer,
-                     base::Unretained(this)),
-      base::BindOnce(&DeviceScheduledUpdateChecker::OnTimerStartResult,
-                     base::Unretained(this)));
 }
 
 void DeviceScheduledUpdateChecker::ResetState() {
-  weak_factory_.InvalidateWeakPtrs();
-  update_check_timer_start_attempts_ = 0;
-  start_update_check_retry_timer_.reset();
   update_check_timer_.reset();
   scheduled_update_check_data_ = base::nullopt;
+  start_update_check_timer_task_executor_.Stop();
 }
 
 base::Time DeviceScheduledUpdateChecker::GetCurrentTime() {

chrome/browser/chromeos/policy/device_scheduled_update_checker.h

@@ -11,6 +11,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/time/time.h"
+#include "chrome/browser/chromeos/policy/task_executor_with_retries.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/dbus/power/native_timer.h"
 
@@ -90,20 +91,21 @@ class DeviceScheduledUpdateChecker {
   // Callback triggered when scheduled update check setting has changed.
   void OnScheduledUpdateCheckDataChanged();
 
-  // Sets |update_check_timer_| based on |scheduled_update_check_data_|. If the
+  // Must only be run via |start_update_check_timer_task_executor_|. Sets
+  // |update_check_timer_| based on |scheduled_update_check_data_|. If the
   // |update_check_timer_| can't be started due to an error in
-  // |CalculateNextUpdateCheckTime| then reschedules itself to try again.
-  // Requires |scheduled_update_check_data_| to be set.
+  // |CalculateNextUpdateCheckTime| then reschedules itself via
+  // |start_update_check_timer_task_executor_|. Requires
+  // |scheduled_update_check_data_| to be set.
   void StartUpdateCheckTimer();
 
-  // Called upon starting |update_check_timer_| or
-  // |start_update_check_retry_timer_|. Indicates whether or not the timer was
-  // started successfully.
+  // Called upon starting |update_check_timer_|. Indicates whether or not the
+  // timer was started successfully.
   void OnTimerStartResult(bool result);
 
-  // Cancels any pending |StartUpdateCheckTimer| calls and reschedules it after
-  // a delay.
-  void RetryStartUpdateCheckTimer();
+  // Called when |start_update_check_timer_task_executor_|'s retry limit has
+  // been reached.
+  void OnStartUpdateCheckTimerRetryFailure();
 
   // Reset all state and cancel all pending tasks.
   void ResetState();
@@ -114,32 +116,22 @@ class DeviceScheduledUpdateChecker {
   // Returns time ticks from boot including time ticks spent during sleeping.
   virtual base::TimeTicks GetTicksSinceBoot();
 
-  // The number of attempts for which |update_check_timer_| has tried to be
-  // started.
-  int update_check_timer_start_attempts_ = 0;
-
   // Used to retrieve Chrome OS settings. Not owned.
   chromeos::CrosSettings* const cros_settings_;
 
+  // Used to observe when settings change.
   std::unique_ptr<chromeos::CrosSettings::ObserverSubscription>
       cros_settings_observer_;
 
   // Currently active scheduled update check policy.
   base::Optional<ScheduledUpdateCheckData> scheduled_update_check_data_;
 
+  // Used to run and retry |StartUpdateCheckTimer| if it fails.
+  TaskExecutorWithRetries start_update_check_timer_task_executor_;
+
   // Timer that is scheduled to check for updates.
   std::unique_ptr<chromeos::NativeTimer> update_check_timer_;
 
-  // Timer that retries |StartUpdateCheckTimer| in case it fails to start
-  // |update_check_timer_|. This needs to be suspend aware as well because the
-  // retry needs to be done before the time for the next update check and
-  // between that time the device maybe suspended indefinitely. Consequently, if
-  // this timer doesn't run in suspend then there is a chance to miss the next
-  // update check.
-  std::unique_ptr<chromeos::NativeTimer> start_update_check_retry_timer_;
-
-  base::WeakPtrFactory<DeviceScheduledUpdateChecker> weak_factory_{this};
-
   DISALLOW_COPY_AND_ASSIGN(DeviceScheduledUpdateChecker);
 };
 

chrome/browser/chromeos/policy/task_executor_with_retries.cc

+// Copyright 2019 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/policy/task_executor_with_retries.h"
+
+#include <utility>
+
+namespace policy {
+
+TaskExecutorWithRetries::TaskExecutorWithRetries(
+    const std::string& description,
+    GetTicksSinceBootFn get_ticks_since_boot_fn,
+    int max_retries,
+    base::TimeDelta retry_time)
+    : description_(description),
+      get_ticks_since_boot_fn_(std::move(get_ticks_since_boot_fn)),
+      max_retries_(max_retries),
+      retry_time_(retry_time) {}
+
+TaskExecutorWithRetries::~TaskExecutorWithRetries() = default;
+
+void TaskExecutorWithRetries::Start(AsyncTask task,
+                                    RetryFailureCb retry_failure_cb) {
+  ResetState();
+  task_ = std::move(task);
+  retry_failure_cb_ = std::move(retry_failure_cb);
+  task_.Run();
+}
+
+void TaskExecutorWithRetries::Stop() {
+  ResetState();
+}
+
+void TaskExecutorWithRetries::ScheduleRetry() {
+  // This method may be called from |retry_timer_|'s |OnExpiration| method. It's
+  // best to schedule |RetryTask| as a separate task as it destroys the same
+  // |NativeTimer| object inside it. For easier state maintenance there can only
+  // be one retry task pending at a time.
+  weak_factory_.InvalidateWeakPtrs();
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(&TaskExecutorWithRetries::RetryTask,
+                                weak_factory_.GetWeakPtr()));
+}
+
+void TaskExecutorWithRetries::OnRetryTimerStartResult(bool result) {
+  if (!result) {
+    LOG(ERROR) << "Failed to start retry timer";
+    ScheduleRetry();
+    return;
+  }
+}
+
+void TaskExecutorWithRetries::ResetState() {
+  num_retries_ = 0;
+  task_.Reset();
+  retry_failure_cb_.Reset();
+  retry_timer_.reset();
+  weak_factory_.InvalidateWeakPtrs();
+}
+
+void TaskExecutorWithRetries::RetryTask() {
+  // Retrying has a limit. In the unlikely scenario this is met, reset all
+  // state. Now an update check can only happen when a new policy comes in or
+  // Chrome is restarted.
+  if (num_retries_ >= max_retries_) {
+    LOG(ERROR) << "Task runner " << description_
+               << " aborting task after max retries";
+    std::move(retry_failure_cb_).Run();
+    ResetState();
+    return;
+  }
+
+  // There can only be one pending call to |task_| at any given time, delete any
+  // pending calls by resetting the timer. The old timer must be destroyed
+  // before creating a new timer with the same tag as per the semantics of
+  // |NativeTimer|. That's why using std::make_unique with the assignment
+  // operator would not have worked here. Safe to use "this" for the result
+  // callback as |retry_timer_| can't outlive its parent.
+  ++num_retries_;
+  retry_timer_.reset();
+  retry_timer_ = std::make_unique<chromeos::NativeTimer>(description_);
+  retry_timer_->Start(
+      get_ticks_since_boot_fn_.Run() + retry_time_, task_,
+      base::BindOnce(&TaskExecutorWithRetries::OnRetryTimerStartResult,
+                     base::Unretained(this)));
+}
+
+}  // namespace policy

chrome/browser/chromeos/policy/task_executor_with_retries.h

+// Copyright 2019 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_POLICY_TASK_EXECUTOR_WITH_RETRIES_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_TASK_EXECUTOR_WITH_RETRIES_H_
+
+#include <memory>
+#include <string>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+#include "chromeos/dbus/power/native_timer.h"
+
+namespace policy {
+
+// This class runs a task that can fail. In case of failure it retries the task
+// using NativeTimer i.e. the retry can wake the device up from suspend and is
+// suspend aware. Any callbacks passed to its API will not be invoked if an
+// object of this class is destroyed.
+class TaskExecutorWithRetries {
+ public:
+  using AsyncTask = base::RepeatingClosure;
+  using RetryFailureCb = base::OnceClosure;
+  using GetTicksSinceBootFn = base::RepeatingCallback<base::TimeTicks(void)>;
+
+  // |description| - String identifying this object.
+  // |get_ticks_since_boot_fn| - Callback that returns current ticks from boot.
+  // Used for scheduling retry timer.
+  // |max_retries| - Maximum number of retries after which trying the task is
+  // given up.
+  // |retry_time| - Time between each retry.
+  TaskExecutorWithRetries(const std::string& description,
+                          GetTicksSinceBootFn get_ticks_since_boot_fn,
+                          int max_retries,
+                          base::TimeDelta retry_time);
+  ~TaskExecutorWithRetries();
+
+  // Runs |task| and caches |retry_failure_cb| which will be called when
+  // |max_retries_| is reached and |task_| couldn't be run successfully.
+  // Consecutive calls override any state and pending callbacks associated with
+  // the previous call.
+  void Start(AsyncTask task, RetryFailureCb retry_failure_cb);
+
+  // Resets state and stops all pending callbacks.
+  void Stop();
+
+  // Cancels all outstanding |RetryTask| calls and schedules a new |RetryTask|
+  // call on the calling sequence.
+  void ScheduleRetry();
+
+ private:
+  // Called upon starting |retry_timer_|. Indicates whether or not the timer was
+  // started successfully.
+  void OnRetryTimerStartResult(bool result);
+
+  // Resets state including stopping all pending callbacks.
+  void ResetState();
+
+  // Starts |retry_timer_| to schedule |task_| at |retry_time_| interval.
+  void RetryTask();
+
+  // String identifying this object. Used with the |retry_timer_| API.
+  const std::string description_;
+
+  // Used to get current time ticks from boot.
+  const GetTicksSinceBootFn get_ticks_since_boot_fn_;
+
+  // Maximum number of retries after which trying the task is given up.
+  const int max_retries_;
+
+  // Time between each retry.
+  const base::TimeDelta retry_time_;
+
+  // Current retry iteration. Capped at |max_retries_|.
+  int num_retries_ = 0;
+
+  // The task to run.
+  AsyncTask task_;
+
+  // Callback to call after |max_retries_| have been reached and |task_| wasn't
+  // successfully scheduled.
+  RetryFailureCb retry_failure_cb_;
+
+  // Timer used to retry |task_|.
+  std::unique_ptr<chromeos::NativeTimer> retry_timer_;
+
+  base::WeakPtrFactory<TaskExecutorWithRetries> weak_factory_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(TaskExecutorWithRetries);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_TASK_EXECUTOR_WITH_RETRIES_H_