Decouple Sync from Android master sync as soon as master sync is enabled

This CL modifies the behavior of the DecoupleSyncFromAndroidMasterSync
feature. When the feature is enabled, Sync will only decouple from
master sync and be re-enabled once the master toggle is first re-enabled.
The fact that the decoupling happened is persisted using the APIs from
crrev.com/c/2398524.

We also do some non-related code brush up:
- Shorten the implementation of some methods in AndroidSyncSettingsUtils.
- Deprecate one test constructor of AndroidSyncSettings.
- Add missing assertOnUiThread() to the methods to add/remove an
observer in AndroidSyncSettings.
- Restrict AndroidSyncSettingsTest#testAccountInitialization to the
case where the feature is disabled, which should have been done in
crrev.com/c/2366895.

Bug: 1125622
Change-Id: Ia37a1075b159754337c3c7308d9b789b8008ec21
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2398504
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810274}

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

@@ -56,6 +56,8 @@ public class AndroidSyncSettings {
 
     private boolean mMasterSyncEnabled;
 
+    private boolean mShouldDecoupleSyncFromMasterSync;
+
     private final ObserverList<AndroidSyncSettingsObserver> mObservers = new ObserverList<>();
 
     /**
@@ -87,14 +89,16 @@ public class AndroidSyncSettings {
         sInstance = instance;
     }
 
-    /**
-     * @param account The sync account if sync is enabled, null otherwise.
-     */
     // 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.
+    /**
+     * WARNING: Consider using |overrideForTests()| to inject a mock instead.
+     * @param account The sync account if sync is enabled, null otherwise.
+     */
     @VisibleForTesting
+    @Deprecated
     public AndroidSyncSettings(@Nullable Account account) {
         ThreadUtils.assertOnUiThread();
         mContractAuthority = getContractAuthority();
@@ -104,6 +108,14 @@ public class AndroidSyncSettings {
         updateCachedSettings();
         updateSyncability();
 
+        ProfileSyncService syncService = ProfileSyncService.get();
+        if (syncService != null
+                && ChromeFeatureList.isEnabled(
+                        ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)) {
+            // Read initial persisted value.
+            mShouldDecoupleSyncFromMasterSync = syncService.getDecoupledFromAndroidMasterSync();
+        }
+
         SyncStatusObserver androidOsListener = new SyncStatusObserver() {
             @Override
             public void onStatusChanged(int which) {
@@ -153,9 +165,7 @@ public class AndroidSyncSettings {
      */
     public boolean doesMasterSyncSettingAllowChromeSync() {
         ThreadUtils.assertOnUiThread();
-        return mMasterSyncEnabled
-                || ChromeFeatureList.isEnabled(
-                        ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC);
+        return mMasterSyncEnabled || mShouldDecoupleSyncFromMasterSync;
     }
 
     /**
@@ -200,6 +210,7 @@ public class AndroidSyncSettings {
      * Add a new AndroidSyncSettingsObserver.
      */
     public void registerObserver(AndroidSyncSettingsObserver observer) {
+        ThreadUtils.assertOnUiThread();
         mObservers.addObserver(observer);
     }
 
@@ -207,6 +218,7 @@ public class AndroidSyncSettings {
      * Remove an AndroidSyncSettingsObserver that was previously added.
      */
     public void unregisterObserver(AndroidSyncSettingsObserver observer) {
+        ThreadUtils.assertOnUiThread();
         mObservers.removeObserver(observer);
     }
 
@@ -262,7 +274,8 @@ public class AndroidSyncSettings {
     }
 
     /**
-     * Update the three cached settings from the content resolver.
+     * Update the three cached settings from the content resolver and the
+     * master sync decoupling setting.
      *
      * @return Whether either chromeSyncEnabled or masterSyncEnabled changed.
      */
@@ -284,6 +297,17 @@ public class AndroidSyncSettings {
             mMasterSyncEnabled = mSyncContentResolverDelegate.getMasterSyncAutomatically();
         }
 
+        if (mAccount != null && ProfileSyncService.get() != null
+                && ChromeFeatureList.isEnabled(
+                        ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
+                && mMasterSyncEnabled && !mShouldDecoupleSyncFromMasterSync) {
+            // Re-enabling master sync at least once should cause Sync to no longer care whether
+            // the former is enabled or not. This fact should be persisted via ProfileSyncService
+            // so it's known on the next startup.
+            mShouldDecoupleSyncFromMasterSync = true;
+            ProfileSyncService.get().setDecoupledFromAndroidMasterSync();
+        }
+
         return oldChromeSyncEnabled != mChromeSyncEnabled
                 || oldMasterSyncEnabled != mMasterSyncEnabled;
     }

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

@@ -88,6 +88,7 @@ public class ProfileSyncService {
      */
     @VisibleForTesting
     public static void overrideForTests(ProfileSyncService profileSyncService) {
+        ThreadUtils.assertOnUiThread();
         sProfileSyncService = profileSyncService;
         sInitialized = true;
     }

chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java

@@ -9,6 +9,10 @@ import static androidx.test.espresso.action.ViewActions.click;
 import static androidx.test.espresso.assertion.ViewAssertions.doesNotExist;
 import static androidx.test.espresso.matcher.ViewMatchers.withText;
 
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.when;
+
 import static org.chromium.components.browser_ui.widget.highlight.ViewHighlighterTestUtils.checkHighlightOff;
 import static org.chromium.components.browser_ui.widget.highlight.ViewHighlighterTestUtils.checkHighlightPulse;
 
@@ -35,6 +39,9 @@ import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
 
 import org.chromium.base.ActivityState;
 import org.chromium.base.ApplicationStatus;
@@ -59,7 +66,8 @@ import org.chromium.chrome.browser.partnerbookmarks.PartnerBookmarksShim;
 import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
 import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
 import org.chromium.chrome.browser.profiles.Profile;
-import org.chromium.chrome.browser.sync.AndroidSyncSettingsTestUtils;
+import org.chromium.chrome.browser.sync.AndroidSyncSettings;
+import org.chromium.chrome.browser.sync.AndroidSyncSettings.AndroidSyncSettingsObserver;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.ui.messages.snackbar.Snackbar;
 import org.chromium.chrome.browser.ui.messages.snackbar.SnackbarManager;
@@ -106,6 +114,8 @@ public class BookmarkTest {
     @Rule
     public ChromeRenderTestRule mRenderTestRule =
             ChromeRenderTestRule.Builder.withPublicCorpus().build();
+    @Rule
+    public final MockitoRule mMockitoRule = MockitoJUnit.rule();
 
     private static final String TEST_PAGE_URL_GOOGLE = "/chrome/test/data/android/google.html";
     private static final String TEST_PAGE_TITLE_GOOGLE = "The Google";
@@ -125,6 +135,8 @@ public class BookmarkTest {
     private String mTestPageFoo;
     private EmbeddedTestServer mTestServer;
     private @Nullable BookmarkActivity mBookmarkActivity;
+    @Mock
+    private AndroidSyncSettings mAndroidSyncSettings;
 
     @BeforeClass
     public static void setUpBeforeActivityLaunched() {
@@ -145,8 +157,17 @@ public class BookmarkTest {
                     mActivityTestRule.getActivity().getActivityTab().getWebContents()));
             mBookmarkBridge = mActivityTestRule.getActivity().getBookmarkBridgeForTesting();
 
-            // Stub Android master sync state to make sure promos aren't suppressed.
-            AndroidSyncSettingsTestUtils.setUpAndroidSyncSettingsForTesting();
+            // Stub AndroidSyncSettings state to make sure promos aren't suppressed.
+            when(mAndroidSyncSettings.doesMasterSyncSettingAllowChromeSync()).thenReturn(true);
+            when(mAndroidSyncSettings.isSyncEnabled()).thenReturn(false);
+            when(mAndroidSyncSettings.isChromeSyncEnabled()).thenReturn(false);
+            doNothing()
+                    .when(mAndroidSyncSettings)
+                    .registerObserver(any(AndroidSyncSettingsObserver.class));
+            doNothing()
+                    .when(mAndroidSyncSettings)
+                    .unregisterObserver(any(AndroidSyncSettingsObserver.class));
+            AndroidSyncSettings.overrideForTests(mAndroidSyncSettings);
         });
         mTestServer = EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
         mTestPage = mTestServer.getURL(TEST_PAGE_URL_GOOGLE);

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

@@ -4,6 +4,10 @@
 
 package org.chromium.chrome.browser.sync;
 
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
 import static org.chromium.chrome.browser.sync.AndroidSyncSettingsTestUtils.getDoesMasterSyncAllowSyncOnUiThread;
 import static org.chromium.chrome.browser.sync.AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread;
 import static org.chromium.chrome.browser.sync.AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread;
@@ -20,8 +24,10 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TestRule;
 import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
 
-import org.chromium.base.FeatureList;
 import org.chromium.base.test.BaseJUnit4ClassRunner;
 import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
@@ -116,21 +122,25 @@ public class AndroidSyncSettingsTest {
     @Rule
     public TestRule mChromeBrowserRule = new ChromeBrowserTestRule();
     @Rule
-    public TestRule mProcessorRule = new Features.JUnitProcessor();
+    public TestRule mProcessorRule = new Features.InstrumentationProcessor();
+    @Rule
+    public final MockitoRule mMockitoRule = MockitoJUnit.rule();
 
     private AndroidSyncSettings mAndroidSyncSettings;
     private CountingMockSyncContentResolverDelegate mSyncContentResolverDelegate;
+    @Mock
+    private ProfileSyncService mProfileSyncService;
     private Account mAccount;
     private Account mAlternateAccount;
     private String mAuthority = AndroidSyncSettings.getContractAuthority();
 
     @Before
     public void setUp() {
-        FeatureList.setTestCanUseDefaultsForTesting();
-
         mSyncContentResolverDelegate = new CountingMockSyncContentResolverDelegate();
-        TestThreadUtils.runOnUiThreadBlocking(
-                () -> SyncContentResolverDelegate.overrideForTests(mSyncContentResolverDelegate));
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            SyncContentResolverDelegate.overrideForTests(mSyncContentResolverDelegate);
+            ProfileSyncService.overrideForTests(mProfileSyncService);
+        });
 
         FakeAccountManagerFacade fakeAccountManagerFacade = new FakeAccountManagerFacade(null);
         AccountManagerFacadeProvider.setInstanceForTests(fakeAccountManagerFacade);
@@ -156,16 +166,6 @@ public class AndroidSyncSettingsTest {
         }
     }
 
-    private void setMasterSyncAllowsChromeSync() {
-        if (!ChromeFeatureList.isEnabled(
-                    ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)) {
-            mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
-            Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
-        }
-        // If DecoupleSyncFromAndroidMasterSync is enabled, no need for any
-        // setup since master sync doesn't influence sync.
-    }
-
     @After
     public void tearDown() {
         AccountManagerFacadeProvider.resetInstanceForTests();
@@ -174,6 +174,7 @@ public class AndroidSyncSettingsTest {
     @Test
     @SmallTest
     @Feature({"Sync"})
+    @Features.DisableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
     public void testAccountInitialization() {
         createAndroidSyncSettings();
 
@@ -250,9 +251,9 @@ public class AndroidSyncSettingsTest {
     @SmallTest
     @Feature({"Sync"})
     public void testToggleAccountSyncFromApplication() {
-        createAndroidSyncSettings();
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
 
-        setMasterSyncAllowsChromeSync();
+        createAndroidSyncSettings();
 
         TestThreadUtils.runOnUiThreadBlocking(() -> mAndroidSyncSettings.enableChromeSync());
         Assert.assertTrue(getIsSyncEnabledOnUiThread());
@@ -265,9 +266,9 @@ public class AndroidSyncSettingsTest {
     @SmallTest
     @Feature({"Sync"})
     public void testToggleSyncabilityForMultipleAccounts() {
-        createAndroidSyncSettings();
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
 
-        setMasterSyncAllowsChromeSync();
+        createAndroidSyncSettings();
 
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             mAndroidSyncSettings.enableChromeSync();
@@ -292,9 +293,9 @@ public class AndroidSyncSettingsTest {
     @SmallTest
     @Feature({"Sync"})
     public void testSyncSettingsCaching() {
-        createAndroidSyncSettings();
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
 
-        setMasterSyncAllowsChromeSync();
+        createAndroidSyncSettings();
 
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             mAndroidSyncSettings.enableChromeSync();
@@ -418,20 +419,71 @@ public class AndroidSyncSettingsTest {
     @SmallTest
     @Feature({"Sync"})
     @Features.EnableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
-    // TODO(crbug.com/1105795): Remove this test after DecoupleSyncFromAndroidMasterSync has
-    // launched, since testToggleChromeSyncFromSettings() covers the same functionality.
-    public void testSyncStateDoesNotDependOnMasterSync() {
+    public void testMasterSyncAllowsSyncIfReEnabledOnce() {
+        // Master sync is disabled on startup and the user hasn't gone through the decoupling flow
+        // in the past.
         mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
+        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            when(mProfileSyncService.getDecoupledFromAndroidMasterSync()).thenReturn(false);
+        });
+
+        // Sync must remain disabled as long as master sync is not re-enabled.
         createAndroidSyncSettings();
+        Assert.assertFalse(getDoesMasterSyncAllowSyncOnUiThread());
+        Assert.assertFalse(getIsSyncEnabledOnUiThread());
 
-        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
-        Assert.assertTrue(getIsChromeSyncEnabledOnUiThread());
+        // Re-enable master sync at least once. Sync is now re-enabled and decoupled from master
+        // sync.
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> doNothing().when(mProfileSyncService).setDecoupledFromAndroidMasterSync());
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
         Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertTrue(getIsSyncEnabledOnUiThread());
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> verify(mProfileSyncService).setDecoupledFromAndroidMasterSync());
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
+        Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
+        Assert.assertTrue(getIsSyncEnabledOnUiThread());
+    }
 
-        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, false);
-        Assert.assertFalse(getIsChromeSyncEnabledOnUiThread());
+    @Test
+    @SmallTest
+    @Feature({"Sync"})
+    @Features.EnableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
+    public void testSyncStateRespectsPersistedDecouplingStateIfFeatureEnabled() {
+        // Master sync is disabled on startup, but the user has gone through the decoupling flow in
+        // the past and has the DecoupleSyncFromAndroidMasterSync feature enabled.
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
+        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            when(mProfileSyncService.getDecoupledFromAndroidMasterSync()).thenReturn(true);
+        });
+
+        // Chrome becomes aware of the previous decoupling, so Sync can be enabled despite master
+        // sync being disabled.
+        createAndroidSyncSettings();
         Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
+        Assert.assertTrue(getIsSyncEnabledOnUiThread());
+    }
+
+    @Test
+    @SmallTest
+    @Feature({"Sync"})
+    @Features.DisableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
+    public void testSyncStateDoesNotRespectPersistedDecouplingStateIfFeatureDisabled() {
+        // The user went through the master sync decoupling flow in the past, but has the
+        // DecoupleSyncFromAndroidMasterSync feature disabled.
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
+        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            when(mProfileSyncService.getDecoupledFromAndroidMasterSync()).thenReturn(true);
+        });
+
+        // Chrome should ignore that a previous decoupling happened. Sync should again respect
+        // master sync and stay disabled.
+        createAndroidSyncSettings();
+        Assert.assertFalse(getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertFalse(getIsSyncEnabledOnUiThread());
     }
 }

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

@@ -9,16 +9,13 @@ import androidx.annotation.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 
-import java.util.concurrent.Callable;
-
 /**
  * A utility class for mocking AndroidSyncSettings.
  */
 public class AndroidSyncSettingsTestUtils {
     /**
-     * Sets up AndroidSyncSettings with a mock SyncContentResolverDelegate and with Android's master
-     * sync setting enabled. (This setting is found, e.g., under Settings > Accounts & Sync >
-     * Auto-sync data.)
+     * Sets up a MockSyncContentResolverDelegate with Android's master sync setting enabled.
+     * (This setting is found, e.g., under Settings > Accounts & Sync > Auto-sync data.)
      */
     @CalledByNative
     @VisibleForTesting
@@ -27,36 +24,20 @@ public class AndroidSyncSettingsTestUtils {
         delegate.setMasterSyncAutomatically(true);
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> SyncContentResolverDelegate.overrideForTests(delegate));
-        // 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(null));
     }
 
     public static boolean getIsSyncEnabledOnUiThread() {
-        return TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
-            @Override
-            public Boolean call() {
-                return AndroidSyncSettings.get().isSyncEnabled();
-            }
-        });
+        return TestThreadUtils.runOnUiThreadBlockingNoException(
+                () -> AndroidSyncSettings.get().isSyncEnabled());
     }
 
     public static boolean getIsChromeSyncEnabledOnUiThread() {
-        return TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
-            @Override
-            public Boolean call() {
-                return AndroidSyncSettings.get().isChromeSyncEnabled();
-            }
-        });
+        return TestThreadUtils.runOnUiThreadBlockingNoException(
+                () -> AndroidSyncSettings.get().isChromeSyncEnabled());
     }
 
     public static boolean getDoesMasterSyncAllowSyncOnUiThread() {
-        return TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
-            @Override
-            public Boolean call() {
-                return AndroidSyncSettings.get().doesMasterSyncSettingAllowChromeSync();
-            }
-        });
+        return TestThreadUtils.runOnUiThreadBlockingNoException(
+                () -> AndroidSyncSettings.get().doesMasterSyncSettingAllowChromeSync());
     }
 }