[Android][WebSignin] Improve sign-in error retry flow

This CL improves the sign-in error retry flow by adding a sign-in state
check before signing in and keep WebSigninBridge when there is a
sign-in error.

Bug: 1133704
Change-Id: Id4c1bba4f5af997ccfeb43972ba622eb5268d823
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2440582Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Alice Wang <aliceywang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812672}

chrome/android/java/src/org/chromium/chrome/browser/signin/account_picker/AccountPickerDelegate.java

@@ -24,6 +24,9 @@ import org.chromium.components.signin.AccountManagerFacadeProvider;
 import org.chromium.components.signin.AccountUtils;
 import org.chromium.components.signin.base.CoreAccountInfo;
 import org.chromium.components.signin.base.GoogleServiceAuthError;
+import org.chromium.components.signin.identitymanager.ConsentLevel;
+import org.chromium.components.signin.identitymanager.IdentityManager;
+import org.chromium.components.signin.metrics.SignoutReason;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.ui.base.WindowAndroid;
 
@@ -40,6 +43,7 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
     private final WebSigninBridge.Factory mWebSigninBridgeFactory;
     private final String mContinueUrl;
     private final SigninManager mSigninManager;
+    private final IdentityManager mIdentityManager;
     private @Nullable WebSigninBridge mWebSigninBridge;
     private Callback<GoogleServiceAuthError> mOnSignInErrorCallback;
 
@@ -60,6 +64,8 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
         mContinueUrl = continueUrl;
         mSigninManager = IdentityServicesProvider.get().getSigninManager(
                 Profile.getLastUsedRegularProfile());
+        mIdentityManager = IdentityServicesProvider.get().getIdentityManager(
+                Profile.getLastUsedRegularProfile());
     }
 
     /**
@@ -75,6 +81,14 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
      */
     public void signIn(CoreAccountInfo coreAccountInfo,
             Callback<GoogleServiceAuthError> onSignInErrorCallback) {
+        if (mIdentityManager.getPrimaryAccountInfo(ConsentLevel.NOT_REQUIRED) != null) {
+            // In case an error is fired because cookies are taking longer to generate than usual,
+            // if user retries the sign-in from the error screen, we need to sign out the user
+            // first before signing in again.
+            destroyWebSigninBridge();
+            // TODO(https://crbug.com/1133752): Revise sign-out reason
+            mSigninManager.signOut(SignoutReason.ABORT_SIGNIN);
+        }
         mOnSignInErrorCallback = onSignInErrorCallback;
         mWebSigninBridge = mWebSigninBridgeFactory.create(
                 Profile.getLastUsedRegularProfile(), coreAccountInfo, this);
@@ -152,7 +166,6 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
     public void onSigninFailed(GoogleServiceAuthError error) {
         ThreadUtils.assertOnUiThread();
         mOnSignInErrorCallback.onResult(error);
-        destroyWebSigninBridge();
     }
 
     /**

chrome/android/junit/src/org/chromium/chrome/browser/signin/account_picker/AccountPickerDelegateTest.java

@@ -5,10 +5,12 @@
 package org.chromium.chrome.browser.signin.account_picker;
 
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -72,6 +74,9 @@ public class AccountPickerDelegateTest {
     @Mock
     private SigninManager mSigninManagerMock;
 
+    @Mock
+    private IdentityManager mIdentityManagerMock;
+
     @Mock
     private Profile mProfileMock;
 
@@ -88,9 +93,6 @@ public class AccountPickerDelegateTest {
 
     private AccountPickerDelegate mDelegate;
 
-    private final IdentityManager mIdentityManager =
-            new IdentityManager(/* nativeIdentityManager= */ 0, /* OAuth2TokenService= */ null);
-
     @Before
     public void setUp() {
         initMocks(this);
@@ -99,7 +101,8 @@ public class AccountPickerDelegateTest {
 
         Profile.setLastUsedProfileForTesting(mProfileMock);
         IdentityServicesProvider.setInstanceForTests(mock(IdentityServicesProvider.class));
-        when(IdentityServicesProvider.get().getIdentityManager(any())).thenReturn(mIdentityManager);
+        when(IdentityServicesProvider.get().getIdentityManager(any()))
+                .thenReturn(mIdentityManagerMock);
         when(IdentityServicesProvider.get().getSigninManager(any())).thenReturn(mSigninManagerMock);
 
         mDelegate = new AccountPickerDelegate(
@@ -145,6 +148,27 @@ public class AccountPickerDelegateTest {
         verify(mWebSigninBridgeMock).destroy();
     }
 
+    @Test
+    public void testSigninTriggersSignoutIfAlreadySignedIn() {
+        // In case an error is fired because cookies are taking longer to generate than usual,
+        // if user retries the sign-in from the error screen, we need to sign out the user
+        // first before signing in again.
+        Account account =
+                mAccountManagerTestRule.addAccount(AccountManagerTestRule.TEST_ACCOUNT_EMAIL);
+        CoreAccountInfo coreAccountInfo = mAccountManagerTestRule.toCoreAccountInfo(account.name);
+        mDelegate.signIn(coreAccountInfo, error -> {});
+        when(mIdentityManagerMock.getPrimaryAccountInfo(anyInt())).thenReturn(coreAccountInfo);
+
+        mDelegate.signIn(coreAccountInfo, error -> {});
+        InOrder calledInOrder = inOrder(mWebSigninBridgeMock, mSigninManagerMock,
+                mWebSigninBridgeFactoryMock, mSigninManagerMock);
+        calledInOrder.verify(mWebSigninBridgeMock).destroy();
+        calledInOrder.verify(mSigninManagerMock).signOut(anyInt());
+        calledInOrder.verify(mWebSigninBridgeFactoryMock)
+                .create(mProfileMock, coreAccountInfo, mDelegate);
+        calledInOrder.verify(mSigninManagerMock).signin(eq(coreAccountInfo), any());
+    }
+
     @Test
     public void testSignInFailedWithConnectionError() {
         Account account =
@@ -155,7 +179,9 @@ public class AccountPickerDelegateTest {
         mDelegate.signIn(coreAccountInfo, mockCallback);
         mDelegate.onSigninFailed(error);
         verify(mockCallback).onResult(error);
-        verify(mWebSigninBridgeMock).destroy();
+        // WebSigninBridge should be kept alive in case cookies are taking longer to
+        // generate than usual
+        verify(mWebSigninBridgeMock, never()).destroy();
     }
 
     @Test