Use WallClockTimer replace OneShotTimer in enterprise reporting timer

WallClockTimer is able to run the task with wall clock delay which is not
paused when the computer is sleep.

WallClockTimer is currently in relaunch_notification component. enterprise
reporting will temporarily depends on it. WallClockTimer will be moved into
base later.

Also, removing the RequestTimer and let ReportScheduler use WallClockTimer
directly.

Bug: 1039673
Change-Id: Ic3a2c695717c9fee6af5172d7a12e2afb2ed561b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2005648
Commit-Queue: Owen Min <zmin@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734125}

chrome/browser/BUILD.gn

@@ -3143,8 +3143,6 @@ jumbo_static_library("browser") {
       "enterprise_reporting/report_scheduler.h",
       "enterprise_reporting/report_uploader.cc",
       "enterprise_reporting/report_uploader.h",
-      "enterprise_reporting/request_timer.cc",
-      "enterprise_reporting/request_timer.h",
       "feedback/feedback_dialog_utils.cc",
       "feedback/feedback_dialog_utils.h",
       "feedback/feedback_uploader_chrome.cc",

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc

@@ -30,7 +30,6 @@
 #include "chrome/browser/chromeos/policy/wildcard_login_checker.h"
 #include "chrome/browser/enterprise_reporting/report_generator.h"
 #include "chrome/browser/enterprise_reporting/report_scheduler.h"
-#include "chrome/browser/enterprise_reporting/request_timer.h"
 #include "chrome/browser/invalidation/deprecated_profile_invalidation_provider_factory.h"
 #include "chrome/browser/invalidation/profile_invalidation_provider_factory.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
@@ -783,8 +782,7 @@ void UserCloudPolicyManagerChromeOS::StartReportSchedulerIfReady(
   }
 
   report_scheduler_ = std::make_unique<enterprise_reporting::ReportScheduler>(
-      client(), std::make_unique<enterprise_reporting::RequestTimer>(),
-      std::make_unique<enterprise_reporting::ReportGenerator>());
+      client(), std::make_unique<enterprise_reporting::ReportGenerator>());
 
   report_scheduler_->OnDMTokenUpdated();
 }

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc

@@ -28,7 +28,6 @@
 #include "chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/enterprise_reporting/report_scheduler.h"
-#include "chrome/browser/enterprise_reporting/request_timer.h"
 #include "chrome/browser/policy/cloud/cloud_policy_test_utils.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
@@ -893,10 +892,8 @@ TEST_P(UserCloudPolicyManagerChromeOSTest, TestReportSchedulerCreation) {
   EXPECT_TRUE(manager_->GetReportSchedulerForTesting());
 
   // Make sure the |report_scheduler| submit the request to DM Server.
-  enterprise_reporting::RequestTimer* request_timer =
-      manager_->GetReportSchedulerForTesting()->GetRequestTimerForTesting();
-  EXPECT_TRUE(request_timer->IsFirstTimerRunning());
-  EXPECT_FALSE(request_timer->IsRepeatTimerRunning());
+  EXPECT_TRUE(manager_->GetReportSchedulerForTesting()
+                  ->IsNextReportScheduledForTesting());
 }
 
 TEST_P(UserCloudPolicyManagerChromeOSTest, TestReportSchedulerDelayedCreation) {
@@ -941,10 +938,8 @@ TEST_P(UserCloudPolicyManagerChromeOSTest, TestReportSchedulerDelayedCreation) {
   EXPECT_TRUE(manager_->GetReportSchedulerForTesting());
 
   // Make sure the |report_scheduler| submit the request to DM Server.
-  enterprise_reporting::RequestTimer* request_timer =
-      manager_->GetReportSchedulerForTesting()->GetRequestTimerForTesting();
-  EXPECT_TRUE(request_timer->IsFirstTimerRunning());
-  EXPECT_FALSE(request_timer->IsRepeatTimerRunning());
+  EXPECT_TRUE(manager_->GetReportSchedulerForTesting()
+                  ->IsNextReportScheduledForTesting());
 }
 
 TEST_P(UserCloudPolicyManagerChromeOSTest, TestSkipReportSchedulerCreation) {

chrome/browser/enterprise_reporting/DEPS

+# Using WallClockTimer from relaunch notification until it's moved into base.
+include_rules = [
+  "+chrome/browser/ui/views/relaunch_notification/wall_clock_timer.h",
+]

chrome/browser/enterprise_reporting/report_scheduler.cc

@@ -14,7 +14,6 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/enterprise_reporting/prefs.h"
-#include "chrome/browser/enterprise_reporting/request_timer.h"
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/policy/browser_dm_token_storage.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
@@ -29,8 +28,9 @@ namespace em = enterprise_management;
 
 namespace enterprise_reporting {
 namespace {
-const int kDefaultUploadIntervalHours =
-    24;  // Default upload interval is 24 hours.
+
+constexpr base::TimeDelta kDefaultUploadInterval =
+    base::TimeDelta::FromHours(24);  // Default upload interval is 24 hours.
 const int kMaximumRetry = 10;  // Retry 10 times takes about 15 to 19 hours.
 const int kMaximumTrackedProfiles = 21;
 
@@ -44,10 +44,8 @@ bool IsReportingEnabled() {
 
 ReportScheduler::ReportScheduler(
     policy::CloudPolicyClient* client,
-    std::unique_ptr<RequestTimer> request_timer,
     std::unique_ptr<ReportGenerator> report_generator)
     : cloud_policy_client_(std::move(client)),
-      request_timer_(std::move(request_timer)),
       report_generator_(std::move(report_generator)) {
   RegisterPrefObserver();
 }
@@ -60,15 +58,15 @@ ReportScheduler::~ReportScheduler() {
   }
 }
 
+bool ReportScheduler::IsNextReportScheduledForTesting() const {
+  return request_timer_.IsRunning();
+}
+
 void ReportScheduler::SetReportUploaderForTesting(
     std::unique_ptr<ReportUploader> uploader) {
   report_uploader_ = std::move(uploader);
 }
 
-RequestTimer* ReportScheduler::GetRequestTimerForTesting() const {
-  return request_timer_.get();
-}
-
 void ReportScheduler::OnDMTokenUpdated() {
   OnReportEnabledPrefChanged();
 }
@@ -103,8 +101,7 @@ void ReportScheduler::OnReportEnabledPrefChanged() {
 }
 
 void ReportScheduler::StopRequestTimer() {
-  if (request_timer_)
-    request_timer_->Stop();
+  request_timer_.Stop();
 }
 
 bool ReportScheduler::SetupBrowserPolicyClientRegistration() {
@@ -128,19 +125,19 @@ bool ReportScheduler::SetupBrowserPolicyClientRegistration() {
 }
 
 void ReportScheduler::Start() {
-  base::TimeDelta upload_interval =
-      base::TimeDelta::FromHours(kDefaultUploadIntervalHours);
-  base::TimeDelta first_request_delay =
-      upload_interval -
-      (base::Time::Now() -
-       g_browser_process->local_state()->GetTime(kLastUploadTimestamp));
-  // The first report delay is based on the |lastUploadTimestamp| in the
+  // The |next_upload_time| is based on the |lastUploadTimestamp| in the
   // |local_state|, after that, it's 24 hours for each succeeded upload.
-  VLOG(1) << "Schedule the first report in about "
-          << first_request_delay.InHours() << " hour(s) and "
-          << first_request_delay.InMinutes() % 60 << " minute(s).";
-  request_timer_->Start(
-      FROM_HERE, first_request_delay, upload_interval,
+  base::Time next_upload_time =
+      g_browser_process->local_state()->GetTime(kLastUploadTimestamp) +
+      kDefaultUploadInterval;
+  if (VLOG_IS_ON(1)) {
+    base::TimeDelta first_request_delay = next_upload_time - base::Time::Now();
+    VLOG(1) << "Schedule the first report in about "
+            << first_request_delay.InHours() << " hour(s) and "
+            << first_request_delay.InMinutes() % 60 << " minute(s).";
+  }
+  request_timer_.Start(
+      FROM_HERE, next_upload_time,
       base::BindRepeating(&ReportScheduler::GenerateAndUploadReport,
                           base::Unretained(this)));
 }
@@ -182,10 +179,16 @@ void ReportScheduler::OnReportUploaded(ReportUploader::ReportStatus status) {
     case ReportUploader::kTransientError:
       // Stop retrying and schedule the next report to avoid stale report.
       // Failure count is not reset so retry delay remains.
-      g_browser_process->local_state()->SetTime(kLastUploadTimestamp,
-                                                base::Time::Now());
-      if (IsReportingEnabled())
-        request_timer_->Reset();
+      {
+        base::Time now = base::Time::Now();
+        g_browser_process->local_state()->SetTime(kLastUploadTimestamp, now);
+        if (IsReportingEnabled()) {
+          request_timer_.Start(
+              FROM_HERE, now + kDefaultUploadInterval,
+              base::BindRepeating(&ReportScheduler::GenerateAndUploadReport,
+                                  base::Unretained(this)));
+        }
+      }
       break;
     case ReportUploader::kPersistentError:
       // No future upload until Chrome relaunch or pref change event.

chrome/browser/enterprise_reporting/report_scheduler.h

@@ -16,6 +16,7 @@
 #include "chrome/browser/enterprise_reporting/report_generator.h"
 #include "chrome/browser/enterprise_reporting/report_uploader.h"
 #include "chrome/browser/profiles/profile_manager_observer.h"
+#include "chrome/browser/ui/views/relaunch_notification/wall_clock_timer.h"
 #include "components/prefs/pref_change_registrar.h"
 
 namespace policy {
@@ -24,21 +25,21 @@ class CloudPolicyClient;
 
 namespace enterprise_reporting {
 
-class RequestTimer;
-
 // Schedules the next report and handles retry in case of error. It also cancels
 // all pending uploads if the report policy is turned off.
 class ReportScheduler : public ProfileManagerObserver {
  public:
   ReportScheduler(policy::CloudPolicyClient* client,
-                  std::unique_ptr<RequestTimer> request_timer,
                   std::unique_ptr<ReportGenerator> report_generator);
 
   ~ReportScheduler() override;
 
-  void SetReportUploaderForTesting(std::unique_ptr<ReportUploader> uploader);
+  // Returns true if next report has been scheduled. The report will be
+  // scheduled only if the previous report is uploaded successfully and the
+  // reporting policy is still enabled.
+  bool IsNextReportScheduledForTesting() const;
 
-  RequestTimer* GetRequestTimerForTesting() const;
+  void SetReportUploaderForTesting(std::unique_ptr<ReportUploader> uploader);
 
   void OnDMTokenUpdated();
 
@@ -81,7 +82,7 @@ class ReportScheduler : public ProfileManagerObserver {
 
   policy::CloudPolicyClient* cloud_policy_client_;
 
-  std::unique_ptr<RequestTimer> request_timer_;
+  WallClockTimer request_timer_;
 
   std::unique_ptr<ReportUploader> report_uploader_;
 

chrome/browser/enterprise_reporting/request_timer.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/enterprise_reporting/request_timer.h"
-
-#include "base/time/time.h"
-
-namespace enterprise_reporting {
-
-RequestTimer::RequestTimer() = default;
-RequestTimer::~RequestTimer() = default;
-
-void RequestTimer::Start(const base::Location& posted_from,
-                         base::TimeDelta first_delay,
-                         base::TimeDelta repeat_delay,
-                         base::RepeatingClosure user_task) {
-  // Create |repeat_request_timer| but not start it because the first request is
-  // sent after |first_delay|.
-  repeat_request_timer_ = std::make_unique<base::RetainingOneShotTimer>(
-      FROM_HERE, repeat_delay, user_task);
-
-  if (first_delay >= repeat_delay) {
-    repeat_request_timer_->Reset();
-  } else {
-    first_request_timer_.Start(FROM_HERE, first_delay,
-                               base::BindOnce(user_task));
-  }
-}
-
-void RequestTimer::Stop() {
-  if (repeat_request_timer_)
-    repeat_request_timer_->Stop();
-  first_request_timer_.Stop();
-}
-
-void RequestTimer::Reset() {
-  DCHECK(!first_request_timer_.IsRunning());
-  DCHECK(repeat_request_timer_);
-  repeat_request_timer_->Reset();
-}
-
-bool RequestTimer::IsRepeatTimerRunning() const {
-  return repeat_request_timer_ && repeat_request_timer_->IsRunning();
-}
-
-bool RequestTimer::IsFirstTimerRunning() const {
-  return first_request_timer_.IsRunning();
-}
-
-}  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/request_timer.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_ENTERPRISE_REPORTING_REQUEST_TIMER_H_
-#define CHROME_BROWSER_ENTERPRISE_REPORTING_REQUEST_TIMER_H_
-
-#include <memory>
-
-#include "base/timer/timer.h"
-
-namespace enterprise_reporting {
-
-// A Retaining timer which runs the same task with same delay after every reset
-// except the first one which has a special delay. This is used by
-// ReportScheduler to schedule the next report.
-class RequestTimer {
- public:
-  RequestTimer();
-  virtual ~RequestTimer();
-
-  // Starts the timer. The first task will be ran after |first_delay|. The
-  // following task will be ran with |repeat_delay|. If |first_delay| is larger
-  // than the |repeat_delay|, the first request will be fired after
-  // |repeat_delay| instead. Also, please note that the repeating task is ran
-  // once per Reset call.
-  virtual void Start(const base::Location& posted_from,
-                     base::TimeDelta first_delay,
-                     base::TimeDelta repeat_delay,
-                     base::RepeatingClosure user_task);
-  // Stops the timer. The running task will not be abandon.
-  virtual void Stop();
-  // Resets the timer, ran the task again after |repat_delay| that is set in
-  // Start(); This is only available after the first task is ran.
-  virtual void Reset();
-
-  bool IsRepeatTimerRunning() const;
-  bool IsFirstTimerRunning() const;
-
- private:
-  base::OneShotTimer first_request_timer_;
-  std::unique_ptr<base::RetainingOneShotTimer> repeat_request_timer_;
-
-  DISALLOW_COPY_AND_ASSIGN(RequestTimer);
-};
-
-}  // namespace enterprise_reporting
-
-#endif  // CHROME_BROWSER_ENTERPRISE_REPORTING_REQUEST_TIMER_H_

chrome/browser/enterprise_reporting/request_timer_unittest.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/enterprise_reporting/request_timer.h"
-
-#include "base/test/task_environment.h"
-#include "base/time/time.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace enterprise_reporting {
-class RequestTimerTest : public ::testing::Test {
- public:
-  RequestTimerTest()
-      : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
-
-  void RunTask() { task_count_ += 1; }
-
- protected:
-  void StartTask(int first_delay, int repeat_delay) {
-    timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(first_delay),
-                 base::TimeDelta::FromSeconds(repeat_delay),
-                 base::BindRepeating(&RequestTimerTest::RunTask,
-                                     base::Unretained(this)));
-  }
-
-  base::test::TaskEnvironment task_environment_;
-  int task_count_ = 0;
-  RequestTimer timer_;
-};
-
-TEST_F(RequestTimerTest, StartTimer) {
-  StartTask(5, 10);
-  ASSERT_TRUE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-}
-
-TEST_F(RequestTimerTest, StartTimerWithLargerFirstDelay) {
-  StartTask(20, 10);
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_TRUE(timer_.IsRepeatTimerRunning());
-}
-
-TEST_F(RequestTimerTest, RunFirstTask) {
-  StartTask(5, 10);
-
-  // The first task hasn't been ran yet.
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(4));
-  ASSERT_TRUE(timer_.IsFirstTimerRunning());
-  ASSERT_EQ(0, task_count_);
-
-  // The first task is due.
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  ASSERT_EQ(1, task_count_);
-}
-
-TEST_F(RequestTimerTest, RunFirstTaskWithoutDelay) {
-  StartTask(0, 10);
-
-  // First task is posted and ran immediately.
-  task_environment_.RunUntilIdle();
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  ASSERT_EQ(1, task_count_);
-}
-
-TEST_F(RequestTimerTest, RunFirstTaskWithNegativeDelay) {
-  StartTask(-1, 10);
-
-  // First task is posted and ran immediately.
-  task_environment_.RunUntilIdle();
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  ASSERT_EQ(1, task_count_);
-}
-
-TEST_F(RequestTimerTest, RunRepeatingTaskTest) {
-  StartTask(1, 10);
-
-  // Run the first task.
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
-
-  // Reset and wait for the second task.
-  timer_.Reset();
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_TRUE(timer_.IsRepeatTimerRunning());
-
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(10));
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  ASSERT_EQ(2, task_count_);
-
-  // Reset and wait for the third task.
-  timer_.Reset();
-  ASSERT_TRUE(timer_.IsRepeatTimerRunning());
-
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(10));
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  ASSERT_EQ(3, task_count_);
-}
-
-TEST_F(RequestTimerTest, NotRuningRepeatingTaskWithoutResetTest) {
-  StartTask(1, 10);
-
-  // Run the first task.
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
-  ASSERT_EQ(1, task_count_);
-
-  // The repeat task is not ran without reset.
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(15));
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  ASSERT_EQ(1, task_count_);
-
-  // Reset the task, the repeat task is ran only once.
-  timer_.Reset();
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(60));
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  ASSERT_EQ(2, task_count_);
-}
-
-TEST_F(RequestTimerTest, StopFirstTask) {
-  StartTask(1, 10);
-
-  timer_.Stop();
-
-  // The task is stopped.
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(10));
-  ASSERT_EQ(0, task_count_);
-}
-
-TEST_F(RequestTimerTest, StopRepeatTask) {
-  StartTask(1, 10);
-
-  // Run the first task.
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
-  ASSERT_EQ(1, task_count_);
-
-  // Start the repeat task, stop after a while.
-  timer_.Reset();
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(3));
-  timer_.Stop();
-
-  // The task is stopped.
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(60));
-  ASSERT_EQ(1, task_count_);
-}
-
-TEST_F(RequestTimerTest, StopWithoutStart) {
-  // Timer is able to be stopped without being started.
-  timer_.Stop();
-  ASSERT_FALSE(timer_.IsFirstTimerRunning());
-  ASSERT_FALSE(timer_.IsRepeatTimerRunning());
-}
-
-}  // namespace enterprise_reporting

chrome/browser/policy/chrome_browser_cloud_management_controller.cc

@@ -19,7 +19,6 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/enterprise_reporting/report_generator.h"
 #include "chrome/browser/enterprise_reporting/report_scheduler.h"
-#include "chrome/browser/enterprise_reporting/request_timer.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/policy/browser_dm_token_storage.h"
@@ -445,10 +444,9 @@ void ChromeBrowserCloudManagementController::CreateReportScheduler() {
           ->GetSharedURLLoaderFactory(),
       nullptr, CloudPolicyClient::DeviceDMTokenCallback());
   cloud_policy_client_->AddObserver(this);
-  auto timer = std::make_unique<enterprise_reporting::RequestTimer>();
   auto generator = std::make_unique<enterprise_reporting::ReportGenerator>();
   report_scheduler_ = std::make_unique<enterprise_reporting::ReportScheduler>(
-      cloud_policy_client_.get(), std::move(timer), std::move(generator));
+      cloud_policy_client_.get(), std::move(generator));
 }
 
 }  // namespace policy

chrome/browser/ui/views/relaunch_notification/wall_clock_timer.cc

@@ -44,7 +44,7 @@ void WallClockTimer::Stop() {
   RemoveObserver();
 }
 
-bool WallClockTimer::IsRunning() {
+bool WallClockTimer::IsRunning() const {
   return timer_.IsRunning();
 }
 

chrome/browser/ui/views/relaunch_notification/wall_clock_timer.h

@@ -60,7 +60,7 @@ class WallClockTimer : public base::PowerObserver {
 
   void Stop();
 
-  bool IsRunning();
+  bool IsRunning() const;
 
   // base::PowerObserver:
   void OnResume() override;

chrome/test/BUILD.gn

@@ -3848,7 +3848,6 @@ test("unit_tests") {
       "../browser/enterprise_reporting/report_request_queue_generator_unittest.cc",
       "../browser/enterprise_reporting/report_scheduler_unittest.cc",
       "../browser/enterprise_reporting/report_uploader_unittest.cc",
-      "../browser/enterprise_reporting/request_timer_unittest.cc",
       "../browser/first_run/first_run_unittest.cc",
       "../browser/font_family_cache_unittest.cc",