Build CloudPolicyClient asynchronously

CloudPolicyClient must be built on the main UI thread, this means that
it needs to be built asynchronously.

Bug: chromium:1078512
Change-Id: I7aca6e9085091a8ac4d2c6209e10260f1295c450
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368318
Commit-Queue: Zach Trudo <zatrudo@google.com>
Reviewed-by: Leonid Baraz <lbaraz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802846}

chrome/browser/policy/messaging_layer/public/report_client.h

@@ -37,43 +37,13 @@ namespace reporting {
 // }
 class ReportingClient {
  public:
-  // Uploader is passed to Storage in order to upload messages using the
-  // UploadClient.
-  class Uploader : public Storage::UploaderInterface {
-   public:
-    using UploadCallback = base::OnceCallback<Status(
-        std::unique_ptr<std::vector<EncryptedRecord>>)>;
-
-    static StatusOr<std::unique_ptr<Uploader>> Create(
-        UploadCallback upload_callback);
-
-    ~Uploader() override;
-    Uploader(const Uploader& other) = delete;
-    Uploader& operator=(const Uploader& other) = delete;
-
-    void ProcessRecord(StatusOr<EncryptedRecord> data,
-                       base::OnceCallback<void(bool)> processed_cb) override;
-
-    void Completed(Status final_status) override;
-
-   private:
-    explicit Uploader(UploadCallback upload_callback_);
-
-    void RunUpload();
-
-    UploadCallback upload_callback_;
-
-    bool completed_;
-    std::unique_ptr<std::vector<EncryptedRecord>> encrypted_records_;
-    scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
-  };
-
   struct Configuration {
     Configuration();
     ~Configuration();
 
-    scoped_refptr<StorageModule> storage_;
-    scoped_refptr<EncryptionModule> encryption_;
+    std::unique_ptr<policy::CloudPolicyClient> cloud_policy_client;
+    scoped_refptr<StorageModule> storage;
+    scoped_refptr<EncryptionModule> encryption;
   };
 
   using CreateReportQueueResponse = StatusOr<std::unique_ptr<ReportQueue>>;
@@ -82,8 +52,11 @@ class ReportingClient {
       base::OnceCallback<void(CreateReportQueueResponse)>;
 
   using UpdateConfigurationCallback =
-      base::OnceCallback<void(const Configuration&,
+      base::OnceCallback<void(std::unique_ptr<Configuration>,
                               base::OnceCallback<void(Status)>)>;
+  using BuildCloudPolicyClientCallback = base::OnceCallback<void(
+      base::OnceCallback<void(
+          StatusOr<std::unique_ptr<policy::CloudPolicyClient>>)>)>;
 
   using InitCompleteCallback = base::OnceCallback<void(Status)>;
 
@@ -137,6 +110,7 @@ class ReportingClient {
   class InitializingContext : public TaskRunnerContext<Status> {
    public:
     InitializingContext(
+        BuildCloudPolicyClientCallback build_client_cb,
         Storage::StartUploadCb start_upload_cb,
         UpdateConfigurationCallback update_config_cb,
         InitCompleteCallback init_complete_cb,
@@ -152,6 +126,10 @@ class ReportingClient {
         StatusOr<InitializationStateTracker::ReleaseLeaderCallback>
             promo_result);
 
+    void ConfigureCloudPolicyClient();
+    void OnCloudPolicyClientConfigured(
+        StatusOr<std::unique_ptr<policy::CloudPolicyClient>> client_result);
+
     // ConfigureStorageModule will build a StorageModule and add it to the
     // |client_config_|.
     void ConfigureStorageModule();
@@ -169,12 +147,13 @@ class ReportingClient {
     // Complete calls response with |client_config_|
     void Complete(Status status);
 
+    BuildCloudPolicyClientCallback build_client_cb_;
     Storage::StartUploadCb start_upload_cb_;
     UpdateConfigurationCallback update_config_cb_;
     scoped_refptr<InitializationStateTracker> init_state_tracker_;
 
     InitializationStateTracker::ReleaseLeaderCallback release_leader_cb_;
-    Configuration client_config_;
+    std::unique_ptr<Configuration> client_config_;
   };
 
   ~ReportingClient();
@@ -194,11 +173,46 @@ class ReportingClient {
       std::unique_ptr<ReportQueueConfiguration> config,
       CreateReportQueueCallback create_cb);
 
+  // Sets up the ReportingClient for testing with a specified CloudPolicyClient.
+  static void Setup_test(std::unique_ptr<policy::CloudPolicyClient> client);
   // Resets the singleton object. Should only be used in tests when the current
   // TaskEnvironment will be invalidated.
   static void Reset_test();
 
  private:
+  // Uploader is passed to Storage in order to upload messages using the
+  // UploadClient.
+  class Uploader : public Storage::UploaderInterface {
+   public:
+    using UploadCallback = base::OnceCallback<Status(
+        std::unique_ptr<std::vector<EncryptedRecord>>)>;
+
+    static StatusOr<std::unique_ptr<Uploader>> Create(
+        UploadCallback upload_callback);
+
+    ~Uploader() override;
+    Uploader(const Uploader& other) = delete;
+    Uploader& operator=(const Uploader& other) = delete;
+
+    void ProcessRecord(StatusOr<EncryptedRecord> data,
+                       base::OnceCallback<void(bool)> processed_cb) override;
+
+    void Completed(Status final_status) override;
+
+   private:
+    explicit Uploader(UploadCallback upload_callback_);
+
+    static void RunUpload(
+        UploadCallback upload_callback,
+        std::unique_ptr<std::vector<EncryptedRecord>> encrypted_records);
+
+    UploadCallback upload_callback_;
+
+    bool completed_{false};
+    std::unique_ptr<std::vector<EncryptedRecord>> encrypted_records_;
+    scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
+  };
+
   // Holds the creation request for a ReportQueue.
   class CreateReportQueueRequest {
    public:
@@ -222,7 +236,7 @@ class ReportingClient {
 
   void OnPushComplete();
   void OnInitState(bool reporting_client_configured);
-  void OnConfigResult(const Configuration& config,
+  void OnConfigResult(std::unique_ptr<Configuration> config,
                       base::OnceCallback<void(Status)> continue_init_cb);
   void OnInitializationComplete(Status init_status);
 
@@ -236,13 +250,11 @@ class ReportingClient {
   // Queue for storing creation requests while the ReportingClient is
   // initializing.
   scoped_refptr<SharedQueue<CreateReportQueueRequest>> create_request_queue_;
-
   scoped_refptr<InitializationStateTracker> init_state_tracker_;
+  BuildCloudPolicyClientCallback build_cloud_policy_client_cb_;
 
-  scoped_refptr<StorageModule> storage_;
-  scoped_refptr<EncryptionModule> encryption_;
   std::unique_ptr<UploadClient> upload_client_;
-  Configuration config_;
+  std::unique_ptr<Configuration> config_;
 };
 
 }  // namespace reporting

chrome/browser/policy/messaging_layer/public/report_client_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/memory/singleton.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/task/post_task.h"
 #include "base/test/task_environment.h"
 #include "chrome/browser/policy/messaging_layer/public/report_queue.h"
 #include "chrome/browser/policy/messaging_layer/public/report_queue_configuration.h"
@@ -13,9 +14,20 @@
 #include "chrome/browser/policy/messaging_layer/util/status_macros.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "components/policy/core/common/cloud/dm_token.h"
+#include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "components/policy/proto/record_constants.pb.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+// #if defined(OS_CHROMEOS)
+// #include "chrome/browser/chromeos/settings/device_settings_service.h"
+// #include "components/policy/proto/chrome_device_policy.pb.h"
+// #else
+// #include "chrome/browser/policy/chrome_browser_policy_connector.h"
+// #endif
+
 namespace reporting {
 namespace {
 
@@ -25,29 +37,33 @@ using reporting::Priority;
 
 class TestCallbackWaiter {
  public:
-  TestCallbackWaiter()
-      : completed_(base::WaitableEvent::ResetPolicy::MANUAL,
-                   base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+  TestCallbackWaiter() : run_loop_(std::make_unique<base::RunLoop>()) {}
 
-  virtual void Signal() {
-    DCHECK(!completed_.IsSignaled());
-    completed_.Signal();
-  }
+  virtual void Signal() { run_loop_->Quit(); }
 
-  void Wait() { completed_.Wait(); }
-  void Reset() { completed_.Reset(); }
+  void Wait() { run_loop_->Run(); }
+  void Reset() {
+    run_loop_.reset();
+    run_loop_ = std::make_unique<base::RunLoop>();
+  }
 
  protected:
-  base::WaitableEvent completed_;
+  std::unique_ptr<base::RunLoop> run_loop_;
 };
 
 class ReportingClientTest : public testing::Test {
  public:
+  void SetUp() override {
+    auto client = std::make_unique<policy::MockCloudPolicyClient>();
+    client->SetDMToken(
+        policy::DMToken::CreateValidTokenForTesting("FAKE_DM_TOKEN").value());
+    ReportingClient::Setup_test(std::move(client));
+  }
+
   void TearDown() override { ReportingClient::Reset_test(); }
 
  protected:
-  base::test::TaskEnvironment task_envrionment_{
-      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  content::BrowserTaskEnvironment task_envrionment_;
   const DMToken dm_token_ = DMToken::CreateValidTokenForTesting("TOKEN");
   const Destination destination_ = Destination::UPLOAD_EVENTS;
   const Priority priority_ = Priority::IMMEDIATE;

chrome/browser/policy/messaging_layer/public/report_queue_unittest.cc

@@ -26,6 +26,7 @@
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "components/policy/core/common/cloud/dm_token.h"
 #include "components/policy/proto/record_constants.pb.h"
+#include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -144,8 +145,7 @@ class ReportQueueTest : public testing::Test {
 
   MOCK_METHOD(Status, MockedPolicyCheck, (), ());
 
-  base::test::TaskEnvironment task_envrionment_{
-      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  content::BrowserTaskEnvironment task_envrionment_;
 
   const Priority priority_;
 

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

@@ -24,6 +24,8 @@
 #include "components/policy/core/common/cloud/cloud_policy_client.h"
 #include "components/policy/proto/record.pb.h"
 #include "components/policy/proto/record_constants.pb.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 
 namespace reporting {
 
@@ -111,7 +113,14 @@ void AppInstallReportUploader::OnPopResult(StatusOr<base::Value> pop_result) {
 void AppInstallReportUploader::StartUpload(base::Value record) {
   ClientCallback cb = base::BindOnce(
       &AppInstallReportUploader::OnUploadComplete, base::Unretained(this));
-  client_->UploadAppInstallReport(std::move(record), std::move(cb));
+  base::PostTask(FROM_HERE, {content::BrowserThread::UI},
+                 base::BindOnce(
+                     [](policy::CloudPolicyClient* client, base::Value record,
+                        ClientCallback cb) {
+                       client->UploadExtensionInstallReport(std::move(record),
+                                                            std::move(cb));
+                     },
+                     client_, std::move(record), std::move(cb)));
 }
 
 void AppInstallReportUploader::OnUploadComplete(bool success) {

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

@@ -22,6 +22,7 @@
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "components/policy/proto/record.pb.h"
 #include "components/policy/proto/record_constants.pb.h"
+#include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace reporting {
@@ -51,19 +52,14 @@ MATCHER_P(MatchValue, expected, "matches base::Value") {
 
 class TestCallbackWaiter {
  public:
-  TestCallbackWaiter()
-      : completed_(base::WaitableEvent::ResetPolicy::MANUAL,
-                   base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+  TestCallbackWaiter() : run_loop_(std::make_unique<base::RunLoop>()) {}
 
-  virtual void Signal() {
-    DCHECK(!completed_.IsSignaled());
-    completed_.Signal();
-  }
+  virtual void Signal() { run_loop_->Quit(); }
 
-  void Wait() { completed_.Wait(); }
+  void Wait() { run_loop_->Run(); }
 
  protected:
-  base::WaitableEvent completed_;
+  std::unique_ptr<base::RunLoop> run_loop_;
 };
 
 class AppInstallReportHandlerTest : public testing::Test {
@@ -76,8 +72,7 @@ class AppInstallReportHandlerTest : public testing::Test {
   }
 
  protected:
-  base::test::TaskEnvironment task_envrionment_{
-      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  content::BrowserTaskEnvironment task_envrionment_;
 
   policy::MockCloudPolicyClient client_;
 };
@@ -104,7 +99,7 @@ TEST_F(AppInstallReportHandlerTest, AcceptsValidRecord) {
   TestCallbackWaiter waiter;
   TestRecord test_record;
   EXPECT_CALL(client_,
-              UploadAppInstallReport_(MatchValue(test_record.data()), _))
+              UploadExtensionInstallReport_(MatchValue(test_record.data()), _))
       .WillOnce(WithArgs<1>(
           Invoke([&waiter](AppInstallReportHandler::ClientCallback& callback) {
             std::move(callback).Run(true);
@@ -118,7 +113,7 @@ TEST_F(AppInstallReportHandlerTest, AcceptsValidRecord) {
 }
 
 TEST_F(AppInstallReportHandlerTest, DeniesInvalidDestination) {
-  EXPECT_CALL(client_, UploadAppInstallReport_(_, _)).Times(0);
+  EXPECT_CALL(client_, UploadExtensionInstallReport_(_, _)).Times(0);
   AppInstallReportHandler handler(&client_);
 
   TestRecord test_record;
@@ -130,7 +125,7 @@ TEST_F(AppInstallReportHandlerTest, DeniesInvalidDestination) {
 }
 
 TEST_F(AppInstallReportHandlerTest, DeniesInvalidData) {
-  EXPECT_CALL(client_, UploadAppInstallReport_(_, _)).Times(0);
+  EXPECT_CALL(client_, UploadExtensionInstallReport_(_, _)).Times(0);
   AppInstallReportHandler handler(&client_);
 
   TestRecord test_record;
@@ -145,7 +140,7 @@ TEST_F(AppInstallReportHandlerTest, ReportsUnsuccessfulCall) {
 
   TestRecord test_record;
   EXPECT_CALL(client_,
-              UploadAppInstallReport_(MatchValue(test_record.data()), _))
+              UploadExtensionInstallReport_(MatchValue(test_record.data()), _))
       .WillOnce(WithArgs<1>(
           Invoke([&waiter](AppInstallReportHandler::ClientCallback& callback) {
             std::move(callback).Run(false);
@@ -164,13 +159,10 @@ class TestCallbackWaiterWithCounter : public TestCallbackWaiter {
       : counter_limit_(counter_limit) {}
 
   void Signal() override {
-    DCHECK(!completed_.IsSignaled());
-    const int new_counter = --counter_limit_;
-    DCHECK_GE(new_counter, 0);
-    if (new_counter > 0) {
-      return;
+    DCHECK_GT(counter_limit_, 0);
+    if (--counter_limit_ == 0) {
+      run_loop_->Quit();
     }
-    completed_.Signal();
   }
 
  private:
@@ -183,7 +175,7 @@ TEST_F(AppInstallReportHandlerTest, AcceptsMultipleValidRecords) {
 
   TestRecord test_record;
   EXPECT_CALL(client_,
-              UploadAppInstallReport_(MatchValue(test_record.data()), _))
+              UploadExtensionInstallReport_(MatchValue(test_record.data()), _))
       .WillRepeatedly(WithArgs<1>(
           Invoke([&waiter](AppInstallReportHandler::ClientCallback& callback) {
             std::move(callback).Run(true);

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

@@ -20,6 +20,8 @@
 #include "components/policy/core/common/cloud/user_cloud_policy_manager.h"
 #include "components/policy/proto/record.pb.h"
 #include "components/policy/proto/record_constants.pb.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
@@ -218,14 +220,20 @@ DmServerUploadService::DmServerUploadService(
       upload_cb_(upload_cb),
       sequenced_task_runner_(base::ThreadPool::CreateSequencedTaskRunner({})) {}
 
-DmServerUploadService::~DmServerUploadService() = default;
+DmServerUploadService::~DmServerUploadService() {
+  if (client_) {
+    base::PostTask(
+        FROM_HERE, {content::BrowserThread::UI},
+        base::BindOnce(
+            [](std::unique_ptr<policy::CloudPolicyClient> cloud_policy_client) {
+              cloud_policy_client.reset();
+            },
+            std::move(client_)));
+  }
+}
 
 Status DmServerUploadService::EnqueueUpload(
     std::unique_ptr<std::vector<EncryptedRecord>> records) {
-  if (!GetClient()->is_registered()) {
-    return Status(error::UNAVAILABLE, "DmServer is currently unavailable.");
-  }
-
   Start<DmServerUploader>(
       std::move(records), &record_handlers_,
       base::BindOnce(&DmServerUploadService::UploadCompletion,

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

@@ -3,19 +3,20 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/policy/messaging_layer/upload/upload_client.h"
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
 #include "base/json/json_writer.h"
 #include "base/test/task_environment.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "base/values.h"
 #include "chrome/browser/policy/messaging_layer/upload/app_install_report_handler.h"
+#include "components/account_id/account_id.h"
 #include "components/policy/core/common/cloud/dm_token.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "components/policy/proto/record.pb.h"
 #include "components/policy/proto/record_constants.pb.h"
-
-#include "base/bind.h"
-#include "base/files/file_path.h"
-#include "base/test/test_mock_time_task_runner.h"
-#include "components/account_id/account_id.h"
+#include "content/public/test/browser_task_environment.h"
 #include "services/network/test/test_network_connection_tracker.h"
 
 namespace reporting {
@@ -29,19 +30,14 @@ using testing::WithArgs;
 
 class TestCallbackWaiter {
  public:
-  TestCallbackWaiter()
-      : completed_(base::WaitableEvent::ResetPolicy::MANUAL,
-                   base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+  TestCallbackWaiter() : run_loop_(std::make_unique<base::RunLoop>()) {}
 
-  virtual void Signal() {
-    DCHECK(!completed_.IsSignaled());
-    completed_.Signal();
-  }
+  virtual void Signal() { run_loop_->Quit(); }
 
-  void Wait() { completed_.Wait(); }
+  void Wait() { run_loop_->Run(); }
 
  protected:
-  base::WaitableEvent completed_;
+  std::unique_ptr<base::RunLoop> run_loop_;
 };
 
 class TestCallbackWaiterWithCounter : public TestCallbackWaiter {
@@ -50,10 +46,9 @@ class TestCallbackWaiterWithCounter : public TestCallbackWaiter {
       : counter_limit_(counter_limit) {}
 
   void Signal() override {
-    DCHECK(!completed_.IsSignaled());
     DCHECK_GT(counter_limit_, 0);
     if (--counter_limit_ == 0) {
-      completed_.Signal();
+      run_loop_->Quit();
     }
   }
 
@@ -62,8 +57,7 @@ class TestCallbackWaiterWithCounter : public TestCallbackWaiter {
 };
 
 TEST(UploadClientTest, CreateUploadClient) {
-  base::test::TaskEnvironment task_envrionment{
-      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  content::BrowserTaskEnvironment task_envrionment_;
 
   const int kExpectedCallTimes = 10;
   const uint64_t kGenerationId = 1234;
@@ -74,7 +68,7 @@ TEST(UploadClientTest, CreateUploadClient) {
   client->SetDMToken(
       policy::DMToken::CreateValidTokenForTesting("FAKE_DM_TOKEN").value());
 
-  EXPECT_CALL(*client, UploadAppInstallReport_(_, _))
+  EXPECT_CALL(*client, UploadExtensionInstallReport_(_, _))
       .WillRepeatedly(WithArgs<1>(
           Invoke([&waiter](AppInstallReportHandler::ClientCallback& callback) {
             std::move(callback).Run(true);