Handle BinaryFCMService registration conflicts

Race conditions between token unregistration and registration are
handled by queueing registrations when unregistrations are pending.

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

chrome/browser/safe_browsing/cloud_content_scanning/binary_fcm_service.cc

@@ -79,6 +79,11 @@ BinaryFCMService::~BinaryFCMService() {
 }
 
 void BinaryFCMService::GetInstanceID(GetInstanceIDCallback callback) {
+  if (pending_unregistrations_count_ > 0) {
+    QueueGetInstanceIDCallback(std::move(callback));
+    return;
+  }
+
   instance_id_driver_->GetInstanceID(kBinaryFCMServiceAppId)
       ->GetToken(
           kBinaryFCMServiceSenderId, instance_id::kGCMScope,
@@ -89,6 +94,18 @@ void BinaryFCMService::GetInstanceID(GetInstanceIDCallback callback) {
                          weakptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
+void BinaryFCMService::QueueGetInstanceIDCallback(
+    GetInstanceIDCallback callback) {
+  pending_token_calls_.push_back(
+      base::BindOnce(&BinaryFCMService::GetInstanceID,
+                     weakptr_factory_.GetWeakPtr(), std::move(callback)));
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&BinaryFCMService::MaybeRunNextQueuedOperation,
+                     weakptr_factory_.GetWeakPtr()),
+      delay_between_pending_attempts_);
+}
+
 void BinaryFCMService::SetCallbackForToken(const std::string& token,
                                            OnMessageCallback callback) {
   message_token_map_[token] = std::move(callback);
@@ -108,10 +125,13 @@ void BinaryFCMService::ClearCallbackForToken(const std::string& token) {
 void BinaryFCMService::UnregisterInstanceID(
     const std::string& instance_id,
     UnregisterInstanceIDCallback callback) {
-  instance_id_caller_counts_[instance_id]--;
-  if (instance_id_caller_counts_[instance_id] == 0) {
-    UnregisterInstanceIDImpl(instance_id, std::move(callback));
-    instance_id_caller_counts_.erase(instance_id);
+  if (instance_id_caller_counts_.contains(instance_id)) {
+    DCHECK_NE(0u, instance_id_caller_counts_[instance_id]);
+    instance_id_caller_counts_[instance_id]--;
+    if (instance_id_caller_counts_[instance_id] == 0) {
+      UnregisterInstanceIDImpl(instance_id, std::move(callback));
+      instance_id_caller_counts_.erase(instance_id);
+    }
   }
 }
 
@@ -133,14 +153,7 @@ void BinaryFCMService::OnGetInstanceID(GetInstanceIDCallback callback,
       MaybeRunNextQueuedOperation();
     }
   } else if (result == instance_id::InstanceID::ASYNC_OPERATION_PENDING) {
-    pending_token_calls_.push_back(
-        base::BindOnce(&BinaryFCMService::GetInstanceID,
-                       weakptr_factory_.GetWeakPtr(), std::move(callback)));
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(&BinaryFCMService::MaybeRunNextQueuedOperation,
-                       weakptr_factory_.GetWeakPtr()),
-        delay_between_pending_attempts_);
+    QueueGetInstanceIDCallback(std::move(callback));
   } else {
     std::move(callback).Run(kInvalidId);
   }
@@ -244,6 +257,7 @@ bool BinaryFCMService::CanHandle(const std::string& app_id) const {
 void BinaryFCMService::UnregisterInstanceIDImpl(
     const std::string& instance_id,
     UnregisterInstanceIDCallback callback) {
+  ++pending_unregistrations_count_;
   instance_id_driver_->GetInstanceID(kBinaryFCMServiceAppId)
       ->DeleteToken(kBinaryFCMServiceSenderId, instance_id::kGCMScope,
                     base::BindOnce(&BinaryFCMService::OnInstanceIDUnregistered,
@@ -255,6 +269,7 @@ void BinaryFCMService::OnInstanceIDUnregistered(
     const std::string& token,
     UnregisterInstanceIDCallback callback,
     instance_id::InstanceID::Result result) {
+  --pending_unregistrations_count_;
   switch (result) {
     case instance_id::InstanceID::Result::SUCCESS:
       std::move(callback).Run(true);

chrome/browser/safe_browsing/cloud_content_scanning/binary_fcm_service.h

@@ -97,6 +97,8 @@ class BinaryFCMService : public gcm::GCMAppHandler {
                        const std::string& instance_id,
                        instance_id::InstanceID::Result result);
 
+  void QueueGetInstanceIDCallback(GetInstanceIDCallback callback);
+
   // Run the next queued operation, and post a task for another operation if
   // necessary.
   void MaybeRunNextQueuedOperation();
@@ -117,6 +119,9 @@ class BinaryFCMService : public gcm::GCMAppHandler {
   // Queue of pending GetToken calls.
   std::deque<base::OnceClosure> pending_token_calls_;
 
+  // Count of unregistrations currently happening asynchronously.
+  size_t pending_unregistrations_count_ = 0;
+
   // Delay between attempts to dequeue pending operations. Not constant so we
   // can override it in tests.
   base::TimeDelta delay_between_pending_attempts_ =

chrome/browser/safe_browsing/cloud_content_scanning/binary_fcm_service_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/notreached.h"
 #include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/time/time.h"
 #include "chrome/browser/gcm/gcm_profile_service_factory.h"
@@ -524,6 +525,68 @@ TEST_F(BinaryFCMServiceTest, UnregisterTwoTokensTwoCalls) {
   content::RunAllTasksUntilIdle();
 }
 
+TEST_F(BinaryFCMServiceTest, UnregisterTwoTokenConflict) {
+  MockInstanceIDDriver driver;
+  MockInstanceID instance_id;
+  ON_CALL(driver, GetInstanceID(_)).WillByDefault(Return(&instance_id));
+  binary_fcm_service_.reset();
+  binary_fcm_service_ = std::make_unique<BinaryFCMService>(
+      gcm::GCMProfileServiceFactory::GetForProfile(&profile_)->driver(),
+      &driver);
+  binary_fcm_service_->SetQueuedOperationDelayForTesting(base::TimeDelta());
+  std::string first_id = BinaryFCMService::kInvalidId;
+  std::string second_id = BinaryFCMService::kInvalidId;
+
+  // Both calls to GetToken return the same value since we mock a case where the
+  // second GetToken call happens before the first DeleteToken call resolves.
+  EXPECT_CALL(instance_id, GetToken(_, _, _, _, _, _))
+      .Times(2)
+      .WillRepeatedly(
+          Invoke([](const std::string&, const std::string&, base::TimeDelta,
+                    const std::map<std::string, std::string>&,
+                    std::set<instance_id::InstanceID::Flags>,
+                    instance_id::InstanceID::GetTokenCallback callback) {
+            std::move(callback).Run("token",
+                                    instance_id::InstanceID::Result::SUCCESS);
+          }));
+
+  EXPECT_CALL(instance_id, DeleteToken(_, _, _))
+      .WillOnce(
+          Invoke([this, &second_id](
+                     const std::string&, const std::string&,
+                     instance_id::InstanceID::DeleteTokenCallback callback) {
+            // Call the second GetInstanceID here to have a conflict.
+            binary_fcm_service_->GetInstanceID(base::BindLambdaForTesting(
+                [&second_id](const std::string& instance_id) {
+                  second_id = instance_id;
+                }));
+            std::move(callback).Run(instance_id::InstanceID::Result::SUCCESS);
+          }));
+
+  // Get the first token.
+  binary_fcm_service_->GetInstanceID(base::BindLambdaForTesting(
+      [&first_id](const std::string& instance_id) { first_id = instance_id; }));
+
+  // Get the second token and unregister the first token. This is a conflict as
+  // they are the same token.
+  binary_fcm_service_->UnregisterInstanceID(
+      first_id,
+      base::BindOnce([](bool unregister) { EXPECT_TRUE(unregister); }));
+  task_environment_.RunUntilIdle();
+
+  // Unregister the second token.
+  EXPECT_CALL(instance_id, DeleteToken(_, _, _))
+      .WillOnce(
+          Invoke([](const std::string&, const std::string&,
+                    instance_id::InstanceID::DeleteTokenCallback callback) {
+            std::move(callback).Run(instance_id::InstanceID::Result::SUCCESS);
+          }));
+  binary_fcm_service_->UnregisterInstanceID(
+      second_id,
+      base::BindOnce([](bool unregister) { EXPECT_TRUE(unregister); }));
+  task_environment_.RunUntilIdle();
+}
+
 TEST_F(BinaryFCMServiceTest, QueuesGetInstanceIDOnRetriableError) {
   MockInstanceIDDriver driver;
   MockInstanceID instance_id;