Prevent Android from causing AndroidSyncSettings use on background thread

AndroidSyncSettings registers a listener that is called by the Android
system on a background thread. Before this CL, the listener directly
modified the state of the class when notified. Because
AndroidSyncSettings is also used from another thread (UI), this required
it to be implemented in a thread-safe manner.

In this CL, we modify the listener to rather post the update to the UI
thread, one step towards having this class be used from a single thread.
As a bonus, objects that in turn observe AndroidSyncSettings will now
only be notified on the UI thread, which solves the linked bug. Cleaning
up the observers so they don't post to the UI thread themselves is left
for the next CL.

We also take the time to brush up MockSyncContentResolverDelegate, namely
simplify its notification logic and use a global lock for the class.

Bug: 1028568
Change-Id: Iac820125bc37796eade94b503b8e0983b171108f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2415150Reviewed-by: Friedrich [CET] <fhorschig@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#808289}

chrome/android/java/src/org/chromium/chrome/browser/sync/AndroidSyncSettings.java

@@ -18,6 +18,7 @@ import org.chromium.base.ContextUtils;
 import org.chromium.base.ObserverList;
 import org.chromium.base.StrictModeContext;
 import org.chromium.base.ThreadUtils;
+import org.chromium.base.task.PostTask;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.signin.IdentityServicesProvider;
@@ -27,6 +28,7 @@ import org.chromium.components.signin.identitymanager.ConsentLevel;
 import org.chromium.components.signin.identitymanager.IdentityManager;
 import org.chromium.components.sync.SyncContentResolverDelegate;
 import org.chromium.components.sync.SystemSyncContentResolverDelegate;
+import org.chromium.content_public.browser.UiThreadTaskTraits;
 
 /**
  * WARNING: Chrome will be decoupled from Android auto-sync (crbug.com/1105795).
@@ -102,6 +104,13 @@ public class AndroidSyncSettings {
      * @param callback Callback that will be called after updating account is finished.
      * @param account The sync account if sync is enabled, null otherwise.
      */
+    // TODO(crbug.com/1125622): Once this class is used only on the UI thread, |callback|
+    // can be removed (syncability update will be synchronous). Same goes for the corresponding
+    // parameter in |updateAccount()|.
+    // TODO(crbug.com/1125622): Exposing these testing constructors that don't register the
+    // singleton instance can be dangerous when there's code that explicitly calls |get()|
+    // (in that case, a new object would be returned, not the one constructed by the test).
+    // Consider exposing them as static methods that also register a singleton instance.
     @VisibleForTesting
     public AndroidSyncSettings(SyncContentResolverDelegate syncContentResolverDelegate,
             @Nullable Runnable callback, @Nullable Account account) {
@@ -112,19 +121,21 @@ public class AndroidSyncSettings {
         updateCachedSettings();
         updateSyncability(callback);
 
-        mSyncContentResolverDelegate.addStatusChangeListener(
-                ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS, new SyncStatusObserver() {
-                    @Override
-                    public void onStatusChanged(int which) {
-                        if (which == ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS) {
-                            // Sync settings have changed; update our cached values.
-                            if (updateCachedSettings()) {
-                                // If something actually changed, tell our observers.
-                                notifyObservers();
-                            }
-                        }
+        SyncStatusObserver androidOsListener = new SyncStatusObserver() {
+            @Override
+            public void onStatusChanged(int which) {
+                if (which != ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS) return;
+                // This is called by Android on a background thread, but AndroidSyncSettings
+                // methods should be called from the UI thread, so post a task.
+                PostTask.postTask(UiThreadTaskTraits.DEFAULT, () -> {
+                    if (updateCachedSettings()) {
+                        notifyObservers();
                     }
                 });
+            }
+        };
+        mSyncContentResolverDelegate.addStatusChangeListener(
+                ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS, androidOsListener);
     }
 
     /**

chrome/android/javatests/src/org/chromium/chrome/browser/password_manager/settings/PasswordViewingTypeTest.java

@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.password_manager.settings;
 
+import static org.mockito.Mockito.when;
+
 import android.accounts.Account;
 
 import androidx.test.filters.SmallTest;
@@ -14,6 +16,9 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.RuleChain;
 import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
 
 import org.chromium.base.test.BaseJUnit4ClassRunner;
 import org.chromium.base.test.util.Feature;
@@ -23,7 +28,6 @@ import org.chromium.chrome.browser.sync.AndroidSyncSettings;
 import org.chromium.chrome.browser.sync.ProfileSyncService;
 import org.chromium.chrome.test.ChromeBrowserTestRule;
 import org.chromium.components.browser_ui.settings.ChromeBasePreference;
-import org.chromium.components.sync.test.util.MockSyncContentResolverDelegate;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 
 /**
@@ -44,24 +48,26 @@ public class PasswordViewingTypeTest {
     public final RuleChain mRuleChain =
             RuleChain.outerRule(mChromeBrowserTestRule).around(mSettingsActivityTestRule);
 
+    @Rule
+    public final MockitoRule mMockitoRule = MockitoJUnit.rule();
+
     private ChromeBasePreference mPasswordsPref;
-    private MockSyncContentResolverDelegate mSyncContentResolverDelegate;
     private String mAuthority;
     private Account mAccount;
+    @Mock
+    private AndroidSyncSettings mAndroidSyncSettings;
 
     @Before
     public void setUp() {
         mAccount = mChromeBrowserTestRule.addAccount("account@example.com");
-        mSyncContentResolverDelegate = new MockSyncContentResolverDelegate();
         mSettingsActivityTestRule.startSettingsActivity();
         MainSettings mainSettings = mSettingsActivityTestRule.getFragment();
         mPasswordsPref =
                 (ChromeBasePreference) mainSettings.findPreference(MainSettings.PREF_PASSWORDS);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
-            AndroidSyncSettings.overrideForTests(
-                    new AndroidSyncSettings(mSyncContentResolverDelegate));
+            AndroidSyncSettings.overrideForTests(mAndroidSyncSettings);
+            setSyncability(false);
             mAuthority = AndroidSyncSettings.getContractAuthority();
-            AndroidSyncSettings.get().updateAccount(mAccount);
         });
     }
 
@@ -87,19 +93,12 @@ public class PasswordViewingTypeTest {
     /**
      * Turn syncability on/off.
      */
-    private void setSyncability(boolean syncState) throws InterruptedException {
-        // Turn on syncability
-        mSyncContentResolverDelegate.setMasterSyncAutomatically(syncState);
-        mSyncContentResolverDelegate.waitForLastNotificationCompleted();
-
-        // First sync
-        mSyncContentResolverDelegate.setIsSyncable(mAccount, mAuthority, (syncState) ? 1 : 0);
-        mSyncContentResolverDelegate.waitForLastNotificationCompleted();
-
-        if (syncState) {
-            mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, syncState);
-            mSyncContentResolverDelegate.waitForLastNotificationCompleted();
-        }
+    private void setSyncability(boolean syncState) {
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            when(mAndroidSyncSettings.isSyncEnabled()).thenReturn(syncState);
+            when(mAndroidSyncSettings.isChromeSyncEnabled()).thenReturn(syncState);
+            when(mAndroidSyncSettings.doesMasterSyncSettingAllowChromeSync()).thenReturn(true);
+        });
     }
 
     /**
@@ -109,7 +108,7 @@ public class PasswordViewingTypeTest {
     @Test
     @SmallTest
     @Feature({"Sync"})
-    public void testUserRedirectSyncSettings() throws InterruptedException {
+    public void testUserRedirectSyncSettings() {
         setSyncability(true);
         overrideProfileSyncService(false);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
@@ -127,7 +126,7 @@ public class PasswordViewingTypeTest {
      */
     @Test
     @SmallTest
-    public void testSyncingNativePasswordView() throws InterruptedException {
+    public void testSyncingNativePasswordView() {
         setSyncability(true);
         overrideProfileSyncService(true);
         Assert.assertEquals(
@@ -140,7 +139,7 @@ public class PasswordViewingTypeTest {
      */
     @Test
     @SmallTest
-    public void testNonSyncingNativePasswordView() throws InterruptedException {
+    public void testNonSyncingNativePasswordView() {
         setSyncability(false);
         Assert.assertEquals(
                 PasswordSettings.class.getCanonicalName(), mPasswordsPref.getFragment());

chrome/android/javatests/src/org/chromium/chrome/browser/sync/AndroidSyncSettingsTestUtils.java

@@ -9,6 +9,9 @@ import androidx.annotation.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.components.sync.SyncContentResolverDelegate;
 import org.chromium.components.sync.test.util.MockSyncContentResolverDelegate;
+import org.chromium.content_public.browser.test.util.TestThreadUtils;
+
+import java.util.concurrent.Callable;
 
 /**
  * A utility class for mocking AndroidSyncSettings.
@@ -25,7 +28,6 @@ public class AndroidSyncSettingsTestUtils {
         setUpAndroidSyncSettingsForTesting(new MockSyncContentResolverDelegate());
     }
 
-    @VisibleForTesting
     public static void setUpAndroidSyncSettingsForTesting(SyncContentResolverDelegate delegate) {
         delegate.setMasterSyncAutomatically(true);
         // Explicitly pass null account to AndroidSyncSettings ctor. Normally, AndroidSyncSettings
@@ -33,4 +35,31 @@ public class AndroidSyncSettingsTestUtils {
         // before profiles are initialized (when IdentityManager doesn't exist yet).
         AndroidSyncSettings.overrideForTests(new AndroidSyncSettings(delegate, null, null));
     }
+
+    public static boolean getIsSyncEnabledOnUiThread() {
+        return TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
+            @Override
+            public Boolean call() {
+                return AndroidSyncSettings.get().isSyncEnabled();
+            }
+        });
+    }
+
+    public static boolean getIsChromeSyncEnabledOnUiThread() {
+        return TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
+            @Override
+            public Boolean call() {
+                return AndroidSyncSettings.get().isChromeSyncEnabled();
+            }
+        });
+    }
+
+    public static boolean getDoesMasterSyncAllowSyncOnUiThread() {
+        return TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
+            @Override
+            public Boolean call() {
+                return AndroidSyncSettings.get().doesMasterSyncSettingAllowChromeSync();
+            }
+        });
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java

@@ -27,6 +27,7 @@ import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.chrome.test.util.browser.signin.MockChangeEventChecker;
 import org.chromium.chrome.test.util.browser.sync.SyncTestUtil;
+import org.chromium.components.sync.test.util.MockSyncContentResolverDelegate;
 import org.chromium.content_public.browser.test.util.Criteria;
 import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
@@ -107,10 +108,16 @@ public class SyncTest {
             SigninHelper.updateAccountRenameData(eventChecker, oldAccount.name);
 
             // Tell the fake content resolver that a rename had happen and copy over the sync
-            // settings. This would normally be done by the
-            // SystemSyncTestRule.getSyncContentResolver().
-            mSyncTestRule.getSyncContentResolver().renameAccounts(
-                    oldAccount, newAccount, AndroidSyncSettings.getContractAuthority());
+            // settings.
+            MockSyncContentResolverDelegate contentResolver =
+                    mSyncTestRule.getSyncContentResolver();
+            String authority = AndroidSyncSettings.getContractAuthority();
+            int oldIsSyncable = contentResolver.getIsSyncable(oldAccount, authority);
+            contentResolver.setIsSyncable(newAccount, authority, oldIsSyncable);
+            if (oldIsSyncable > 0) {
+                contentResolver.setSyncAutomatically(newAccount, authority,
+                        contentResolver.getSyncAutomatically(oldAccount, authority));
+            }
 
             // Starts the rename process. Normally, this is triggered by the broadcast
             // listener as well.
@@ -223,8 +230,6 @@ public class SyncTest {
 
         // Disabling master sync first. Sync should be off.
         mSyncTestRule.getSyncContentResolver().setMasterSyncAutomatically(false);
-        // TODO(crbug.com/921025): Master sync shouldn't influence isSyncRequested, so at this point
-        // isSyncRequested should be true; only canSyncFeatureStart should be false.
         Assert.assertFalse(SyncTestUtil.isSyncRequested());
         Assert.assertFalse(SyncTestUtil.canSyncFeatureStart());
 
@@ -235,9 +240,6 @@ public class SyncTest {
 
         // Re-enabling Chrome sync should not turn sync back on.
         mSyncTestRule.getSyncContentResolver().setSyncAutomatically(account, authority, true);
-        // TODO(crbug.com/921025): Master sync (which is still disabled) shouldn't influence
-        // isSyncRequested, so at this point isSyncRequested should be true (but
-        // canSyncFeatureStart should remain false).
         Assert.assertFalse(SyncTestUtil.isSyncRequested());
         Assert.assertFalse(SyncTestUtil.canSyncFeatureStart());
 

components/sync/test/android/javatests/src/org/chromium/components/sync/test/util/MockSyncContentResolverDelegate.java

@@ -11,18 +11,12 @@ import android.os.Bundle;
 
 import androidx.annotation.VisibleForTesting;
 
-import org.junit.Assert;
-
-import org.chromium.base.ThreadUtils;
-import org.chromium.base.task.AsyncTask;
 import org.chromium.components.sync.SyncContentResolverDelegate;
 
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.Semaphore;
-import java.util.concurrent.TimeUnit;
 
 /**
  * Fake implementation of the {@link SyncContentResolverDelegate}.
@@ -35,65 +29,65 @@ import java.util.concurrent.TimeUnit;
 public class MockSyncContentResolverDelegate implements SyncContentResolverDelegate {
     private final Set<String> mSyncAutomaticallySet;
     private final Map<String, Boolean> mIsSyncableMap;
-    private final Object mSyncableMapLock = new Object();
-
-    private final Set<AsyncSyncStatusObserver> mObservers;
-
+    private final Set<SyncStatusObserver> mObservers;
     private boolean mMasterSyncAutomatically;
-    private boolean mDisableObserverNotifications;
 
-    private Semaphore mPendingObserverCount;
+    private Object mLock = new Object();
 
     public MockSyncContentResolverDelegate() {
         mSyncAutomaticallySet = new HashSet<String>();
         mIsSyncableMap = new HashMap<String, Boolean>();
-        mObservers = new HashSet<AsyncSyncStatusObserver>();
+        mObservers = new HashSet<SyncStatusObserver>();
     }
 
     @Override
-    public Object addStatusChangeListener(int mask, SyncStatusObserver callback) {
+    public Object addStatusChangeListener(int mask, SyncStatusObserver observer) {
         if (mask != ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS) {
             throw new IllegalArgumentException("This implementation only supports "
                     + "ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS as the mask");
         }
-        AsyncSyncStatusObserver asyncSyncStatusObserver = new AsyncSyncStatusObserver(callback);
-        synchronized (mObservers) {
-            mObservers.add(asyncSyncStatusObserver);
+        synchronized (mLock) {
+            mObservers.add(observer);
         }
-        return asyncSyncStatusObserver;
+        return observer;
     }
 
     @Override
     public void removeStatusChangeListener(Object handle) {
-        synchronized (mObservers) {
+        synchronized (mLock) {
             mObservers.remove(handle);
         }
     }
 
+    // Once this returns, observers are guaranteed to have been synchronously notified.
     @Override
     @VisibleForTesting
     public void setMasterSyncAutomatically(boolean sync) {
-        if (mMasterSyncAutomatically == sync) return;
-
-        mMasterSyncAutomatically = sync;
+        synchronized (mLock) {
+            if (mMasterSyncAutomatically == sync) return;
+            mMasterSyncAutomatically = sync;
+        }
         notifyObservers();
     }
 
     @Override
     public boolean getMasterSyncAutomatically() {
-        return mMasterSyncAutomatically;
+        synchronized (mLock) {
+            return mMasterSyncAutomatically;
+        }
     }
 
     @Override
     public boolean getSyncAutomatically(Account account, String authority) {
-        synchronized (mSyncableMapLock) {
+        synchronized (mLock) {
             return mSyncAutomaticallySet.contains(createKey(account, authority));
         }
     }
 
+    // Once this returns, observers are guaranteed to have been synchronously notified.
     @Override
     public void setSyncAutomatically(Account account, String authority, boolean sync) {
-        synchronized (mSyncableMapLock) {
+        synchronized (mLock) {
             String key = createKey(account, authority);
             if (sync) {
                 mSyncAutomaticallySet.add(key);
@@ -104,9 +98,10 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg
         notifyObservers();
     }
 
+    // Once this returns, observers are guaranteed to have been synchronously notified.
     @Override
     public void setIsSyncable(Account account, String authority, int syncable) {
-        synchronized (mSyncableMapLock) {
+        synchronized (mLock) {
             String key = createKey(account, authority);
             if (syncable > 0) {
                 mIsSyncableMap.put(key, true);
@@ -121,7 +116,7 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg
 
     @Override
     public int getIsSyncable(Account account, String authority) {
-        synchronized (mSyncableMapLock) {
+        synchronized (mLock) {
             String key = createKey(account, authority);
             if (mIsSyncableMap.containsKey(key)) {
                 return mIsSyncableMap.get(key) ? 1 : 0;
@@ -138,71 +133,11 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg
     }
 
     private void notifyObservers() {
-        if (mDisableObserverNotifications) return;
-        synchronized (mObservers) {
-            mPendingObserverCount = new Semaphore(1 - mObservers.size());
-            for (AsyncSyncStatusObserver observer : mObservers) {
-                observer.notifyObserverAsync(mPendingObserverCount);
+        synchronized (mLock) {
+            for (SyncStatusObserver observer : mObservers) {
+                observer.onStatusChanged(ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS);
             }
         }
     }
 
-    /**
-     * Blocks until the last notification has been issued to all registered observers.
-     * Note that if an observer is removed while a notification is being handled this can
-     * fail to return correctly.
-     *
-     * @throws InterruptedException
-     */
-    @VisibleForTesting
-    public void waitForLastNotificationCompleted() throws InterruptedException {
-        Assert.assertTrue("Timed out waiting for notifications to complete.",
-                mPendingObserverCount.tryAcquire(5, TimeUnit.SECONDS));
-    }
-
-    public void disableObserverNotifications() {
-        mDisableObserverNotifications = true;
-    }
-
-    /**
-      * Simulate an account rename, which copies settings to the new account.
-      */
-    public void renameAccounts(Account oldAccount, Account newAccount, String authority) {
-        int oldIsSyncable = getIsSyncable(oldAccount, authority);
-        setIsSyncable(newAccount, authority, oldIsSyncable);
-        if (oldIsSyncable == 1) {
-            setSyncAutomatically(
-                    newAccount, authority, getSyncAutomatically(oldAccount, authority));
-        }
-    }
-
-    private static class AsyncSyncStatusObserver {
-        private final SyncStatusObserver mSyncStatusObserver;
-
-        private AsyncSyncStatusObserver(SyncStatusObserver syncStatusObserver) {
-            mSyncStatusObserver = syncStatusObserver;
-        }
-
-        private void notifyObserverAsync(final Semaphore pendingObserverCount) {
-            if (ThreadUtils.runningOnUiThread()) {
-                new AsyncTask<Void>() {
-                    @Override
-                    protected Void doInBackground() {
-                        mSyncStatusObserver.onStatusChanged(
-                                ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS);
-                        return null;
-                    }
-
-                    @Override
-                    protected void onPostExecute(Void result) {
-                        pendingObserverCount.release();
-                    }
-                }
-                        .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
-            } else {
-                mSyncStatusObserver.onStatusChanged(ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS);
-                pendingObserverCount.release();
-            }
-        }
-    }
 }