Store the error seen during account reconciliation

Bug: 851455
Change-Id: I4570ad8ae909ec32015f38deb1d3d8339f55582f
Reviewed-on: https://chromium-review.googlesource.com/1095102Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Kush Sinha <sinhak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567620}

components/signin/core/browser/account_reconcilor.cc

@@ -95,7 +95,7 @@ AccountReconcilor::AccountReconcilor(
       registered_with_content_settings_(false),
       is_reconcile_started_(false),
       first_execution_(true),
-      error_during_last_reconcile_(false),
+      error_during_last_reconcile_(GoogleServiceAuthError::AuthErrorNone()),
       reconcile_is_noop_(true),
       chrome_accounts_changed_(false),
       account_reconcilor_lock_count_(0),
@@ -213,7 +213,8 @@ void AccountReconcilor::UnregisterWithCookieManagerService() {
 
 signin_metrics::AccountReconcilorState AccountReconcilor::GetState() {
   if (!is_reconcile_started_) {
-    return error_during_last_reconcile_
+    return (error_during_last_reconcile_.state() !=
+            GoogleServiceAuthError::State::NONE)
                ? signin_metrics::ACCOUNT_RECONCILOR_ERROR
                : signin_metrics::ACCOUNT_RECONCILOR_OK;
   }
@@ -340,7 +341,7 @@ void AccountReconcilor::StartReconcile() {
   add_to_cookie_.clear();
   reconcile_start_time_ = base::Time::Now();
   is_reconcile_started_ = true;
-  error_during_last_reconcile_ = false;
+  error_during_last_reconcile_ = GoogleServiceAuthError::AuthErrorNone();
   reconcile_is_noop_ = true;
 
   // Rely on the GCMS to manage calls to and responses from ListAccounts.
@@ -412,8 +413,13 @@ void AccountReconcilor::OnGaiaAccountsInCookieUpdated(
     FinishReconcile(primary_account, LoadValidAccountsFromTokenService(),
                     std::move(verified_gaia_accounts));
   } else {
-    if (is_reconcile_started_)
-      error_during_last_reconcile_ = true;
+    // We may have seen a series of errors during reconciliation. Delegates may
+    // rely on the severity of the last seen error (see |OnReconcileError|) and
+    // hence do not override a persistent error, if we have seen one.
+    if (is_reconcile_started_ &&
+        !error_during_last_reconcile_.IsPersistentError()) {
+      error_during_last_reconcile_ = error;
+    }
     AbortReconcile();
   }
 }
@@ -550,8 +556,11 @@ void AccountReconcilor::CalculateIfReconcileIsDone() {
   base::TimeDelta duration = base::Time::Now() - reconcile_start_time_;
   // Record the duration if reconciliation was underway and now it is over.
   if (is_reconcile_started_ && add_to_cookie_.empty()) {
-    signin_metrics::LogSigninAccountReconciliationDuration(duration,
-        !error_during_last_reconcile_);
+    bool was_last_reconcile_successful =
+        (error_during_last_reconcile_.state() ==
+         GoogleServiceAuthError::State::NONE);
+    signin_metrics::LogSigninAccountReconciliationDuration(
+        duration, was_last_reconcile_successful);
     timer_->Stop();
   }
 
@@ -623,8 +632,13 @@ void AccountReconcilor::OnAddAccountToCookieCompleted(
           << "Error was " << error.ToString();
   // Always listens to GaiaCookieManagerService. Only proceed if reconciling.
   if (is_reconcile_started_ && MarkAccountAsAddedToCookie(account_id)) {
-    if (error.state() != GoogleServiceAuthError::State::NONE)
-      error_during_last_reconcile_ = true;
+    // We may have seen a series of errors during reconciliation. Delegates may
+    // rely on the severity of the last seen error (see |OnReconcileError|) and
+    // hence do not override a persistent error, if we have seen one.
+    if (error.state() != GoogleServiceAuthError::State::NONE &&
+        !error_during_last_reconcile_.IsPersistentError()) {
+      error_during_last_reconcile_ = error;
+    }
     CalculateIfReconcileIsDone();
     ScheduleStartReconcileIfChromeAccountsChanged();
   }

components/signin/core/browser/account_reconcilor.h

@@ -271,8 +271,15 @@ class AccountReconcilor : public KeyedService,
   // True iff this is the first time the reconcilor is executing.
   bool first_execution_;
 
-  // True iff an error occured during the last attempt to reconcile.
-  bool error_during_last_reconcile_;
+  // 'Most severe' error encountered during the last attempt to reconcile. If
+  // the last reconciliation attempt was successful, this will be
+  // |GoogleServiceAuthError::State::NONE|.
+  // Severity of an error is defined on the basis of
+  // |GoogleServiceAuthError::IsPersistentError()| only, i.e. any persistent
+  // error is considered more severe than all non-persistent errors, but
+  // persistent (or non-persistent) errors do not have an internal severity
+  // ordering among themselves.
+  GoogleServiceAuthError error_during_last_reconcile_;
 
   // Used for Dice migration: migration can happen if the accounts are
   // consistent, which is indicated by reconcile being a no-op.

components/signin/core/browser/account_reconcilor_unittest.cc

@@ -1889,7 +1889,8 @@ TEST_F(AccountReconcilorTest, NoLoopWithBadPrimary) {
                                       GoogleServiceAuthError::AuthErrorNone());
   base::RunLoop().RunUntilIdle();
   ASSERT_FALSE(reconcilor->is_reconcile_started_);
-  ASSERT_TRUE(reconcilor->error_during_last_reconcile_);
+  ASSERT_NE(GoogleServiceAuthError::State::NONE,
+            reconcilor->error_during_last_reconcile_.state());
   testing::Mock::VerifyAndClearExpectations(GetMockReconcilor());
 
   // Now that we've tried once, the token service knows that the primary
@@ -1936,7 +1937,8 @@ TEST_F(AccountReconcilorTest, WontMergeAccountsWithError) {
                                       GoogleServiceAuthError::AuthErrorNone());
   base::RunLoop().RunUntilIdle();
   ASSERT_FALSE(reconcilor->is_reconcile_started_);
-  ASSERT_FALSE(reconcilor->error_during_last_reconcile_);
+  ASSERT_EQ(GoogleServiceAuthError::State::NONE,
+            reconcilor->error_during_last_reconcile_.state());
 }
 
 // Test that delegate timeout is called when the delegate offers a valid