Fix force logout of child accounts on Chrome OS

Do not force logout child accounts on Chrome OS if AccountReconcilor
encounters a retriable network error.

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

chrome/browser/signin/account_reconcilor_factory.cc

@@ -48,19 +48,25 @@ class ChromeOSChildAccountReconcilorDelegate
   }
 
   void OnReconcileError(const GoogleServiceAuthError& error) override {
-    if (error.state() == GoogleServiceAuthError::CONNECTION_FAILED) {
-      // Mark the account to require an online sign in.
-      const user_manager::User* primary_user =
-          user_manager::UserManager::Get()->GetPrimaryUser();
-      user_manager::UserManager::Get()->SaveForceOnlineSignin(
-          primary_user->GetAccountId(), true /* force_online_signin */);
-
-      // Force a logout.
-      UMA_HISTOGRAM_BOOLEAN(
-          "ChildAccountReconcilor.ForcedUserExitOnReconcileError", true);
-      chrome::AttemptUserExit();
+    // If |error| is |GoogleServiceAuthError::State::NONE| or a transient error.
+    if (!error.IsPersistentError()) {
+      return;
     }
+
+    // Mark the account to require an online sign in.
+    const user_manager::User* primary_user =
+        user_manager::UserManager::Get()->GetPrimaryUser();
+    user_manager::UserManager::Get()->SaveForceOnlineSignin(
+        primary_user->GetAccountId(), true /* force_online_signin */);
+
+    // Force a logout.
+    UMA_HISTOGRAM_BOOLEAN(
+        "ChildAccountReconcilor.ForcedUserExitOnReconcileError", true);
+    chrome::AttemptUserExit();
   }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ChromeOSChildAccountReconcilorDelegate);
 };
 #endif
 

components/signin/core/browser/account_reconcilor.cc

@@ -319,6 +319,15 @@ void AccountReconcilor::StartReconcile() {
     return;
   }
 
+  // Begin reconciliation. Reset initial states.
+  for (auto& observer : observer_list_)
+    observer.OnStartReconcile();
+  add_to_cookie_.clear();
+  reconcile_start_time_ = base::Time::Now();
+  is_reconcile_started_ = true;
+  error_during_last_reconcile_ = GoogleServiceAuthError::AuthErrorNone();
+  reconcile_is_noop_ = true;
+
   if (!timeout_.is_max()) {
     // This is NOT a repeating callback but to test it, we need a |MockTimer|,
     // which mocks |Timer| and not |OneShotTimer|. |Timer| currently does not
@@ -329,21 +338,15 @@ void AccountReconcilor::StartReconcile() {
                             base::Unretained(this)));
   }
 
-  if (token_service_->RefreshTokenHasError(
-          signin_manager_->GetAuthenticatedAccountId()) &&
+  const std::string& account_id = signin_manager_->GetAuthenticatedAccountId();
+  if (token_service_->RefreshTokenHasError(account_id) &&
       delegate_->ShouldAbortReconcileIfPrimaryHasError()) {
     VLOG(1) << "AccountReconcilor::StartReconcile: primary has error, abort.";
+    error_during_last_reconcile_ = token_service_->GetAuthError(account_id);
+    AbortReconcile();
     return;
   }
 
-  for (auto& observer : observer_list_)
-    observer.OnStartReconcile();
-  add_to_cookie_.clear();
-  reconcile_start_time_ = base::Time::Now();
-  is_reconcile_started_ = true;
-  error_during_last_reconcile_ = GoogleServiceAuthError::AuthErrorNone();
-  reconcile_is_noop_ = true;
-
   // Rely on the GCMS to manage calls to and responses from ListAccounts.
   std::vector<gaia::ListedAccount> accounts;
   std::vector<gaia::ListedAccount> signed_out_accounts;
@@ -406,7 +409,8 @@ void AccountReconcilor::OnGaiaAccountsInCookieUpdated(
     if (delegate_->ShouldAbortReconcileIfPrimaryHasError() &&
         token_service_->RefreshTokenHasError(primary_account)) {
       VLOG(1) << "Primary account has error, abort.";
-      is_reconcile_started_ = false;
+      DCHECK(is_reconcile_started_);
+      AbortReconcile();
       return;
     }
 
@@ -550,6 +554,9 @@ void AccountReconcilor::AbortReconcile() {
   VLOG(1) << "AccountReconcilor::AbortReconcile: try again later";
   add_to_cookie_.clear();
   CalculateIfReconcileIsDone();
+
+  DCHECK(!is_reconcile_started_);
+  DCHECK(!timer_->IsRunning());
 }
 
 void AccountReconcilor::CalculateIfReconcileIsDone() {
@@ -561,7 +568,20 @@ void AccountReconcilor::CalculateIfReconcileIsDone() {
          GoogleServiceAuthError::State::NONE);
     signin_metrics::LogSigninAccountReconciliationDuration(
         duration, was_last_reconcile_successful);
+
+    // Reconciliation has actually finished (and hence stop the timer), but it
+    // may have ended in some failures. Pass this information to the
+    // |delegate_|.
     timer_->Stop();
+    if (!was_last_reconcile_successful) {
+      // Note: This is the only call to |OnReconcileError| in this file. We MUST
+      // make sure that we do not call |OnReconcileError| multiple times in the
+      // same reconciliation batch.
+      // The enclosing if-condition |is_reconcile_started_ &&
+      // add_to_cookie_.empty()| represents the halting condition for one batch
+      // of reconciliation.
+      delegate_->OnReconcileError(error_during_last_reconcile_);
+    }
   }
 
   is_reconcile_started_ = !add_to_cookie_.empty();
@@ -691,7 +711,17 @@ void AccountReconcilor::set_timer_for_testing(
 }
 
 void AccountReconcilor::HandleReconcileTimeout() {
+  // A reconciliation was still succesfully in progress but could not complete
+  // in the given time. For a delegate, this is equivalent to a
+  // |GoogleServiceAuthError::State::CONNECTION_FAILED|.
+  if (error_during_last_reconcile_.state() ==
+      GoogleServiceAuthError::State::NONE) {
+    error_during_last_reconcile_ = GoogleServiceAuthError(
+        GoogleServiceAuthError::State::CONNECTION_FAILED);
+  }
+
+  // Will stop reconciliation and inform |delegate_| about
+  // |error_during_last_reconcile_|, through |CalculateIfReconcileIsDone|.
   AbortReconcile();
-  delegate_->OnReconcileError(
-      GoogleServiceAuthError(GoogleServiceAuthError::CONNECTION_FAILED));
+  DCHECK(!timer_->IsRunning());
 }

components/signin/core/browser/account_reconcilor.h

@@ -299,7 +299,13 @@ class AccountReconcilor : public KeyedService,
   base::ObserverList<Observer, true> observer_list_;
 
   // A timer to set off reconciliation timeout handlers, if account
-  // reconciliation does not happen in a given timeout duration.
+  // reconciliation does not happen in a given |timeout_| duration.
+  // Any delegate that wants to use this feature must override
+  // |AccountReconcilorDelegate::GetReconcileTimeout|.
+  // Note: This is intended as a safeguard for delegates that want a 'guarantee'
+  // of reconciliation completing within a finite time. It is technically
+  // possible for account reconciliation to be running/waiting forever in cases
+  // such as a network connection not being present.
   std::unique_ptr<base::Timer> timer_;
   base::TimeDelta timeout_;
 

components/signin/core/browser/account_reconcilor_delegate.h

@@ -60,17 +60,22 @@ class AccountReconcilorDelegate {
       const std::vector<gaia::ListedAccount>& gaia_accounts);
 
   // Called when reconcile is finished.
+  // |OnReconcileFinished| is always called at the end of reconciliation, even
+  // when there is an error (except in cases where reconciliation times out
+  // before finishing, see |GetReconcileTimeout|).
   virtual void OnReconcileFinished(const std::string& first_account,
                                    bool reconcile_is_noop) {}
 
   // Returns the desired timeout for account reconciliation. If reconciliation
   // does not happen within this time, it is aborted and |this| delegate is
-  // informed via |OnReconcileError|, with an error state of
-  // GoogleServiceAuthError::CONNECTION_FAILED. If a delegate does not wish to
-  // set a timeout for account reconciliation, it should not override this
-  // method. Default: |base::TimeDelta::Max()|.
+  // informed via |OnReconcileError|, with the 'most severe' error that occurred
+  // during this time (see |AccountReconcilor::error_during_last_reconcile_|).
+  // If a delegate does not wish to set a timeout for account reconciliation, it
+  // should not override this method. Default: |base::TimeDelta::Max()|.
   virtual base::TimeDelta GetReconcileTimeout() const;
 
+  // Called when account reconciliation ends in an error.
+  // |OnReconcileError| is called before |OnReconcileFinished|.
   virtual void OnReconcileError(const GoogleServiceAuthError& error);
 
   void set_reconcilor(AccountReconcilor* reconcilor) {