DeviceScheduledUpdateChecker: Perform update check on timer expiration

This change performs an update check when the update checker timer
expires.

BUG=924762
TEST=Unit tests.

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

chrome/browser/chromeos/BUILD.gn

@@ -1661,6 +1661,8 @@ source_set("chromeos") {
     "policy/off_hours/off_hours_policy_applier.h",
     "policy/off_hours/off_hours_proto_parser.cc",
     "policy/off_hours/off_hours_proto_parser.h",
+    "policy/os_and_policies_update_checker.cc",
+    "policy/os_and_policies_update_checker.h",
     "policy/policy_cert_service.cc",
     "policy/policy_cert_service.h",
     "policy/policy_cert_service_factory.cc",

chrome/browser/chromeos/policy/device_scheduled_update_checker.cc

@@ -44,9 +44,6 @@ base::Optional<base::Time> IncrementMonthAndSetDayOfMonth(
 
 namespace {
 
-// Number of days in a week.
-constexpr int kDaysInAWeek = 7;
-
 // The tag associated to register |update_check_timer_|.
 constexpr char kUpdateCheckTimerTag[] = "DeviceScheduledUpdateChecker";
 
@@ -54,21 +51,16 @@ constexpr char kUpdateCheckTimerTag[] = "DeviceScheduledUpdateChecker";
 constexpr char kStartUpdateCheckTimerTaskRunnerTag[] =
     "StartUpdateCheckTimerTaskRunner";
 
-DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::Frequency GetFrequency(
+DeviceScheduledUpdateChecker::Frequency GetFrequency(
     const std::string& frequency) {
-  if (frequency == "DAILY") {
-    return DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::Frequency::
-        kDaily;
-  }
+  if (frequency == "DAILY")
+    return DeviceScheduledUpdateChecker::Frequency::kDaily;
 
-  if (frequency == "WEEKLY") {
-    return DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::Frequency::
-        kWeekly;
-  }
+  if (frequency == "WEEKLY")
+    return DeviceScheduledUpdateChecker::Frequency::kWeekly;
 
   DCHECK_EQ(frequency, "MONTHLY");
-  return DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::Frequency::
-      kMonthly;
+  return DeviceScheduledUpdateChecker::Frequency::kMonthly;
 }
 
 // Convert the string day of week to an integer value suitable for
@@ -121,12 +113,10 @@ ParseScheduledUpdate(const base::Value* value) {
 
   // Parse extra fields for weekly and monthly frequencies.
   switch (result.frequency) {
-    case DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::Frequency::
-        kDaily:
+    case DeviceScheduledUpdateChecker::Frequency::kDaily:
       break;
 
-    case DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::Frequency::
-        kWeekly: {
+    case DeviceScheduledUpdateChecker::Frequency::kWeekly: {
       const std::string* day_of_week = value->FindStringKey({"day_of_week"});
       if (!day_of_week) {
         LOG(ERROR) << "Day of week missing";
@@ -138,8 +128,7 @@ ParseScheduledUpdate(const base::Value* value) {
       break;
     }
 
-    case DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::Frequency::
-        kMonthly: {
+    case DeviceScheduledUpdateChecker::Frequency::kMonthly: {
       base::Optional<int> day_of_month = value->FindIntKey({"day_of_month"});
       if (!day_of_month) {
         LOG(ERROR) << "Day of month missing";
@@ -172,7 +161,8 @@ base::Optional<base::Time> CalculateNextUpdateCheckWeeklyTime(
   base::TimeDelta delay_from_cur;
   if (day_of_week < cur_local_exploded_time.day_of_week) {
     delay_from_cur = base::TimeDelta::FromDays(
-        day_of_week + kDaysInAWeek - cur_local_exploded_time.day_of_week);
+        day_of_week + update_checker_internal::kDaysInAWeek -
+        cur_local_exploded_time.day_of_week);
   } else {
     delay_from_cur = base::TimeDelta::FromDays(
         day_of_week - cur_local_exploded_time.day_of_week);
@@ -203,7 +193,8 @@ base::Optional<base::Time> CalculateNextUpdateCheckWeeklyTime(
   // correct. Add observers for both those changes and then recalculate update
   // check time again.
   if (cur_time >= update_check_time)
-    update_check_time += base::TimeDelta::FromDays(kDaysInAWeek);
+    update_check_time +=
+        base::TimeDelta::FromDays(update_checker_internal::kDaysInAWeek);
 
   return update_check_time;
 }
@@ -265,8 +256,9 @@ 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.
+// Similarly, |start_update_check_timer_task_executor_| and
+// |os_and_policies_update_checker_| 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),
@@ -279,8 +271,11 @@ DeviceScheduledUpdateChecker::DeviceScheduledUpdateChecker(
           kStartUpdateCheckTimerTaskRunnerTag,
           base::BindRepeating(&DeviceScheduledUpdateChecker::GetTicksSinceBoot,
                               base::Unretained(this)),
-          update_checker_internal::kMaxRetryUpdateCheckIterations,
-          update_checker_internal::kStartUpdateCheckTimerRetryTime) {
+          update_checker_internal::kMaxStartUpdateCheckTimerRetryIterations,
+          update_checker_internal::kStartUpdateCheckTimerRetryTime),
+      os_and_policies_update_checker_(
+          base::BindRepeating(&DeviceScheduledUpdateChecker::GetTicksSinceBoot,
+                              base::Unretained(this))) {
   // Check if policy already exists.
   OnScheduledUpdateCheckDataChanged();
 }
@@ -294,28 +289,36 @@ DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::
 DeviceScheduledUpdateChecker::ScheduledUpdateCheckData::
     ~ScheduledUpdateCheckData() = default;
 
-void DeviceScheduledUpdateChecker::UpdateCheck() {
-  // TODO(crbug.com/924762): Add trigger to do the actual update check.
-  // If a policy exists, schedule the next update check timer.
-  if (!scheduled_update_check_data_)
-    return;
+void DeviceScheduledUpdateChecker::OnUpdateCheckTimerExpired() {
+  // If no policy exists, state should have been reset and this callback
+  // shouldn't have fired.
+  DCHECK(scheduled_update_check_data_);
 
-  // |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)));
+  // Following things needs to be done on every update check event. These will
+  // be done serially to make state management easier -
+  // - Check for download any updates.
+  // - TODO(crbug.com/924762)  Refresh policies.
+  // - Calculate and start the next update check timer.
+  //
+  // |os_and_policies_update_checker_| will be destroyed as part of this object,
+  // so it's safe to use "this" with any callbacks. This overrides any previous
+  // update check calls. Since, the minimum update frequency is daily there is
+  // very little chance of stepping on an existing update check that hasn't
+  // finished for a day. Timeouts will ensure that an update check completes,
+  // successfully or unsuccessfully, way before a day.
+  os_and_policies_update_checker_.Start(
+      base::BindOnce(&DeviceScheduledUpdateChecker::OnUpdateCheckCompletion,
+                     base::Unretained(this)));
 }
 
 void DeviceScheduledUpdateChecker::OnScheduledUpdateCheckDataChanged() {
-  // If the policy is removed then reset all state.
+  // If the policy is removed then reset all state including any existing update
+  // checks.
   const base::Value* value =
       cros_settings_->GetPref(chromeos::kDeviceScheduledUpdateCheck);
   if (!value) {
     ResetState();
+    os_and_policies_update_checker_.Stop();
     return;
   }
 
@@ -327,15 +330,10 @@ void DeviceScheduledUpdateChecker::OnScheduledUpdateCheckDataChanged() {
     LOG(ERROR) << "Failed to parse policy";
     return;
   }
-  scheduled_update_check_data_ = std::move(scheduled_update_check_data);
 
   // Policy has been updated, calculate and set |update_check_timer_| again.
-  start_update_check_timer_task_executor_.Start(
-      base::BindRepeating(&DeviceScheduledUpdateChecker::StartUpdateCheckTimer,
-                          base::Unretained(this)),
-      base::BindOnce(
-          &DeviceScheduledUpdateChecker::OnStartUpdateCheckTimerRetryFailure,
-          base::Unretained(this)));
+  scheduled_update_check_data_ = std::move(scheduled_update_check_data);
+  MaybeStartUpdateCheckTimer();
 }
 
 base::Optional<base::Time>
@@ -347,7 +345,7 @@ DeviceScheduledUpdateChecker::CalculateNextUpdateCheckTime(
   // time and then modify it based on the policy set.
   base::Time update_check_time;
   switch (scheduled_update_check_data_->frequency) {
-    case ScheduledUpdateCheckData::Frequency::kDaily: {
+    case DeviceScheduledUpdateChecker::Frequency::kDaily: {
       base::Time::Exploded update_check_local_exploded_time;
       cur_time.LocalExplode(&update_check_local_exploded_time);
       update_check_local_exploded_time.hour =
@@ -377,7 +375,7 @@ DeviceScheduledUpdateChecker::CalculateNextUpdateCheckTime(
       break;
     }
 
-    case ScheduledUpdateCheckData::Frequency::kWeekly: {
+    case DeviceScheduledUpdateChecker::Frequency::kWeekly: {
       DCHECK(scheduled_update_check_data_->day_of_week);
       base::Optional<base::Time> result = CalculateNextUpdateCheckWeeklyTime(
           cur_time, scheduled_update_check_data_->hour,
@@ -392,7 +390,7 @@ DeviceScheduledUpdateChecker::CalculateNextUpdateCheckTime(
       break;
     }
 
-    case ScheduledUpdateCheckData::Frequency::kMonthly: {
+    case DeviceScheduledUpdateChecker::Frequency::kMonthly: {
       DCHECK(scheduled_update_check_data_->day_of_month);
       base::Optional<base::Time> result = CalculateNextUpdateCheckMonthlyTime(
           cur_time, scheduled_update_check_data_->hour,
@@ -413,6 +411,8 @@ DeviceScheduledUpdateChecker::CalculateNextUpdateCheckTime(
 }
 
 void DeviceScheduledUpdateChecker::StartUpdateCheckTimer() {
+  DCHECK(scheduled_update_check_data_);
+
   // 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();
@@ -442,7 +442,7 @@ void DeviceScheduledUpdateChecker::StartUpdateCheckTimer() {
   // to use "this" while starting the timer.
   update_check_timer_->Start(
       scheduled_update_check_data_->next_update_check_time_ticks,
-      base::BindOnce(&DeviceScheduledUpdateChecker::UpdateCheck,
+      base::BindOnce(&DeviceScheduledUpdateChecker::OnUpdateCheckTimerExpired,
                      base::Unretained(this)),
       base::BindOnce(&DeviceScheduledUpdateChecker::OnTimerStartResult,
                      base::Unretained(this)));
@@ -458,13 +458,39 @@ void DeviceScheduledUpdateChecker::OnTimerStartResult(bool result) {
 }
 
 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.
+  // Retrying has a limit. In the unlikely scenario this is met, let an existing
+  // lingering update check from a previous iteration finish. Reset all other
+  // state including any callbacks. The next update check can only happen when
+  // a new policy comes in or Chrome is restarted.
   LOG(ERROR) << "Failed to start update check timer after all retries";
   ResetState();
 }
 
+void DeviceScheduledUpdateChecker::MaybeStartUpdateCheckTimer() {
+  // No need to start the next update check timer if the policy has been
+  // removed. This can happen if an update check was ongoing, a new policy came
+  // in but failed to start the timer which reset all state in
+  // |OnStartUpdateCheckTimerRetryFailure|.
+  if (!scheduled_update_check_data_)
+    return;
+
+  // Safe to use |this| as |start_update_check_timer_task_executor_| is a member
+  // of |this|.
+  start_update_check_timer_task_executor_.Start(
+      base::BindRepeating(&DeviceScheduledUpdateChecker::StartUpdateCheckTimer,
+                          base::Unretained(this)),
+      base::BindOnce(
+          &DeviceScheduledUpdateChecker::OnStartUpdateCheckTimerRetryFailure,
+          base::Unretained(this)));
+}
+
+void DeviceScheduledUpdateChecker::OnUpdateCheckCompletion(bool result) {
+  // Start the next update check timer irrespective of the current update check
+  // succeeding or not.
+  LOG_IF(ERROR, !result) << "Update check failed";
+  MaybeStartUpdateCheckTimer();
+}
+
 void DeviceScheduledUpdateChecker::ResetState() {
   update_check_timer_.reset();
   scheduled_update_check_data_ = base::nullopt;

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/os_and_policies_update_checker.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"
@@ -29,12 +30,15 @@ base::Optional<base::Time> IncrementMonthAndSetDayOfMonth(
 
 // The maximum iterations allowed to start an update check timer if the
 // operation fails.
-constexpr int kMaxRetryUpdateCheckIterations = 5;
+constexpr int kMaxStartUpdateCheckTimerRetryIterations = 5;
 
 // Time to call |StartUpdateCheckTimer| again in case it failed.
 constexpr base::TimeDelta kStartUpdateCheckTimerRetryTime =
     base::TimeDelta::FromMinutes(5);
 
+// Number of days in a week.
+constexpr int kDaysInAWeek = 7;
+
 }  // namespace update_checker_internal
 
 // This class listens for changes in the scheduled update check policy and then
@@ -44,18 +48,19 @@ class DeviceScheduledUpdateChecker {
   explicit DeviceScheduledUpdateChecker(chromeos::CrosSettings* cros_settings);
   virtual ~DeviceScheduledUpdateChecker();
 
+  // Frequency at which the update check should occur.
+  enum class Frequency {
+    kDaily,
+    kWeekly,
+    kMonthly,
+  };
+
   // Holds the data associated with the current scheduled update check policy.
   struct ScheduledUpdateCheckData {
     ScheduledUpdateCheckData();
     ScheduledUpdateCheckData(const ScheduledUpdateCheckData&);
     ~ScheduledUpdateCheckData();
 
-    enum class Frequency {
-      kDaily,
-      kWeekly,
-      kMonthly,
-    };
-
     int hour;
 
     int minute;
@@ -78,7 +83,7 @@ class DeviceScheduledUpdateChecker {
  protected:
   // Called when |update_check_timer_| fires. Triggers an update check and
   // schedules the next update check based on |scheduled_update_check_data_|.
-  virtual void UpdateCheck();
+  virtual void OnUpdateCheckTimerExpired();
 
   // Calculates next update check time based on |scheduled_update_check_data_|
   // and |cur_local_time|. Returns |base::nullopt| if calculation failed due to
@@ -87,6 +92,10 @@ class DeviceScheduledUpdateChecker {
   virtual base::Optional<base::Time> CalculateNextUpdateCheckTime(
       base::Time cur_local_time);
 
+  // Called when |os_and_policies_update_checker_| has finished successfully or
+  // unsuccessfully after retrying.
+  virtual void OnUpdateCheckCompletion(bool result);
+
  private:
   // Callback triggered when scheduled update check setting has changed.
   void OnScheduledUpdateCheckDataChanged();
@@ -107,7 +116,12 @@ class DeviceScheduledUpdateChecker {
   // been reached.
   void OnStartUpdateCheckTimerRetryFailure();
 
-  // Reset all state and cancel all pending tasks.
+  // Starts |start_update_check_timer_task_executor_| to run the next update
+  // check timer, via |StartUpdateCheckTimer|, only if a policy i.e.
+  // |scheduled_update_check_data_| is set.
+  void MaybeStartUpdateCheckTimer();
+
+  // Reset all state and cancel all pending tasks
   void ResetState();
 
   // Returns current time.
@@ -129,6 +143,9 @@ class DeviceScheduledUpdateChecker {
   // Used to run and retry |StartUpdateCheckTimer| if it fails.
   TaskExecutorWithRetries start_update_check_timer_task_executor_;
 
+  // Used to initiate update checks when |update_check_timer_| fires.
+  OsAndPoliciesUpdateChecker os_and_policies_update_checker_;
+
   // Timer that is scheduled to check for updates.
   std::unique_ptr<chromeos::NativeTimer> update_check_timer_;
 

chrome/browser/chromeos/policy/device_scheduled_update_checker_unittest.cc

@@ -13,10 +13,15 @@
 #include "base/strings/stringprintf.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/time/clock.h"
+#include "base/time/time.h"
 #include "base/values.h"
+#include "chrome/browser/chromeos/policy/os_and_policies_update_checker.h"
 #include "chrome/browser/chromeos/settings/scoped_testing_cros_settings.h"
 #include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_update_engine_client.h"
 #include "chromeos/dbus/power/fake_power_manager_client.h"
+#include "chromeos/dbus/update_engine_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace policy {
@@ -80,13 +85,13 @@ std::string ExplodedTimeDayOfWeekToStringDayOfWeek(int day_of_week) {
   return "SATURDAY";
 }
 
-// Increment |input_base_time| by |hours| and put the result in
-// |output_base_time| and |output_exploded_time|.
-void IncrementTimeByHours(const base::Time& input_time,
-                          int hours,
+// Increment |input_time| by |delta| and put the result in |output_base_time|
+// and |output_exploded_time|.
+void IncrementTimeByHours(base::Time input_time,
+                          base::TimeDelta delta,
                           base::Time* output_time,
                           base::Time::Exploded* output_exploded_time) {
-  *output_time = input_time + base::TimeDelta::FromHours(hours);
+  *output_time = input_time + delta;
   output_time->LocalExplode(output_exploded_time);
 }
 
@@ -103,16 +108,26 @@ class DeviceScheduledUpdateCheckerForTest
         tick_clock_(tick_clock) {}
   ~DeviceScheduledUpdateCheckerForTest() override = default;
 
-  int GetUpdateChecks() const { return update_checks_; }
+  int GetUpdateCheckTimerExpirations() const {
+    return update_check_timer_expirations_;
+  }
+
+  int GetUpdateCheckCompletions() const { return update_check_completions_; }
 
   void SimulateCalculateNextUpdateCheckFailure(bool simulate) {
     simulate_calculate_next_update_check_failure_ = simulate;
   }
 
  private:
-  void UpdateCheck() override {
-    ++update_checks_;
-    DeviceScheduledUpdateChecker::UpdateCheck();
+  void OnUpdateCheckTimerExpired() override {
+    ++update_check_timer_expirations_;
+    DeviceScheduledUpdateChecker::OnUpdateCheckTimerExpired();
+  }
+
+  void OnUpdateCheckCompletion(bool result) override {
+    if (result)
+      ++update_check_completions_;
+    DeviceScheduledUpdateChecker::OnUpdateCheckCompletion(result);
   }
 
   base::Optional<base::Time> CalculateNextUpdateCheckTime(
@@ -135,8 +150,11 @@ class DeviceScheduledUpdateCheckerForTest
   // Clock to use to calculate time ticks.
   const base::TickClock* const tick_clock_;
 
-  // Number of times |UpdateCheck| is called.
-  int update_checks_ = 0;
+  // Number of times |OnUpdateCheckTimerExpired| is called.
+  int update_check_timer_expirations_ = 0;
+
+  // Number of times |OnUpdateCheckCompletion| is called with |result| = true.
+  int update_check_completions_ = 0;
 
   // If set, then |CalculateNextUpdateCheckTime| returns base::nullopt.
   bool simulate_calculate_next_update_check_failure_ = false;
@@ -149,9 +167,16 @@ class DeviceScheduledUpdateCheckerTest : public testing::Test {
   DeviceScheduledUpdateCheckerTest()
       : scoped_task_environment_(
             base::test::ScopedTaskEnvironment::MainThreadType::IO_MOCK_TIME) {
+    auto fake_update_engine_client =
+        std::make_unique<chromeos::FakeUpdateEngineClient>();
+    fake_update_engine_client_ = fake_update_engine_client.get();
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetUpdateEngineClient(
+        std::move(fake_update_engine_client));
+
     chromeos::PowerManagerClient::InitializeFake();
     chromeos::FakePowerManagerClient::Get()->set_tick_clock(
         scoped_task_environment_.GetMockTickClock());
+
     device_scheduled_update_checker_ =
         std::make_unique<DeviceScheduledUpdateCheckerForTest>(
             chromeos::CrosSettings::Get(),
@@ -164,144 +189,264 @@ class DeviceScheduledUpdateCheckerTest : public testing::Test {
     chromeos::PowerManagerClient::Shutdown();
   }
 
+  // Notifies status update from |fake_update_engine_client_|.
+  void NotifyUpdateCheckStatus(
+      chromeos::UpdateEngineClient::UpdateStatusOperation
+          update_status_operation) {
+    chromeos::UpdateEngineClient::Status status = {};
+    status.status = update_status_operation;
+    fake_update_engine_client_->NotifyObserversThatStatusChanged(status);
+  }
+
+  // Returns true only iff all stats match in
+  // |device_scheduled_update_checker_|.
+  bool CheckStats(int expected_update_checks,
+                  int expected_update_check_requests,
+                  int expected_update_check_completions) {
+    if (device_scheduled_update_checker_->GetUpdateCheckTimerExpirations() !=
+        expected_update_checks) {
+      LOG(ERROR)
+          << "Current update check timer expirations: "
+          << device_scheduled_update_checker_->GetUpdateCheckTimerExpirations()
+          << " Expected update check timer expirations: "
+          << expected_update_checks;
+      return false;
+    }
+
+    if (fake_update_engine_client_->request_update_check_call_count() !=
+        expected_update_check_requests) {
+      LOG(ERROR)
+          << "Current update check requests: "
+          << fake_update_engine_client_->request_update_check_call_count()
+          << " Expected update check requests: "
+          << expected_update_check_requests;
+      return false;
+    }
+
+    if (device_scheduled_update_checker_->GetUpdateCheckCompletions() !=
+        expected_update_check_completions) {
+      LOG(ERROR)
+          << "Current update check completions: "
+          << device_scheduled_update_checker_->GetUpdateCheckCompletions()
+          << " Expected update check completions: "
+          << expected_update_check_completions;
+      return false;
+    }
+
+    return true;
+  }
+
   // Sets a daily update check policy and returns true iff it's scheduled
   // correctly. |hours_from_now| must be > 0.
   bool CheckDailyUpdateCheck(int hours_fom_now) {
     DCHECK_GT(hours_fom_now, 0);
     // Calculate time from one hour from now and set the update check policy to
     // happen daily at that time.
-    base::Time update_check_time;
-    base::Time::Exploded update_check_exploded_time;
-    const int increment_update_check_hours_by = hours_fom_now;
-    base::TimeDelta delay_from_now =
-        base::TimeDelta::FromHours(increment_update_check_hours_by);
-    IncrementTimeByHours(scoped_task_environment_.GetMockClock()->Now(),
-                         increment_update_check_hours_by, &update_check_time,
-                         &update_check_exploded_time);
-
-    base::Value scheduled_update_check_value;
-    DecodeJsonStringAndNormalize(
-        CreateDailyScheduledUpdateCheckPolicyJson(
-            update_check_exploded_time.hour, update_check_exploded_time.minute),
-        &scheduled_update_check_value);
+    base::TimeDelta delay_from_now = base::TimeDelta::FromHours(hours_fom_now);
+    auto policy_and_exploded_update_check_time = CreatePolicy(
+        delay_from_now, DeviceScheduledUpdateChecker::Frequency::kDaily);
 
     // Set a new scheduled update setting, fast forward to right before the
     // expected update and then check if an update check is not scheduled.
     const base::TimeDelta small_delay = base::TimeDelta::FromMilliseconds(1);
-    cros_settings_.device_settings()->Set(chromeos::kDeviceScheduledUpdateCheck,
-                                          scheduled_update_check_value);
-    int update_checks = device_scheduled_update_checker_->GetUpdateChecks();
+    cros_settings_.device_settings()->Set(
+        chromeos::kDeviceScheduledUpdateCheck,
+        std::move(policy_and_exploded_update_check_time.first));
+    int expected_update_checks =
+        device_scheduled_update_checker_->GetUpdateCheckTimerExpirations();
+    int expected_update_check_requests =
+        fake_update_engine_client_->request_update_check_call_count();
+    int expected_update_check_completions =
+        device_scheduled_update_checker_->GetUpdateCheckCompletions();
     scoped_task_environment_.FastForwardBy(delay_from_now - small_delay);
-    if (device_scheduled_update_checker_->GetUpdateChecks() != update_checks)
+    if (!CheckStats(expected_update_checks, expected_update_check_requests,
+                    expected_update_check_completions)) {
       return false;
+    }
 
     // Fast forward to the expected update check time and then check if the
     // update check is scheduled.
-    update_checks = device_scheduled_update_checker_->GetUpdateChecks();
+    expected_update_checks += 1;
+    expected_update_check_requests += 1;
+    expected_update_check_completions += 1;
     scoped_task_environment_.FastForwardBy(small_delay);
-    if (device_scheduled_update_checker_->GetUpdateChecks() !=
-        (update_checks + 1)) {
+
+    // Simulate update check succeeding.
+    NotifyUpdateCheckStatus(
+        chromeos::UpdateEngineClient::UpdateStatusOperation::
+            UPDATE_STATUS_UPDATED_NEED_REBOOT);
+    if (!CheckStats(expected_update_checks, expected_update_check_requests,
+                    expected_update_check_completions)) {
       return false;
     }
 
     // An update check should happen every day since the policy is set to daily.
-    update_checks = device_scheduled_update_checker_->GetUpdateChecks();
     const int days = 5;
-    scoped_task_environment_.FastForwardBy(base::TimeDelta::FromDays(days));
-    if (device_scheduled_update_checker_->GetUpdateChecks() !=
-        (update_checks + days)) {
-      return false;
+    for (int i = 0; i < days; i++) {
+      expected_update_checks += 1;
+      expected_update_check_requests += 1;
+      expected_update_check_completions += 1;
+      scoped_task_environment_.FastForwardBy(base::TimeDelta::FromDays(1));
+
+      // Simulate update check succeeding.
+      NotifyUpdateCheckStatus(
+          chromeos::UpdateEngineClient::UpdateStatusOperation::
+              UPDATE_STATUS_UPDATED_NEED_REBOOT);
+      if (!CheckStats(expected_update_checks, expected_update_check_requests,
+                      expected_update_check_completions)) {
+        return false;
+      }
     }
 
     return true;
   }
 
+  // Creates an update check policy starting at a delay of |delta| from now and
+  // recurring with frequency |frequency|.
+  std::pair<base::Value, base::Time::Exploded> CreatePolicy(
+      base::TimeDelta delta,
+      DeviceScheduledUpdateChecker::Frequency frequency) {
+    // Calculate time from one hour from now and set the update check policy to
+    // happen daily at that time.
+    base::Time update_check_time;
+    base::Time::Exploded update_check_exploded_time;
+    IncrementTimeByHours(scoped_task_environment_.GetMockClock()->Now(), delta,
+                         &update_check_time, &update_check_exploded_time);
+
+    base::Value scheduled_update_check_value;
+    switch (frequency) {
+      case DeviceScheduledUpdateChecker::Frequency::kDaily: {
+        DecodeJsonStringAndNormalize(CreateDailyScheduledUpdateCheckPolicyJson(
+                                         update_check_exploded_time.hour,
+                                         update_check_exploded_time.minute),
+                                     &scheduled_update_check_value);
+        break;
+      }
+
+      case DeviceScheduledUpdateChecker::Frequency::kWeekly: {
+        DecodeJsonStringAndNormalize(
+            CreateWeeklyScheduledUpdateCheckPolicyJson(
+                update_check_exploded_time.hour,
+                update_check_exploded_time.minute,
+                ExplodedTimeDayOfWeekToStringDayOfWeek(
+                    update_check_exploded_time.day_of_week)),
+            &scheduled_update_check_value);
+        break;
+      }
+
+      case DeviceScheduledUpdateChecker::Frequency::kMonthly: {
+        DecodeJsonStringAndNormalize(
+            CreateMonthlyScheduledUpdateCheckPolicyJson(
+                update_check_exploded_time.hour,
+                update_check_exploded_time.minute,
+                update_check_exploded_time.day_of_month),
+            &scheduled_update_check_value);
+        break;
+      }
+    }
+    return std::make_pair(std::move(scheduled_update_check_value),
+                          std::move(update_check_exploded_time));
+  }
+
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<DeviceScheduledUpdateCheckerForTest>
       device_scheduled_update_checker_;
   chromeos::ScopedTestingCrosSettings cros_settings_;
+  chromeos::FakeUpdateEngineClient* fake_update_engine_client_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceScheduledUpdateCheckerTest);
 };
 
 TEST_F(DeviceScheduledUpdateCheckerTest, CheckIfDailyUpdateCheckIsScheduled) {
-  EXPECT_TRUE(CheckDailyUpdateCheck(1 /* hours_from_now */));
+  // Check if back to back policies succeed.
+  for (int i = 1; i <= 10; i++)
+    EXPECT_TRUE(CheckDailyUpdateCheck(i));
 }
 
 TEST_F(DeviceScheduledUpdateCheckerTest, CheckIfWeeklyUpdateCheckIsScheduled) {
   // Set the first update check to happen 49 hours from now (i.e. 1 hour from 2
   // days from now) and then weekly after.
-  base::Time update_check_time;
-  base::Time::Exploded update_check_exploded_time;
-  const int increment_update_check_hours_by = 49;
-  base::TimeDelta delay_from_now =
-      base::TimeDelta::FromHours(increment_update_check_hours_by);
-  IncrementTimeByHours(scoped_task_environment_.GetMockClock()->Now(),
-                       increment_update_check_hours_by, &update_check_time,
-                       &update_check_exploded_time);
-
-  base::Value scheduled_update_check_value;
-  ASSERT_NO_FATAL_FAILURE(DecodeJsonStringAndNormalize(
-      CreateWeeklyScheduledUpdateCheckPolicyJson(
-          update_check_exploded_time.hour, update_check_exploded_time.minute,
-          ExplodedTimeDayOfWeekToStringDayOfWeek(
-              update_check_exploded_time.day_of_week)),
-      &scheduled_update_check_value));
+  base::TimeDelta delay_from_now = base::TimeDelta::FromHours(49);
+  auto policy_and_exploded_update_check_time = CreatePolicy(
+      delay_from_now, DeviceScheduledUpdateChecker::Frequency::kWeekly);
 
   // Set a new scheduled update setting, fast forward to right before the
   // expected update and then check if an update check is not scheduled.
+  int expected_update_checks = 0;
+  int expected_update_check_requests = 0;
+  int expected_update_check_completions = 0;
   const base::TimeDelta small_delay = base::TimeDelta::FromMilliseconds(1);
-  cros_settings_.device_settings()->Set(chromeos::kDeviceScheduledUpdateCheck,
-                                        scheduled_update_check_value);
+  cros_settings_.device_settings()->Set(
+      chromeos::kDeviceScheduledUpdateCheck,
+      std::move(policy_and_exploded_update_check_time.first));
   scoped_task_environment_.FastForwardBy(delay_from_now - small_delay);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 0);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
 
   // Fast forward to the expected update check time and then check if the update
   // check is scheduled.
+  expected_update_checks += 1;
+  expected_update_check_requests += 1;
+  expected_update_check_completions += 1;
   scoped_task_environment_.FastForwardBy(small_delay);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 1);
+  // Simulate update check succeeding.
+  NotifyUpdateCheckStatus(chromeos::UpdateEngineClient::UpdateStatusOperation::
+                              UPDATE_STATUS_UPDATED_NEED_REBOOT);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
 
   // An update check should happen weekly since the policy is set to weekly.
+  expected_update_checks += 1;
+  expected_update_check_requests += 1;
+  expected_update_check_completions += 1;
   scoped_task_environment_.FastForwardBy(base::TimeDelta::FromDays(7));
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 2);
+  // Simulate update check succeeding.
+  NotifyUpdateCheckStatus(chromeos::UpdateEngineClient::UpdateStatusOperation::
+                              UPDATE_STATUS_UPDATED_NEED_REBOOT);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
 }
 
 TEST_F(DeviceScheduledUpdateCheckerTest, CheckIfMonthlyUpdateCheckIsScheduled) {
   // Set the first update check to happen 49 hours from now (i.e. 1 hour from 2
   // days from now) and then monthly after.
-  base::Time update_check_time;
-  base::Time::Exploded update_check_exploded_time;
-  const int increment_update_check_hours_by = 49;
-  base::TimeDelta delay_from_now =
-      base::TimeDelta::FromHours(increment_update_check_hours_by);
-  IncrementTimeByHours(scoped_task_environment_.GetMockClock()->Now(),
-                       increment_update_check_hours_by, &update_check_time,
-                       &update_check_exploded_time);
-
-  base::Value scheduled_update_check_value;
-  ASSERT_NO_FATAL_FAILURE(DecodeJsonStringAndNormalize(
-      CreateMonthlyScheduledUpdateCheckPolicyJson(
-          update_check_exploded_time.hour, update_check_exploded_time.minute,
-          update_check_exploded_time.day_of_month),
-      &scheduled_update_check_value));
+  base::TimeDelta delay_from_now = base::TimeDelta::FromHours(49);
+  auto policy_and_exploded_update_check_time = CreatePolicy(
+      delay_from_now, DeviceScheduledUpdateChecker::Frequency::kMonthly);
 
   // Set a new scheduled update setting, fast forward to right before the
   // expected update and then check if an update check is not scheduled.
+  int expected_update_checks = 0;
+  int expected_update_check_requests = 0;
+  int expected_update_check_completions = 0;
   const base::TimeDelta small_delay = base::TimeDelta::FromMilliseconds(1);
-  cros_settings_.device_settings()->Set(chromeos::kDeviceScheduledUpdateCheck,
-                                        scheduled_update_check_value);
+  cros_settings_.device_settings()->Set(
+      chromeos::kDeviceScheduledUpdateCheck,
+      std::move(policy_and_exploded_update_check_time.first));
   scoped_task_environment_.FastForwardBy(delay_from_now - small_delay);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 0);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
 
   // Fast forward to the expected update check time and then check if the update
   // check is scheduled.
+  expected_update_checks += 1;
+  expected_update_check_requests += 1;
+  expected_update_check_completions += 1;
   scoped_task_environment_.FastForwardBy(small_delay);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 1);
+  // Simulate update check succeeding.
+  NotifyUpdateCheckStatus(chromeos::UpdateEngineClient::UpdateStatusOperation::
+                              UPDATE_STATUS_UPDATED_NEED_REBOOT);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
 
   // The next update check should happen at the same day of month next month.
+  expected_update_checks += 1;
+  expected_update_check_requests += 1;
+  expected_update_check_completions += 1;
   base::Optional<base::Time> next_check_time =
       update_checker_internal::IncrementMonthAndSetDayOfMonth(
-          update_check_exploded_time, update_check_exploded_time.day_of_month);
+          policy_and_exploded_update_check_time.second,
+          policy_and_exploded_update_check_time.second.day_of_month);
   // This should be always set in a virtual time environment.
   EXPECT_TRUE(next_check_time);
   delay_from_now =
@@ -309,7 +454,11 @@ TEST_F(DeviceScheduledUpdateCheckerTest, CheckIfMonthlyUpdateCheckIsScheduled) {
   base::Time::Exploded next_check_exploded_time;
   next_check_time->LocalExplode(&next_check_exploded_time);
   scoped_task_environment_.FastForwardBy(delay_from_now);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 2);
+  // Simulate update check succeeding.
+  NotifyUpdateCheckStatus(chromeos::UpdateEngineClient::UpdateStatusOperation::
+                              UPDATE_STATUS_UPDATED_NEED_REBOOT);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
 }
 
 // Checks if an update check timer can't be started, retries are scheduled to
@@ -322,27 +471,23 @@ TEST_F(DeviceScheduledUpdateCheckerTest, CheckRetryLogicEventualSuccess) {
 
   // Calculate time from one hour from now and set the update check policy to
   // happen daily at that time.
-  base::Time update_check_time;
-  base::Time::Exploded update_check_exploded_time;
-  const int increment_update_check_hours_by = 1;
-  IncrementTimeByHours(scoped_task_environment_.GetMockClock()->Now(),
-                       increment_update_check_hours_by, &update_check_time,
-                       &update_check_exploded_time);
-
-  base::Value scheduled_update_check_value;
-  ASSERT_NO_FATAL_FAILURE(DecodeJsonStringAndNormalize(
-      CreateDailyScheduledUpdateCheckPolicyJson(
-          update_check_exploded_time.hour, update_check_exploded_time.minute),
-      &scheduled_update_check_value));
+  base::TimeDelta delay_from_now = base::TimeDelta::FromHours(1);
+  auto policy_and_exploded_update_check_time = CreatePolicy(
+      delay_from_now, DeviceScheduledUpdateChecker::Frequency::kDaily);
 
   // Fast forward time by less than (max retries * retry period) and check that
   // no update has occurred due to failure being simulated.
-  cros_settings_.device_settings()->Set(chromeos::kDeviceScheduledUpdateCheck,
-                                        scheduled_update_check_value);
+  int expected_update_checks = 0;
+  int expected_update_check_requests = 0;
+  int expected_update_check_completions = 0;
+  cros_settings_.device_settings()->Set(
+      chromeos::kDeviceScheduledUpdateCheck,
+      std::move(policy_and_exploded_update_check_time.first));
   scoped_task_environment_.FastForwardBy(
-      (update_checker_internal::kMaxRetryUpdateCheckIterations - 2) *
+      (update_checker_internal::kMaxStartUpdateCheckTimerRetryIterations - 2) *
       update_checker_internal::kStartUpdateCheckTimerRetryTime);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 0);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
 
   // Reset failure mode and fast forward by the retry period. This time it
   // should succeed in setting an update check timer. No update checks should
@@ -351,12 +496,24 @@ TEST_F(DeviceScheduledUpdateCheckerTest, CheckRetryLogicEventualSuccess) {
       false);
   scoped_task_environment_.FastForwardBy(
       update_checker_internal::kStartUpdateCheckTimerRetryTime);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 0);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
 
   // Check if update checks happen daily from now on.
   const int days = 2;
-  scoped_task_environment_.FastForwardBy(base::TimeDelta::FromDays(days));
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), days);
+  for (int i = 0; i < days; i++) {
+    expected_update_checks += 1;
+    expected_update_check_requests += 1;
+    expected_update_check_completions += 1;
+    scoped_task_environment_.FastForwardBy(base::TimeDelta::FromDays(1));
+    // Simulate update check succeeding.
+    NotifyUpdateCheckStatus(
+        chromeos::UpdateEngineClient::UpdateStatusOperation::
+            UPDATE_STATUS_UPDATED_NEED_REBOOT);
+    EXPECT_TRUE(CheckStats(expected_update_checks,
+                           expected_update_check_requests,
+                           expected_update_check_completions));
+  }
 }
 
 // Checks if an update check timer can't be started due to a calculation
@@ -372,9 +529,11 @@ TEST_F(DeviceScheduledUpdateCheckerTest,
   // Fast forward by max retries * retry period and check that no update has
   // happened since failure mode is still set.
   scoped_task_environment_.FastForwardBy(
-      update_checker_internal::kMaxRetryUpdateCheckIterations *
+      update_checker_internal::kMaxStartUpdateCheckTimerRetryIterations *
       update_checker_internal::kStartUpdateCheckTimerRetryTime);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 0);
+  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateCheckTimerExpirations(),
+            0);
+  EXPECT_EQ(fake_update_engine_client_->request_update_check_call_count(), 0);
 
   // At this point all state has been reset. Reset failure mode and check if
   // daily update checks happen.
@@ -396,9 +555,10 @@ TEST_F(DeviceScheduledUpdateCheckerTest,
   // Fast forward by max retries * retry period and check that no update has
   // happened since failure mode is still set.
   scoped_task_environment_.FastForwardBy(
-      update_checker_internal::kMaxRetryUpdateCheckIterations *
+      update_checker_internal::kMaxStartUpdateCheckTimerRetryIterations *
       update_checker_internal::kStartUpdateCheckTimerRetryTime);
-  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateChecks(), 0);
+  EXPECT_EQ(device_scheduled_update_checker_->GetUpdateCheckTimerExpirations(),
+            0);
 
   // At this point all state has been reset. Reset failure mode and check if
   // daily update checks happen.
@@ -407,4 +567,165 @@ TEST_F(DeviceScheduledUpdateCheckerTest,
   EXPECT_TRUE(CheckDailyUpdateCheck(1 /* hours_from_now */));
 }
 
+// Checks when an update check is unsuccessful retries are scheduled.
+TEST_F(DeviceScheduledUpdateCheckerTest, CheckRetryLogicUpdateCheckFailure) {
+  // Set the first update check to happen 49 hours from now (i.e. 1 hour from 2
+  // days from now) and then weekly after.
+  base::TimeDelta delay_from_now = base::TimeDelta::FromHours(1);
+  auto policy_and_exploded_update_check_time = CreatePolicy(
+      delay_from_now, DeviceScheduledUpdateChecker::Frequency::kWeekly);
+
+  // Set a new scheduled update setting, fast forward to expected update check
+  // time and check if it happpens. Update check completion shouldn't happen as
+  // an error is simulated.
+  cros_settings_.device_settings()->Set(
+      chromeos::kDeviceScheduledUpdateCheck,
+      std::move(policy_and_exploded_update_check_time.first));
+  int expected_update_checks = 1;
+  int expected_update_check_requests = 1;
+  int expected_update_check_completions = 0;
+  scoped_task_environment_.FastForwardBy(delay_from_now);
+  NotifyUpdateCheckStatus(
+      chromeos::UpdateEngineClient::UpdateStatusOperation::UPDATE_STATUS_ERROR);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
+
+  // Fast forward for (max retries allowed) and check if each retry increases
+  // the update check requests while we simulate an error.
+  for (int i = 0;
+       i <
+       update_checker_internal::kMaxOsAndPoliciesUpdateCheckerRetryIterations;
+       i++) {
+    expected_update_check_requests += 1;
+    scoped_task_environment_.FastForwardBy(
+        update_checker_internal::kOsAndPoliciesUpdateCheckerRetryTime);
+    // Simulate update check failing.
+    NotifyUpdateCheckStatus(chromeos::UpdateEngineClient::
+                                UpdateStatusOperation::UPDATE_STATUS_ERROR);
+    EXPECT_TRUE(CheckStats(expected_update_checks,
+                           expected_update_check_requests,
+                           expected_update_check_completions));
+  }
+
+  // No retries should be scheduled till the next update check timer fires. Fast
+  // forward to just before the timer firing and check.
+  const base::TimeDelta delay_till_next_update_check_timer =
+      base::TimeDelta::FromDays(update_checker_internal::kDaysInAWeek) -
+      (update_checker_internal::kMaxOsAndPoliciesUpdateCheckerRetryIterations *
+       update_checker_internal::kOsAndPoliciesUpdateCheckerRetryTime);
+  const base::TimeDelta small_delay = base::TimeDelta::FromMilliseconds(1);
+  scoped_task_environment_.FastForwardBy(delay_till_next_update_check_timer -
+                                         small_delay);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
+
+  // Check if the next update check timer fires and an update check is
+  // initiated.
+  expected_update_checks += 1;
+  expected_update_check_requests += 1;
+  scoped_task_environment_.FastForwardBy(small_delay);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
+}
+
+// Checks if an update check is successful after retries.
+TEST_F(DeviceScheduledUpdateCheckerTest,
+       CheckUpdateCheckFailureEventualSuccess) {
+  // Set the first update check to happen 49 hours from now (i.e. 1 hour from 2
+  // days from now) and then weekly after.
+  base::TimeDelta delay_from_now = base::TimeDelta::FromHours(49);
+  auto policy_and_exploded_update_check_time = CreatePolicy(
+      delay_from_now, DeviceScheduledUpdateChecker::Frequency::kWeekly);
+
+  // Set a new scheduled update setting, fast forward to expected update check
+  // time and check if it happpens. Update check completion shouldn't happen as
+  // an error is simulated.
+  cros_settings_.device_settings()->Set(
+      chromeos::kDeviceScheduledUpdateCheck,
+      std::move(policy_and_exploded_update_check_time.first));
+  int expected_update_checks = 1;
+  int expected_update_check_requests = 1;
+  int expected_update_check_completions = 0;
+  scoped_task_environment_.FastForwardBy(delay_from_now);
+  // Simulate update check succeeding.
+  NotifyUpdateCheckStatus(
+      chromeos::UpdateEngineClient::UpdateStatusOperation::UPDATE_STATUS_ERROR);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
+
+  // Fast forward for (max retries allowed - 1) and check if each retry
+  // increases the update check requests while we simulate an error.
+  for (int i = 0;
+       i <
+       (update_checker_internal::kMaxOsAndPoliciesUpdateCheckerRetryIterations -
+        1);
+       i++) {
+    expected_update_check_requests += 1;
+    scoped_task_environment_.FastForwardBy(
+        update_checker_internal::kOsAndPoliciesUpdateCheckerRetryTime);
+    NotifyUpdateCheckStatus(chromeos::UpdateEngineClient::
+                                UpdateStatusOperation::UPDATE_STATUS_ERROR);
+    EXPECT_TRUE(CheckStats(expected_update_checks,
+                           expected_update_check_requests,
+                           expected_update_check_completions));
+  }
+
+  // Simulate success on the last retry attempt. This time the update check
+  // should complete.
+  expected_update_check_requests += 1;
+  expected_update_check_completions += 1;
+  scoped_task_environment_.FastForwardBy(
+      update_checker_internal::kOsAndPoliciesUpdateCheckerRetryTime);
+  NotifyUpdateCheckStatus(chromeos::UpdateEngineClient::UpdateStatusOperation::
+                              UPDATE_STATUS_UPDATED_NEED_REBOOT);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
+}
+
+// Checks if an update check timer can't be started, any previous pending update
+// completion will still be completed.
+TEST_F(DeviceScheduledUpdateCheckerTest,
+       CheckPreviousUpdateCheckCompletionWithTimerStartFailure) {
+  // Calculate time from one hour from now and set the update check policy to
+  // happen daily at that time.
+  base::TimeDelta delay_from_now = base::TimeDelta::FromHours(1);
+  auto policy_and_exploded_update_check_time = CreatePolicy(
+      delay_from_now, DeviceScheduledUpdateChecker::Frequency::kDaily);
+
+  // Set a new scheduled update setting, fast forward to the expected time and
+  // and then check if an update check is scheduled.
+  cros_settings_.device_settings()->Set(
+      chromeos::kDeviceScheduledUpdateCheck,
+      std::move(policy_and_exploded_update_check_time.first));
+  int expected_update_checks = 1;
+  int expected_update_check_requests = 1;
+  int expected_update_check_completions = 0;
+  scoped_task_environment_.FastForwardBy(delay_from_now);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
+
+  // Simulate timer start failure for the next time.
+  chromeos::FakePowerManagerClient::Get()->simulate_start_arc_timer_failure(
+      true);
+
+  // Set a new scheduled update setting, this will fail to schedule the next
+  // update check timer. However, it should still allow the previous update
+  // check to finish.
+  expected_update_check_completions += 1;
+  delay_from_now = base::TimeDelta::FromHours(2);
+  policy_and_exploded_update_check_time = CreatePolicy(
+      delay_from_now, DeviceScheduledUpdateChecker::Frequency::kDaily);
+  cros_settings_.device_settings()->Set(
+      chromeos::kDeviceScheduledUpdateCheck,
+      std::move(policy_and_exploded_update_check_time.first));
+  scoped_task_environment_.FastForwardBy(
+      update_checker_internal::kMaxStartUpdateCheckTimerRetryIterations *
+      update_checker_internal::kStartUpdateCheckTimerRetryTime);
+  // Simulate update check succeeding.
+  NotifyUpdateCheckStatus(chromeos::UpdateEngineClient::UpdateStatusOperation::
+                              UPDATE_STATUS_UPDATED_NEED_REBOOT);
+  EXPECT_TRUE(CheckStats(expected_update_checks, expected_update_check_requests,
+                         expected_update_check_completions));
+}
+
 }  // namespace policy

chrome/browser/chromeos/policy/os_and_policies_update_checker.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/os_and_policies_update_checker.h"
+
+#include <utility>
+
+#include "chrome/browser/chromeos/policy/task_executor_with_retries.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/update_engine_client.h"
+
+namespace policy {
+
+namespace {
+
+// The tag associated to register |update_check_task_executor_|.
+constexpr char kUpdateCheckTaskExecutorTag[] = "UpdateCheckTaskExecutor";
+
+}  // namespace
+
+OsAndPoliciesUpdateChecker::OsAndPoliciesUpdateChecker(
+    TaskExecutorWithRetries::GetTicksSinceBootFn get_ticks_since_boot_fn)
+    : update_check_task_executor_(
+          kUpdateCheckTaskExecutorTag,
+          std::move(get_ticks_since_boot_fn),
+          update_checker_internal::
+              kMaxOsAndPoliciesUpdateCheckerRetryIterations,
+          update_checker_internal::kOsAndPoliciesUpdateCheckerRetryTime) {}
+
+OsAndPoliciesUpdateChecker::~OsAndPoliciesUpdateChecker() = default;
+
+void OsAndPoliciesUpdateChecker::Start(UpdateCheckCompletionCallback cb) {
+  // Override any previous calls by resetting state.
+  ResetState();
+  // Must be set before starting the task runner, as callbacks may be called
+  // synchronously.
+  update_check_completion_cb_ = std::move(cb);
+  // Safe to use "this" as |update_check_task_executor_| is a member of this
+  // class.
+  update_check_task_executor_.Start(
+      base::BindRepeating(&OsAndPoliciesUpdateChecker::StartUpdateCheck,
+                          base::Unretained(this)),
+      base::BindOnce(&OsAndPoliciesUpdateChecker::OnUpdateCheckFailure,
+                     base::Unretained(this)));
+}
+
+void OsAndPoliciesUpdateChecker::Stop() {
+  ResetState();
+}
+
+void OsAndPoliciesUpdateChecker::OnUpdateCheckFailure() {
+  RunCompletionCallbackAndResetState(false);
+}
+
+void OsAndPoliciesUpdateChecker::RunCompletionCallbackAndResetState(
+    bool result) {
+  if (update_check_completion_cb_)
+    std::move(update_check_completion_cb_).Run(result);
+  ResetState();
+}
+
+void OsAndPoliciesUpdateChecker::StartUpdateCheck() {
+  // Only one update check can be pending at any time.
+  weak_factory_.InvalidateWeakPtrs();
+
+  // This could be a retry, reset observer state as adding observers is not
+  // idempotent.
+  chromeos::DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(
+      this);
+
+  // Register observer to keep track of different stages of the update check.
+  chromeos::DBusThreadManager::Get()->GetUpdateEngineClient()->AddObserver(
+      this);
+  chromeos::DBusThreadManager::Get()
+      ->GetUpdateEngineClient()
+      ->RequestUpdateCheck(
+          base::BindRepeating(&OsAndPoliciesUpdateChecker::OnUpdateCheckStarted,
+                              weak_factory_.GetWeakPtr()));
+
+  // Do nothing for the initial idle stage when the update check state machine
+  // has just started.
+  ignore_idle_status_ = true;
+}
+
+void OsAndPoliciesUpdateChecker::UpdateStatusChanged(
+    const chromeos::UpdateEngineClient::Status& status) {
+  // Only ignore idle state if it is the first and only non-error state in the
+  // state machine.
+  if (ignore_idle_status_ &&
+      status.status > chromeos::UpdateEngineClient::UPDATE_STATUS_IDLE) {
+    ignore_idle_status_ = false;
+  }
+
+  switch (status.status) {
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_IDLE:
+      // Exit update only if update engine was in non-idle status before i.e.
+      // no update to download. Otherwise, it's possible that the update request
+      // has not yet been started.
+      if (!ignore_idle_status_)
+        RunCompletionCallbackAndResetState(true);
+      break;
+
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_UPDATED_NEED_REBOOT:
+      RunCompletionCallbackAndResetState(true);
+      break;
+
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_ERROR:
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_NEED_PERMISSION_TO_UPDATE:
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_REPORTING_ERROR_EVENT:
+      update_check_task_executor_.ScheduleRetry();
+      break;
+
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_FINALIZING:
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_VERIFYING:
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_DOWNLOADING:
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_UPDATE_AVAILABLE:
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_CHECKING_FOR_UPDATE:
+    case chromeos::UpdateEngineClient::UpdateStatusOperation::
+        UPDATE_STATUS_ATTEMPTING_ROLLBACK:
+      // Do nothing on intermediate states.
+      break;
+  }
+}
+
+void OsAndPoliciesUpdateChecker::OnUpdateCheckStarted(
+    chromeos::UpdateEngineClient::UpdateCheckResult result) {
+  switch (result) {
+    case chromeos::UpdateEngineClient::UPDATE_RESULT_SUCCESS:
+      // Nothing to do if the update check started successfully.
+      break;
+    case chromeos::UpdateEngineClient::UPDATE_RESULT_FAILED:
+      update_check_task_executor_.ScheduleRetry();
+      break;
+    case chromeos::UpdateEngineClient::UPDATE_RESULT_NOTIMPLEMENTED:
+      // No point retrying if the operation is not implemented. This should
+      // never happen.
+      LOG(ERROR) << "Operation not implemented";
+      RunCompletionCallbackAndResetState(false);
+      break;
+  }
+}
+
+void OsAndPoliciesUpdateChecker::ResetState() {
+  weak_factory_.InvalidateWeakPtrs();
+  chromeos::DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(
+      this);
+  update_check_task_executor_.Stop();
+  ignore_idle_status_ = true;
+}
+
+}  // namespace policy

chrome/browser/chromeos/policy/os_and_policies_update_checker.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_OS_AND_POLICIES_UPDATE_CHECKER_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_OS_AND_POLICIES_UPDATE_CHECKER_H_
+
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+#include "chrome/browser/chromeos/policy/task_executor_with_retries.h"
+#include "chromeos/dbus/power/native_timer.h"
+#include "chromeos/dbus/update_engine_client.h"
+
+namespace policy {
+
+namespace update_checker_internal {
+
+// The maximum iterations allowed to check for and download an update if the
+// operation fails. Used with |os_and_policies_update_checker_|.
+constexpr int kMaxOsAndPoliciesUpdateCheckerRetryIterations = 2;
+
+// Interval at which |os_and_policies_update_checker_| retries checking for and
+// downloading updates.
+constexpr base::TimeDelta kOsAndPoliciesUpdateCheckerRetryTime =
+    base::TimeDelta::FromMinutes(30);
+
+}  // namespace update_checker_internal
+
+// This class is used by the scheduled update check policy to perform the actual
+// device update check.
+class OsAndPoliciesUpdateChecker
+    : public chromeos::UpdateEngineClient::Observer {
+ public:
+  OsAndPoliciesUpdateChecker(
+      TaskExecutorWithRetries::GetTicksSinceBootFn get_ticks_since_boot_fn);
+  ~OsAndPoliciesUpdateChecker() override;
+
+  using UpdateCheckCompletionCallback = base::OnceCallback<void(bool result)>;
+
+  // Starts an update check and possible download and calls |cb| to indicate
+  // success or failure when the process is complete. Overrides any previous
+  // calls to |Start|.
+  void Start(UpdateCheckCompletionCallback cb);
+
+  // Stops any pending update checks. Calls |update_check_completion_cb_| with
+  // false. It is safe to call |Start| after this.
+  void Stop();
+
+ private:
+  // Runs |update_check_completion_cb_| with |result| and runs |ResetState|.
+  void RunCompletionCallbackAndResetState(bool result);
+
+  // Runs when |update_check_task_executor_::Start| has failed after retries.
+  void OnUpdateCheckFailure();
+
+  // Requests update engine to do an update check.
+  void StartUpdateCheck();
+
+  // UpdateEngineClient::Observer overrides.
+  void UpdateStatusChanged(
+      const chromeos::UpdateEngineClient::Status& status) override;
+
+  // Tells whether starting an update check succeeded or not.
+  void OnUpdateCheckStarted(
+      chromeos::UpdateEngineClient::UpdateCheckResult result);
+
+  // Resets all state and cancels any pending update checks.
+  void ResetState();
+
+  // Ignore fist IDLE status that is sent when the update check is initiated.
+  bool ignore_idle_status_ = true;
+
+  // Callback passed to |Start|. Called if |StartUpdateCheck| is unsuccessful
+  // after retries or when an update check finishes successfully.
+  UpdateCheckCompletionCallback update_check_completion_cb_;
+
+  // Scheduled and retries |StartUpdateCheck|.
+  TaskExecutorWithRetries update_check_task_executor_;
+
+  base::WeakPtrFactory<OsAndPoliciesUpdateChecker> weak_factory_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(OsAndPoliciesUpdateChecker);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_OS_AND_POLICIES_UPDATE_CHECKER_H_