Move ExtensionInstallEvents to Encrypted pipeline

Bug: chromium:1078512
Change-Id: I160ad5d61ad05f5e95f63188c40f9084b6e816e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2369659
Commit-Queue: Zach Trudo <zatrudo@google.com>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Leonid Baraz <lbaraz@chromium.org>
Reviewed-by: Swapnil Gupta <swapnilgupta@google.com>
Cr-Commit-Position: refs/heads/master@{#817556}

chrome/browser/chromeos/policy/extension_install_event_log_uploader.h

@@ -5,9 +5,16 @@
 #ifndef CHROME_BROWSER_CHROMEOS_POLICY_EXTENSION_INSTALL_EVENT_LOG_UPLOADER_H_
 #define CHROME_BROWSER_CHROMEOS_POLICY_EXTENSION_INSTALL_EVENT_LOG_UPLOADER_H_
 
+#include <memory>
+
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/policy/install_event_log_uploader_base.h"
+#include "chrome/browser/policy/messaging_layer/public/report_client.h"
+#include "chrome/browser/policy/messaging_layer/public/report_queue.h"
+#include "chrome/browser/policy/messaging_layer/util/status.h"
+#include "chrome/browser/policy/messaging_layer/util/statusor.h"
+#include "chrome/browser/policy/messaging_layer/util/task_runner_context.h"
 
 namespace enterprise_management {
 class ExtensionInstallReportRequest;
@@ -17,8 +24,6 @@ class Profile;
 
 namespace policy {
 
-class CloudPolicyClient;
-
 // Adapter between the system that captures and stores extension install event
 // logs and the policy system which uploads them to the management server.
 class ExtensionInstallEventLogUploader : public InstallEventLogUploaderBase {
@@ -45,8 +50,7 @@ class ExtensionInstallEventLogUploader : public InstallEventLogUploaderBase {
     virtual ~Delegate();
   };
 
-  // |client| must outlive |this|.
-  ExtensionInstallEventLogUploader(CloudPolicyClient* client, Profile* profile);
+  explicit ExtensionInstallEventLogUploader(Profile* profile);
   ~ExtensionInstallEventLogUploader() override;
 
   // Sets the delegate. The delegate must either outlive |this| or be explicitly
@@ -54,7 +58,81 @@ class ExtensionInstallEventLogUploader : public InstallEventLogUploaderBase {
   // delegate cancels the pending log upload, if any.
   void SetDelegate(Delegate* delegate);
 
+  // Sets the report queue if it is not already set.
+  void SetReportQueue(std::unique_ptr<reporting::ReportQueue> report_queue);
+
+  // Meant to be used in tests for creating the ReportQueueConfiguration.
+  void SetBuildReportQueueConfigurationForTests(const std::string& dm_token);
+
+ private:
+  // Ensures that only one ReportQueueBuilder is working at one time.
+  class ReportQueueBuilderLeaderTracker;
+
+  // ReportQueueBuilder builds a ReportQueue and uses |set_report_queue_cb|
+  // to set it in the ExtensionInstallEventLogUploader. ReportQueueBuilder
+  // ensures that only one ReportQueue is built for ExtensionInstallLogUploader.
+  class ReportQueueBuilder : public reporting::TaskRunnerContext<bool> {
+   public:
+    using SetReportQueueCallback =
+        base::OnceCallback<void(std::unique_ptr<reporting::ReportQueue>,
+                                base::OnceCallback<void()>)>;
+
+    using GetReportQueueConfigCallback =
+        base::RepeatingCallback<reporting::StatusOr<
+            std::unique_ptr<reporting::ReportQueueConfiguration>>()>;
+
+    ReportQueueBuilder(
+        SetReportQueueCallback set_report_queue_cb,
+        GetReportQueueConfigCallback get_report_queue_config_cb,
+        scoped_refptr<ReportQueueBuilderLeaderTracker> leader_tracker,
+        base::OnceCallback<void(bool)> completion_cb,
+        scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner);
+
    private:
+    ~ReportQueueBuilder() override;
+
+    // |OnStart| requests leadership promotion from the provided
+    // |leader_tracker|. If there is already a leader, |OnStart| will exit.
+    // Otherwise it will call |BuildReportQueue|.
+    void OnStart() override;
+
+    // |BuildReportQueue| will get the |ReportQueueConfiguration| from the
+    // |get_report_queue_config_cb_| and call ReportClient::CreateReportQueue to
+    // generate a ReportQueue. Sets OnReportQueueResult as the completion
+    // callback for |CreateReportQueue|.
+    void BuildReportQueue();
+
+    // |OnReportQueueResult| will evaluate |report_queue_result|. If it is not
+    // an OK status, it exits the builder with a |Complete| call. On an OK
+    // status it |Schedule|s SetReportQueue.
+    void OnReportQueueResult(
+        reporting::StatusOr<std::unique_ptr<reporting::ReportQueue>>
+            report_queue_result);
+
+    // SetReportQueue will call |set_report_queue_cb_| with the provided
+    // |report_queue|.
+    void SetReportQueue(std::unique_ptr<reporting::ReportQueue> report_queue);
+
+    // |Schedules| |ReleaseLeader|.
+    void Complete();
+
+    // Releases the leader lock if it is held, and then calls |Response|.
+    void ReleaseLeader();
+
+    // Callback for setting the ReportQueue in the calling
+    // |ExtensionInstallEventLogUploader|.
+    SetReportQueueCallback set_report_queue_cb_;
+
+    // Callback for creating the |ReportQueueConfiguration|.
+    GetReportQueueConfigCallback get_report_queue_config_cb_;
+
+    // |leader_tracker_| is used to ensure that only one ReportQueueBuilder is
+    // active at a time.
+    scoped_refptr<ReportQueueBuilderLeaderTracker> leader_tracker_;
+
+    base::OnceCallback<void()> release_leader_cb_;
+  };
+
   // InstallEventLogUploaderBase:
   void CheckDelegateSet() override;
   void PostTaskForStartSerialization() override;
@@ -67,9 +145,31 @@ class ExtensionInstallEventLogUploader : public InstallEventLogUploaderBase {
   void OnSerialized(
       const enterprise_management::ExtensionInstallReportRequest* report);
 
+  // Enqueues the report for upload.
+  void EnqueueReport(
+      const enterprise_management::ExtensionInstallReportRequest& report);
+
+  // Handles the status of the report enqueue.
+  void OnEnqueueDone(reporting::Status status);
+
   // The delegate that provides serialized logs to be uploaded.
   Delegate* delegate_ = nullptr;
 
+  // ReportQueueBuilderLeaderTracker for building the ReportQueue, passed to
+  // each ReportQueueBuilder in order to track which is the leader.
+  scoped_refptr<ReportQueueBuilderLeaderTracker> leader_tracker_;
+
+  // SequencedTaskRunenr for building the ReportQueue.
+  scoped_refptr<base::SequencedTaskRunner> report_queue_builder_task_runner_;
+
+  // Callback to generate a ReportQueueConfiguration.
+  base::RepeatingCallback<reporting::StatusOr<
+      std::unique_ptr<reporting::ReportQueueConfiguration>>()>
+      get_report_queue_config_cb_;
+
+  // ReportQueue for uploading events.
+  std::unique_ptr<reporting::ReportQueue> report_queue_;
+
   // Weak pointer factory for invalidating callbacks passed to the delegate and
   // scheduled retries when the upload request is canceled or |this| is
   // destroyed.

chrome/browser/chromeos/policy/install_event_log_uploader_base.cc

@@ -29,7 +29,13 @@ InstallEventLogUploaderBase::InstallEventLogUploaderBase(
   client_->AddObserver(this);
 }
 
+InstallEventLogUploaderBase::InstallEventLogUploaderBase(Profile* profile)
+    : client_(nullptr),
+      profile_(profile),
+      retry_backoff_ms_(kMinRetryBackoffMs) {}
+
 InstallEventLogUploaderBase::~InstallEventLogUploaderBase() {
+  if (client_)
     client_->RemoveObserver(this);
 }
 
@@ -39,8 +45,12 @@ void InstallEventLogUploaderBase::RequestUpload() {
     return;
 
   upload_requested_ = true;
-  if (client_->is_registered())
+
+  // If the client is set - ensure that it is also registered.
+  // Otherwise start Serialization.
+  if ((client_ && client_->is_registered()) || !client_) {
     StartSerialization();
+  }
 }
 
 void InstallEventLogUploaderBase::CancelUpload() {
@@ -51,6 +61,7 @@ void InstallEventLogUploaderBase::CancelUpload() {
 
 void InstallEventLogUploaderBase::OnRegistrationStateChanged(
     CloudPolicyClient* client) {
+  DCHECK(client_);
   if (!upload_requested_)
     return;
 

chrome/browser/chromeos/policy/install_event_log_uploader_base.h

@@ -21,6 +21,17 @@ class InstallEventLogUploaderBase : public CloudPolicyClient::Observer {
  public:
   // |client| must outlive |this|.
   InstallEventLogUploaderBase(CloudPolicyClient* client, Profile* profile);
+
+  // Will construct a non-CloudPolicyClient::Observer version of
+  // InstallEventLogUploaderBase.
+  // TODO(crbug.com/1078512) This exists to support the move to using
+  // reporting::ReportQueue, which owns its own CloudPolicyClient. Once
+  // ArcInstallEventLogUploader is ready to move to using
+  // reporting::ReportQueue, we can likely do a small refactor removing all
+  // references to CloudPolicyClient from InstallEventLogUploaderBase and its
+  // children.
+  explicit InstallEventLogUploaderBase(Profile* profile);
+
   ~InstallEventLogUploaderBase() override;
 
   // Requests log upload. If there is no pending upload yet, asks the delegate

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc

@@ -271,7 +271,7 @@ void UserCloudPolicyManagerChromeOS::Connect(
   app_install_event_log_uploader_ =
       std::make_unique<ArcAppInstallEventLogUploader>(client(), profile_);
   extension_install_event_log_uploader_ =
-      std::make_unique<ExtensionInstallEventLogUploader>(client(), profile_);
+      std::make_unique<ExtensionInstallEventLogUploader>(profile_);
 
   // Initializes an instance of DlpRulesManager to be responsible for the rules
   // of the data leak prevention policy.

chrome/browser/policy/messaging_layer/upload/app_install_report_handler.cc

@@ -34,12 +34,18 @@ using AppInstallReportUploader =
 
 using UploaderLeaderTracker = AppInstallReportHandler::UploaderLeaderTracker;
 
+UploaderLeaderTracker::UploaderLeaderTracker(policy::CloudPolicyClient* client)
+    : client_(client) {}
+
+UploaderLeaderTracker::~UploaderLeaderTracker() = default;
+
 // static
-scoped_refptr<UploaderLeaderTracker> UploaderLeaderTracker::Create() {
-  return base::WrapRefCounted(new UploaderLeaderTracker());
+scoped_refptr<UploaderLeaderTracker> UploaderLeaderTracker::Create(
+    policy::CloudPolicyClient* client) {
+  return base::WrapRefCounted(new UploaderLeaderTracker(client));
 }
 
-StatusOr<AppInstallReportHandler::ReleaseLeaderCallback>
+StatusOr<std::unique_ptr<UploaderLeaderTracker::LeaderLock>>
 UploaderLeaderTracker::RequestLeaderPromotion() {
   if (has_promoted_app_install_event_uploader_) {
     return Status(error::RESOURCE_EXHAUSTED,
@@ -47,26 +53,39 @@ UploaderLeaderTracker::RequestLeaderPromotion() {
   }
 
   has_promoted_app_install_event_uploader_ = true;
-  return base::BindOnce(&UploaderLeaderTracker::ReleaseLeader,
-                        base::Unretained(this));
+
+  return std::make_unique<LeaderLock>(
+      base::BindOnce(&UploaderLeaderTracker::ReleaseLeader,
+                     base::Unretained(this)),
+      client_);
 }
 
 void UploaderLeaderTracker::ReleaseLeader() {
   has_promoted_app_install_event_uploader_ = false;
 }
 
+UploaderLeaderTracker::LeaderLock::LeaderLock(
+    UploaderLeaderTracker::ReleaseLeaderCallback release_cb,
+    policy::CloudPolicyClient* client)
+    : client_(std::move(client)),
+      release_leader_callback_(std::move(release_cb)) {}
+
+UploaderLeaderTracker::LeaderLock::~LeaderLock() {
+  if (release_leader_callback_) {
+    std::move(release_leader_callback_).Run();
+  }
+}
+
 AppInstallReportUploader::AppInstallReportUploader(
     base::Value report,
     scoped_refptr<SharedQueue<base::Value>> report_queue,
     scoped_refptr<UploaderLeaderTracker> leader_tracker,
-    policy::CloudPolicyClient* client,
     ClientCallback client_cb,
     scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner)
     : TaskRunnerContext<bool>(std::move(client_cb), sequenced_task_runner),
       report_(std::move(report)),
       report_queue_(report_queue),
-      leader_tracker_(leader_tracker),
-      client_(client) {}
+      leader_tracker_(leader_tracker) {}
 
 AppInstallReportUploader::~AppInstallReportUploader() = default;
 
@@ -88,7 +107,7 @@ void AppInstallReportUploader::RequestLeaderPromotion() {
     return;
   }
 
-  release_leader_cb_ = std::move(promo_result.ValueOrDie());
+  leader_lock_ = std::move(promo_result.ValueOrDie());
 
   ScheduleNextPop();
 }
@@ -101,7 +120,7 @@ void AppInstallReportUploader::ScheduleNextPop() {
 void AppInstallReportUploader::OnPopResult(StatusOr<base::Value> pop_result) {
   if (!pop_result.ok()) {
     // There are no more records to process - exit.
-    std::move(release_leader_cb_).Run();
+    leader_lock_.reset();
     Complete();
     return;
   }
@@ -120,7 +139,7 @@ void AppInstallReportUploader::StartUpload(base::Value record) {
                        client->UploadExtensionInstallReport(std::move(record),
                                                             std::move(cb));
                      },
-                     client_, std::move(record), std::move(cb)));
+                     leader_lock_->client(), std::move(record), std::move(cb)));
 }
 
 void AppInstallReportUploader::OnUploadComplete(bool success) {
@@ -139,7 +158,7 @@ AppInstallReportHandler::AppInstallReportHandler(
     policy::CloudPolicyClient* client)
     : RecordHandler(client),
       report_queue_(SharedQueue<base::Value>::Create()),
-      leader_tracker_(UploaderLeaderTracker::Create()),
+      leader_tracker_(UploaderLeaderTracker::Create(client)),
       sequenced_task_runner_(base::ThreadPool::CreateSequencedTaskRunner({})) {}
 
 AppInstallReportHandler::~AppInstallReportHandler() = default;
@@ -155,8 +174,8 @@ Status AppInstallReportHandler::HandleRecord(Record record) {
   // Start an uploader in case any previous uploader has finished running before
   // this record was posted.
   Start<AppInstallReportUploader>(std::move(report), report_queue_,
-                                  leader_tracker_, GetClient(),
-                                  std::move(client_cb), sequenced_task_runner_);
+                                  leader_tracker_, std::move(client_cb),
+                                  sequenced_task_runner_);
 
   return Status::StatusOK();
 }
@@ -189,11 +208,4 @@ StatusOr<base::Value> AppInstallReportHandler::ConvertRecord(
   return std::move(report_result.value());
 }
 
-Status AppInstallReportHandler::ValidateClientState() const {
-  if (!GetClient()->is_registered()) {
-    return Status(error::UNAVAILABLE, "DmServer is currently unavailable");
-  }
-  return Status::StatusOK();
-}
-
 }  // namespace reporting

chrome/browser/policy/messaging_layer/upload/app_install_report_handler.h

@@ -36,16 +36,30 @@ class AppInstallReportHandler : public DmServerUploadService::RecordHandler {
   // and false indicates failure.
   using ClientCallback = base::OnceCallback<void(bool status)>;
 
-  using ReleaseLeaderCallback = base::OnceCallback<void()>;
-  using RequestLeaderPromotionCallback =
-      base::OnceCallback<StatusOr<ReleaseLeaderCallback>()>;
 
   // Tracking the leader needs to outlive |AppInstallReportHandler| so it needs
   // to be wrapped in a scoped_refptr.
   class UploaderLeaderTracker
       : public base::RefCountedThreadSafe<UploaderLeaderTracker> {
    public:
-    static scoped_refptr<UploaderLeaderTracker> Create();
+    using ReleaseLeaderCallback = base::OnceCallback<void()>;
+
+    // Holds the lock on the leader, releases it upon destruction.
+    class LeaderLock {
+     public:
+      LeaderLock(ReleaseLeaderCallback release_cb,
+                 policy::CloudPolicyClient* client);
+      virtual ~LeaderLock();
+
+      policy::CloudPolicyClient* client() { return client_; }
+
+     private:
+      policy::CloudPolicyClient* client_;
+      ReleaseLeaderCallback release_leader_callback_;
+    };
+
+    static scoped_refptr<UploaderLeaderTracker> Create(
+        policy::CloudPolicyClient* cloud_policy_client);
 
     // If there is currently no leader
     // (|has_promoted_app_install_event_uploader_| is false), then the StatusOr
@@ -53,23 +67,29 @@ class AppInstallReportHandler : public DmServerUploadService::RecordHandler {
     // leader an error::RESOURCE_EXHAUSTED is returned (which should be the
     // common case). This will be called on sequence from inside the
     // |AppInstallReportUploader| and so needs no additional protection.
-    StatusOr<ReleaseLeaderCallback> RequestLeaderPromotion();
+    StatusOr<std::unique_ptr<LeaderLock>> RequestLeaderPromotion();
+
+   private:
+    friend class base::RefCountedThreadSafe<UploaderLeaderTracker>;
+
+    explicit UploaderLeaderTracker(policy::CloudPolicyClient* client);
+    virtual ~UploaderLeaderTracker();
 
     // Once a AppInstallEventUploader leader drains the queue of reports, it
     // will release its leadership and return, allowing a new
     // AppInstallEventUploader to take leadership and upload events.
     void ReleaseLeader();
 
-   private:
-    friend class base::RefCountedThreadSafe<UploaderLeaderTracker>;
-    virtual ~UploaderLeaderTracker() = default;
-
-    UploaderLeaderTracker() = default;
+    // CloudPolicyClient allows calls to the reporting server.
+    policy::CloudPolicyClient* client_;
 
     // Flag indicates whether a leader has been promoted.
     bool has_promoted_app_install_event_uploader_{false};
   };
 
+  using RequestLeaderPromotionCallback =
+      base::OnceCallback<StatusOr<UploaderLeaderTracker::LeaderLock>()>;
+
   // AppInstallReportUploader handles enqueuing events on the |report_queue_|,
   // and uploading those events with the |client_|.
   class AppInstallReportUploader : public TaskRunnerContext<bool> {
@@ -78,7 +98,6 @@ class AppInstallReportHandler : public DmServerUploadService::RecordHandler {
         base::Value report,
         scoped_refptr<SharedQueue<base::Value>> report_queue,
         scoped_refptr<UploaderLeaderTracker> leader_tracker,
-        policy::CloudPolicyClient* client,
         ClientCallback client_cb,
         scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner);
 
@@ -127,9 +146,7 @@ class AppInstallReportHandler : public DmServerUploadService::RecordHandler {
     base::Value report_;
     scoped_refptr<SharedQueue<base::Value>> report_queue_;
     scoped_refptr<UploaderLeaderTracker> leader_tracker_;
-    ReleaseLeaderCallback release_leader_cb_;
-
-    policy::CloudPolicyClient* const client_;
+    std::unique_ptr<UploaderLeaderTracker::LeaderLock> leader_lock_;
   };
 
   explicit AppInstallReportHandler(policy::CloudPolicyClient* client);
@@ -150,9 +167,6 @@ class AppInstallReportHandler : public DmServerUploadService::RecordHandler {
   // Convert record into base::Value for upload (override for subclass).
   virtual StatusOr<base::Value> ConvertRecord(const Record& record) const;
 
-  // Helper method. Validates CloudPolicyClient state.
-  Status ValidateClientState() const;
-
   scoped_refptr<SharedQueue<base::Value>> report_queue_;
   scoped_refptr<UploaderLeaderTracker> leader_tracker_;
   scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;

chrome/browser/policy/messaging_layer/upload/app_install_report_handler_unittest.cc

@@ -63,17 +63,17 @@ class TestCallbackWaiter {
 
 class AppInstallReportHandlerTest : public testing::Test {
  public:
-  AppInstallReportHandlerTest() = default;
+  AppInstallReportHandlerTest()
+      : client_(std::make_unique<policy::MockCloudPolicyClient>()) {}
 
+ protected:
   void SetUp() override {
-    client_.SetDMToken(
+    client_->SetDMToken(
         policy::DMToken::CreateValidTokenForTesting("FAKE_DM_TOKEN").value());
   }
+  content::BrowserTaskEnvironment task_environment_;
 
- protected:
-  content::BrowserTaskEnvironment task_envrionment_;
-
-  policy::MockCloudPolicyClient client_;
+  std::unique_ptr<policy::MockCloudPolicyClient> client_;
 };
 
 class TestRecord : public Record {
@@ -97,7 +97,7 @@ class TestRecord : public Record {
 TEST_F(AppInstallReportHandlerTest, AcceptsValidRecord) {
   TestCallbackWaiter waiter;
   TestRecord test_record;
-  EXPECT_CALL(client_,
+  EXPECT_CALL(*client_,
               UploadExtensionInstallReport_(MatchValue(test_record.data()), _))
       .WillOnce(WithArgs<1>(
           Invoke([&waiter](AppInstallReportHandler::ClientCallback& callback) {
@@ -105,15 +105,15 @@ TEST_F(AppInstallReportHandlerTest, AcceptsValidRecord) {
             waiter.Signal();
           })));
 
-  AppInstallReportHandler handler(&client_);
+  AppInstallReportHandler handler(client_.get());
   Status handle_status = handler.HandleRecord(test_record);
   EXPECT_OK(handle_status);
   waiter.Wait();
 }
 
 TEST_F(AppInstallReportHandlerTest, DeniesInvalidDestination) {
-  EXPECT_CALL(client_, UploadExtensionInstallReport_(_, _)).Times(0);
-  AppInstallReportHandler handler(&client_);
+  EXPECT_CALL(*client_, UploadExtensionInstallReport_(_, _)).Times(0);
+  AppInstallReportHandler handler(client_.get());
 
   TestRecord test_record;
   test_record.set_destination(Destination::MEET_DEVICE_TELEMETRY);
@@ -124,8 +124,8 @@ TEST_F(AppInstallReportHandlerTest, DeniesInvalidDestination) {
 }
 
 TEST_F(AppInstallReportHandlerTest, DeniesInvalidData) {
-  EXPECT_CALL(client_, UploadExtensionInstallReport_(_, _)).Times(0);
-  AppInstallReportHandler handler(&client_);
+  EXPECT_CALL(*client_, UploadExtensionInstallReport_(_, _)).Times(0);
+  AppInstallReportHandler handler(client_.get());
 
   TestRecord test_record;
   test_record.set_data("BAD_DATA");
@@ -138,7 +138,7 @@ TEST_F(AppInstallReportHandlerTest, ReportsUnsuccessfulCall) {
   TestCallbackWaiter waiter;
 
   TestRecord test_record;
-  EXPECT_CALL(client_,
+  EXPECT_CALL(*client_,
               UploadExtensionInstallReport_(MatchValue(test_record.data()), _))
       .WillOnce(WithArgs<1>(
           Invoke([&waiter](AppInstallReportHandler::ClientCallback& callback) {
@@ -146,7 +146,7 @@ TEST_F(AppInstallReportHandlerTest, ReportsUnsuccessfulCall) {
             waiter.Signal();
           })));
 
-  AppInstallReportHandler handler(&client_);
+  AppInstallReportHandler handler(client_.get());
   Status handle_status = handler.HandleRecord(test_record);
   EXPECT_OK(handle_status);
   waiter.Wait();
@@ -173,7 +173,7 @@ TEST_F(AppInstallReportHandlerTest, AcceptsMultipleValidRecords) {
   TestCallbackWaiterWithCounter waiter{kExpectedCallTimes};
 
   TestRecord test_record;
-  EXPECT_CALL(client_,
+  EXPECT_CALL(*client_,
               UploadExtensionInstallReport_(MatchValue(test_record.data()), _))
       .WillRepeatedly(WithArgs<1>(
           Invoke([&waiter](AppInstallReportHandler::ClientCallback& callback) {
@@ -181,7 +181,7 @@ TEST_F(AppInstallReportHandlerTest, AcceptsMultipleValidRecords) {
             waiter.Signal();
           })));
 
-  AppInstallReportHandler handler(&client_);
+  AppInstallReportHandler handler(client_.get());
 
   for (int i = 0; i < kExpectedCallTimes; i++) {
     Status handle_status = handler.HandleRecord(test_record);

chrome/browser/policy/messaging_layer/upload/dm_server_upload_service.cc

@@ -88,11 +88,10 @@ class CollectorCallback {
 };
 }  // namespace
 
-DmServerUploadService::RecordHandler::RecordHandler(CloudPolicyClient* client)
+DmServerUploadService::RecordHandler::RecordHandler(
+    policy::CloudPolicyClient* client)
     : client_(client) {}
 
-DmServerUploadService::RecordHandler::~RecordHandler() = default;
-
 DmServerUploader::DmServerUploader(
     std::unique_ptr<std::vector<EncryptedRecord>> records,
     scoped_refptr<SharedVector<std::unique_ptr<RecordHandler>>> handlers,
@@ -349,9 +348,8 @@ DmServerUploadService::DmServerUploadService(
     ReportSuccessfulUploadCallback upload_cb)
     : client_(std::move(client)),
       upload_cb_(upload_cb),
-      sequenced_task_runner_(base::ThreadPool::CreateSequencedTaskRunner({})),
-      record_handlers_(SharedVector<std::unique_ptr<RecordHandler>>::Create()) {
-}
+      record_handlers_(SharedVector<std::unique_ptr<RecordHandler>>::Create()),
+      sequenced_task_runner_(base::ThreadPool::CreateSequencedTaskRunner({})) {}
 
 DmServerUploadService::~DmServerUploadService() {
   if (client_) {

chrome/browser/policy/messaging_layer/upload/dm_server_upload_service.h

@@ -52,7 +52,7 @@ class DmServerUploadService {
   class RecordHandler {
    public:
     explicit RecordHandler(policy::CloudPolicyClient* client);
-    virtual ~RecordHandler();
+    virtual ~RecordHandler() = default;
 
     virtual Status HandleRecord(Record record) = 0;
 
@@ -91,8 +91,8 @@ class DmServerUploadService {
     void IsHandlerVectorEmptyCheck(bool handler_is_empty);
 
     // ProcessRecords verifies that the records provided are parseable and sets
-    // the |Record|s up for handling by the |RecordHandler|s. On completion,
-    // ProcessRecords |Schedule|s |HandleRecords|.
+    // the |Record|s up for handling by the |RecordHandlers|s. On
+    // completion, ProcessRecords |Schedule|s |HandleRecords|.
     void ProcessRecords();
 
     // HandleRecords sends the records to the |record_handlers_|, allowing them
@@ -125,7 +125,7 @@ class DmServerUploadService {
         SequencingInformation sequencing_information);
 
     std::unique_ptr<std::vector<EncryptedRecord>> encrypted_records_;
-    const scoped_refptr<SharedVector<std::unique_ptr<RecordHandler>>> handlers_;
+    scoped_refptr<SharedVector<std::unique_ptr<RecordHandler>>> handlers_;
 
     // generation_id_ will be set to the generation of the first record in
     // encrypted_records_.
@@ -173,9 +173,9 @@ class DmServerUploadService {
 
   std::unique_ptr<policy::CloudPolicyClient> client_;
   ReportSuccessfulUploadCallback upload_cb_;
-  scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
-
   scoped_refptr<SharedVector<std::unique_ptr<RecordHandler>>> record_handlers_;
+
+  scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
 };
 
 }  // namespace reporting

chrome/browser/policy/messaging_layer/upload/dm_server_upload_service_unittest.cc

@@ -116,9 +116,7 @@ class TestCallbackWaiter {
 
 class TestRecordHandler : public DmServerUploadService::RecordHandler {
  public:
-  explicit TestRecordHandler(policy::CloudPolicyClient* client)
-      : RecordHandler(client) {}
-
+  TestRecordHandler() : RecordHandler(/*client=*/nullptr) {}
   ~TestRecordHandler() override = default;
 
   MOCK_METHOD(Status, HandleRecord, (Record));
@@ -132,8 +130,7 @@ class DmServerUploaderTest : public testing::Test {
                       DmServerUploadService::RecordHandler>>::Create()) {}
 
   void SetUp() override {
-    std::unique_ptr<TestRecordHandler> handler_ptr(
-        new TestRecordHandler(&client_));
+    std::unique_ptr<TestRecordHandler> handler_ptr(new TestRecordHandler());
     handler_ = handler_ptr.get();
     handlers_->PushBack(std::move(handler_ptr), base::DoNothing());
     records_ = std::make_unique<std::vector<EncryptedRecord>>();
@@ -153,9 +150,6 @@ class DmServerUploaderTest : public testing::Test {
   std::unique_ptr<std::vector<EncryptedRecord>> records_;
 
   const base::TimeDelta kMaxDelay_ = base::TimeDelta::FromSeconds(1);
-
- private:
-  policy::MockCloudPolicyClient client_;
 };
 
 TEST_F(DmServerUploaderTest, ProcessesRecord) {