[profiles] DiceInterceptedSessionStartupHelper calls multilogin directly

This CL changes the way DiceInterceptedSessionStartupHelper adds the
account on the web.
Previously it was mostly waiting for the reconcilor to trigger, and
making sure that the process is not stuck (by forcing calls to list
accounts).

The new behavior is more proactive: when the profile is new, it blocks
the reconcilor and forces a call to multilogin directly.
The advantages are:
- it is easier to control and hopefully it should be less brittle.
- it should be faster:
  * the previous flow was doing an extra /ListAccounts call after the
    reconciliation, which is now removed
  * the previous flow was using MergeSession while the new flow uses
    multilogin, which should be faster on desktop (no need to fetch an
    access token)

When the profile is not new, it's not possible to simply use multilogin,
as there may already be accounts in the profile. The reconcilor is still
used in this case, as it has the logic to handle this.

Note: it would be possible to use MergeSession instead of Multilogin, as
this would work for all profiles (including existing profiles). However
MergeSession is now discouraged and thus we avoid adding a new usage
here.

This CL also improves histograms.

Bug: 1151313
Change-Id: I4871eb123eb37081595164bd5280adf2b0b1290b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2600909
Commit-Queue: David Roger <droger@chromium.org>
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Auto-Submit: David Roger <droger@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840690}

chrome/browser/signin/dice_intercepted_session_startup_helper.cc

@@ -17,9 +17,12 @@
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/common/webui_url_constants.h"
+#include "components/signin/public/base/multilogin_parameters.h"
 #include "components/signin/public/identity_manager/accounts_cookie_mutator.h"
 #include "components/signin/public/identity_manager/accounts_in_cookie_jar_info.h"
+#include "components/signin/public/identity_manager/set_accounts_in_cookie_result.h"
 #include "content/public/browser/web_contents.h"
+#include "google_apis/gaia/gaia_auth_fetcher.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "url/gurl.h"
@@ -37,13 +40,23 @@ bool CookieInfoContains(const signin::AccountsInCookieJarInfo& cookie_info,
                       }) != accounts.end();
 }
 
+void RecordSessionStartupDuration(const std::string& histogram_name,
+                                  base::TimeDelta duration) {
+  base::UmaHistogramCustomTimes(histogram_name, duration,
+                                /*min=*/base::TimeDelta::FromMilliseconds(1),
+                                /*max=*/base::TimeDelta::FromSeconds(30), 50);
+}
+
 }  // namespace
 
 DiceInterceptedSessionStartupHelper::DiceInterceptedSessionStartupHelper(
     Profile* profile,
+    bool is_new_profile,
     CoreAccountId account_id,
     content::WebContents* tab_to_move)
-    : profile_(profile), account_id_(account_id) {
+    : profile_(profile),
+      use_multilogin_(is_new_profile),
+      account_id_(account_id) {
   Observe(tab_to_move);
 }
 
@@ -64,24 +77,26 @@ void DiceInterceptedSessionStartupHelper::Startup(base::OnceClosure callback) {
       identity_manager->GetAccountsInCookieJar();
   if (cookie_info.accounts_are_fresh &&
       CookieInfoContains(cookie_info, account_id_)) {
-    MoveTab();
+    MoveTab(use_multilogin_ ? Result::kMultiloginNothingToDo
+                            : Result::kReconcilorNothingToDo);
   } else {
-    // TODO(https://crbug.com/1051864): cookie notifications are not triggered
-    // 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_.Observe(identity_manager);
+    // Set the timeout.
     on_cookie_update_timeout_.Reset(base::BindOnce(
-        &DiceInterceptedSessionStartupHelper::MoveTab, base::Unretained(this)));
+        &DiceInterceptedSessionStartupHelper::MoveTab, base::Unretained(this),
+        use_multilogin_ ? Result::kMultiloginTimeout
+                        : Result::kReconcilorTimeout));
     // Adding accounts to the cookies can be an expensive operation. In
     // particular the ExternalCCResult fetch may time out after multiple seconds
     // (see kExternalCCResultTimeoutSeconds and https://crbug.com/750316#c37).
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE, on_cookie_update_timeout_.callback(),
         base::TimeDelta::FromSeconds(12));
+
+    accounts_in_cookie_observer_.Observe(identity_manager);
+    if (use_multilogin_)
+      StartupMultilogin(identity_manager);
+    else
+      StartupReconcilor(identity_manager);
   }
 }
 
@@ -94,11 +109,14 @@ void DiceInterceptedSessionStartupHelper::OnAccountsInCookieUpdated(
     return;
   if (!CookieInfoContains(accounts_in_cookie_jar_info, account_id_))
     return;
-  MoveTab();
+
+  MoveTab(use_multilogin_ ? Result::kMultiloginOtherSuccess
+                          : Result::kReconcilorSuccess);
 }
 
 void DiceInterceptedSessionStartupHelper::OnStateChanged(
     signin_metrics::AccountReconcilorState state) {
+  DCHECK(!use_multilogin_);
   if (state == signin_metrics::ACCOUNT_RECONCILOR_ERROR) {
     reconcile_error_encountered_ = true;
     return;
@@ -118,15 +136,57 @@ void DiceInterceptedSessionStartupHelper::OnStateChanged(
   }
 }
 
-void DiceInterceptedSessionStartupHelper::MoveTab() {
+void DiceInterceptedSessionStartupHelper::StartupMultilogin(
+    signin::IdentityManager* identity_manager) {
+  // Lock the reconcilor to avoid making multiple multilogin calls.
+  reconcilor_lock_ = std::make_unique<AccountReconcilor::Lock>(
+      AccountReconcilorFactory::GetForProfile(profile_));
+
+  // Start the multilogin call.
+  signin::MultiloginParameters params = {
+      /*mode=*/gaia::MultiloginMode::MULTILOGIN_UPDATE_COOKIE_ACCOUNTS_ORDER,
+      /*accounts_to_send=*/{account_id_}};
+  identity_manager->GetAccountsCookieMutator()->SetAccountsInCookie(
+      params, gaia::GaiaSource::kChrome,
+      base::BindOnce(
+          &DiceInterceptedSessionStartupHelper::OnSetAccountInCookieCompleted,
+          weak_factory_.GetWeakPtr()));
+}
+
+void DiceInterceptedSessionStartupHelper::StartupReconcilor(
+    signin::IdentityManager* identity_manager) {
+  // TODO(https://crbug.com/1051864): cookie notifications are not triggered
+  // 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();
+}
+
+void DiceInterceptedSessionStartupHelper::OnSetAccountInCookieCompleted(
+    signin::SetAccountsInCookieResult result) {
+  DCHECK(use_multilogin_);
+  Result session_startup_result = Result::kMultiloginOtherSuccess;
+  switch (result) {
+    case signin::SetAccountsInCookieResult::kSuccess:
+      session_startup_result = Result::kMultiloginSuccess;
+      break;
+    case signin::SetAccountsInCookieResult::kTransientError:
+      session_startup_result = Result::kMultiloginTransientError;
+      break;
+    case signin::SetAccountsInCookieResult::kPersistentError:
+      session_startup_result = Result::kMultiloginPersistentError;
+      break;
+  }
+
+  MoveTab(session_startup_result);
+}
+
+void DiceInterceptedSessionStartupHelper::MoveTab(Result result) {
   accounts_in_cookie_observer_.Reset();
   reconcilor_observer_.Reset();
   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_);
+  reconcilor_lock_.reset();
 
   GURL url_to_open = GURL(chrome::kChromeUINewTabURL);
   // If the intercepted web contents is still alive, close it now.
@@ -140,8 +200,20 @@ void DiceInterceptedSessionStartupHelper::MoveTab() {
                         ui::PAGE_TRANSITION_AUTO_BOOKMARK);
   Navigate(&params);
 
-  base::UmaHistogramTimes("Signin.Intercept.SessionStartupDuration",
-                          base::TimeTicks::Now() - session_startup_time_);
+  base::UmaHistogramEnumeration("Signin.Intercept.SessionStartupResult",
+                                result);
+  base::TimeDelta duration = base::TimeTicks::Now() - session_startup_time_;
+  if (use_multilogin_) {
+    RecordSessionStartupDuration(
+        "Signin.Intercept.SessionStartupDuration.Multilogin", duration);
+  } else {
+    RecordSessionStartupDuration(
+        "Signin.Intercept.SessionStartupDuration.Reconcilor", duration);
+    // 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 (callback_)
     std::move(callback_).Run();

chrome/browser/signin/dice_intercepted_session_startup_helper.h

@@ -7,6 +7,7 @@
 
 #include "base/callback_forward.h"
 #include "base/cancelable_callback.h"
+#include "base/memory/weak_ptr.h"
 #include "base/scoped_observation.h"
 #include "base/time/time.h"
 #include "components/signin/core/browser/account_reconcilor.h"
@@ -20,6 +21,8 @@ class WebContents;
 
 namespace signin {
 struct AccountsInCookieJarInfo;
+class IdentityManager;
+enum class SetAccountsInCookieResult;
 }
 
 class GoogleServiceAuthError;
@@ -35,12 +38,28 @@ class DiceInterceptedSessionStartupHelper
       public signin::IdentityManager::Observer,
       public AccountReconcilor::Observer {
  public:
+  // Used in UMA histograms, do not reorder or remove values.
+  enum class Result {
+    kReconcilorNothingToDo = 0,
+    kMultiloginNothingToDo = 1,
+    kReconcilorSuccess = 2,       // The account was added by the reconcilor.
+    kMultiloginSuccess = 3,       // The account was added by this object.
+    kMultiloginOtherSuccess = 4,  // The account was added by something else.
+    kMultiloginTimeout = 5,
+    kReconcilorTimeout = 6,
+    kMultiloginTransientError = 7,
+    kMultiloginPersistentError = 8,
+
+    kMaxValue = kMultiloginPersistentError
+  };
+
   // |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
   // profile.
   // |tab_to_move| is the tab where the interception happened, in the source
   // profile.
   DiceInterceptedSessionStartupHelper(Profile* profile,
+                                      bool is_new_profile,
                                       CoreAccountId account_id,
                                       content::WebContents* tab_to_move);
 
@@ -63,11 +82,22 @@ class DiceInterceptedSessionStartupHelper
   void OnStateChanged(signin_metrics::AccountReconcilorState state) override;
 
  private:
+  // For new profiles, the account is added directly using multilogin.
+  void StartupMultilogin(signin::IdentityManager* identity_manager);
+
+  // For existing profiles, simply wait for the reconcilor to update the
+  // accounts.
+  void StartupReconcilor(signin::IdentityManager* identity_manager);
+
+  // Called when multilogin completes.
+  void OnSetAccountInCookieCompleted(signin::SetAccountsInCookieResult result);
+
   // Creates a browser with a new tab, and closes the intercepted tab if it's
   // still open.
-  void MoveTab();
+  void MoveTab(Result result);
 
   Profile* const profile_;
+  bool use_multilogin_;
   CoreAccountId account_id_;
   base::OnceClosure callback_;
   bool reconcile_error_encountered_ = false;
@@ -76,10 +106,13 @@ class DiceInterceptedSessionStartupHelper
       accounts_in_cookie_observer_{this};
   base::ScopedObservation<AccountReconcilor, AccountReconcilor::Observer>
       reconcilor_observer_{this};
+  std::unique_ptr<AccountReconcilor::Lock> reconcilor_lock_;
   base::TimeTicks session_startup_time_;
   // Timeout while waiting for the account to be added to the cookies in the new
   // profile.
   base::CancelableOnceCallback<void()> on_cookie_update_timeout_;
+
+  base::WeakPtrFactory<DiceInterceptedSessionStartupHelper> weak_factory_{this};
 };
 
 #endif  // CHROME_BROWSER_SIGNIN_DICE_INTERCEPTED_SESSION_STARTUP_HELPER_H_

chrome/browser/signin/dice_web_signin_interceptor.cc

@@ -271,16 +271,16 @@ void DiceWebSigninInterceptor::CreateBrowserAfterSigninInterception(
     CoreAccountId account_id,
     content::WebContents* intercepted_contents,
     std::unique_ptr<ScopedDiceWebSigninInterceptionBubbleHandle> bubble_handle,
-    bool show_customization_bubble) {
+    bool is_new_profile) {
   DCHECK(!session_startup_helper_);
   DCHECK(bubble_handle);
   interception_bubble_handle_ = std::move(bubble_handle);
   session_startup_helper_ =
       std::make_unique<DiceInterceptedSessionStartupHelper>(
-          profile_, account_id, intercepted_contents);
+          profile_, is_new_profile, account_id, intercepted_contents);
   session_startup_helper_->Startup(
       base::BindOnce(&DiceWebSigninInterceptor::OnNewBrowserCreated,
-                     base::Unretained(this), show_customization_bubble));
+                     base::Unretained(this), is_new_profile));
 }
 
 void DiceWebSigninInterceptor::Shutdown() {
@@ -477,21 +477,22 @@ void DiceWebSigninInterceptor::OnNewSignedInProfileCreated(
     return;
   }
 
-  bool show_customization_bubble = false;
-  if (profile_color.has_value()) {
-    // The profile color is defined only when the profile has just been created
-    // (with interception type kMultiUser or kEnterprise). If the profile is not
-    // new (kProfileSwitch), then the color is not updated.
+  // The profile color is defined only when the profile has just been created
+  // (with interception type kMultiUser or kEnterprise). If the profile is not
+  // new (kProfileSwitch) or if it is a guest profile, then the color is not
+  // updated.
+  bool is_new_profile = profile_color.has_value();
+  if (is_new_profile) {
     base::UmaHistogramTimes(
         "Signin.Intercept.ProfileCreationDuration",
         base::TimeTicks::Now() - profile_creation_start_time_);
     ProfileMetrics::LogProfileAddNewUser(
         ProfileMetrics::ADD_NEW_USER_SIGNIN_INTERCEPTION);
-    // Apply the new color to the profile.
-    ThemeServiceFactory::GetForProfile(new_profile)
-        ->BuildAutogeneratedThemeFromColor(*profile_color);
-    // Show the customization UI to allow changing the color.
-    show_customization_bubble = !new_profile->IsEphemeralGuestProfile();
+    if (!new_profile->IsEphemeralGuestProfile()) {
+      // Apply the new color to the profile.
+      ThemeServiceFactory::GetForProfile(new_profile)
+          ->BuildAutogeneratedThemeFromColor(*profile_color);
+    }
   } else {
     base::UmaHistogramTimes(
         "Signin.Intercept.ProfileSwitchDuration",
@@ -503,16 +504,15 @@ void DiceWebSigninInterceptor::OnNewSignedInProfileCreated(
   DiceWebSigninInterceptorFactory::GetForProfile(new_profile)
       ->CreateBrowserAfterSigninInterception(
           account_id_, web_contents(), std::move(interception_bubble_handle_),
-          show_customization_bubble);
+          is_new_profile);
   Reset();
 }
 
-void DiceWebSigninInterceptor::OnNewBrowserCreated(
-    bool show_customization_bubble) {
+void DiceWebSigninInterceptor::OnNewBrowserCreated(bool is_new_profile) {
   DCHECK(interception_bubble_handle_);
   interception_bubble_handle_.reset();  // Close the bubble now.
   session_startup_helper_.reset();
-  if (show_customization_bubble) {
+  if (is_new_profile && !profile_->IsEphemeralGuestProfile()) {
     Browser* browser = chrome::FindBrowserWithProfile(profile_);
     DCHECK(browser);
     delegate_->ShowProfileCustomizationBubble(browser);

chrome/browser/signin/dice_web_signin_interceptor.h

@@ -205,14 +205,12 @@ class DiceWebSigninInterceptor : public KeyedService,
   // `intercepted_contents` may be null if the tab was already closed.
   // The intercepted web contents belong to the source profile (which is not the
   // profile attached to this service).
-  // `show_customization_bubble` indicates whether the customization bubble
-  // should be shown after the browser is opened.
   void CreateBrowserAfterSigninInterception(
       CoreAccountId account_id,
       content::WebContents* intercepted_contents,
       std::unique_ptr<ScopedDiceWebSigninInterceptionBubbleHandle>
           bubble_handle,
-      bool show_customization_bubble);
+      bool is_new_profile);
 
   // Returns the outcome of the interception heuristic.
   // If the outcome is kInterceptProfileSwitch, the target profile is returned
@@ -283,7 +281,7 @@ class DiceWebSigninInterceptor : public KeyedService,
 
   // Called when the new browser is created after interception. Passed as
   // callback to `session_startup_helper_`.
-  void OnNewBrowserCreated(bool show_customization_bubble);
+  void OnNewBrowserCreated(bool is_new_profile);
 
   // Returns a 8-bit hash of the email that can be persisted.
   static std::string GetPersistentEmailHash(const std::string& email);

tools/metrics/histograms/enums.xml

@@ -68425,6 +68425,18 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
   <int value="3" label="Not displayed"/>
 </enum>
 
+<enum name="SigninInterceptSessionStartupResult">
+  <int value="0" label="Nothing to do (Reconcilor)"/>
+  <int value="1" label="Nothing to do (Multilogin)"/>
+  <int value="2" label="Success (reconcilor)"/>
+  <int value="3" label="Success (multilogin)"/>
+  <int value="4" label="Success (other)"/>
+  <int value="5" label="Timeout (multilogin)"/>
+  <int value="6" label="Timeout (reconcilor)"/>
+  <int value="7" label="Transient error (multilogin)"/>
+  <int value="8" label="Persistent error (multilogin)"/>
+</enum>
+
 <enum name="SigninInvalidGaiaCredentialsReason">
   <int value="0" label="Unknown"/>
   <int value="1" label="Credentials rejected by server"/>

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

@@ -439,6 +439,10 @@ prefs when the profile is loaded.  -->
 
 <histogram name="Signin.Intercept.SessionStartupDuration" units="ms"
     expires_after="2021-08-12">
+  <obsolete>
+    Replaced in M89 by Signin.Intercept.SessionStartupDuration.Multilogin and
+    Signin.Intercept.SessionStartupDuration.Reconcilor.
+  </obsolete>
   <owner>alexilin@chromium.org</owner>
   <owner>droger@chromium.org</owner>
   <summary>
@@ -447,6 +451,20 @@ prefs when the profile is loaded.  -->
   </summary>
 </histogram>
 
+<histogram name="Signin.Intercept.SessionStartupDuration.{Method}" units="ms"
+    expires_after="2021-08-12">
+  <owner>alexilin@chromium.org</owner>
+  <owner>droger@chromium.org</owner>
+  <summary>
+    Records the duration of session startup time after signin interception using
+    {Method}. This includes waiting for the account to be available on the web.
+  </summary>
+  <token key="Method">
+    <variant name="Multilogin"/>
+    <variant name="Reconcilor"/>
+  </token>
+</histogram>
+
 <histogram name="Signin.Intercept.SessionStartupReconcileError"
     enum="BooleanPresent" expires_after="2021-08-12">
   <owner>alexilin@chromium.org</owner>
@@ -458,6 +476,16 @@ prefs when the profile is loaded.  -->
   </summary>
 </histogram>
 
+<histogram name="Signin.Intercept.SessionStartupResult"
+    enum="SigninInterceptSessionStartupResult" expires_after="2021-08-12">
+  <owner>alexilin@chromium.org</owner>
+  <owner>droger@chromium.org</owner>
+  <summary>
+    Records the result of session startup after signin interception, which adds
+    the account on the web.
+  </summary>
+</histogram>
+
 <histogram base="true" name="Signin.InterceptResult"
     enum="SigninInterceptResult" expires_after="2021-08-12">
 <!-- Name completed by histogram_suffixes name="SigninInterceptType" -->