diff --git a/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManagerImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManagerImpl.java index 6caf8d2a51488c..5d26d856a1cef7 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManagerImpl.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManagerImpl.java @@ -168,7 +168,9 @@ private SigninManagerImpl( mAccountManagerFacade.addObserver(this); Promise> coreAccountInfosPromise = mAccountManagerFacade.getCoreAccountInfos(); - if (coreAccountInfosPromise.isFulfilled()) { + if (coreAccountInfosPromise.isFulfilled() + && (mAccountManagerFacade.didAccountFetchSucceed() + || !coreAccountInfosPromise.getResult().isEmpty())) { seedThenReloadAllAccountsFromSystem( CoreAccountInfo.getIdFrom( identityManager.getPrimaryAccountInfo(ConsentLevel.SIGNIN))); @@ -177,8 +179,8 @@ private SigninManagerImpl( } /** - * Triggered during SigninManagerAndroidWrapper's KeyedService::Shutdown. - * Drop references with external services and native. + * Triggered during SigninManagerAndroidWrapper's KeyedService::Shutdown. Drop references with + * external services and native. */ @VisibleForTesting @CalledByNative @@ -203,6 +205,12 @@ public void onCoreAccountInfosChanged() { mAccountManagerFacade.getCoreAccountInfos(); assert coreAccountInfosPromise.isFulfilled(); List coreAccountInfos = coreAccountInfosPromise.getResult(); + if (!mAccountManagerFacade.didAccountFetchSucceed() && coreAccountInfos.isEmpty()) { + // If the account fetch did not succeed, the AccountManagerFacade falls back to an empty + // list. Do nothing when this is the case. + return; + } + @Nullable CoreAccountInfo primaryAccountInfo = mIdentityManager.getPrimaryAccountInfo(ConsentLevel.SIGNIN); diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerImplTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerImplTest.java index a107fcd05513e3..c74a03aa5205ba 100644 --- a/chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerImplTest.java +++ b/chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerImplTest.java @@ -161,21 +161,6 @@ public void setUp() { .thenReturn(true); AccountManagerFacadeProvider.setInstanceForTests(mFakeAccountManagerFacade); - - // TODO(crbug/1491005): Verify the first call to - // seedAccountsThenReloadAllAccountsWithPrimaryAccount that is made on creation of the - // SigninManager when the SeedAccountsRevamp flag is enabled. This can be done by creating - // the SigninManager in the test method directly. - mSigninManager = - (SigninManagerImpl) - SigninManagerImpl.create( - NATIVE_SIGNIN_MANAGER, - mProfile, - mAccountTrackerService, - mIdentityManager, - mIdentityMutator, - mSyncService); - mSigninManager.addSignInStateObserver(mSignInStateObserver); } @After @@ -187,23 +172,17 @@ public void tearDown() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) - public void testAccountManagerFacadeObserverAddedOnCreate_seedAccountsRevampEnabled() { + public void + testAccountManagerFacadeObserverAddedOnCreate_accountFetchSucceeded_seedAccountsRevampEnabled() { CoreAccountInfo coreAccountInfo = CoreAccountInfo.createFromEmailAndGaiaId("email@domain.com", "gaia-id"); AccountManagerFacade accountManagerFacadeMock = Mockito.mock(AccountManagerFacade.class); when(accountManagerFacadeMock.getCoreAccountInfos()) .thenReturn(Promise.fulfilled(List.of(coreAccountInfo))); + when(accountManagerFacadeMock.didAccountFetchSucceed()).thenReturn(true); AccountManagerFacadeProvider.setInstanceForTests(accountManagerFacadeMock); - mSigninManager = - (SigninManagerImpl) - SigninManagerImpl.create( - NATIVE_SIGNIN_MANAGER, - mProfile, - mAccountTrackerService, - mIdentityManager, - mIdentityMutator, - mSyncService); + createSigninManager(); verify(accountManagerFacadeMock).addObserver(mSigninManager); verify(mIdentityMutator) @@ -211,9 +190,47 @@ public void testAccountManagerFacadeObserverAddedOnCreate_seedAccountsRevampEnab List.of(coreAccountInfo), null); } + @Test + @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) + public void + testAccountManagerFacadeObserverAddedOnCreate_accountFetchFailed_accountListPopulated_seedAccountsRevampEnabled() { + CoreAccountInfo coreAccountInfo = + CoreAccountInfo.createFromEmailAndGaiaId("email@domain.com", "gaia-id"); + AccountManagerFacade accountManagerFacadeMock = Mockito.mock(AccountManagerFacade.class); + when(accountManagerFacadeMock.getCoreAccountInfos()) + .thenReturn(Promise.fulfilled(List.of(coreAccountInfo))); + when(accountManagerFacadeMock.didAccountFetchSucceed()).thenReturn(false); + AccountManagerFacadeProvider.setInstanceForTests(accountManagerFacadeMock); + + createSigninManager(); + + verify(accountManagerFacadeMock).addObserver(mSigninManager); + verify(mIdentityMutator) + .seedAccountsThenReloadAllAccountsWithPrimaryAccount( + List.of(coreAccountInfo), null); + } + + @Test + @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) + public void + testAccountManagerFacadeObserverAddedOnCreate_accountFetchFailed_accountListEmpty_seedAccountsRevampEnabled() { + AccountManagerFacade accountManagerFacadeMock = Mockito.mock(AccountManagerFacade.class); + when(accountManagerFacadeMock.getCoreAccountInfos()) + .thenReturn(Promise.fulfilled(List.of())); + when(accountManagerFacadeMock.didAccountFetchSucceed()).thenReturn(false); + AccountManagerFacadeProvider.setInstanceForTests(accountManagerFacadeMock); + + createSigninManager(); + + verify(accountManagerFacadeMock).addObserver(mSigninManager); + verify(mIdentityMutator, never()) + .seedAccountsThenReloadAllAccountsWithPrimaryAccount(any(), any()); + } + @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void testOnCoreAccountInfosChanged_seedAccountsRevampEnabled() { + createSigninManager(); mFakeAccountManagerFacade.addAccount(ACCOUNT_INFO); List coreAccountInfos = @@ -226,6 +243,7 @@ public void testOnCoreAccountInfosChanged_seedAccountsRevampEnabled() { @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void testOnCoreAccountInfosChanged_signoutWhenPrimaryAccountIsRemoved_seedAccountsRevampEnabled() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( NATIVE_IDENTITY_MANAGER, ConsentLevel.SIGNIN)) .thenReturn(CoreAccountInfo.createFromEmailAndGaiaId("test@email.com", "test-id")); @@ -238,6 +256,7 @@ public void testOnCoreAccountInfosChanged_seedAccountsRevampEnabled() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signinAndTurnSyncOn() { + createSigninManager(); if (SigninFeatureMap.isEnabled(SigninFeatures.ENTERPRISE_POLICY_ON_SIGNIN)) { when(mNativeMock.getUserAcceptedAccountManagement(anyLong())).thenReturn(true); } @@ -293,6 +312,7 @@ public void signinAndTurnSyncOn() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signinAndTurnSyncOn_seedAccountsRevampEnabled() { + createSigninManager(); if (SigninFeatureMap.isEnabled(SigninFeatures.ENTERPRISE_POLICY_ON_SIGNIN)) { when(mNativeMock.getUserAcceptedAccountManagement(anyLong())).thenReturn(true); } @@ -357,6 +377,7 @@ public void signinAndTurnSyncOn_seedAccountsRevampEnabled() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signinNoTurnSyncOn() { + createSigninManager(); when(mIdentityMutator.setPrimaryAccount(any(), anyInt(), anyInt(), any())) .thenReturn(PrimaryAccountError.NO_ERROR); @@ -391,6 +412,7 @@ public void signinNoTurnSyncOn() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signinNoTurnSyncOn_seedAccountsRevampEnabled() { + createSigninManager(); mFakeAccountManagerFacade.addAccount(ACCOUNT_INFO); when(mIdentityMutator.setPrimaryAccount(any(), anyInt(), anyInt(), any())) .thenReturn(PrimaryAccountError.NO_ERROR); @@ -438,6 +460,7 @@ public void signinNoTurnSyncOn_seedAccountsRevampEnabled() { SigninFeatures.SEED_ACCOUNTS_REVAMP }) public void signinNoTurnSyncOn_enterprisePoliciesOnSignin() { + createSigninManager(); when(mIdentityMutator.setPrimaryAccount(any(), anyInt(), anyInt(), any())) .thenReturn(PrimaryAccountError.NO_ERROR); @@ -490,6 +513,7 @@ public void signinNoTurnSyncOn_enterprisePoliciesOnSignin() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutNonSyncingAccountFromJavaWithManagedDomain() { + createSigninManager(); when(mNativeMock.getManagementDomain(NATIVE_SIGNIN_MANAGER)).thenReturn("TestDomain"); // Trigger the sign out flow! @@ -508,6 +532,7 @@ public void signOutNonSyncingAccountFromJavaWithManagedDomain() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutNonSyncingAccountFromJavaWithManagedDomain_seedAccountsRevampEnabled() { + createSigninManager(); when(mNativeMock.getManagementDomain(NATIVE_SIGNIN_MANAGER)).thenReturn("TestDomain"); // Trigger the sign out flow! @@ -528,6 +553,7 @@ public void signOutNonSyncingAccountFromJavaWithManagedDomain_seedAccountsRevamp @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutSyncingAccountFromJavaWithManagedDomain() { + createSigninManager(); when(mNativeMock.getManagementDomain(NATIVE_SIGNIN_MANAGER)).thenReturn("TestDomain"); // Trigger the sign out flow! @@ -546,6 +572,7 @@ public void signOutSyncingAccountFromJavaWithManagedDomain() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutSyncingAccountFromJavaWithManagedDomain_seedAccountsRevampEnabled() { + createSigninManager(); when(mNativeMock.getManagementDomain(NATIVE_SIGNIN_MANAGER)).thenReturn("TestDomain"); // Trigger the sign out flow! @@ -566,6 +593,7 @@ public void signOutSyncingAccountFromJavaWithManagedDomain_seedAccountsRevampEna @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutNonSyncingAccountFromJavaWithNullDomain() { + createSigninManager(); mSigninManager.signOut(SignoutReason.TEST); // The primary account should be cleared *before* clearing any account data. @@ -582,6 +610,7 @@ public void signOutNonSyncingAccountFromJavaWithNullDomain() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutNonSyncingAccountFromJavaWithNullDomain_seedAccountsRevampEnabled() { + createSigninManager(); mSigninManager.signOut(SignoutReason.TEST); // The primary account should be cleared *before* clearing any account data. @@ -600,6 +629,7 @@ public void signOutNonSyncingAccountFromJavaWithNullDomain_seedAccountsRevampEna @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutSyncingAccountFromJavaWithNullDomain() { + createSigninManager(); // Simulate sign-out with non-managed account. when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) @@ -621,6 +651,7 @@ public void signOutSyncingAccountFromJavaWithNullDomain() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutSyncingAccountFromJavaWithNullDomain_seedAccountsRevampEnabled() { + createSigninManager(); // Simulate sign-out with non-managed account. when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) @@ -645,6 +676,7 @@ public void signOutSyncingAccountFromJavaWithNullDomain_seedAccountsRevampEnable @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) @EnableFeatures(ChromeFeatureList.SYNC_ANDROID_LIMIT_NTP_PROMO_IMPRESSIONS) public void syncPromoShowCountResetWhenSignOutSyncingAccount() { + createSigninManager(); ChromeSharedPreferences.getInstance() .writeInt( ChromePreferenceKeys.SYNC_PROMO_SHOW_COUNT.createKey( @@ -678,6 +710,7 @@ public void syncPromoShowCountResetWhenSignOutSyncingAccount() { SigninFeatures.SEED_ACCOUNTS_REVAMP }) public void syncPromoShowCountResetWhenSignOutSyncingAccount_seedAccountsRevampEnabled() { + createSigninManager(); ChromeSharedPreferences.getInstance() .writeInt( ChromePreferenceKeys.SYNC_PROMO_SHOW_COUNT.createKey( @@ -708,6 +741,7 @@ public void syncPromoShowCountResetWhenSignOutSyncingAccount_seedAccountsRevampE @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutSyncingAccountFromJavaWithNullDomainAndForceWipe() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) .thenReturn(ACCOUNT_INFO); @@ -729,6 +763,7 @@ public void signOutSyncingAccountFromJavaWithNullDomainAndForceWipe() { @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutSyncingAccountFromJavaWithNullDomainAndForceWipe_seedAccountsRevampEnabled() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) .thenReturn(ACCOUNT_INFO); @@ -753,6 +788,7 @@ public void signOutSyncingAccountFromJavaWithNullDomainAndForceWipe() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void revokeSyncConsentFromJavaWithNullDomain() { + createSigninManager(); SigninManager.SignOutCallback callback = mock(SigninManager.SignOutCallback.class); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) @@ -774,6 +810,7 @@ public void revokeSyncConsentFromJavaWithNullDomain() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void revokeSyncConsentFromJavaWithNullDomain_seedAccountsRevampEnabled() { + createSigninManager(); SigninManager.SignOutCallback callback = mock(SigninManager.SignOutCallback.class); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) @@ -795,6 +832,7 @@ public void revokeSyncConsentFromJavaWithNullDomain_seedAccountsRevampEnabled() @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void revokeSyncConsentFromJavaWithNullDomainAndWipeData_noLocalUpm() { + createSigninManager(); when(mPasswordManagerUtilBridgeNativeMock.usesSplitStoresAndUPMForLocal(mPrefService)) .thenReturn(false); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( @@ -831,6 +869,7 @@ public void revokeSyncConsentFromJavaWithNullDomainAndWipeData_noLocalUpm() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void revokeSyncConsentFromJavaWithNullDomainAndWipeData_withLocalUpm() { + createSigninManager(); when(mPasswordManagerUtilBridgeNativeMock.usesSplitStoresAndUPMForLocal(mPrefService)) .thenReturn(true); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( @@ -866,6 +905,7 @@ public void revokeSyncConsentFromJavaWithNullDomainAndWipeData_withLocalUpm() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void wipeSyncDataOnly_noLocalUpm() { + createSigninManager(); when(mPasswordManagerUtilBridgeNativeMock.usesSplitStoresAndUPMForLocal(mPrefService)) .thenReturn(false); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( @@ -899,6 +939,7 @@ public void wipeSyncDataOnly_noLocalUpm() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void wipeSyncDataOnly_withLocalUpm() { + createSigninManager(); when(mPasswordManagerUtilBridgeNativeMock.usesSplitStoresAndUPMForLocal(mPrefService)) .thenReturn(true); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( @@ -931,6 +972,7 @@ public void wipeSyncDataOnly_withLocalUpm() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedOut() { + createSigninManager(); mFakeAccountManagerFacade.addAccount( AccountUtils.createAccountFromName(ACCOUNT_INFO.getEmail())); @@ -945,6 +987,7 @@ public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedOut() { @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedOut_seedAccountsRevampEnabled() { + createSigninManager(); mFakeAccountManagerFacade.addAccount(ACCOUNT_INFO); mIdentityManager.onAccountsCookieDeletedByUserAction(); @@ -957,6 +1000,7 @@ public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedOut() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInAndSync() { + createSigninManager(); mFakeAccountManagerFacade.addAccount( AccountUtils.createAccountFromName(ACCOUNT_INFO.getEmail())); @@ -971,6 +1015,7 @@ public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInAndSync( @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInAndSync_seedAccountsRevampEnabled() { + createSigninManager(); mFakeAccountManagerFacade.addAccount( AccountUtils.createAccountFromName(ACCOUNT_INFO.getEmail())); @@ -984,6 +1029,7 @@ public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInAndSync( @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInWithoutSync() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( NATIVE_IDENTITY_MANAGER, ConsentLevel.SIGNIN)) .thenReturn(ACCOUNT_INFO); @@ -1001,10 +1047,8 @@ public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInWithoutS @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInWithoutSync_seedAccountsRevampEnabled() { - // TODO(crbug.com/1491005): Figure out why adding an account to the fake account manager - // changes its id to `id[gaia-id-user_at_domain.com]` and fix this. - mFakeAccountManagerFacade.addAccount( - AccountUtils.createAccountFromName(ACCOUNT_INFO.getEmail())); + createSigninManager(); + mFakeAccountManagerFacade.addAccount(ACCOUNT_INFO); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( NATIVE_IDENTITY_MANAGER, ConsentLevel.SIGNIN)) .thenReturn( @@ -1022,6 +1066,7 @@ public void clearingAccountCookieDoesNotTriggerSignoutWhenUserIsSignedInWithoutS @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void callbackNotifiedWhenNoOperationIsInProgress() { + createSigninManager(); AtomicInteger callCount = new AtomicInteger(0); mSigninManager.runAfterOperationInProgress(callCount::incrementAndGet); @@ -1031,6 +1076,7 @@ public void callbackNotifiedWhenNoOperationIsInProgress() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void callbackNotifiedWhenNoOperationIsInProgress_seedAccountsRevampEnabled() { + createSigninManager(); AtomicInteger callCount = new AtomicInteger(0); mSigninManager.runAfterOperationInProgress(callCount::incrementAndGet); @@ -1046,6 +1092,7 @@ public void callbackNotifiedWhenNoOperationIsInProgress_seedAccountsRevampEnable SigninFeatures.SEED_ACCOUNTS_REVAMP }) public void callbackNotifiedOnSignout() { + createSigninManager(); doAnswer( invocation -> { mIdentityManager.onPrimaryAccountChanged( @@ -1073,6 +1120,7 @@ public void callbackNotifiedOnSignout() { @DisableFeatures(ChromeFeatureList.SYNC_ANDROID_LIMIT_NTP_PROMO_IMPRESSIONS) @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void callbackNotifiedOnSignout_seedAccountsRevampEnabled() { + createSigninManager(); doAnswer( invocation -> { mIdentityManager.onPrimaryAccountChanged( @@ -1096,6 +1144,7 @@ public void callbackNotifiedOnSignout_seedAccountsRevampEnabled() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void callbackNotifiedOnSignin() { + createSigninManager(); if (SigninFeatureMap.isEnabled(SigninFeatures.ENTERPRISE_POLICY_ON_SIGNIN)) { when(mNativeMock.getUserAcceptedAccountManagement(anyLong())).thenReturn(true); } @@ -1134,6 +1183,7 @@ public void callbackNotifiedOnSignin() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void callbackNotifiedOnSignin_seedAccountsRevampEnabled() { + createSigninManager(); if (SigninFeatureMap.isEnabled(SigninFeatures.ENTERPRISE_POLICY_ON_SIGNIN)) { when(mNativeMock.getUserAcceptedAccountManagement(anyLong())).thenReturn(true); } @@ -1173,6 +1223,7 @@ public void callbackNotifiedOnSignin_seedAccountsRevampEnabled() { @Test(expected = AssertionError.class) @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signinfailsWhenAlreadySignedIn() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) .thenReturn(ACCOUNT_INFO); @@ -1182,15 +1233,18 @@ public void signinfailsWhenAlreadySignedIn() { @Test(expected = AssertionError.class) @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signinfailsWhenAlreadySignedIn_seedAccountsRevampEnabled() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) .thenReturn(ACCOUNT_INFO); + mSigninManager.signinAndEnableSync(ACCOUNT_INFO, SigninAccessPoint.UNKNOWN, null); } @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signInStateObserverCallOnSignIn() { + createSigninManager(); if (SigninFeatureMap.isEnabled(SigninFeatures.ENTERPRISE_POLICY_ON_SIGNIN)) { when(mNativeMock.getUserAcceptedAccountManagement(anyLong())).thenReturn(true); } @@ -1232,6 +1286,7 @@ public void signInStateObserverCallOnSignIn() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signInStateObserverCallOnSignIn_seedAccountsRevampEnabled() { + createSigninManager(); if (SigninFeatureMap.isEnabled(SigninFeatures.ENTERPRISE_POLICY_ON_SIGNIN)) { when(mNativeMock.getUserAcceptedAccountManagement(anyLong())).thenReturn(true); } @@ -1281,6 +1336,7 @@ public void signInStateObserverCallOnSignIn_seedAccountsRevampEnabled() { SigninFeatures.SEED_ACCOUNTS_REVAMP }) public void signInStateObserverCallOnSignOut() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) .thenReturn(ACCOUNT_INFO); @@ -1296,6 +1352,7 @@ public void signInStateObserverCallOnSignOut() { @DisableFeatures(ChromeFeatureList.SYNC_ANDROID_LIMIT_NTP_PROMO_IMPRESSIONS) @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signInStateObserverCallOnSignOut_seedAccountsRevampEnabled() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) .thenReturn(ACCOUNT_INFO); @@ -1310,6 +1367,7 @@ public void signInStateObserverCallOnSignOut_seedAccountsRevampEnabled() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutNotAllowedForChildAccounts() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) .thenReturn(ACCOUNT_INFO); @@ -1322,6 +1380,7 @@ public void signOutNotAllowedForChildAccounts() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signOutNotAllowedForChildAccounts_seedAccountsRevampEnabled() { + createSigninManager(); when(mIdentityManagerNativeMock.getPrimaryAccountInfo( eq(NATIVE_IDENTITY_MANAGER), anyInt())) .thenReturn(ACCOUNT_INFO); @@ -1334,6 +1393,7 @@ public void signOutNotAllowedForChildAccounts_seedAccountsRevampEnabled() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signInShouldBeSupportedForNonDemoUsers() { + createSigninManager(); when(mExternalAuthUtils.canUseGooglePlayServices()).thenReturn(true); // Make sure that the user is not a demo user. @@ -1349,6 +1409,7 @@ public void signInShouldBeSupportedForNonDemoUsers() { @Test @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signInShouldBeSupportedForNonDemoUsers_seedAccountsRevampEnabled() { + createSigninManager(); when(mExternalAuthUtils.canUseGooglePlayServices()).thenReturn(true); // Make sure that the user is not a demo user. @@ -1364,6 +1425,7 @@ public void signInShouldBeSupportedForNonDemoUsers_seedAccountsRevampEnabled() { @Test @DisableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signInShouldNotBeSupportedWhenGooglePlayServicesIsRequiredAndNotAvailable() { + createSigninManager(); when(mExternalAuthUtils.canUseGooglePlayServices()).thenReturn(false); // Make sure that the user is not a demo user. @@ -1379,6 +1441,7 @@ public void signInShouldNotBeSupportedWhenGooglePlayServicesIsRequiredAndNotAvai @EnableFeatures(SigninFeatures.SEED_ACCOUNTS_REVAMP) public void signInShouldNotBeSupportedWhenGooglePlayServicesIsRequiredAndNotAvailable_seedAccountsRevampEnabled() { + createSigninManager(); when(mExternalAuthUtils.canUseGooglePlayServices()).thenReturn(false); // Make sure that the user is not a demo user. @@ -1389,4 +1452,17 @@ public void signInShouldNotBeSupportedWhenGooglePlayServicesIsRequiredAndNotAvai assertFalse(mSigninManager.isSigninSupported(/* requireUpdatedPlayServices= */ true)); } + + private void createSigninManager() { + mSigninManager = + (SigninManagerImpl) + SigninManagerImpl.create( + NATIVE_SIGNIN_MANAGER, + mProfile, + mAccountTrackerService, + mIdentityManager, + mIdentityMutator, + mSyncService); + mSigninManager.addSignInStateObserver(mSignInStateObserver); + } } diff --git a/components/signin/public/android/java/src/org/chromium/components/signin/AccountManagerFacade.java b/components/signin/public/android/java/src/org/chromium/components/signin/AccountManagerFacade.java index 9c766905b52461..6520768808ae74 100644 --- a/components/signin/public/android/java/src/org/chromium/components/signin/AccountManagerFacade.java +++ b/components/signin/public/android/java/src/org/chromium/components/signin/AccountManagerFacade.java @@ -140,10 +140,14 @@ void updateCredentials( * * @param account The {@link Account} to confirm the credentials for. * @param activity The {@link Activity} context to use for launching a new authenticator-defined - * sub-Activity to prompt the user to confirm the account's password. + * sub-Activity to prompt the user to confirm the account's password. * @param callback The callback to indicate whether the user successfully confirmed their - * knowledge of the account's credentials. + * knowledge of the account's credentials. */ @AnyThread void confirmCredentials(Account account, Activity activity, Callback callback); + + /** Whether fetching the list of accounts from the device eventually succeeded. */ + // TODO(crbug.com/330304719): Handle this with exceptions rather than a boolean. + boolean didAccountFetchSucceed(); } diff --git a/components/signin/public/android/java/src/org/chromium/components/signin/AccountManagerFacadeImpl.java b/components/signin/public/android/java/src/org/chromium/components/signin/AccountManagerFacadeImpl.java index c628a6996badb7..af2d6fed282761 100644 --- a/components/signin/public/android/java/src/org/chromium/components/signin/AccountManagerFacadeImpl.java +++ b/components/signin/public/android/java/src/org/chromium/components/signin/AccountManagerFacadeImpl.java @@ -80,8 +80,11 @@ public class AccountManagerFacadeImpl implements AccountManagerFacade { private @Nullable AsyncTask> mFetchGaiaIdsTask; private int mNumberOfRetries; + private boolean mDidAccountFetchSucceed; - /** @param delegate the AccountManagerDelegate to use as a backend */ + /** + * @param delegate the AccountManagerDelegate to use as a backend + */ public AccountManagerFacadeImpl(AccountManagerDelegate delegate) { ThreadUtils.assertOnUiThread(); mDelegate = delegate; @@ -262,6 +265,11 @@ public void confirmCredentials(Account account, Activity activity, Callback allAccounts) { - boolean didBackoffSucceed = true; + mDidAccountFetchSucceed = true; if (allAccounts == null) { + mDidAccountFetchSucceed = false; if (shouldRetry()) { // Wait for a fixed amount of time then try to fetch the accounts again. PostTask.postDelayedTask( @@ -356,14 +365,13 @@ protected void onPostExecute(@Nullable List allAccounts) { // We shouldn't wait indefinitely for the account fetching to succeed, at it // might block certain features. Fall back to an empty list to allow the // user to proceed. - allAccounts = List.of(); - didBackoffSucceed = false; + allAccounts = mAllAccounts.get() == null ? mAllAccounts.get() : List.of(); } } if (mNumberOfRetries != 0) { RecordHistogram.recordBooleanHistogram( - "Signin.GetAccountsBackoffSuccess", didBackoffSucceed); - if (didBackoffSucceed) { + "Signin.GetAccountsBackoffSuccess", mDidAccountFetchSucceed); + if (mDidAccountFetchSucceed) { RecordHistogram.recordExactLinearHistogram( "Signin.GetAccountsBackoffRetries", mNumberOfRetries, diff --git a/components/signin/public/android/java/src/org/chromium/components/signin/test/util/FakeAccountManagerFacade.java b/components/signin/public/android/java/src/org/chromium/components/signin/test/util/FakeAccountManagerFacade.java index e1abbdedf2a8dc..86677050e83d5e 100644 --- a/components/signin/public/android/java/src/org/chromium/components/signin/test/util/FakeAccountManagerFacade.java +++ b/components/signin/public/android/java/src/org/chromium/components/signin/test/util/FakeAccountManagerFacade.java @@ -185,6 +185,11 @@ public void confirmCredentials(Account account, Activity activity, Callback