PerUserTopicSubscriptionManager: avoid subscriptions backoff bypassing

In the previous form UpdateSubscribedTopics() replaced pending
subscription request even if the same request was scheduled and this
led to bypassing exponential backoff.

This patch ensures that old subscription request preserved and adds
early exit from StartPendingSubscriptionRequest().

Bug: 1020117
Change-Id: I0994e8dc4256e9fc5b77ca2d8753f3cb356ddb4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2043796
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739421}

components/invalidation/impl/per_user_topic_subscription_manager.cc

@@ -276,11 +276,19 @@ void PerUserTopicSubscriptionManager::UpdateSubscribedTopics(
   DropAllSavedSubscriptionsOnTokenChange();
 
   for (const auto& topic : topics) {
+    auto it = pending_subscriptions_.find(topic.first);
+    if (it != pending_subscriptions_.end() &&
+        it->second->type == PerUserTopicSubscriptionRequest::SUBSCRIBE) {
+      // Do not update SubscriptionEntry if there is no changes, to not loose
+      // backoff timer.
+      continue;
+    }
+
     // If the topic isn't subscribed yet, schedule the subscription.
     if (topic_to_private_topic_.find(topic.first) ==
         topic_to_private_topic_.end()) {
-      // If there was already a pending request for this topic, it'll get
-      // destroyed and replaced by the new one.
+      // If there was already a pending unsubscription request for this topic,
+      // it'll get destroyed and replaced by the new one.
       pending_subscriptions_[topic.first] = std::make_unique<SubscriptionEntry>(
           topic.first,
           base::BindRepeating(
@@ -297,6 +305,12 @@ void PerUserTopicSubscriptionManager::UpdateSubscribedTopics(
        it != topic_to_private_topic_.end();) {
     Topic topic = it->first;
     if (topics.find(topic) == topics.end()) {
+      // Unsubscription request may only replace pending subscription request,
+      // because topic immediately deleted from |topic_to_private_topic_| when
+      // unsubsciption request scheduled.
+      DCHECK(pending_subscriptions_.count(topic) == 0 ||
+             pending_subscriptions_[topic]->type ==
+                 PerUserTopicSubscriptionRequest::SUBSCRIBE);
       // If there was already a pending request for this topic, it'll get
       // destroyed and replaced by the new one.
       pending_subscriptions_[topic] = std::make_unique<SubscriptionEntry>(
@@ -316,6 +330,9 @@ void PerUserTopicSubscriptionManager::UpdateSubscribedTopics(
       ++it;
     }
   }
+  // TODO(crbug.com/1020117): remove pending subscriptions for topics we're not
+  // longer interested. Replacement by unsubscription request might be
+  // required, because request can already reach the server.
 
   // Kick off the process of actually processing the (un)subscriptions we just
   // scheduled.
@@ -345,9 +362,10 @@ void PerUserTopicSubscriptionManager::StartPendingSubscriptionRequest(
                  << " which is not in the registration map";
     return;
   }
-  // If retry was scheduled, it will end up in this function and redundant
-  // request cancellation.
-  it->second->request_retry_timer_.Stop();
+  if (it->second->request_retry_timer_.IsRunning()) {
+    // A retry is already scheduled for this request; nothing to do.
+    return;
+  }
 
   PerUserTopicSubscriptionRequest::Builder builder;
   it->second->request = builder.SetInstanceIdToken(instance_id_token_)

components/invalidation/impl/per_user_topic_subscription_manager_unittest.cc

@@ -300,7 +300,7 @@ TEST_F(PerUserTopicSubscriptionManagerTest, ShouldRepeatRequestsOnFailure) {
       ids, kFakeInstanceIdToken);
   // This should have resulted in a request for an access token. Return one.
   identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
-      "access_token", base::Time::Now());
+      "access_token", base::Time::Max());
 
   // Wait for the subscription requests to happen.
   base::RunLoop().RunUntilIdle();
@@ -314,6 +314,13 @@ TEST_F(PerUserTopicSubscriptionManagerTest, ShouldRepeatRequestsOnFailure) {
   // The second attempt will succeed.
   AddCorrectSubscriptionResponce();
 
+  // Repeating subscriptions shouldn't bypass backoff.
+  // This should have resulted in a request for an access token. Return one.
+  per_user_topic_subscription_manager->UpdateSubscribedTopics(
+      ids, kFakeInstanceIdToken);
+  identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
+      "access_token", base::Time::Max());
+
   // Initial backoff is 2 seconds with 20% jitter, so the minimum possible delay
   // is 1600ms. Advance time to just before that; nothing should have changed
   // yet.