Modify CloudPolicyClient to support multiple concurrent requests.

CloudPolicyClient previously only allowed a single outstanding request at once,
and issuing a new request would cause the previous request to silently fail.
This behavior made sense for policy fetches, but not for other types of
out-of-band requests (status uploads, certificate uploads, etc).

CloudPolicyClient now allows multiple requests at once, and differentiates
between policy-fetch/registration related requests and out-of-band requests.

BUG=452563

Review URL: https://codereview.chromium.org/885653007

Cr-Commit-Position: refs/heads/master@{#315084}

components/policy/core/common/cloud/cloud_policy_client.h

@@ -14,6 +14,7 @@
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
 #include "base/time/time.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
@@ -216,6 +217,9 @@ class POLICY_EXPORT CloudPolicyClient {
 
   scoped_refptr<net::URLRequestContextGetter> GetRequestContext();
 
+  // Returns the number of active requests.
+  int GetActiveRequestCountForTest() const;
+
  protected:
   // A set of (policy type, settings entity ID) pairs to fetch.
   typedef std::set<std::pair<std::string, std::string>> PolicyTypeSet;
@@ -249,6 +253,7 @@ class POLICY_EXPORT CloudPolicyClient {
 
   // Callback for certificate upload requests.
   void OnCertificateUploadCompleted(
+      const DeviceManagementRequestJob* job,
       const StatusCallback& callback,
       DeviceManagementStatus status,
       int net_error,
@@ -256,11 +261,15 @@ class POLICY_EXPORT CloudPolicyClient {
 
   // Callback for status upload requests.
   void OnStatusUploadCompleted(
+      const DeviceManagementRequestJob* job,
       const StatusCallback& callback,
       DeviceManagementStatus status,
       int net_error,
       const enterprise_management::DeviceManagementResponse& response);
 
+  // Helper to remove a job from request_jobs_.
+  void RemoveJob(const DeviceManagementRequestJob* job);
+
   // Observer notification helpers.
   void NotifyPolicyFetched();
   void NotifyRegistrationStateChanged();
@@ -293,7 +302,14 @@ class POLICY_EXPORT CloudPolicyClient {
 
   // Used for issuing requests to the cloud.
   DeviceManagementService* service_;
-  scoped_ptr<DeviceManagementRequestJob> request_job_;
+
+  // Only one outstanding policy fetch is allowed, so this is tracked in
+  // its own member variable.
+  scoped_ptr<DeviceManagementRequestJob> policy_fetch_request_job_;
+
+  // All of the outstanding non-policy-fetch request jobs. These jobs are
+  // silently cancelled if Unregister() is called.
+  ScopedVector<DeviceManagementRequestJob> request_jobs_;
 
   // The policy responses returned by the last policy fetch operation.
   ResponseMap responses_;

components/policy/core/common/cloud/cloud_policy_client_unittest.cc

@@ -8,6 +8,7 @@
 #include <set>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -479,7 +480,7 @@ TEST_F(CloudPolicyClientTest, UnregisterFailure) {
 TEST_F(CloudPolicyClientTest, PolicyFetchWithExtensionPolicy) {
   Register();
 
-  // Setup the |expected_responses| and |policy_response_|.
+  // Set up the |expected_responses| and |policy_response_|.
   static const char* kExtensions[] = {
     "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
     "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
@@ -603,6 +604,76 @@ TEST_F(CloudPolicyClientTest, UploadStatus) {
   EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
 }
 
+TEST_F(CloudPolicyClientTest, UploadStatusWhilePolicyFetchActive) {
+  Register();
+  MockDeviceManagementJob* upload_status_job = nullptr;
+  EXPECT_CALL(service_,
+              CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_STATUS,
+                        request_context_))
+      .WillOnce(service_.CreateAsyncJob(&upload_status_job));
+  EXPECT_CALL(service_, StartJob(_, _, _, _, _, _, _));
+  EXPECT_CALL(upload_observer_, OnUploadComplete(true)).Times(1);
+  CloudPolicyClient::StatusCallback callback = base::Bind(
+      &MockUploadObserver::OnUploadComplete,
+      base::Unretained(&upload_observer_));
+  em::DeviceStatusReportRequest device_status;
+  em::SessionStatusReportRequest session_status;
+  client_->UploadDeviceStatus(&device_status, &session_status, callback);
+
+  // Now initiate a policy fetch - this should not cancel the upload job.
+  ExpectPolicyFetch(kDMToken, dm_protocol::kValueUserAffiliationNone);
+  EXPECT_CALL(observer_, OnPolicyFetched(_));
+  client_->FetchPolicy();
+  CheckPolicyResponse();
+
+  upload_status_job->SendResponse(DM_STATUS_SUCCESS, upload_status_response_);
+  EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
+}
+
+TEST_F(CloudPolicyClientTest, MultipleActiveRequests) {
+  Register();
+
+  // Set up pending upload status job.
+  MockDeviceManagementJob* upload_status_job = nullptr;
+  EXPECT_CALL(service_,
+              CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_STATUS,
+                        request_context_))
+      .WillOnce(service_.CreateAsyncJob(&upload_status_job));
+  EXPECT_CALL(service_, StartJob(_, _, _, _, _, _, _));
+  CloudPolicyClient::StatusCallback callback = base::Bind(
+      &MockUploadObserver::OnUploadComplete,
+      base::Unretained(&upload_observer_));
+  em::DeviceStatusReportRequest device_status;
+  em::SessionStatusReportRequest session_status;
+  client_->UploadDeviceStatus(&device_status, &session_status, callback);
+
+  // Set up pending upload certificate job.
+  MockDeviceManagementJob* upload_certificate_job = nullptr;
+  EXPECT_CALL(service_,
+              CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_CERTIFICATE,
+                        request_context_))
+      .WillOnce(service_.CreateAsyncJob(&upload_certificate_job));
+  EXPECT_CALL(service_, StartJob(_, _, _, _, _, _, _));
+
+  // Expect two calls on our upload observer, one for the status upload and
+  // one for the certificate upload.
+  CloudPolicyClient::StatusCallback callback2 = base::Bind(
+      &MockUploadObserver::OnUploadComplete,
+      base::Unretained(&upload_observer_));
+  client_->UploadCertificate(kDeviceCertificate, callback2);
+  EXPECT_EQ(2, client_->GetActiveRequestCountForTest());
+
+  // Now satisfy both active jobs.
+  EXPECT_CALL(upload_observer_, OnUploadComplete(true)).Times(2);
+  upload_status_job->SendResponse(DM_STATUS_SUCCESS, upload_status_response_);
+  EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
+  upload_certificate_job->SendResponse(DM_STATUS_SUCCESS,
+                                       upload_certificate_response_);
+  EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
+
+  EXPECT_EQ(0, client_->GetActiveRequestCountForTest());
+}
+
 TEST_F(CloudPolicyClientTest, UploadStatusFailure) {
   Register();
 
@@ -623,4 +694,27 @@ TEST_F(CloudPolicyClientTest, UploadStatusFailure) {
   EXPECT_EQ(DM_STATUS_REQUEST_FAILED, client_->status());
 }
 
+TEST_F(CloudPolicyClientTest, RequestCancelOnUnregister) {
+  Register();
+
+  // Set up pending upload status job.
+  MockDeviceManagementJob* upload_status_job = nullptr;
+  EXPECT_CALL(service_,
+              CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_STATUS,
+                        request_context_))
+      .WillOnce(service_.CreateAsyncJob(&upload_status_job));
+  EXPECT_CALL(service_, StartJob(_, _, _, _, _, _, _));
+  CloudPolicyClient::StatusCallback callback = base::Bind(
+      &MockUploadObserver::OnUploadComplete,
+      base::Unretained(&upload_observer_));
+  em::DeviceStatusReportRequest device_status;
+  em::SessionStatusReportRequest session_status;
+  client_->UploadDeviceStatus(&device_status, &session_status, callback);
+  EXPECT_EQ(1, client_->GetActiveRequestCountForTest());
+  EXPECT_CALL(observer_, OnRegistrationStateChanged(_));
+  ExpectUnregistration(kDMToken);
+  client_->Unregister();
+  EXPECT_EQ(0, client_->GetActiveRequestCountForTest());
+}
+
 }  // namespace policy