PerUserTopicSubscriptionManager: on failed requests, don't back off twice

When a subscription request failed, PerUserTopicSubscriptionManager used
to call BackoffEntry::InformOfRequest(false) twice, i.e. it'd back off
more than expected. This CL fixes that and adds a test for proper
backoff.

Bug: 1020117
Change-Id: I248f3fc3a1e44b9e6f542b9c45d7fc1b187a9345
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1991426Reviewed-by: Tim Schumann <tschumann@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730224}

components/invalidation/impl/per_user_topic_subscription_manager.cc

@@ -409,8 +409,6 @@ void PerUserTopicSubscriptionManager::ScheduleRequestForRepetition(
   pending_subscriptions_[topic]->completion_callback = base::BindOnce(
       &PerUserTopicSubscriptionManager::SubscriptionFinishedForTopic,
       base::Unretained(this));
-  // TODO(crbug.com/1020117): We already called InformOfRequest(false) before in
-  // SubscriptionFinishedForTopic(), should probably not call it again here?
   pending_subscriptions_[topic]->request_backoff_.InformOfRequest(false);
   pending_subscriptions_[topic]->request_retry_timer_.Start(
       FROM_HERE,
@@ -429,7 +427,6 @@ void PerUserTopicSubscriptionManager::SubscriptionFinishedForTopic(
     ActOnSuccessfulSubscription(topic, private_topic_name, type);
   } else {
     auto it = pending_subscriptions_.find(topic);
-    it->second->request_backoff_.InformOfRequest(false);
     if (code.IsAuthFailure()) {
       // Re-request access token and try subscription requests again.
       RequestAccessToken();

components/invalidation/impl/per_user_topic_subscription_manager_unittest.cc

@@ -181,8 +181,13 @@ class PerUserTopicSubscriptionManagerTest : public testing::Test {
         CreateStatusForTest(net::OK, std::string() /* response_body */));
   }
 
+  void FastForwardTimeBy(base::TimeDelta delta) {
+    task_environment_.FastForwardBy(delta);
+  }
+
  private:
-  base::test::SingleThreadTaskEnvironment task_environment_;
+  base::test::SingleThreadTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
   data_decoder::test::InProcessDataDecoder in_process_data_decoder_;
   network::TestURLLoaderFactory url_loader_factory_;
   TestingPrefServiceSimple pref_service_;
@@ -255,18 +260,40 @@ TEST_F(PerUserTopicSubscriptionManagerTest, ShouldRepeatRequestsOnFailure) {
   ASSERT_TRUE(per_user_topic_subscription_manager->GetSubscribedTopicsForTest()
                   .empty());
 
+  // The first subscription attempt will fail.
   AddCorrectSubscriptionResponce(
-      /* private_topic */ std::string(), kFakeInstanceIdToken,
+      /*private_topic=*/std::string(), kFakeInstanceIdToken,
       net::HTTP_INTERNAL_SERVER_ERROR);
 
   per_user_topic_subscription_manager->UpdateSubscribedTopics(
       ids, kFakeInstanceIdToken);
   base::RunLoop().RunUntilIdle();
 
+  // Since the subscriptions failed, the requests should still be pending.
   EXPECT_TRUE(per_user_topic_subscription_manager->GetSubscribedTopicsForTest()
                   .empty());
   EXPECT_FALSE(
       per_user_topic_subscription_manager->HaveAllRequestsFinishedForTest());
+
+  // The second attempt will succeed.
+  AddCorrectSubscriptionResponce();
+
+  // 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.
+  FastForwardTimeBy(base::TimeDelta::FromMilliseconds(1500));
+  EXPECT_TRUE(per_user_topic_subscription_manager->GetSubscribedTopicsForTest()
+                  .empty());
+  EXPECT_FALSE(
+      per_user_topic_subscription_manager->HaveAllRequestsFinishedForTest());
+
+  // The maximum backoff is 2 seconds; advance to just past that. Now all
+  // subscriptions should have finished.
+  FastForwardTimeBy(base::TimeDelta::FromMilliseconds(600));
+  EXPECT_FALSE(per_user_topic_subscription_manager->GetSubscribedTopicsForTest()
+                   .empty());
+  EXPECT_TRUE(
+      per_user_topic_subscription_manager->HaveAllRequestsFinishedForTest());
 }
 
 TEST_F(PerUserTopicSubscriptionManagerTest, ShouldRepeatRequestsOnForbidden) {