Fix signin flakes of metrics demographics android browsertests

The fix consists to let the SyncAuthManager do a retry to fetch the access token when the first request is canceled.

Bug: 1050313
Change-Id: I173830f5b28b9fe2ec2a2d67032b52fc2eef344e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2062294
Commit-Queue: Vincent Boisselle <vincb@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746051}

chrome/browser/metrics/metrics_service_user_demographics_browsertest.cc

@@ -114,9 +114,7 @@ class MetricsServiceUserDemographicsBrowserTest
   DISALLOW_COPY_AND_ASSIGN(MetricsServiceUserDemographicsBrowserTest);
 };
 
-// Disabled on Android: https://crbug.com/1052440
 // TODO(crbug/1016118): Add the remaining test cases.
-#if !defined(OS_ANDROID)
 IN_PROC_BROWSER_TEST_P(MetricsServiceUserDemographicsBrowserTest,
                        AddSyncedUserBirthYearAndGenderToProtoData) {
   test::DemographicsTestParams param = GetParam();
@@ -163,7 +161,6 @@ IN_PROC_BROWSER_TEST_P(MetricsServiceUserDemographicsBrowserTest,
 
   test_profile_harness->service()->GetUserSettings()->SetSyncRequested(false);
 }
-#endif // !defined(OS_ANDROID)
 
 #if defined(OS_CHROMEOS)
 // Cannot test for the enabled feature on Chrome OS because there are always

components/sync/driver/sync_auth_manager.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "components/signin/public/identity_manager/access_token_fetcher.h"
@@ -54,6 +55,11 @@ constexpr net::BackoffEntry::Policy kRequestAccessTokenBackoffPolicy = {
 
 }  // namespace
 
+// Enables the retry of the token fetch without backoff on the first fetch
+// cancellation.
+const base::Feature kSyncRetryFirstCanceledTokenFetch = {
+    "SyncRetryFirstCanceledTokenFetch", base::FEATURE_ENABLED_BY_DEFAULT};
+
 SyncAuthManager::SyncAuthManager(
     signin::IdentityManager* identity_manager,
     const AccountStateChangedCallback& account_state_changed,
@@ -499,6 +505,17 @@ void SyncAuthManager::AccessTokenFetched(
   ongoing_access_token_fetch_.reset();
   DCHECK(!request_access_token_retry_timer_.IsRunning());
 
+  // Retry without backoff when the request is canceled for the first time. For
+  // more details, see inline comments of
+  // PrimaryAccountAccessTokenFetcher::OnAccessTokenFetchComplete.
+  if (base::FeatureList::IsEnabled(kSyncRetryFirstCanceledTokenFetch) &&
+      error.state() == GoogleServiceAuthError::REQUEST_CANCELED &&
+      !access_token_retried_) {
+    access_token_retried_ = true;
+    RequestAccessToken();
+    return;
+  }
+
   access_token_ = access_token_info.token;
   partial_token_status_.token_response_time = base::Time::Now();
   partial_token_status_.last_get_token_error = error;

components/sync/driver/sync_auth_manager.h

@@ -29,6 +29,8 @@ struct AccessTokenInfo;
 
 namespace syncer {
 
+extern const base::Feature kSyncRetryFirstCanceledTokenFetch;
+
 struct SyncCredentials;
 
 // SyncAuthManager tracks the account to be used for Sync and its authentication
@@ -198,6 +200,12 @@ class SyncAuthManager : public signin::IdentityManager::Observer {
   // |has_token| and |next_token_request_time| get computed on demand.
   SyncTokenStatus partial_token_status_;
 
+  // Whether there was a retry done to fetch the access token when the request
+  // was cancelled for the first time. This works around an issue that can only
+  // happen once during browser startup, so it's sufficient to have a single
+  // retry (i.e. not per request).
+  bool access_token_retried_ = false;
+
   base::WeakPtrFactory<SyncAuthManager> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(SyncAuthManager);

components/sync/driver/sync_auth_manager_unittest.cc

@@ -349,6 +349,80 @@ TEST_F(SyncAuthManagerTest,
             GoogleServiceAuthError::AuthErrorNone());
 }
 
+TEST_F(
+    SyncAuthManagerTest,
+    RetriesAccessTokenFetchWithBackoffOnFirstCancelTransientFailWhenDisabled) {
+  // Disable the first retry without backoff on cancellation.
+  base::test::ScopedFeatureList local_feature;
+  local_feature.InitAndDisableFeature(kSyncRetryFirstCanceledTokenFetch);
+
+  CoreAccountId account_id =
+      identity_env()->MakePrimaryAccountAvailable("test@email.com").account_id;
+  auto auth_manager = CreateAuthManager();
+  auth_manager->RegisterForAuthNotifications();
+  ASSERT_EQ(auth_manager->GetActiveAccountInfo().account_info.account_id,
+            account_id);
+
+  auth_manager->ConnectionOpened();
+
+  identity_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithError(
+      GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
+
+  // Expect retry with backoff.
+  EXPECT_TRUE(auth_manager->IsRetryingAccessTokenFetchForTest());
+}
+
+TEST_F(SyncAuthManagerTest,
+       RetriesAccessTokenFetchWithoutBackoffOnceOnFirstCancelTransientFailure) {
+  CoreAccountId account_id =
+      identity_env()->MakePrimaryAccountAvailable("test@email.com").account_id;
+  auto auth_manager = CreateAuthManager();
+  auth_manager->RegisterForAuthNotifications();
+  ASSERT_EQ(auth_manager->GetActiveAccountInfo().account_info.account_id,
+            account_id);
+
+  auth_manager->ConnectionOpened();
+
+  identity_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithError(
+      GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
+
+  // Expect no backoff the first time the request is canceled.
+  EXPECT_FALSE(auth_manager->IsRetryingAccessTokenFetchForTest());
+
+  // Cancel the retry as well.
+  identity_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithError(
+      GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
+
+  // Expect retry with backoff when the first retry was also canceled.
+  EXPECT_TRUE(auth_manager->IsRetryingAccessTokenFetchForTest());
+}
+
+TEST_F(SyncAuthManagerTest,
+       RetriesAccessTokenFetchOnFirstCancelTransientFailure) {
+  CoreAccountId account_id =
+      identity_env()->MakePrimaryAccountAvailable("test@email.com").account_id;
+  auto auth_manager = CreateAuthManager();
+  auth_manager->RegisterForAuthNotifications();
+  ASSERT_EQ(auth_manager->GetActiveAccountInfo().account_info.account_id,
+            account_id);
+
+  auth_manager->ConnectionOpened();
+
+  identity_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithError(
+      GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
+
+  // Expect no backoff the first time the request is canceled.
+  EXPECT_FALSE(auth_manager->IsRetryingAccessTokenFetchForTest());
+
+  // Retry is a success.
+  identity_env()->WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
+      "access_token", base::Time::Now() + base::TimeDelta::FromHours(1));
+
+  ASSERT_EQ(auth_manager->GetCredentials().access_token, "access_token");
+  // Don't expect any backoff when the retry is a success.
+  EXPECT_FALSE(auth_manager->IsRetryingAccessTokenFetchForTest());
+}
+
 TEST_F(SyncAuthManagerTest, AbortsAccessTokenFetchOnPersistentFailure) {
   CoreAccountId account_id =
       identity_env()->MakePrimaryAccountAvailable("test@email.com").account_id;