Ensure AndroidSyncSettings is used only from the UI thread

After crrev.com/c/2415150, the class is no longer used from a background
thread. This allows for a number of simplifications.
- Removing all the synchronization code (synchronized blocks and lock).
- updateSyncability() is now synchronous, so no need to expose the
artificial callback parameter in ctor/updateAccount(). The associated
code in the test file (CallbackHelper and updateAccountAndWait()) can
also be removed.

This also requires some adaptations:
- Every call to the public API is guarded by assertOnUiThread().
- Tests that called the API from the test thread now use the methods
in AndroidSyncSettingsTestUtil on runOnUIThreadBlocking().

Non-related to all this, the CL also takes care of the quick cleanup
post crrev.com/c/2415150: AndroidSyncSettingsObserver implementations
don't have to post a task anymore, since they are now notified directly
in the UI thread.

Fixed: 1028568
Change-Id: I8526cf67b26c97b3303510fbf0f4f0fc737617f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2414194
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808359}

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkPromoHeader.java

@@ -15,7 +15,6 @@ import androidx.annotation.VisibleForTesting;
 import androidx.recyclerview.widget.RecyclerView;
 import androidx.recyclerview.widget.RecyclerView.ViewHolder;
 
-import org.chromium.base.task.PostTask;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
@@ -37,7 +36,6 @@ import org.chromium.components.signin.AccountsChangeObserver;
 import org.chromium.components.signin.base.CoreAccountInfo;
 import org.chromium.components.signin.identitymanager.ConsentLevel;
 import org.chromium.components.signin.metrics.SigninAccessPoint;
-import org.chromium.content_public.browser.UiThreadTaskTraits;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
@@ -240,11 +238,8 @@ class BookmarkPromoHeader implements AndroidSyncSettingsObserver, SignInStateObs
     // AndroidSyncSettingsObserver implementation.
     @Override
     public void androidSyncSettingsChanged() {
-        // AndroidSyncSettings calls this method from non-UI threads.
-        PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT, () -> {
-            mPromoState = calculatePromoState();
-            triggerPromoUpdate();
-        });
+        mPromoState = calculatePromoState();
+        triggerPromoUpdate();
     }
 
     // SignInStateObserver implementation.

chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentTabsManager.java

@@ -436,6 +436,8 @@ public class RecentTabsManager implements AndroidSyncSettingsObserver, SignInSta
     }
 
     private void update() {
+        // TODO(crbug.com/1129853): Re-evaluate whether it's necessary to post
+        // a task.
         PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT, () -> {
             if (mIsDestroyed) return;
             updateForeignSessions();

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

@@ -16,7 +16,6 @@ import android.widget.LinearLayout;
 import android.widget.TextView;
 
 import org.chromium.base.IntentUtils;
-import org.chromium.base.task.PostTask;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.settings.SettingsLauncher;
 import org.chromium.chrome.browser.settings.SettingsLauncherImpl;
@@ -25,7 +24,6 @@ import org.chromium.chrome.browser.sync.AndroidSyncSettings;
 import org.chromium.chrome.browser.sync.AndroidSyncSettings.AndroidSyncSettingsObserver;
 import org.chromium.chrome.browser.sync.settings.SyncAndServicesSettings;
 import org.chromium.components.signin.metrics.SigninAccessPoint;
-import org.chromium.content_public.browser.UiThreadTaskTraits;
 
 /**
  * A View that shows the user the next step they must complete to start syncing their data (eg.
@@ -211,7 +209,6 @@ public class SyncPromoView extends LinearLayout implements AndroidSyncSettingsOb
     // AndroidSyncStateObserver
     @Override
     public void androidSyncSettingsChanged() {
-        // AndroidSyncSettings calls this method from non-UI threads.
-        PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT, this::update);
+        update();
     }
 }

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

@@ -12,7 +12,6 @@ import androidx.annotation.VisibleForTesting;
 import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.metrics.RecordHistogram;
-import org.chromium.base.task.PostTask;
 import org.chromium.chrome.browser.identity.UniqueIdentificationGenerator;
 import org.chromium.chrome.browser.identity.UniqueIdentificationGeneratorFactory;
 import org.chromium.chrome.browser.profiles.Profile;
@@ -21,7 +20,6 @@ import org.chromium.chrome.browser.signin.SigninManager;
 import org.chromium.components.sync.ModelType;
 import org.chromium.components.sync.PassphraseType;
 import org.chromium.components.sync.StopSource;
-import org.chromium.content_public.browser.UiThreadTaskTraits;
 
 /**
  * SyncController handles the coordination of sync state between the invalidation controller,
@@ -169,7 +167,7 @@ public class SyncController implements ProfileSyncService.SyncStateChangedListen
      */
     @Override
     public void androidSyncSettingsChanged() {
-        PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT, () -> { updateSyncStateFromAndroid(); });
+        updateSyncStateFromAndroid();
     }
 
     /**

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

@@ -33,7 +33,7 @@ public class AndroidSyncSettingsTestUtils {
         // Explicitly pass null account to AndroidSyncSettings ctor. Normally, AndroidSyncSettings
         // ctor uses IdentityManager to get the sync account, but some native tests call this method
         // before profiles are initialized (when IdentityManager doesn't exist yet).
-        AndroidSyncSettings.overrideForTests(new AndroidSyncSettings(delegate, null, null));
+        AndroidSyncSettings.overrideForTests(new AndroidSyncSettings(delegate, null));
     }
 
     public static boolean getIsSyncEnabledOnUiThread() {

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

@@ -69,13 +69,13 @@ public class SyncAndServicesSettingsTest {
         final ChromeSwitchPreference syncSwitch = getSyncSwitch(fragment);
 
         Assert.assertTrue(syncSwitch.isChecked());
-        Assert.assertTrue(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
         mSyncTestRule.togglePreference(syncSwitch);
         Assert.assertFalse(syncSwitch.isChecked());
-        Assert.assertFalse(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertFalse(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
         mSyncTestRule.togglePreference(syncSwitch);
         Assert.assertTrue(syncSwitch.isChecked());
-        Assert.assertTrue(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
     }
 
     /**
@@ -89,7 +89,7 @@ public class SyncAndServicesSettingsTest {
         mSyncTestRule.stopSync();
         SyncAndServicesSettings fragment = startSyncAndServicesPreferences();
         closeFragment(fragment);
-        Assert.assertFalse(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertFalse(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
     }
 
     /**
@@ -146,15 +146,15 @@ public class SyncAndServicesSettingsTest {
         Assert.assertTrue(
                 "There should be server cards", mSyncTestRule.hasServerAutofillCreditCards());
 
-        Assert.assertTrue(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
         SyncAndServicesSettings fragment = startSyncAndServicesPreferences();
         assertSyncOnState(fragment);
         ChromeSwitchPreference syncSwitch = getSyncSwitch(fragment);
         Assert.assertTrue(syncSwitch.isChecked());
-        Assert.assertTrue(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
         mSyncTestRule.togglePreference(syncSwitch);
         Assert.assertFalse(syncSwitch.isChecked());
-        Assert.assertFalse(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertFalse(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
 
         closeFragment(fragment);
 
@@ -208,7 +208,7 @@ public class SyncAndServicesSettingsTest {
         fragment = startSyncAndServicesPreferences();
         Assert.assertNull("Sync error card should not be shown", getSyncErrorCard(fragment));
         assertSyncOffState(fragment);
-        Assert.assertFalse(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertFalse(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
     }
 
     @Test
@@ -226,7 +226,7 @@ public class SyncAndServicesSettingsTest {
         fragment = startSyncAndServicesPreferences();
         Assert.assertNull("Sync error card should not be shown", getSyncErrorCard(fragment));
         assertSyncOffState(fragment);
-        Assert.assertFalse(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertFalse(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
     }
 
     @Test
@@ -240,7 +240,7 @@ public class SyncAndServicesSettingsTest {
         ChromeSwitchPreference syncSwitch = getSyncSwitch(fragment);
         mSyncTestRule.togglePreference(syncSwitch);
         Assert.assertTrue(syncSwitch.isChecked());
-        Assert.assertTrue(getAndroidSyncSettings().isChromeSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
         // FirstSetupComplete should be set.
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> { Assert.assertTrue(ProfileSyncService.get().isFirstSetupComplete()); });
@@ -426,8 +426,4 @@ public class SyncAndServicesSettingsTest {
         Assert.assertTrue(
                 "The sync switch should be enabled.", getSyncSwitch(fragment).isEnabled());
     }
-
-    private static AndroidSyncSettings getAndroidSyncSettings() {
-        return TestThreadUtils.runOnUiThreadBlockingNoException(AndroidSyncSettings::get);
-    }
 }

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

@@ -150,7 +150,7 @@ public class SyncTest {
         Account account = mSyncTestRule.setUpAccountAndSignInForTesting();
         String authority = AndroidSyncSettings.getContractAuthority();
 
-        Assert.assertTrue(getAndroidSyncSettings().isSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread());
         Assert.assertTrue(SyncTestUtil.isSyncRequested());
 
         // Disabling Android sync should turn Chrome sync engine off.
@@ -170,7 +170,7 @@ public class SyncTest {
     public void testStopAndStartSyncThroughAndroidMasterSync() {
         mSyncTestRule.setUpAccountAndSignInForTesting();
 
-        Assert.assertTrue(getAndroidSyncSettings().isSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread());
         Assert.assertTrue(SyncTestUtil.isSyncRequested());
 
         // Disabling Android's master sync should turn Chrome sync engine off.
@@ -191,7 +191,7 @@ public class SyncTest {
         Account account = mSyncTestRule.setUpAccountAndSignInForTesting();
         String authority = AndroidSyncSettings.getContractAuthority();
 
-        Assert.assertTrue(getAndroidSyncSettings().isSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread());
         Assert.assertTrue(SyncTestUtil.isSyncRequested());
         Assert.assertTrue(SyncTestUtil.canSyncFeatureStart());
 
@@ -224,7 +224,7 @@ public class SyncTest {
         Account account = mSyncTestRule.setUpAccountAndSignInForTesting();
         String authority = AndroidSyncSettings.getContractAuthority();
 
-        Assert.assertTrue(getAndroidSyncSettings().isSyncEnabled());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread());
         Assert.assertTrue(SyncTestUtil.isSyncRequested());
         Assert.assertTrue(SyncTestUtil.canSyncFeatureStart());
 
@@ -262,8 +262,4 @@ public class SyncTest {
         mSyncTestRule.startSync();
         Assert.assertFalse(SyncTestUtil.isSyncRequested());
     }
-
-    private static AndroidSyncSettings getAndroidSyncSettings() {
-        return TestThreadUtils.runOnUiThreadBlockingNoException(AndroidSyncSettings::get);
-    }
 }

components/sync/android/java/src/org/chromium/components/sync/SystemSyncContentResolverDelegate.java

@@ -11,6 +11,8 @@ import android.os.Bundle;
 
 /**
  * A SyncContentResolverDelegate that simply forwards calls to ContentResolver.
+ * Note that SyncContentResolverDelegate is not an Android concept. In particular,
+ * it's not this class that will notify observers, Android will directly do that.
  */
 public class SystemSyncContentResolverDelegate implements SyncContentResolverDelegate {
     @Override

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

@@ -19,12 +19,12 @@ import java.util.Map;
 import java.util.Set;
 
 /**
- * Fake implementation of the {@link SyncContentResolverDelegate}.
- *
- * This implementation only supports status change listeners for the type
- * SYNC_OBSERVER_TYPE_SETTINGS. Differently from the real implementation, it
- * doesn't allow calling the auto-sync methods (e.g. getIsSyncable()) with a null
- * account.
+ * Fake thread-safe implementation of {@link SyncContentResolverDelegate}, so tests can
+ * emulate certain auto-sync settings (e.g. mimic a user disabling the master sync toggle).
+ * Synchronously notifies observers every time a setter is called, even if the state didn't
+ * change. Differently from the actual android.os.ContentResolver APIs, it only supports
+ * observers for the SYNC_OBSERVER_TYPE_SETTINGS type and it doesn't allow querying
+ * settings for a null account.
  */
 public class MockSyncContentResolverDelegate implements SyncContentResolverDelegate {
     private final Set<String> mSyncAutomaticallySet;