[Android] Reimplement asynchronous methods in AccountManagerFacade

Before this CL, asynchronous methods in AccountManagerFacade were using
AsyncTask to avoid blocking the main thread if account list cache hasn't
been populated yet. However, this approach has two major drawbacks:
1. Every call to these methods blocks one thread from thread pool until
   the cache is populated.
2. Even if the cache is already populated, call still creates AsyncTask,
   incurring unnecessary allocations and thread hops.
This CL introduces AccountManagerFacade.runAfterCacheIsPopulated that
keeps a list of callbacks to execute after the cache is populated. If
the cache has already been populated, this method just posts the
callback to the main thread. All asynchronous account-related methods in
AccountManagerFacade are reimplemented using runAfterCacheIsPopulated.

Bug: 887969
Change-Id: I856b6c7f9395a70857d079dedfbf3f4f6403f901
Reviewed-on: https://chromium-review.googlesource.com/c/1238559
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597536}

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

@@ -66,7 +66,6 @@ android_library("signin_javatests") {
     "//third_party/android_deps:android_support_annotations_java",
     "//third_party/jsr-305:jsr_305_javalib",
     "//third_party/junit",
-    "//third_party/mockito:mockito_java",
   ]
 
   java_files = [ "javatests/src/org/chromium/components/signin/test/AccountManagerFacadeTest.java" ]

components/signin/core/browser/android/java/src/org/chromium/components/signin/AccountManagerFacade.java

@@ -93,6 +93,8 @@ public class AccountManagerFacade {
             new CachedMetrics.TimesHistogramSample(
                     "Signin.AndroidPopulateAccountCacheWaitingTime", TimeUnit.MILLISECONDS);
 
+    private final ArrayList<Runnable> mCallbacksWaitingForCachePopulation = new ArrayList<>();
+
     private int mUpdateTasksCounter;
     private final ArrayList<Runnable> mCallbacksWaitingForPendingUpdates = new ArrayList<>();
 
@@ -222,6 +224,34 @@ public class AccountManagerFacade {
         return new Account(name, GOOGLE_ACCOUNT_TYPE);
     }
 
+    /**
+     * Runs a callback after the account list cache is populated. In the callback
+     * {@link #getGoogleAccounts()} and similar methods are guaranteed to return instantly (without
+     * blocking and waiting for the cache to be populated). If the cache has already been populated,
+     * the callback will be posted on UI thread.
+     * @param runnable The callback to call after cache is populated. Invoked on the main thread.
+     */
+    @MainThread
+    @VisibleForTesting
+    public void runAfterCacheIsPopulated(Runnable runnable) {
+        ThreadUtils.assertOnUiThread();
+        if (isCachePopulated()) {
+            ThreadUtils.postOnUiThread(runnable);
+            return;
+        }
+        mCallbacksWaitingForCachePopulation.add(runnable);
+    }
+
+    /**
+     * Returns whether the account cache has already been populated. {@link #getGoogleAccounts()}
+     * and similar methods will return instantly if the cache has been populated, otherwise these
+     * methods may block waiting for the cache to be populated.
+     */
+    @AnyThread
+    public boolean isCachePopulated() {
+        return mFilteredAccounts.get() != null;
+    }
+
     /**
      * Retrieves a list of the Google account names on the device.
      *
@@ -255,13 +285,7 @@ public class AccountManagerFacade {
      */
     @MainThread
     public void tryGetGoogleAccountNames(final Callback<List<String>> callback) {
-        tryGetGoogleAccounts(accounts -> {
-            List<String> accountNames = new ArrayList<>();
-            for (Account account : accounts) {
-                accountNames.add(account.name);
-            }
-            callback.onResult(accountNames);
-        });
+        runAfterCacheIsPopulated(() -> callback.onResult(tryGetGoogleAccountNames()));
     }
 
     /**
@@ -270,7 +294,8 @@ public class AccountManagerFacade {
     @MainThread
     public void getGoogleAccountNames(
             final Callback<AccountManagerResult<List<String>>> callback) {
-        getGoogleAccounts(accounts -> {
+        runAfterCacheIsPopulated(() -> {
+            final AccountManagerResult<Account[]> accounts = mFilteredAccounts.get();
             final AccountManagerResult<List<String>> result;
             if (accounts.hasValue()) {
                 List<String> accountNames = new ArrayList<>(accounts.getValue().length);
@@ -285,16 +310,6 @@ public class AccountManagerFacade {
         });
     }
 
-    /**
-     * Returns whether the account cache has already been populated. {@link #getGoogleAccounts()}
-     * and similar methods will return instantly if the cache has been populated, otherwise these
-     * methods may block waiting for the cache to be populated.
-     */
-    @AnyThread
-    public boolean isCachePopulated() {
-        return mFilteredAccounts.get() != null;
-    }
-
     /**
      * Retrieves all Google accounts on the device.
      *
@@ -324,28 +339,9 @@ public class AccountManagerFacade {
     /**
      * Asynchronous version of {@link #getGoogleAccounts()}.
      */
-    // Incorrectly infers that this is called on a worker thread because of AsyncTask doInBackground
-    // overriding.
-    @SuppressWarnings("WrongThread")
     @MainThread
     public void getGoogleAccounts(final Callback<AccountManagerResult<Account[]>> callback) {
-        ThreadUtils.assertOnUiThread();
-        new AsyncTask<AccountManagerResult<Account[]>>() {
-            @Override
-            protected AccountManagerResult<Account[]> doInBackground() {
-                try {
-                    return new AccountManagerResult<>(getGoogleAccounts());
-                } catch (AccountManagerDelegateException ex) {
-                    return new AccountManagerResult<>(ex);
-                }
-            }
-
-            @Override
-            protected void onPostExecute(AccountManagerResult<Account[]> accounts) {
-                callback.onResult(accounts);
-            }
-        }
-                .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
+        runAfterCacheIsPopulated(() -> callback.onResult(mFilteredAccounts.get()));
     }
 
     /**
@@ -364,24 +360,9 @@ public class AccountManagerFacade {
     /**
      * Asynchronous version of {@link #tryGetGoogleAccounts()}.
      */
-    // Incorrectly infers that this is called on a worker thread because of AsyncTask doInBackground
-    // overriding.
-    @SuppressWarnings("WrongThread")
     @MainThread
     public void tryGetGoogleAccounts(final Callback<Account[]> callback) {
-        ThreadUtils.assertOnUiThread();
-        new AsyncTask<Account[]>() {
-            @Override
-            protected Account[] doInBackground() {
-                return tryGetGoogleAccounts();
-            }
-
-            @Override
-            protected void onPostExecute(Account[] accounts) {
-                callback.onResult(accounts);
-            }
-        }
-                .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
+        runAfterCacheIsPopulated(() -> callback.onResult(tryGetGoogleAccounts()));
     }
 
     /**
@@ -398,7 +379,7 @@ public class AccountManagerFacade {
      */
     @MainThread
     public void hasGoogleAccounts(final Callback<Boolean> callback) {
-        tryGetGoogleAccounts(accounts -> callback.onResult(accounts.length > 0));
+        runAfterCacheIsPopulated(() -> callback.onResult(hasGoogleAccounts()));
     }
 
     private String canonicalizeName(String name) {
@@ -435,17 +416,7 @@ public class AccountManagerFacade {
      */
     @MainThread
     public void getAccountFromName(String accountName, final Callback<Account> callback) {
-        final String canonicalName = canonicalizeName(accountName);
-        tryGetGoogleAccounts(accounts -> {
-            Account accountForName = null;
-            for (Account account : accounts) {
-                if (canonicalizeName(account.name).equals(canonicalName)) {
-                    accountForName = account;
-                    break;
-                }
-            }
-            callback.onResult(accountForName);
-        });
+        runAfterCacheIsPopulated(() -> callback.onResult(getAccountFromName(accountName)));
     }
 
     /**
@@ -464,7 +435,7 @@ public class AccountManagerFacade {
     @VisibleForTesting
     @MainThread
     public void hasAccountForName(String accountName, final Callback<Boolean> callback) {
-        getAccountFromName(accountName, account -> callback.onResult(account != null));
+        runAfterCacheIsPopulated(() -> callback.onResult(hasAccountForName(accountName)));
     }
 
     /**
@@ -746,6 +717,11 @@ public class AccountManagerFacade {
 
         @Override
         protected void onPostExecute(Void v) {
+            for (Runnable callback : mCallbacksWaitingForCachePopulation) {
+                callback.run();
+            }
+            mCallbacksWaitingForCachePopulation.clear();
+
             fireOnAccountsChangedNotification();
             decrementUpdateCounter();
         }

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

@@ -4,20 +4,29 @@
 
 package org.chromium.components.signin.test;
 
-import static org.mockito.Mockito.doAnswer;
+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;
 
-import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.mockito.Mockito;
 
+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 java.util.concurrent.CountDownLatch;
 
@@ -26,28 +35,106 @@ import java.util.concurrent.CountDownLatch;
  */
 @RunWith(BaseJUnit4ClassRunner.class)
 public class AccountManagerFacadeTest {
-    @Test
-    @SmallTest
-    public void testIsCachePopulated() throws AccountManagerDelegateException {
-        AccountManagerDelegate delegate = Mockito.mock(AccountManagerDelegate.class);
+    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);
 
-        final Account account = AccountManagerFacade.createAccountFromName("test@gmail.com");
-        final CountDownLatch blockGetAccounts = new CountDownLatch(1);
-        doAnswer(invocation -> {
+        // 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.
-            blockGetAccounts.await();
-            return new Account[] {account};
-        }).when(delegate).getAccountsSync();
+            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();
+        }
 
-        AccountManagerFacade.overrideAccountManagerFacadeForTests(delegate);
-        AccountManagerFacade facade = AccountManagerFacade.get();
+        @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();
+        }
+    };
+
+    @Before
+    public void setUp() throws Exception {
+        AccountManagerFacade.overrideAccountManagerFacadeForTests(mDelegate);
+    }
+
+    @Test
+    @SmallTest
+    public void testIsCachePopulated() throws AccountManagerDelegateException {
         // Cache shouldn't be populated until getAccountsSync is unblocked.
-        Assert.assertFalse(facade.isCachePopulated());
+        assertFalse(AccountManagerFacade.get().isCachePopulated());
 
-        blockGetAccounts.countDown();
+        mDelegate.unblockGetAccounts();
         // Wait for cache population to finish.
         AccountManagerFacade.get().getGoogleAccounts();
-        Assert.assertTrue(facade.isCachePopulated());
+        assertTrue(AccountManagerFacade.get().isCachePopulated());
+    }
+
+    @Test
+    @SmallTest
+    public void testRunAfterCacheIsPopulated() throws InterruptedException {
+        CountDownLatch firstCounter = new CountDownLatch(1);
+        ThreadUtils.runOnUiThreadBlocking(() -> {
+            // Add callback. This should be done on the main thread.
+            AccountManagerFacade.get().runAfterCacheIsPopulated(firstCounter::countDown);
+        });
+        assertEquals("Callback shouldn't be invoked until cache is populated", 1,
+                firstCounter.getCount());
+
+        mDelegate.unblockGetAccounts();
+        // Cache should be populated & callback should be invoked
+        firstCounter.await();
+
+        CountDownLatch secondCounter = new CountDownLatch(1);
+        ThreadUtils.runOnUiThreadBlocking(() -> {
+            AccountManagerFacade.get().runAfterCacheIsPopulated(secondCounter::countDown);
+            assertEquals("Callback should be posted on UI thread, not executed synchronously", 1,
+                    secondCounter.getCount());
+        });
+        InstrumentationRegistry.getInstrumentation().waitForIdleSync();
+        assertEquals(
+                "Callback should be posted to UI thread right away", 0, secondCounter.getCount());
     }
 }