PerUserTopicSubscriptionManager: avoid access token backoff bypassing

RequestAccessToken() has several calling sides and in the old
implementation any call would bypass the exponential backoff.

The fix is to just exit early if access token request is backed off.

This CL also adds test for repeating access token request on failure,
which ensures that access token requested again after backoff time and
UpdateSubscribedTopics() call doesn't lead to backoff bypassing.

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

components/invalidation/impl/per_user_topic_subscription_manager.cc

@@ -483,12 +483,15 @@ void PerUserTopicSubscriptionManager::RemoveObserver(Observer* observer) {
 
 void PerUserTopicSubscriptionManager::RequestAccessToken() {
   // Only one active request at a time.
-  if (access_token_fetcher_ != nullptr)
+  if (access_token_fetcher_ != nullptr) {
     return;
-  // TODO(crbug.com/1020117): If the timer is already running, then this method
-  // should probably early-out instead of starting a request immediately. As it
-  // is, this might bypass the exponential backoff.
-  request_access_token_retry_timer_.Stop();
+  }
+  if (request_access_token_retry_timer_.IsRunning()) {
+    // Previous access token request failed and new request shouldn't be issued
+    // until backoff timer passed.
+    return;
+  }
+
   access_token_.clear();
   access_token_fetcher_ = identity_provider_->FetchAccessToken(
       "fcm_invalidation", {kFCMOAuthScope},

components/invalidation/impl/per_user_topic_subscription_manager_unittest.cc

@@ -29,6 +29,7 @@
 
 using testing::_;
 using testing::Contains;
+using testing::NiceMock;
 using testing::Not;
 
 namespace syncer {
@@ -53,6 +54,10 @@ const char kFakeInstanceIdToken[] = "fake_instance_id_token";
 class MockIdentityDiagnosticsObserver
     : public signin::IdentityManager::DiagnosticsObserver {
  public:
+  MOCK_METHOD3(OnAccessTokenRequested,
+               void(const CoreAccountId&,
+                    const std::string&,
+                    const identity::ScopeSet&));
   MOCK_METHOD2(OnAccessTokenRemovedFromCache,
                void(const CoreAccountId&, const identity::ScopeSet&));
 };
@@ -273,7 +278,7 @@ TEST_F(PerUserTopicSubscriptionManagerTest, ShouldRepeatRequestsOnFailure) {
   // For this test, we want to manually control when access tokens are returned.
   identity_test_env()->SetAutomaticIssueOfAccessTokens(false);
 
-  MockIdentityDiagnosticsObserver identity_observer;
+  NiceMock<MockIdentityDiagnosticsObserver> identity_observer;
   identity_test_env()->identity_manager()->AddDiagnosticsObserver(
       &identity_observer);
 
@@ -330,12 +335,66 @@ TEST_F(PerUserTopicSubscriptionManagerTest, ShouldRepeatRequestsOnFailure) {
       &identity_observer);
 }
 
+TEST_F(PerUserTopicSubscriptionManagerTest,
+       ShouldRepeatAccessTokenRequestOnFailure) {
+  // For this test, we want to manually control when access tokens are returned.
+  identity_test_env()->SetAutomaticIssueOfAccessTokens(false);
+
+  NiceMock<MockIdentityDiagnosticsObserver> identity_observer;
+  identity_test_env()->identity_manager()->AddDiagnosticsObserver(
+      &identity_observer);
+
+  auto ids = GetSequenceOfTopics(kInvalidationObjectIdsCount);
+
+  auto per_user_topic_subscription_manager = BuildRegistrationManager();
+  ASSERT_TRUE(per_user_topic_subscription_manager->GetSubscribedTopicsForTest()
+                  .empty());
+
+  // Emulate failure on first access token request.
+  EXPECT_CALL(identity_observer, OnAccessTokenRequested(_, _, _));
+  per_user_topic_subscription_manager->UpdateSubscribedTopics(
+      ids, kFakeInstanceIdToken);
+  identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithError(
+      GoogleServiceAuthError(GoogleServiceAuthError::CONNECTION_FAILED));
+  testing::Mock::VerifyAndClearExpectations(&identity_observer);
+
+  // 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.
+  EXPECT_CALL(identity_observer, OnAccessTokenRequested(_, _, _)).Times(0);
+  // UpdateSubscribedTopics() call shouldn't lead to backoff bypassing.
+  per_user_topic_subscription_manager->UpdateSubscribedTopics(
+      ids, kFakeInstanceIdToken);
+  FastForwardTimeBy(base::TimeDelta::FromMilliseconds(1500));
+  testing::Mock::VerifyAndClearExpectations(&identity_observer);
+
+  // The maximum backoff is 2 seconds; advance to just past that. Now access
+  // token should be requested.
+  EXPECT_CALL(identity_observer, OnAccessTokenRequested(_, _, _));
+  FastForwardTimeBy(base::TimeDelta::FromMilliseconds(600));
+  testing::Mock::VerifyAndClearExpectations(&identity_observer);
+
+  // Add valid responses to access token and subscription requests and ensure
+  // that subscribtions finished.
+  identity_test_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
+      "valid_access_token", base::Time::Max());
+  AddCorrectSubscriptionResponce();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(per_user_topic_subscription_manager->GetSubscribedTopicsForTest()
+                   .empty());
+  EXPECT_TRUE(
+      per_user_topic_subscription_manager->HaveAllRequestsFinishedForTest());
+
+  identity_test_env()->identity_manager()->RemoveDiagnosticsObserver(
+      &identity_observer);
+}
+
 TEST_F(PerUserTopicSubscriptionManagerTest,
        ShouldInvalidateAccessTokenOnUnauthorized) {
   // For this test, we need to manually control when access tokens are returned.
   identity_test_env()->SetAutomaticIssueOfAccessTokens(false);
 
-  MockIdentityDiagnosticsObserver identity_observer;
+  NiceMock<MockIdentityDiagnosticsObserver> identity_observer;
   identity_test_env()->identity_manager()->AddDiagnosticsObserver(
       &identity_observer);
 
@@ -392,7 +451,7 @@ TEST_F(PerUserTopicSubscriptionManagerTest,
   // For this test, we need to manually control when access tokens are returned.
   identity_test_env()->SetAutomaticIssueOfAccessTokens(false);
 
-  MockIdentityDiagnosticsObserver identity_observer;
+  NiceMock<MockIdentityDiagnosticsObserver> identity_observer;
   identity_test_env()->identity_manager()->AddDiagnosticsObserver(
       &identity_observer);