[Signin][Android] Suppress sign-in promo if account cache isn't populated yet

Fixes an ANR that happens on Chrome startup. Now, if the list of
accounts is not available when launchSigninPromoIfNeeded is invoked,
the promo is suppressed.

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

chrome/android/java/src/org/chromium/chrome/browser/signin/SigninPromoUtil.java

@@ -12,13 +12,11 @@ import androidx.annotation.VisibleForTesting;
 import androidx.collection.ArraySet;
 
 import org.chromium.base.annotations.CalledByNative;
-import org.chromium.base.supplier.Supplier;
 import org.chromium.chrome.browser.ChromeVersionInfo;
 import org.chromium.chrome.browser.preferences.Pref;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.signin.AccountManagerFacadeProvider;
 import org.chromium.components.signin.AccountUtils;
-import org.chromium.components.signin.identitymanager.IdentityManager;
 import org.chromium.components.signin.metrics.SigninAccessPoint;
 import org.chromium.components.user_prefs.UserPrefs;
 import org.chromium.ui.base.WindowAndroid;
@@ -39,24 +37,29 @@ public class SigninPromoUtil {
      * @return Whether the signin promo is shown.
      */
     public static boolean launchSigninPromoIfNeeded(final Activity activity) {
+        if (!AccountManagerFacadeProvider.getInstance().isCachePopulated()) {
+            // Suppress the promo if the account list isn't available yet.
+            return false;
+        }
+
         SigninPreferencesManager preferencesManager = SigninPreferencesManager.getInstance();
         int currentMajorVersion = ChromeVersionInfo.getProductMajorVersion();
+        boolean isSignedIn = IdentityServicesProvider.get()
+                                     .getIdentityManager(Profile.getLastUsedRegularProfile())
+                                     .hasPrimaryAccount();
         boolean wasSignedIn =
                 TextUtils.isEmpty(UserPrefs.get(Profile.getLastUsedRegularProfile())
                                           .getString(Pref.GOOGLE_SERVICES_LAST_USERNAME));
-        List<String> accountNames = AccountUtils.toAccountNames(
-                AccountManagerFacadeProvider.getInstance().tryGetGoogleAccounts());
-        Supplier<Set<String>> accountNamesSupplier = () -> new ArraySet<>(accountNames);
-        IdentityManager identityManager = IdentityServicesProvider.get().getIdentityManager(
-                Profile.getLastUsedRegularProfile());
-        if (!shouldLaunchSigninPromo(preferencesManager, currentMajorVersion,
-                    identityManager.hasPrimaryAccount(), wasSignedIn, accountNamesSupplier)) {
+        Set<String> accountNames = new ArraySet<>(AccountUtils.toAccountNames(
+                AccountManagerFacadeProvider.getInstance().tryGetGoogleAccounts()));
+        if (!shouldLaunchSigninPromo(preferencesManager, currentMajorVersion, isSignedIn,
+                    wasSignedIn, accountNames)) {
             return false;
         }
 
         SigninUtils.startSigninActivityIfAllowed(activity, SigninAccessPoint.SIGNIN_PROMO);
         preferencesManager.setSigninPromoLastShownVersion(currentMajorVersion);
-        preferencesManager.setSigninPromoLastAccountNames(new ArraySet<>(accountNames));
+        preferencesManager.setSigninPromoLastAccountNames(accountNames);
         return true;
     }
 
@@ -66,15 +69,13 @@ public class SigninPromoUtil {
      * @param currentMajorVersion the current major version of Chrome
      * @param isSignedIn is user currently signed in
      * @param wasSignedIn has used manually signed out
-     * @param accountNamesSupplier the supplier of the set of account names currently on device.
-     *         Supplier is used here because AccountManagerFacade cache may be not populated yet, so
-     *         it makes sense to check other flags before getting accounts.
+     * @param accountNames the list of accounts on the device
      * @return Whether the signin promo should be shown.
      */
     @VisibleForTesting
     static boolean shouldLaunchSigninPromo(SigninPreferencesManager preferencesManager,
             int currentMajorVersion, boolean isSignedIn, boolean wasSignedIn,
-            Supplier<Set<String>> accountNamesSupplier) {
+            Set<String> accountNames) {
         int lastPromoMajorVersion = preferencesManager.getSigninPromoLastShownVersion();
         if (lastPromoMajorVersion == 0) {
             preferencesManager.setSigninPromoLastShownVersion(currentMajorVersion);
@@ -90,9 +91,7 @@ public class SigninPromoUtil {
         // Promo can be shown at most once every 2 Chrome major versions.
         if (currentMajorVersion < lastPromoMajorVersion + 2) return false;
 
-        // Defer getting accounts, as AccountManagerFacade cache may be not populated yet.
-        Set<String> accountNames = accountNamesSupplier.get();
-        // Don't show if there are no Google accounts on the device.
+        // Don't show if the account list isn't available yet or there are no accounts in it.
         if (accountNames.isEmpty()) return false;
 
         // Don't show if no new accounts have been added after the last time promo was shown.

chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninPromoUtilTest.java

@@ -25,6 +25,7 @@ import org.chromium.base.supplier.Supplier;
 import org.chromium.base.test.BaseRobolectricTestRunner;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Set;
 
 /** Tests for {@link SigninPromoUtil}. */
@@ -45,17 +46,6 @@ public class SigninPromoUtilTest {
         verifyNoMoreInteractions(ignoreStubs(mPreferencesManager));
     }
 
-    /**
-     * Returns account list supplier that throws unchecked exception, causing the test to fail. This
-     * is helpful to ensure that other signals for {@link SigninPromoUtil#shouldLaunchSigninPromo}
-     * are checked before getting account list (as account list may not be ready yet).
-     */
-    private Supplier<Set<String>> shouldNotTryToGetAccounts() {
-        return () -> {
-            throw new RuntimeException("Should not try to get accounts!");
-        };
-    }
-
     /**
      * Creates a {@link Supplier} that returns a list of accounts provided to this method.
      * @param accountNames The account names to return from {@link Supplier}
@@ -68,7 +58,7 @@ public class SigninPromoUtilTest {
     public void whenNoLastShownVersionShouldReturnFalseAndSaveVersion() {
         when(mPreferencesManager.getSigninPromoLastShownVersion()).thenReturn(0);
         Assert.assertFalse(SigninPromoUtil.shouldLaunchSigninPromo(
-                mPreferencesManager, 42, false, false, shouldNotTryToGetAccounts()));
+                mPreferencesManager, 42, false, false, ImmutableSet.of("test@gmail.com")));
         verify(mPreferencesManager).setSigninPromoLastShownVersion(42);
     }
 
@@ -76,28 +66,28 @@ public class SigninPromoUtilTest {
     public void whenSignedInShouldReturnFalse() {
         when(mPreferencesManager.getSigninPromoLastShownVersion()).thenReturn(38);
         Assert.assertFalse(SigninPromoUtil.shouldLaunchSigninPromo(
-                mPreferencesManager, 42, true, false, shouldNotTryToGetAccounts()));
+                mPreferencesManager, 42, true, false, ImmutableSet.of("test@gmail.com")));
     }
 
     @Test
     public void whenWasSignedInShouldReturnFalse() {
         when(mPreferencesManager.getSigninPromoLastShownVersion()).thenReturn(38);
         Assert.assertFalse(SigninPromoUtil.shouldLaunchSigninPromo(
-                mPreferencesManager, 42, false, true, shouldNotTryToGetAccounts()));
+                mPreferencesManager, 42, false, true, ImmutableSet.of("test@gmail.com")));
     }
 
     @Test
     public void whenVersionDifferenceTooSmallShouldReturnFalse() {
         when(mPreferencesManager.getSigninPromoLastShownVersion()).thenReturn(41);
         Assert.assertFalse(SigninPromoUtil.shouldLaunchSigninPromo(
-                mPreferencesManager, 42, false, false, shouldNotTryToGetAccounts()));
+                mPreferencesManager, 42, false, false, ImmutableSet.of("test@gmail.com")));
     }
 
     @Test
     public void whenNoAccountsShouldReturnFalse() {
         when(mPreferencesManager.getSigninPromoLastShownVersion()).thenReturn(38);
         Assert.assertFalse(SigninPromoUtil.shouldLaunchSigninPromo(
-                mPreferencesManager, 42, false, false, accountsSupplier()));
+                mPreferencesManager, 42, false, false, Collections.emptySet()));
     }
 
     @Test
@@ -106,7 +96,7 @@ public class SigninPromoUtilTest {
         // Old implementation hasn't been storing account list
         when(mPreferencesManager.getSigninPromoLastAccountNames()).thenReturn(null);
         Assert.assertTrue(SigninPromoUtil.shouldLaunchSigninPromo(
-                mPreferencesManager, 42, false, false, accountsSupplier("test@gmail.com")));
+                mPreferencesManager, 42, false, false, ImmutableSet.of("test@gmail.com")));
     }
 
     @Test
@@ -115,7 +105,7 @@ public class SigninPromoUtilTest {
         when(mPreferencesManager.getSigninPromoLastAccountNames())
                 .thenReturn(ImmutableSet.of("test@gmail.com"));
         Assert.assertTrue(SigninPromoUtil.shouldLaunchSigninPromo(mPreferencesManager, 42, false,
-                false, accountsSupplier("test@gmail.com", "test2@gmail.com")));
+                false, ImmutableSet.of("test@gmail.com", "test2@gmail.com")));
     }
 
     @Test
@@ -124,7 +114,7 @@ public class SigninPromoUtilTest {
         when(mPreferencesManager.getSigninPromoLastAccountNames())
                 .thenReturn(ImmutableSet.of("test@gmail.com"));
         Assert.assertFalse(SigninPromoUtil.shouldLaunchSigninPromo(
-                mPreferencesManager, 42, false, false, accountsSupplier("test@gmail.com")));
+                mPreferencesManager, 42, false, false, ImmutableSet.of("test@gmail.com")));
     }
 
     @Test
@@ -133,6 +123,6 @@ public class SigninPromoUtilTest {
         when(mPreferencesManager.getSigninPromoLastAccountNames())
                 .thenReturn(ImmutableSet.of("test@gmail.com", "test2@gmail.com"));
         Assert.assertFalse(SigninPromoUtil.shouldLaunchSigninPromo(
-                mPreferencesManager, 42, false, false, accountsSupplier("test2@gmail.com")));
+                mPreferencesManager, 42, false, false, ImmutableSet.of("test2@gmail.com")));
     }
 }