Don't show Signin promo in NTP when the account cache isn't ready

User can hit ANR on NTP because Chrome wants to display a Signin promo.
Signin promo requires a list of Android accounts on a device to display
the user name and the profile picture. The promo view is built
synchronously meaning that the UI thread may be blocked waiting for a
list of accounts.

The list of accounts is requested through the AccountManagerFacade that
maintains the accounts cache. As soon as this cache is populated, it's
safe to synchronously request the list of accounts.

This CL adds an additional check to SignInPromo to not build the promo
view until the cache is populated. The AccountsChangeObserver will
notify the SignInPromo when the cache is built.

Bug: 971618
Change-Id: I3e719bd16ceb077933de410e403d6f4692cf4578
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1710575Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688456}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedSurfaceMediator.java

@@ -324,7 +324,7 @@ class FeedSurfaceMediator
     private class FeedSignInPromo extends SignInPromo {
         FeedSignInPromo(SigninManager signinManager) {
             super(signinManager);
-            updateSignInPromo();
+            maybeUpdateSignInPromo();
         }
 
         @Override
@@ -333,15 +333,20 @@ class FeedSurfaceMediator
 
             super.setVisibilityInternal(visible);
             mCoordinator.updateHeaderViews(visible);
+            maybeUpdateSignInPromo();
         }
 
         @Override
         protected void notifyDataChanged() {
-            updateSignInPromo();
+            maybeUpdateSignInPromo();
         }
 
         /** Update the content displayed in {@link PersonalizedSigninPromoView}. */
-        private void updateSignInPromo() {
+        private void maybeUpdateSignInPromo() {
+            // Only call #setupPromoViewFromCache() if SignInPromo is visible to avoid potentially
+            // blocking the UI thread for several seconds if the accounts cache is not populated
+            // yet.
+            if (!isVisible()) return;
             SigninPromoUtil.setupPromoViewFromCache(mSigninPromoController, mProfileDataCache,
                     mCoordinator.getSigninPromoView(), null);
         }

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java

@@ -45,6 +45,13 @@ public class SignInPromo extends OptionalLeaf {
      */
     private boolean mCanSignIn;
 
+    /**
+     * Whether the list of accounts is ready to be displayed. An attempt to display SignInPromo
+     * while accounts are not ready may cause ANR since the UI thread would be synchronously waiting
+     * for the accounts list.
+     */
+    private boolean mAccountsReady;
+
     /**
      * Whether personalized suggestions can be shown. If it's not the case, we have no reason to
      * offer the user to sign in.
@@ -59,6 +66,7 @@ public class SignInPromo extends OptionalLeaf {
         Context context = ContextUtils.getApplicationContext();
 
         mCanSignIn = signinManager.isSignInAllowed() && !signinManager.isSignedInOnNative();
+        mAccountsReady = AccountManagerFacade.get().isCachePopulated();
         updateVisibility();
 
         int imageSize = context.getResources().getDimensionPixelSize(R.dimen.user_picture_size);
@@ -97,7 +105,7 @@ public class SignInPromo extends OptionalLeaf {
     public static boolean shouldCreatePromo() {
         return !sDisablePromoForTests
                 && !ChromePreferenceManager.getInstance().readBoolean(
-                           ChromePreferenceManager.NTP_SIGNIN_PROMO_DISMISSED, false)
+                        ChromePreferenceManager.NTP_SIGNIN_PROMO_DISMISSED, false)
                 && !getSuppressionStatus();
     }
 
@@ -137,7 +145,8 @@ public class SignInPromo extends OptionalLeaf {
     }
 
     private void updateVisibility() {
-        setVisibilityInternal(!mDismissed && mCanSignIn && mCanShowPersonalizedSuggestions);
+        setVisibilityInternal(
+                !mDismissed && mCanSignIn && mCanShowPersonalizedSuggestions && mAccountsReady);
     }
 
     @Override
@@ -170,6 +179,9 @@ public class SignInPromo extends OptionalLeaf {
         return mSigninObserver;
     }
 
+    /**
+     * Observer to get notifications about various sign-in events.
+     */
     @VisibleForTesting
     public class SigninObserver implements SignInStateObserver, SignInAllowedObserver,
                                            ProfileDataCache.Observer, AccountsChangeObserver {
@@ -223,6 +235,9 @@ public class SignInPromo extends OptionalLeaf {
         // AccountsChangeObserver implementation.
         @Override
         public void onAccountsChanged() {
+            mAccountsReady = AccountManagerFacade.get().isCachePopulated();
+            // We don't change the visibility here to avoid the promo popping up in the feed
+            // unexpectedly. If accounts are ready, the promo will be shown up on the next reload.
             notifyDataChanged();
         }
 

chrome/android/javatests/src/org/chromium/chrome/browser/feed/FeedNewTabPageTest.java

@@ -55,6 +55,7 @@ import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.chrome.test.util.browser.RecyclerViewTestUtils;
 import org.chromium.chrome.test.util.browser.suggestions.SuggestionsDependenciesRule;
 import org.chromium.chrome.test.util.browser.suggestions.mostvisited.FakeMostVisitedSites;
+import org.chromium.components.signin.test.util.AccountManagerTestRule;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.net.test.EmbeddedTestServer;
 
@@ -77,6 +78,9 @@ public class FeedNewTabPageTest {
     @Rule
     public SuggestionsDependenciesRule mSuggestionsDeps = new SuggestionsDependenciesRule();
 
+    @Rule
+    public AccountManagerTestRule mAccountManagerTestRule = new AccountManagerTestRule();
+
     private Tab mTab;
     private FeedNewTabPage mNtp;
     private ViewGroup mTileGridLayout;
@@ -113,7 +117,7 @@ public class FeedNewTabPageTest {
     @Test
     @MediumTest
     @Feature({"FeedNewTabPage"})
-    public void testSignInPromo() throws Exception {
+    public void testSignInPromo() {
         SignInPromo.SigninObserver signinObserver = mNtp.getMediatorForTesting()
                                                             .getSignInPromoForTesting()
                                                             .getSigninObserverForTesting();
@@ -192,6 +196,27 @@ public class FeedNewTabPageTest {
                 ChromePreferenceManager.NTP_SIGNIN_PROMO_DISMISSED, dismissed);
     }
 
+    @Test
+    @MediumTest
+    @Feature({"FeedNewTabPage"})
+    @AccountManagerTestRule.BlockGetAccounts
+    public void testSignInPromo_AccountsNotReady() {
+        // Check that the sign-in promo is not shown if accounts are not ready.
+        onView(instanceOf(RecyclerView.class))
+                .perform(RecyclerViewActions.scrollToPosition(SIGNIN_PROMO_POSITION));
+        onView(withId(R.id.signin_promo_view_container)).check(doesNotExist());
+
+        // Wait for accounts cache population to finish and reload ntp.
+        mAccountManagerTestRule.unblockGetAccountsAndWaitForAccountsPopulated();
+        TestThreadUtils.runOnUiThreadBlocking(() -> mTab.reload());
+        NewTabPageTestUtils.waitForNtpLoaded(mTab);
+
+        // Check that the sign-in promo is displayed this time.
+        onView(instanceOf(RecyclerView.class))
+                .perform(RecyclerViewActions.scrollToPosition(SIGNIN_PROMO_POSITION));
+        onView(withId(R.id.signin_promo_view_container)).check(matches(isDisplayed()));
+    }
+
     @Test
     @MediumTest
     @Feature({"FeedNewTabPage"})

chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java

@@ -275,6 +275,28 @@ public class NewTabPageAdapterTest {
         }
     }
 
+    /**
+     * Shadow implementation of {@link AccountManagerFacade} allowing to mock the cache population
+     * status.
+     * TODO(https://crbug.com/914920): replace this with a mock after a fake implementation for
+     * AccountManagerFacade is available.
+     */
+    @Implements(AccountManagerFacade.class)
+    public static class ShadowAccountManagerFacade {
+        private static boolean sPopulated;
+
+        public ShadowAccountManagerFacade() {}
+
+        @Implementation
+        protected boolean isCachePopulated() {
+            return sPopulated;
+        }
+
+        public static void setCachePopulated() {
+            sPopulated = true;
+        }
+    }
+
     @Before
     public void setUp() {
         // These tests fail on touchless builds, see https://crbug.com/981870.
@@ -1036,6 +1058,22 @@ public class NewTabPageAdapterTest {
         assertFalse(isSignInPromoVisible());
     }
 
+    @Test
+    @Feature({"Ntp"})
+    @Config(shadows = {ShadowAccountManagerFacade.class})
+    public void testSigninPromoAccountsNotReady() {
+        useArticleCategory();
+        when(mMockSigninManager.isSignInAllowed()).thenReturn(true);
+        when(mMockSigninManager.isSignedInOnNative()).thenReturn(false);
+        resetUiDelegate();
+        reloadNtp();
+        assertFalse(isSignInPromoVisible());
+
+        ShadowAccountManagerFacade.setCachePopulated();
+        reloadNtp();
+        assertTrue(isSignInPromoVisible());
+    }
+
     @Test
     @Feature({"Ntp"})
     public void testAllDismissedVisibility() {

components/signin/core/browser/android/BUILD.gn

@@ -100,6 +100,7 @@ android_library("signin_java_test_support") {
 
   java_files = [
     "javatests/src/org/chromium/components/signin/test/util/AccountHolder.java",
+    "javatests/src/org/chromium/components/signin/test/util/AccountManagerTestRule.java",
     "javatests/src/org/chromium/components/signin/test/util/FakeAccountManagerDelegate.java",
     "javatests/src/org/chromium/components/signin/test/util/FakeProfileDataSource.java",
   ]

components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/AccountManagerFacadeTest.java

@@ -8,11 +8,6 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
-import android.accounts.Account;
-import android.accounts.AuthenticatorDescription;
-import android.app.Activity;
-import android.content.Intent;
-import android.support.annotation.Nullable;
 import android.support.test.InstrumentationRegistry;
 import android.support.test.filters.SmallTest;
 
@@ -20,13 +15,11 @@ import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-import org.chromium.base.Callback;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.BaseJUnit4ClassRunner;
-import org.chromium.components.signin.AccountManagerDelegate;
 import org.chromium.components.signin.AccountManagerDelegateException;
 import org.chromium.components.signin.AccountManagerFacade;
-import org.chromium.components.signin.AccountsChangeObserver;
+import org.chromium.components.signin.test.util.FakeAccountManagerDelegate;
 
 import java.util.concurrent.CountDownLatch;
 
@@ -35,65 +28,9 @@ import java.util.concurrent.CountDownLatch;
  */
 @RunWith(BaseJUnit4ClassRunner.class)
 public class AccountManagerFacadeTest {
-    private BlockingAccountManagerDelegate mDelegate = new BlockingAccountManagerDelegate();
-    // TODO(https://crbug.com/885235): Use Mockito instead when it no longer produces test errors.
-    private static class BlockingAccountManagerDelegate implements AccountManagerDelegate {
-        private final CountDownLatch mBlockGetAccounts = new CountDownLatch(1);
-
-        // getAccountsSync always returns the same accounts, so there's no way to track observers.
-        @Override
-        public void registerObservers() {}
-        @Override
-        public void addObserver(AccountsChangeObserver observer) {}
-        @Override
-        public void removeObserver(AccountsChangeObserver observer) {}
-
-        @Override
-        public Account[] getAccountsSync() {
-            // Block background thread that's trying to get accounts from the delegate.
-            try {
-                mBlockGetAccounts.await();
-            } catch (InterruptedException e) {
-                throw new RuntimeException(e);
-            }
-            return new Account[] {AccountManagerFacade.createAccountFromName("test@gmail.com")};
-        }
-
-        void unblockGetAccounts() {
-            mBlockGetAccounts.countDown();
-        }
-
-        @Override
-        public String getAuthToken(Account account, String authTokenScope) {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public void invalidateAuthToken(String authToken) {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public AuthenticatorDescription[] getAuthenticatorTypes() {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public boolean hasFeatures(Account account, String[] features) {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public void createAddAccountIntent(Callback<Intent> callback) {
-            throw new UnsupportedOperationException();
-        }
-
-        @Override
-        public void updateCredentials(
-                Account account, Activity activity, @Nullable Callback<Boolean> callback) {
-            throw new UnsupportedOperationException();
-        }
-    };
+    private FakeAccountManagerDelegate mDelegate =
+            new FakeAccountManagerDelegate(FakeAccountManagerDelegate.DISABLE_PROFILE_DATA_SOURCE,
+                    FakeAccountManagerDelegate.ENABLE_BLOCK_GET_ACCOUNTS);
 
     @Before
     public void setUp() throws Exception {

components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/AccountManagerTestRule.java

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.components.signin.test.util;
+
+import org.junit.rules.TestRule;
+import org.junit.runner.Description;
+import org.junit.runners.model.Statement;
+
+import org.chromium.components.signin.AccountManagerFacade;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * JUnit4 rule for overriding behaviour of {@link AccountManagerFacade} for tests.
+ */
+public class AccountManagerTestRule implements TestRule {
+    private FakeAccountManagerDelegate mDelegate;
+
+    /**
+     * Test method annotation signaling that the account population should be blocked until {@link
+     * #unblockGetAccountsAndWaitForAccountsPopulated()} is called.
+     */
+    @Target(ElementType.METHOD)
+    @Retention(RetentionPolicy.RUNTIME)
+    public @interface BlockGetAccounts {}
+
+    @Override
+    public Statement apply(Statement statement, Description description) {
+        return new Statement() {
+            @Override
+            public void evaluate() throws Throwable {
+                final int profileDataSourceFlag =
+                        FakeAccountManagerDelegate.DISABLE_PROFILE_DATA_SOURCE;
+                final int blockGetAccountsFlag =
+                        description.getAnnotation(BlockGetAccounts.class) == null
+                        ? FakeAccountManagerDelegate.DISABLE_BLOCK_GET_ACCOUNTS
+                        : FakeAccountManagerDelegate.ENABLE_BLOCK_GET_ACCOUNTS;
+                mDelegate =
+                        new FakeAccountManagerDelegate(profileDataSourceFlag, blockGetAccountsFlag);
+                AccountManagerFacade.overrideAccountManagerFacadeForTests(mDelegate);
+                statement.evaluate();
+            }
+        };
+    }
+
+    /**
+     * Unblocks all get accounts requests to {@link AccountManagerFacade}.
+     * Has no effect in tests that are not annotated with {@link BlockGetAccounts}.
+     */
+    public void unblockGetAccountsAndWaitForAccountsPopulated() {
+        mDelegate.unblockGetAccounts();
+        AccountManagerFacade.get().tryGetGoogleAccounts();
+    }
+}

components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/FakeAccountManagerDelegate.java

@@ -60,15 +60,34 @@ public class FakeAccountManagerDelegate implements AccountManagerDelegate {
     /** Use {@link FakeProfileDataSource}. */
     public static final int ENABLE_PROFILE_DATA_SOURCE = 1;
 
+    /** Controls whether FakeAccountManagerDelegate should block get accounts. */
+    @IntDef({ENABLE_BLOCK_GET_ACCOUNTS, DISABLE_BLOCK_GET_ACCOUNTS})
+    @Retention(RetentionPolicy.SOURCE)
+    public @interface BlockGetAccountsFlag {}
+
+    /** Disables block get accounts: {@link #getAccountInfosSync} will return immediately. */
+    public static final int DISABLE_BLOCK_GET_ACCOUNTS = 0;
+    /** Block get accounts until {@link #unblockGetAccounts()} is called. */
+    public static final int ENABLE_BLOCK_GET_ACCOUNTS = 1;
+
     private final Set<AccountHolder> mAccounts = new LinkedHashSet<>();
     private final ObserverList<AccountsChangeObserver> mObservers = new ObserverList<>();
     private boolean mRegisterObserversCalled;
     private FakeProfileDataSource mFakeProfileDataSource;
+    private final CountDownLatch mBlockGetAccounts = new CountDownLatch(1);
 
     public FakeAccountManagerDelegate(@ProfileDataSourceFlag int profileDataSourceFlag) {
+        this(profileDataSourceFlag, DISABLE_BLOCK_GET_ACCOUNTS);
+    }
+
+    public FakeAccountManagerDelegate(@ProfileDataSourceFlag int profileDataSourceFlag,
+            @BlockGetAccountsFlag int blockGetAccountsFlag) {
         if (profileDataSourceFlag == ENABLE_PROFILE_DATA_SOURCE) {
             mFakeProfileDataSource = new FakeProfileDataSource();
         }
+        if (blockGetAccountsFlag == DISABLE_BLOCK_GET_ACCOUNTS) {
+            mBlockGetAccounts.countDown();
+        }
     }
 
     public void setProfileData(
@@ -104,6 +123,13 @@ public class FakeAccountManagerDelegate implements AccountManagerDelegate {
 
     @Override
     public Account[] getAccountsSync() throws AccountManagerDelegateException {
+        // Blocks thread that's trying to get accounts from the delegate.
+        try {
+            mBlockGetAccounts.await();
+        } catch (InterruptedException e) {
+            throw new RuntimeException(e);
+        }
+
         return getAccountsSyncNoThrow();
     }
 
@@ -117,6 +143,10 @@ public class FakeAccountManagerDelegate implements AccountManagerDelegate {
         return result.toArray(new Account[0]);
     }
 
+    public void unblockGetAccounts() {
+        mBlockGetAccounts.countDown();
+    }
+
     /**
      * Add an AccountHolder.
      *