Enterprise reporting scheduler hookup.

ReportingScheduler now call real report generator and report uploader.

Reporting will stop if we can't generate any report or report uploading
returns a persistent error. There will be no future reports until Chrome
relaunch or pref change notification is received.

Bug: 956237
Change-Id: If19bb773ab0b29d810169c9454991e4947e4249e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1721629
Commit-Queue: Owen Min <zmin@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682447}

chrome/browser/enterprise_reporting/report_generator.cc

@@ -58,7 +58,7 @@ void ReportGenerator::Generate(ReportCallback callback) {
     return;
   }
 
-  requests_.push_back(
+  requests_.push(
       std::make_unique<em::ChromeDesktopReportRequest>(basic_request_));
   GetNextProfileReport(0);
 }
@@ -167,7 +167,7 @@ void ReportGenerator::OnProfileReportReady(
              maximum_report_size_) {
     // The new full Profile report is too big to be appended into the current
     // request, move it to the next request if possible.
-    requests_.push_back(
+    requests_.push(
         std::make_unique<em::ChromeDesktopReportRequest>(basic_request_));
     requests_.back()
         ->mutable_browser_report()

chrome/browser/enterprise_reporting/report_generator.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_GENERATOR_H_
 
 #include <memory>
+#include <queue>
 #include <string>
 #include <vector>
 
@@ -19,13 +20,13 @@ namespace enterprise_reporting {
 
 class ReportGenerator {
  public:
-  using ReportCallback = base::OnceCallback<void(
-      std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>>)>;
+  using Requests = std::queue<std::unique_ptr<em::ChromeDesktopReportRequest>>;
+  using ReportCallback = base::OnceCallback<void(Requests)>;
 
   ReportGenerator();
-  ~ReportGenerator();
+  virtual ~ReportGenerator();
 
-  void Generate(ReportCallback callback);
+  virtual void Generate(ReportCallback callback);
 
   void SetMaximumReportSizeForTesting(size_t size);
 
@@ -65,7 +66,7 @@ class ReportGenerator {
 
   ReportCallback callback_;
 
-  std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>> requests_;
+  Requests requests_;
 
   // Basic information that is shared among requests.
   em::ChromeDesktopReportRequest basic_request_;

chrome/browser/enterprise_reporting/report_generator_unittest.cc

@@ -126,10 +126,11 @@ class ReportGeneratorTest : public ::testing::Test {
     base::RunLoop run_loop;
     std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>> rets;
     generator_.Generate(base::BindLambdaForTesting(
-        [&run_loop,
-         &rets](std::vector<std::unique_ptr<em::ChromeDesktopReportRequest>>
-                    requests) {
-          rets = std::move(requests);
+        [&run_loop, &rets](ReportGenerator::Requests requests) {
+          while (!requests.empty()) {
+            rets.push_back(std::move(requests.front()));
+            requests.pop();
+          }
           run_loop.Quit();
         }));
     run_loop.Run();

chrome/browser/enterprise_reporting/report_scheduler.cc

@@ -7,6 +7,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/syslog_logging.h"
 #include "base/task/post_task.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
@@ -27,6 +28,7 @@ namespace enterprise_reporting {
 namespace {
 const int kDefaultUploadIntervalHours =
     24;  // Default upload interval is 24 hours.
+const int kMaximumRetry = 10;  // Retry 10 times takes about 15 to 19 hours.
 
 // Reads DM token and client id. Returns true if boths are non empty.
 bool GetDMTokenAndDeviceId(std::string* dm_token, std::string* client_id) {
@@ -52,14 +54,21 @@ bool IsReportingEnabled() {
 
 ReportScheduler::ReportScheduler(
     std::unique_ptr<policy::CloudPolicyClient> client,
-    std::unique_ptr<RequestTimer> request_timer)
+    std::unique_ptr<RequestTimer> request_timer,
+    std::unique_ptr<ReportGenerator> report_generator)
     : cloud_policy_client_(std::move(client)),
-      request_timer_(std::move(request_timer)) {
+      request_timer_(std::move(request_timer)),
+      report_generator_(std::move(report_generator)) {
   RegisterPerfObserver();
 }
 
 ReportScheduler::~ReportScheduler() = default;
 
+void ReportScheduler::SetReportUploaderForTesting(
+    std::unique_ptr<ReportUploader> uploader) {
+  report_uploader_ = std::move(uploader);
+}
+
 void ReportScheduler::RegisterPerfObserver() {
   pref_change_registrar_.Init(g_browser_process->local_state());
   pref_change_registrar_.Add(
@@ -103,27 +112,48 @@ void ReportScheduler::Start() {
 }
 
 void ReportScheduler::GenerateAndUploadReport() {
-  VLOG(1) << "Uploading enterprise report.";
-  // TODO(zmin): Generates a real request.
-  std::unique_ptr<em::ChromeDesktopReportRequest> request =
-      std::make_unique<em::ChromeDesktopReportRequest>();
-  cloud_policy_client_->UploadChromeDesktopReport(
-      std::move(request),
-      base::BindRepeating(&ReportScheduler::OnReportUploaded,
-                          base::Unretained(this)));
+  VLOG(1) << "Generating enterprise report.";
+  report_generator_->Generate(base::BindOnce(
+      &ReportScheduler::OnReportGenerated, base::Unretained(this)));
 }
 
-void ReportScheduler::OnReportUploaded(bool status) {
-  if (status) {
-    VLOG(1) << "The enterprise report has been uploaded.";
-    g_browser_process->local_state()->SetTime(kLastUploadTimestamp,
-                                              base::Time::Now());
-    if (IsReportingEnabled())
-      request_timer_->Reset();
+void ReportScheduler::OnReportGenerated(ReportGenerator::Requests requests) {
+  if (requests.empty()) {
+    SYSLOG(ERROR)
+        << "No cloud report can be generated. Likely the report is too large.";
+    // We can't generate any report, stop the reporting.
     return;
   }
-  VLOG(1) << "The enterprise report has not been uploaded.";
-  // TODO(zmin): Implement retry logic
+  VLOG(1) << "Uploading enterprise report.";
+  if (!report_uploader_) {
+    report_uploader_ = std::make_unique<ReportUploader>(
+        cloud_policy_client_.get(), kMaximumRetry);
+  }
+  report_uploader_->SetRequestAndUpload(
+      std::move(requests), base::BindOnce(&ReportScheduler::OnReportUploaded,
+                                          base::Unretained(this)));
+}
+
+void ReportScheduler::OnReportUploaded(ReportUploader::ReportStatus status) {
+  VLOG(1) << "The enterprise report upload result " << status;
+  switch (status) {
+    case ReportUploader::kSuccess:
+      // Schedule the next report for success. Reset uploader to reset failure
+      // count.
+      report_uploader_.reset();
+      FALLTHROUGH;
+    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();
+      break;
+    case ReportUploader::kPersistentError:
+      // No future upload until Chrome relaunch or perf change event.
+      break;
+  }
 }
 
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/report_scheduler.h

@@ -6,9 +6,12 @@
 #define CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_SCHEDULER_H_
 
 #include <memory>
+#include <queue>
 #include <string>
 
 #include "base/macros.h"
+#include "chrome/browser/enterprise_reporting/report_generator.h"
+#include "chrome/browser/enterprise_reporting/report_uploader.h"
 #include "components/prefs/pref_change_registrar.h"
 
 namespace policy {
@@ -24,10 +27,13 @@ class RequestTimer;
 class ReportScheduler {
  public:
   ReportScheduler(std::unique_ptr<policy::CloudPolicyClient> client,
-                  std::unique_ptr<RequestTimer> request_timer);
+                  std::unique_ptr<RequestTimer> request_timer,
+                  std::unique_ptr<ReportGenerator> report_generator);
 
   ~ReportScheduler();
 
+  void SetReportUploaderForTesting(std::unique_ptr<ReportUploader> uploader);
+
  private:
   // Observes CloudReportingEnabled policy.
   void RegisterPerfObserver();
@@ -42,8 +48,11 @@ class ReportScheduler {
   // Generates a report and uploads it.
   void GenerateAndUploadReport();
 
+  // Callback once report is generated.
+  void OnReportGenerated(ReportGenerator::Requests requests);
+
   // Callback once report upload request is finished.
-  void OnReportUploaded(bool status);
+  void OnReportUploaded(ReportUploader::ReportStatus status);
 
   // Policy value watcher
   PrefChangeRegistrar pref_change_registrar_;
@@ -52,6 +61,10 @@ class ReportScheduler {
 
   std::unique_ptr<RequestTimer> request_timer_;
 
+  std::unique_ptr<ReportUploader> report_uploader_;
+
+  std::unique_ptr<ReportGenerator> report_generator_;
+
   DISALLOW_COPY_AND_ASSIGN(ReportScheduler);
 };
 

chrome/browser/enterprise_reporting/report_scheduler_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/test/gmock_callback_support.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "chrome/browser/enterprise_reporting/prefs.h"
@@ -19,6 +20,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::base::test::RunOnceCallback;
 using ::testing::_;
 using ::testing::Invoke;
 using ::testing::WithArgs;
@@ -69,6 +71,35 @@ class FakeRequestTimer : public RequestTimer {
   DISALLOW_COPY_AND_ASSIGN(FakeRequestTimer);
 };
 
+ACTION_P(ScheduleGeneratorCallback, request_number) {
+  ReportGenerator::Requests requests;
+  for (int i = 0; i < request_number; i++)
+    requests.push(std::make_unique<em::ChromeDesktopReportRequest>());
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(arg0), std::move(requests)));
+}
+
+class MockReportGenerator : public ReportGenerator {
+ public:
+  void Generate(ReportCallback callback) override { OnGenerate(callback); }
+  MOCK_METHOD1(OnGenerate, void(ReportCallback& callback));
+};
+
+class MockReportUploader : public ReportUploader {
+ public:
+  MockReportUploader() : ReportUploader(nullptr, 0) {}
+  ~MockReportUploader() override = default;
+  void SetRequestAndUpload(Requests requests,
+                           ReportCallback callback) override {
+    OnSetRequestAndUpload(requests, callback);
+  }
+  MOCK_METHOD2(OnSetRequestAndUpload,
+               void(Requests& requests, ReportCallback& callback));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockReportUploader);
+};
+
 class ReportSchedulerTest : public ::testing::Test {
  public:
   ReportSchedulerTest()
@@ -84,6 +115,10 @@ class ReportSchedulerTest : public ::testing::Test {
     client_ = client_ptr_.get();
     timer_ptr_ = std::make_unique<FakeRequestTimer>();
     timer_ = timer_ptr_.get();
+    generator_ptr_ = std::make_unique<MockReportGenerator>();
+    generator_ = generator_ptr_.get();
+    uploader_ptr_ = std::make_unique<MockReportUploader>();
+    uploader_ = uploader_ptr_.get();
     Init(true, kDMToken, kClientId);
   }
 
@@ -97,7 +132,9 @@ class ReportSchedulerTest : public ::testing::Test {
 
   void CreateScheduler() {
     scheduler_ = std::make_unique<ReportScheduler>(std::move(client_ptr_),
-                                                   std::move(timer_ptr_));
+                                                   std::move(timer_ptr_),
+                                                   std::move(generator_ptr_));
+    scheduler_->SetReportUploaderForTesting(std::move(uploader_ptr_));
   }
 
   void SetLastUploadInHour(int gap) {
@@ -125,19 +162,30 @@ class ReportSchedulerTest : public ::testing::Test {
     }
   }
 
+  ReportGenerator::Requests CreateRequests(int number) {
+    ReportGenerator::Requests requests;
+    for (int i = 0; i < number; i++)
+      requests.push(std::make_unique<em::ChromeDesktopReportRequest>());
+    return requests;
+  }
+
   base::test::ScopedFeatureList scoped_feature_list_;
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   ScopedTestingLocalState local_state_;
 
   std::unique_ptr<ReportScheduler> scheduler_;
-  FakeRequestTimer* timer_;
   policy::MockCloudPolicyClient* client_;
+  FakeRequestTimer* timer_;
+  MockReportGenerator* generator_;
+  MockReportUploader* uploader_;
   policy::FakeBrowserDMTokenStorage storage_;
   base::Time previous_set_last_upload_timestamp_;
 
  private:
   std::unique_ptr<policy::MockCloudPolicyClient> client_ptr_;
   std::unique_ptr<FakeRequestTimer> timer_ptr_;
+  std::unique_ptr<MockReportGenerator> generator_ptr_;
+  std::unique_ptr<MockReportUploader> uploader_ptr_;
   DISALLOW_COPY_AND_ASSIGN(ReportSchedulerTest);
 };
 
@@ -161,8 +209,10 @@ TEST_F(ReportSchedulerTest, NoReportWithoutClientId) {
 
 TEST_F(ReportSchedulerTest, UploadReportSucceeded) {
   EXPECT_CALL(*client_, SetupRegistration(kDMToken, kClientId, _));
-  EXPECT_CALL(*client_, UploadChromeDesktopReportProxy(_, _))
-      .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(true)));
+  EXPECT_CALL(*generator_, OnGenerate(_))
+      .WillOnce(WithArgs<0>(ScheduleGeneratorCallback(1)));
+  EXPECT_CALL(*uploader_, OnSetRequestAndUpload(_, _))
+      .WillOnce(RunOnceCallback<1>(ReportUploader::kSuccess));
 
   CreateScheduler();
   EXPECT_TRUE(timer_->is_running());
@@ -180,12 +230,43 @@ TEST_F(ReportSchedulerTest, UploadReportSucceeded) {
   ExpectLastUploadTimestampUpdated(true);
 
   ::testing::Mock::VerifyAndClearExpectations(client_);
+  ::testing::Mock::VerifyAndClearExpectations(generator_);
 }
 
-TEST_F(ReportSchedulerTest, UploadFailed) {
+TEST_F(ReportSchedulerTest, UploadReportTransientError) {
   EXPECT_CALL(*client_, SetupRegistration(kDMToken, kClientId, _));
-  EXPECT_CALL(*client_, UploadChromeDesktopReportProxy(_, _))
-      .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)));
+  EXPECT_CALL(*generator_, OnGenerate(_))
+      .WillOnce(WithArgs<0>(ScheduleGeneratorCallback(1)));
+  EXPECT_CALL(*uploader_, OnSetRequestAndUpload(_, _))
+      .WillOnce(RunOnceCallback<1>(ReportUploader::kTransientError));
+
+  CreateScheduler();
+  EXPECT_TRUE(timer_->is_running());
+
+  timer_->Fire();
+
+  // timer is paused until the report is finished.
+  EXPECT_FALSE(timer_->is_running());
+
+  // Run pending task.
+  scoped_task_environment_.FastForwardBy(base::TimeDelta());
+
+  // Next report is scheduled.
+  EXPECT_TRUE(timer_->is_running());
+  ExpectLastUploadTimestampUpdated(true);
+
+  ::testing::Mock::VerifyAndClearExpectations(client_);
+  ::testing::Mock::VerifyAndClearExpectations(generator_);
+}
+
+TEST_F(ReportSchedulerTest, UploadReportPersistentError) {
+  EXPECT_CALL(*client_, SetupRegistration(kDMToken, kClientId, _))
+      .WillOnce(WithArgs<0>(
+          Invoke(client_, &policy::MockCloudPolicyClient::SetDMToken)));
+  EXPECT_CALL(*generator_, OnGenerate(_))
+      .WillOnce(WithArgs<0>(ScheduleGeneratorCallback(1)));
+  EXPECT_CALL(*uploader_, OnSetRequestAndUpload(_, _))
+      .WillOnce(RunOnceCallback<1>(ReportUploader::kPersistentError));
 
   CreateScheduler();
   EXPECT_TRUE(timer_->is_running());
@@ -202,7 +283,45 @@ TEST_F(ReportSchedulerTest, UploadFailed) {
   EXPECT_FALSE(timer_->is_running());
   ExpectLastUploadTimestampUpdated(false);
 
+  // Turn off and on reporting to resume.
+  ToggleCloudReport(false);
+  ToggleCloudReport(true);
+  EXPECT_TRUE(timer_->is_running());
+
+  ::testing::Mock::VerifyAndClearExpectations(client_);
+  ::testing::Mock::VerifyAndClearExpectations(generator_);
+}
+
+TEST_F(ReportSchedulerTest, NoReportGenerate) {
+  EXPECT_CALL(*client_, SetupRegistration(kDMToken, kClientId, _))
+      .WillOnce(WithArgs<0>(
+          Invoke(client_, &policy::MockCloudPolicyClient::SetDMToken)));
+  EXPECT_CALL(*generator_, OnGenerate(_))
+      .WillOnce(WithArgs<0>(ScheduleGeneratorCallback(0)));
+  EXPECT_CALL(*uploader_, OnSetRequestAndUpload(_, _)).Times(0);
+
+  CreateScheduler();
+  EXPECT_TRUE(timer_->is_running());
+
+  timer_->Fire();
+
+  // timer is paused until the report is finished.
+  EXPECT_FALSE(timer_->is_running());
+
+  // Run pending task.
+  scoped_task_environment_.FastForwardBy(base::TimeDelta());
+
+  // Next report is not scheduled.
+  EXPECT_FALSE(timer_->is_running());
+  ExpectLastUploadTimestampUpdated(false);
+
+  // Turn off and on reporting to resume.
+  ToggleCloudReport(false);
+  ToggleCloudReport(true);
+  EXPECT_TRUE(timer_->is_running());
+
   ::testing::Mock::VerifyAndClearExpectations(client_);
+  ::testing::Mock::VerifyAndClearExpectations(generator_);
 }
 
 TEST_F(ReportSchedulerTest, TimerDelayWithLastUploadTimestamp) {
@@ -220,6 +339,7 @@ TEST_F(ReportSchedulerTest, TimerDelayWithLastUploadTimestamp) {
             timer_->repeat_delay());
 
   ::testing::Mock::VerifyAndClearExpectations(client_);
+  ::testing::Mock::VerifyAndClearExpectations(generator_);
 }
 
 TEST_F(ReportSchedulerTest, TimerDelayWithoutLastUploadTimestamp) {
@@ -249,12 +369,15 @@ TEST_F(ReportSchedulerTest,
   ExpectLastUploadTimestampUpdated(false);
 
   ::testing::Mock::VerifyAndClearExpectations(client_);
+  ::testing::Mock::VerifyAndClearExpectations(generator_);
 }
 
 TEST_F(ReportSchedulerTest, ReportingIsDisabledWhileNewReportIsPosted) {
   EXPECT_CALL(*client_, SetupRegistration(kDMToken, kClientId, _));
-  EXPECT_CALL(*client_, UploadChromeDesktopReportProxy(_, _))
-      .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(true)));
+  EXPECT_CALL(*generator_, OnGenerate(_))
+      .WillOnce(WithArgs<0>(ScheduleGeneratorCallback(1)));
+  EXPECT_CALL(*uploader_, OnSetRequestAndUpload(_, _))
+      .WillOnce(RunOnceCallback<1>(ReportUploader::kSuccess));
 
   CreateScheduler();
   EXPECT_TRUE(timer_->is_running());
@@ -271,6 +394,7 @@ TEST_F(ReportSchedulerTest, ReportingIsDisabledWhileNewReportIsPosted) {
   EXPECT_FALSE(timer_->is_running());
 
   ::testing::Mock::VerifyAndClearExpectations(client_);
+  ::testing::Mock::VerifyAndClearExpectations(generator_);
 }
 
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/report_uploader.cc

@@ -34,9 +34,8 @@ ReportUploader::ReportUploader(policy::CloudPolicyClient* client,
       maximum_number_of_retries_(maximum_number_of_retries) {}
 ReportUploader::~ReportUploader() = default;
 
-void ReportUploader::SetRequestAndUpload(
-    std::queue<std::unique_ptr<em::ChromeDesktopReportRequest>> requests,
-    ReportCallback callback) {
+void ReportUploader::SetRequestAndUpload(Requests requests,
+                                         ReportCallback callback) {
   requests_ = std::move(requests);
   callback_ = std::move(callback);
   Upload();

chrome/browser/enterprise_reporting/report_uploader.h

@@ -48,18 +48,17 @@ class ReportUploader {
                        // invalid dm token.
   };
 
+  using Requests = std::queue<std::unique_ptr<em::ChromeDesktopReportRequest>>;
   // A callback to notify the upload result.
   using ReportCallback = base::OnceCallback<void(ReportStatus status)>;
 
   ReportUploader(policy::CloudPolicyClient* client,
                  int maximum_number_of_retries);
-  ~ReportUploader();
+  virtual ~ReportUploader();
 
   // Sets a list of requests and upload it. Request will be uploaded one after
   // another.
-  void SetRequestAndUpload(
-      std::queue<std::unique_ptr<em::ChromeDesktopReportRequest>> requests,
-      ReportCallback callback);
+  virtual void SetRequestAndUpload(Requests requests, ReportCallback callback);
 
  private:
   // Uploads the first request in the queue.
@@ -81,7 +80,7 @@ class ReportUploader {
 
   policy::CloudPolicyClient* client_;
   ReportCallback callback_;
-  std::queue<std::unique_ptr<em::ChromeDesktopReportRequest>> requests_;
+  Requests requests_;
 
   net::BackoffEntry backoff_entry_;
   base::OneShotTimer backoff_request_timer_;

chrome/browser/enterprise_reporting/report_uploader_unittest.cc

@@ -35,7 +35,7 @@ class ReportUploaderTest : public ::testing::Test {
       int number_of_request,
       ReportUploader::ReportStatus expected_status) {
     DCHECK_LE(number_of_request, 2) << "Please update kOsUserNames above.";
-    std::queue<std::unique_ptr<em::ChromeDesktopReportRequest>> requests;
+    ReportUploader::Requests requests;
     for (int i = 0; i < number_of_request; i++) {
       auto request = std::make_unique<em::ChromeDesktopReportRequest>();
       request->set_os_user_name(kOsUserNames[i]);