Fix instance ID registration conflict in binary_upload_service

The conflict exists because the initial authentication request calls
RunAuthorizationCallbacks right before unregistering its instance ID,
causing a race that mostly results in the request that was waiting for
authentication to have its ID unregistered, causing it to timeout. The
fix in this CL is to only start that request after the authentication
request has finished unregistering its ID.

Bug: 1090504
Change-Id: Ieb5a7da3a7e75662105a0e00d956473f0f624c4f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225287Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776019}

chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.cc

@@ -368,12 +368,30 @@ void BinaryUploadService::FinishRequest(Request* request,
 
     // The BinaryFCMService will handle all recoverable errors. In case of
     // unrecoverable error, there's nothing we can do here.
-    binary_fcm_service_->UnregisterInstanceID(instance_id, base::DoNothing());
+    binary_fcm_service_->UnregisterInstanceID(
+        instance_id,
+        base::BindOnce(&BinaryUploadService::InstanceIDUnregisteredCallback,
+                       weakptr_factory_.GetWeakPtr()));
+  } else {
+    // |binary_fcm_service_| can be null in tests, but
+    // InstanceIDUnregisteredCallback should be called anyway so the requests
+    // waiting on authentication can complete.
+    InstanceIDUnregisteredCallback(true);
   }
 
   active_tokens_.erase(token_it);
 }
 
+void BinaryUploadService::InstanceIDUnregisteredCallback(bool) {
+  // Calling RunAuthorizationCallbacks after the instance ID of the initial
+  // authentication is unregistered avoids registration/unregistration conflicts
+  // with normal requests.
+  if (!authorization_callbacks_.empty() &&
+      can_upload_enterprise_data_.has_value()) {
+    RunAuthorizationCallbacks();
+  }
+}
+
 void BinaryUploadService::RecordRequestMetrics(
     Request* request,
     Result result,
@@ -505,7 +523,6 @@ void BinaryUploadService::ValidateDataUploadRequestCallback(
     DeepScanningClientResponse response) {
   pending_validate_data_upload_request_ = false;
   can_upload_enterprise_data_ = result == BinaryUploadService::Result::SUCCESS;
-  RunAuthorizationCallbacks();
 }
 
 void BinaryUploadService::RunAuthorizationCallbacks() {

chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.h

@@ -173,6 +173,11 @@ class BinaryUploadService : public KeyedService {
   // different URL than scans for Advanced Protection users.
   static GURL GetUploadUrl(bool is_advanced_protection_request);
 
+ protected:
+  void FinishRequest(Request* request,
+                     Result result,
+                     DeepScanningClientResponse response);
+
  private:
   friend class BinaryUploadServiceTest;
 
@@ -197,10 +202,6 @@ class BinaryUploadService : public KeyedService {
 
   void OnTimeout(Request* request);
 
-  void FinishRequest(Request* request,
-                     Result result,
-                     DeepScanningClientResponse response);
-
   bool IsActive(Request* request);
 
   void MaybeUploadForDeepScanningCallback(std::unique_ptr<Request> request,
@@ -210,6 +211,9 @@ class BinaryUploadService : public KeyedService {
   void ValidateDataUploadRequestCallback(BinaryUploadService::Result result,
                                          DeepScanningClientResponse response);
 
+  // Callback once a request's instance ID is unregistered.
+  void InstanceIDUnregisteredCallback(bool);
+
   void RecordRequestMetrics(Request* request,
                             Result result,
                             const DeepScanningClientResponse& response);

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate_browsertest.cc

@@ -46,8 +46,8 @@ class FakeBinaryUploadService : public BinaryUploadService {
   // Finish the authentication request. Called after ShowForWebContents to
   // simulate an async callback.
   void ReturnAuthorizedResponse() {
-    authorization_request_->FinishRequest(authorization_result_,
-                                          DeepScanningClientResponse());
+    FinishRequest(authorization_request_.get(), authorization_result_,
+                  DeepScanningClientResponse());
   }
 
   void SetResponseForText(BinaryUploadService::Result result,