[signin] Fix timeout issues in DiceInterceptionSessionStartupHelper

This is a tentative fix, as I could not repro the issue.
It is theoritically possible that the call to
TriggerCookieJarUpdate()
does nothing.

The scenario would look like this:
- Reconcilor starts
- Reconcilor does a ListAccounts
- At this point DiceInterceptedSessionStartupHelper calls
  TriggerCookieJarUpdate, but this is a no-op because a ListAccount is
  already running.
- Reconcilor finishes running and adds the account, but
  OnAccountsInCookieUpdated is never called (because bug 1051864)
- this results in a timeout of DiceInterceptedSessionStartupHelper

The CL also adds an histogram to track reconcilor errors, as they would
explain the timeout as well.

Bug: 1151313
Change-Id: I5082ffa377c3c3c4e605d75ae79545811593ac3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2550814Reviewed-by: Caitlin Fischer <caitlinfischer@google.com>
Reviewed-by: Monica Basta <msalama@chromium.org>
Commit-Queue: Monica Basta <msalama@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830521}

chrome/browser/signin/dice_intercepted_session_startup_helper.cc

@@ -12,6 +12,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
+#include "chrome/browser/signin/account_reconcilor_factory.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
@@ -66,11 +67,13 @@ void DiceInterceptedSessionStartupHelper::Startup(base::OnceClosure callback) {
     MoveTab();
   } else {
     // TODO(https://crbug.com/1051864): cookie notifications are not triggered
-    // when the account is added by the reconcilor. Force an explicit cookie
-    // update.
+    // when the account is added by the reconcilor. Observe the reconcilor and
+    // re-trigger the cookie update when it completes.
+    reconcilor_observer_.Observe(
+        AccountReconcilorFactory::GetForProfile(profile_));
     identity_manager->GetAccountsCookieMutator()->TriggerCookieJarUpdate();
 
-    accounts_in_cookie_observer_.Add(identity_manager);
+    accounts_in_cookie_observer_.Observe(identity_manager);
     on_cookie_update_timeout_.Reset(base::BindOnce(
         &DiceInterceptedSessionStartupHelper::MoveTab, base::Unretained(this)));
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
@@ -91,10 +94,39 @@ void DiceInterceptedSessionStartupHelper::OnAccountsInCookieUpdated(
   MoveTab();
 }
 
+void DiceInterceptedSessionStartupHelper::OnStateChanged(
+    signin_metrics::AccountReconcilorState state) {
+  if (state == signin_metrics::ACCOUNT_RECONCILOR_ERROR) {
+    reconcile_error_encountered_ = true;
+    return;
+  }
+
+  // TODO(https://crbug.com/1051864): remove this when the cookie updates are
+  // correctly sent after reconciliation.
+  if (state == signin_metrics::ACCOUNT_RECONCILOR_OK) {
+    signin::IdentityManager* identity_manager =
+        IdentityManagerFactory::GetForProfile(profile_);
+    // GetAccountsInCookieJar() automatically re-schedules a /ListAccounts call
+    // if the cookie is not fresh.
+    signin::AccountsInCookieJarInfo cookie_info =
+        identity_manager->GetAccountsInCookieJar();
+    OnAccountsInCookieUpdated(cookie_info,
+                              GoogleServiceAuthError::AuthErrorNone());
+  }
+}
+
 void DiceInterceptedSessionStartupHelper::MoveTab() {
-  accounts_in_cookie_observer_.RemoveAll();
+  if (accounts_in_cookie_observer_.IsObserving())
+    accounts_in_cookie_observer_.RemoveObservation();
+  if (reconcilor_observer_.IsObserving())
+    reconcilor_observer_.RemoveObservation();
   on_cookie_update_timeout_.Cancel();
 
+  // TODO(https://crbug.com/1151313): Remove this histogram when the cause
+  // for the timeouts is understood.
+  base::UmaHistogramBoolean("Signin.Intercept.SessionStartupReconcileError",
+                            reconcile_error_encountered_);
+
   // If the intercepted web contents is still alive, close it now.
   if (web_contents()) {
     // Update the URL once again to catch any potential navigation happening

chrome/browser/signin/dice_intercepted_session_startup_helper.h

@@ -7,8 +7,9 @@
 
 #include "base/callback_forward.h"
 #include "base/cancelable_callback.h"
-#include "base/scoped_observer.h"
+#include "base/scoped_observation.h"
 #include "base/time/time.h"
+#include "components/signin/core/browser/account_reconcilor.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "google_apis/gaia/core_account_id.h"
@@ -32,7 +33,8 @@ class Profile;
 // in the content area (cookies).
 class DiceInterceptedSessionStartupHelper
     : public content::WebContentsObserver,
-      public signin::IdentityManager::Observer {
+      public signin::IdentityManager::Observer,
+      public AccountReconcilor::Observer {
  public:
   // |profile| is the new profile that was created after signin interception.
   // |account_id| is the main account for the profile, it's already in the
@@ -58,6 +60,9 @@ class DiceInterceptedSessionStartupHelper
       const signin::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
       const GoogleServiceAuthError& error) override;
 
+  // AccountReconcilor::Observer:
+  void OnStateChanged(signin_metrics::AccountReconcilorState state) override;
+
  private:
   // Creates a browser with a new tab, and closes the intercepted tab if it's
   // still open.
@@ -66,8 +71,12 @@ class DiceInterceptedSessionStartupHelper
   Profile* const profile_;
   CoreAccountId account_id_;
   base::OnceClosure callback_;
-  ScopedObserver<signin::IdentityManager, signin::IdentityManager::Observer>
+  bool reconcile_error_encountered_ = false;
+  base::ScopedObservation<signin::IdentityManager,
+                          signin::IdentityManager::Observer>
       accounts_in_cookie_observer_{this};
+  base::ScopedObservation<AccountReconcilor, AccountReconcilor::Observer>
+      reconcilor_observer_{this};
   base::TimeTicks session_startup_time_;
   // Timeout while waiting for the account to be added to the cookies in the new
   // profile.

tools/metrics/histograms/histograms_xml/signin/histograms.xml

@@ -417,6 +417,17 @@ prefs when the profile is loaded.  -->
   </summary>
 </histogram>
 
+<histogram name="Signin.Intercept.SessionStartupReconcileError"
+    enum="BooleanPresent" expires_after="2021-08-12">
+  <owner>alexilin@chromium.org</owner>
+  <owner>droger@chromium.org</owner>
+  <summary>
+    Records whether a reconciliation errors happens while an account is moved to
+    another profile after signin interception, specifically when the cookie is
+    being generated by the account reconcilor.
+  </summary>
+</histogram>
+
 <histogram base="true" name="Signin.InterceptResult"
     enum="SigninInterceptResult" expires_after="2021-08-12">
 <!-- Name completed by histogram_suffixes name="SigninInterceptType" -->