Crash in callers of cloud policy client

Cloud policy client cannot upload reports if not registered.
Instead of crashing in the cloud policy client code itself, return
unregistered error to the callers.
To keep behavior the same for now, move the unregistered crash to
the callers of report upload functions. They were not checking
unregistered state before.

Bug: b:256553070
Change-Id: Ibb371bc3ca3c2cb319dfc6bf63155e8ffcfc8b9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4218473
Reviewed-by: Dominique Fauteux-Chapleau <domfc@chromium.org>
Commit-Queue: Miriam Polzer <mpolzer@google.com>
Reviewed-by: Artem Sumaneev <asumaneev@google.com>
Reviewed-by: Roman Sorokin <rsorokin@google.com>
Cr-Commit-Position: refs/heads/main@{#1104987}

chrome/browser/ash/policy/reporting/arc_app_install_event_log_manager_unittest.cc

@@ -229,7 +229,7 @@ class ArcAppInstallEventLogManagerTest : public testing::Test {
   }
 
   void ExpectUploadAndCaptureCallback(
-      CloudPolicyClient::StatusCallback* callback) {
+      CloudPolicyClient::ResultCallback* callback) {
     events_value_.clear();
     BuildReport();
 
@@ -238,8 +238,8 @@ class ArcAppInstallEventLogManagerTest : public testing::Test {
         .WillOnce(MoveArg<1>(callback));
   }
 
-  void ReportUploadSuccess(CloudPolicyClient::StatusCallback callback) {
-    std::move(callback).Run(true /* success */);
+  void ReportUploadSuccess(CloudPolicyClient::ResultCallback callback) {
+    std::move(callback).Run(CloudPolicyClient::Result(DM_STATUS_SUCCESS));
     FlushNonDelayedTasks();
   }
 
@@ -250,8 +250,8 @@ class ArcAppInstallEventLogManagerTest : public testing::Test {
     EXPECT_CALL(cloud_policy_client_,
                 UploadAppInstallReport_(MatchEvents(&events_value_), _))
         .WillOnce(Invoke([](base::Value::Dict&,
-                            CloudPolicyClient::StatusCallback& callback) {
-          std::move(callback).Run(true /* success */);
+                            CloudPolicyClient::ResultCallback& callback) {
+          std::move(callback).Run(CloudPolicyClient::Result(DM_STATUS_SUCCESS));
         }));
   }
 
@@ -587,7 +587,7 @@ TEST_F(ArcAppInstallEventLogManagerTest, RequestUploadAddUpload) {
   FastForwardTo(kExpeditedUploadDelay - kOneMs);
   Mock::VerifyAndClearExpectations(&cloud_policy_client_);
 
-  CloudPolicyClient::StatusCallback upload_callback;
+  CloudPolicyClient::ResultCallback upload_callback;
   ExpectUploadAndCaptureCallback(&upload_callback);
   FastForwardTo(kExpeditedUploadDelay);
   Mock::VerifyAndClearExpectations(&cloud_policy_client_);
@@ -630,7 +630,7 @@ TEST_F(ArcAppInstallEventLogManagerTest, RequestUploadAddExpeditedUpload) {
   FastForwardTo(kExpeditedUploadDelay - kOneMs);
   Mock::VerifyAndClearExpectations(&cloud_policy_client_);
 
-  CloudPolicyClient::StatusCallback upload_callback;
+  CloudPolicyClient::ResultCallback upload_callback;
   ExpectUploadAndCaptureCallback(&upload_callback);
   FastForwardTo(kExpeditedUploadDelay);
   Mock::VerifyAndClearExpectations(&cloud_policy_client_);
@@ -681,7 +681,7 @@ TEST_F(ArcAppInstallEventLogManagerTest, RequestExpeditedUploadAddUpload) {
   Mock::VerifyAndClearExpectations(&cloud_policy_client_);
   EXPECT_FALSE(base::PathExists(log_file_path_));
 
-  CloudPolicyClient::StatusCallback upload_callback;
+  CloudPolicyClient::ResultCallback upload_callback;
   ExpectUploadAndCaptureCallback(&upload_callback);
   FastForwardTo(offset + kExpeditedUploadDelay);
   Mock::VerifyAndClearExpectations(&cloud_policy_client_);

chrome/browser/ash/policy/reporting/arc_app_install_event_log_uploader_unittest.cc

@@ -132,12 +132,14 @@ class ArcAppInstallEventLogUploaderTest : public testing::Test {
 
     EXPECT_CALL(client_, UploadAppInstallReport_(MatchValue(&value_report_), _))
         .WillOnce(WithArgs<1>(
-            Invoke([=](CloudPolicyClient::StatusCallback& callback) {
-              std::move(callback).Run(success);
+            Invoke([=](CloudPolicyClient::ResultCallback& callback) {
+              std::move(callback).Run(CloudPolicyClient::Result(
+                  success ? DM_STATUS_SUCCESS
+                          : DM_STATUS_TEMPORARY_UNAVAILABLE));
             })));
   }
 
-  void CaptureUpload(CloudPolicyClient::StatusCallback* callback) {
+  void CaptureUpload(CloudPolicyClient::ResultCallback* callback) {
     value_report_.clear();
     base::Value::Dict context = reporting::GetContext(/*profile=*/nullptr);
     base::Value::List events = ConvertArcAppProtoToValue(&log_, context);
@@ -155,7 +157,7 @@ class ArcAppInstallEventLogUploaderTest : public testing::Test {
   }
 
   void CompleteSerializeAndCaptureUpload(
-      CloudPolicyClient::StatusCallback* callback) {
+      CloudPolicyClient::ResultCallback* callback) {
     CompleteSerialize();
     CaptureUpload(callback);
   }
@@ -191,7 +193,7 @@ TEST_F(ArcAppInstallEventLogUploaderTest, RequestSerializeRequestAndUpload) {
   RegisterClient();
   CreateUploader();
 
-  CloudPolicyClient::StatusCallback status_callback;
+  CloudPolicyClient::ResultCallback status_callback;
   CompleteSerializeAndCaptureUpload(&status_callback);
   uploader_->RequestUpload();
   Mock::VerifyAndClearExpectations(&delegate_);
@@ -202,7 +204,7 @@ TEST_F(ArcAppInstallEventLogUploaderTest, RequestSerializeRequestAndUpload) {
 
   EXPECT_CALL(delegate_, OnUploadSuccess());
   EXPECT_CALL(delegate_, SerializeForUpload_(_)).Times(0);
-  std::move(status_callback).Run(true);
+  std::move(status_callback).Run(CloudPolicyClient::Result(DM_STATUS_SUCCESS));
 }
 
 // Make a log upload request. Have serialization begin. Make a second upload
@@ -256,8 +258,8 @@ TEST_F(ArcAppInstallEventLogUploaderTest, RequestSerializeAndCancel) {
   RegisterClient();
   CreateUploader();
 
-  CloudPolicyClient::StatusCallback status_callback;
-  CompleteSerializeAndCaptureUpload(&status_callback);
+  CloudPolicyClient::ResultCallback result_callback;
+  CompleteSerializeAndCaptureUpload(&result_callback);
   uploader_->RequestUpload();
   Mock::VerifyAndClearExpectations(&client_);
 
@@ -358,8 +360,8 @@ TEST_F(ArcAppInstallEventLogUploaderTest,
   RegisterClient();
   CreateUploader();
 
-  CloudPolicyClient::StatusCallback status_callback;
-  CompleteSerializeAndCaptureUpload(&status_callback);
+  CloudPolicyClient::ResultCallback result_callback;
+  CompleteSerializeAndCaptureUpload(&result_callback);
   uploader_->RequestUpload();
   Mock::VerifyAndClearExpectations(&delegate_);
   Mock::VerifyAndClearExpectations(&client_);
@@ -452,8 +454,8 @@ TEST_F(ArcAppInstallEventLogUploaderTest, RequestAndRemoveDelegate) {
   RegisterClient();
   CreateUploader();
 
-  CloudPolicyClient::StatusCallback status_callback;
-  CompleteSerializeAndCaptureUpload(&status_callback);
+  CloudPolicyClient::ResultCallback result_callback;
+  CompleteSerializeAndCaptureUpload(&result_callback);
   uploader_->RequestUpload();
   Mock::VerifyAndClearExpectations(&client_);
 

chrome/browser/ash/policy/reporting/install_event_log_uploader_base.cc

@@ -83,13 +83,18 @@ void InstallEventLogUploaderBase::OnRegistrationStateChanged(
   }
 }
 
-void InstallEventLogUploaderBase::OnUploadDone(bool success) {
-  if (success) {
+void InstallEventLogUploaderBase::OnUploadDone(
+    CloudPolicyClient::Result result) {
+  // TODO(b/256553070): Do not crash if the client is unregistered.
+  CHECK(!result.IsClientNotRegisteredError());
+
+  if (result.IsSuccess()) {
     upload_requested_ = false;
     retry_backoff_ms_ = kMinRetryBackoffMs;
     OnUploadSuccess();
     return;
   }
+
   PostTaskForStartSerialization();
   if (FastUploadForTestsEnabled())
     return;

chrome/browser/ash/policy/reporting/install_event_log_uploader_base.h

@@ -70,11 +70,10 @@ class InstallEventLogUploaderBase : public CloudPolicyClient::Observer {
   // a callback.
   virtual void StartSerialization() = 0;
 
-  // Notification by the client that the most recent log upload has succeeded if
-  // |success| is |true| or retries have been exhausted if |success| is |false|.
+  // |result| contains the result of the most recent log upload.
   // Forwards success to the delegate and schedules a retry with exponential
   // backoff in case of failure.
-  void OnUploadDone(bool success);
+  void OnUploadDone(CloudPolicyClient::Result result);
 
   // Notifies delegate on success of upload.
   virtual void OnUploadSuccess() = 0;

chrome/browser/enterprise/connectors/reporting/realtime_reporting_client.cc

@@ -362,9 +362,12 @@ void RealtimeReportingClient::ReportRealtimeEvent(
 
   auto upload_callback = base::BindOnce(
       [](base::Value::Dict wrapper, bool per_profile, std::string dm_token,
-         bool uploaded) {
+         policy::CloudPolicyClient::Result upload_result) {
+        // TODO(b/256553070): Do not crash if the client is unregistered.
+        CHECK(!upload_result.IsClientNotRegisteredError());
+
         // Show the report on chrome://safe-browsing, if appropriate.
-        wrapper.Set("uploaded_successfully", uploaded);
+        wrapper.Set("uploaded_successfully", upload_result.IsSuccess());
         wrapper.Set(per_profile ? "profile_dm_token" : "browser_dm_token",
                     std::move(dm_token));
         safe_browsing::WebUIInfoSingleton::GetInstance()->AddToReportingEvents(