[Sync] Ensure isSyncable is set when signed in.

This is a more extensive version of http://crrev.com/1075343003 for
landing on trunk.

Previously, if isSyncable is false, but sync is enabled for Chrome,
even if you sign in, Chrome would never set syncable to true.

This CL ensures that syncable is always set to true if there is an
account signed in, regardless of whether chrome sync is enabled or not.

Additionally, it sets syncable to false when no account is signed in,
to ensure that the user does not see a non-functional switch in the
Android settings.

The new AccountManager setup in the test is necessary so that the test accounts can be cleaned up by the loop in updateSyncability().

BUG=475299
TEST=Regression tests added, plus:
Ensure that
$ adb shell dumpsys content | grep chrome
should show syncable=0 and enabled=true
Sign in to Chrome. Now dumpsys should say syncable=1 and enabled=true.

Review URL: https://codereview.chromium.org/1062973004

Cr-Commit-Position: refs/heads/master@{#326122}

3 files changed, 68 insertions, 13 deletions

diff --git a/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java b/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java
index d24366c..3587e76 100644
--- a/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java
+++ b/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java
@@ -160,6 +160,7 @@ public class AndroidSyncSettings {
     public void updateAccount(Account account) {
         synchronized (mLock) {
             mAccount = account;
+            updateSyncability();
         }
         if (updateCachedSettings()) {
             notifyObservers();
@@ -193,8 +194,8 @@ public class AndroidSyncSettings {
 
     private void setChromeSyncEnabled(boolean value) {
         synchronized (mLock) {
+            updateSyncability();
             if (value == mChromeSyncEnabled || mAccount == null) return;
-            ensureSyncable();
             mChromeSyncEnabled = value;
 
             StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
@@ -205,18 +206,22 @@ public class AndroidSyncSettings {
     }
 
     /**
-     * Ensure Chrome is registered with the Android Sync Manager.
+     * Ensure Chrome is registered with the Android Sync Manager iff signed in.
      *
      * This is what causes the "Chrome" option to appear in Settings -> Accounts -> Sync .
      * This function must be called within a synchronized block.
      */
-    private void ensureSyncable() {
-        if (mIsSyncable || mAccount == null) return;
+    private void updateSyncability() {
+        boolean shouldBeSyncable = mAccount != null;
+        if (mIsSyncable == shouldBeSyncable) return;
 
-        mIsSyncable = true;
+        mIsSyncable = shouldBeSyncable;
 
         StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
-        mSyncContentResolverDelegate.setIsSyncable(mAccount, mContractAuthority, 1);
+        // Make account syncable if there is one.
+        if (shouldBeSyncable) {
+            mSyncContentResolverDelegate.setIsSyncable(mAccount, mContractAuthority, 1);
+        }
 
         // Disable the syncability of Chrome for all other accounts. Don't use
         // our cache as we're touching many accounts that aren't signed in, so this saves
diff --git a/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java b/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java
index aeba67f..13c43f6 100644
--- a/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java
+++ b/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java
@@ -12,6 +12,9 @@ import android.test.suitebuilder.annotation.SmallTest;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.Feature;
 import org.chromium.sync.AndroidSyncSettings.AndroidSyncSettingsObserver;
+import org.chromium.sync.signin.AccountManagerHelper;
+import org.chromium.sync.test.util.AccountHolder;
+import org.chromium.sync.test.util.MockAccountManager;
 import org.chromium.sync.test.util.MockSyncContentResolverDelegate;
 
 /**
@@ -81,16 +84,17 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase {
     private Account mAccount;
     private Account mAlternateAccount;
     private MockSyncSettingsObserver mSyncSettingsObserver;
+    private MockAccountManager mAccountManager;
 
     @Override
     protected void setUp() throws Exception {
         mSyncContentResolverDelegate = new CountingMockSyncContentResolverDelegate();
         Context context = getInstrumentation().getTargetContext();
+        setupTestAccounts(context);
+
         AndroidSyncSettings.overrideForTests(context, mSyncContentResolverDelegate);
         mAndroid = AndroidSyncSettings.get(context);
         mAuthority = mAndroid.getContractAuthority();
-        mAccount = new Account("account@example.com", "com.google");
-        mAlternateAccount = new Account("alternateAccount@example.com", "com.google");
         mAndroid.updateAccount(mAccount);
 
         mSyncSettingsObserver = new MockSyncSettingsObserver();
@@ -99,6 +103,21 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase {
         super.setUp();
     }
 
+    private void setupTestAccounts(Context context) {
+        mAccountManager = new MockAccountManager(context, context);
+        AccountManagerHelper.overrideAccountManagerHelperForTests(context, mAccountManager);
+        mAccount = setupTestAccount("account@example.com");
+        mAlternateAccount = setupTestAccount("alternate@example.com");
+    }
+
+    private Account setupTestAccount(String accountName) {
+        Account account = AccountManagerHelper.createAccountFromName(accountName);
+        AccountHolder.Builder accountHolder =
+                AccountHolder.create().account(account).password("password").alwaysAccept(true);
+        mAccountManager.addAccountHolderExplicitly(accountHolder.build());
+        return account;
+    }
+
     private void enableChromeSyncOnUiThread() {
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
@@ -294,4 +313,39 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase {
         assertFalse("disableChromeSync shouldn't observers",
                 mSyncSettingsObserver.receivedNotification());
     }
+
+    @SmallTest
+    @Feature({"Sync"})
+    public void testIsSyncableOnSigninAndNotOnSignout() throws InterruptedException {
+        assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1);
+        mAndroid.updateAccount(null);
+        assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 0);
+        mAndroid.updateAccount(mAccount);
+        assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1);
+    }
+
+    /**
+     * Regression test for crbug.com/475299.
+     */
+    @SmallTest
+    @Feature({"Sync"})
+    public void testSyncableIsAlwaysSetWhenEnablingSync() throws InterruptedException {
+        // Setup bad state.
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
+        mSyncContentResolverDelegate.waitForLastNotificationCompleted();
+        mSyncContentResolverDelegate.setIsSyncable(mAccount, mAuthority, 1);
+        mSyncContentResolverDelegate.waitForLastNotificationCompleted();
+        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
+        mSyncContentResolverDelegate.waitForLastNotificationCompleted();
+        mSyncContentResolverDelegate.setIsSyncable(mAccount, mAuthority, 0);
+        mSyncContentResolverDelegate.waitForLastNotificationCompleted();
+        assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 0);
+        assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority));
+
+        // Ensure bug is fixed.
+        mAndroid.enableChromeSync();
+        assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1);
+        // Should still be enabled.
+        assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority));
+    }
 }
diff --git a/sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java b/sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java
index be2622c..ea0b003 100644
--- a/sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java
+++ b/sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java
@@ -114,10 +114,6 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg
         synchronized (mSyncableMapLock) {
             switch (syncable) {
                 case 0:
-                    if (mSyncAutomaticallySet.contains(key)) {
-                        mSyncAutomaticallySet.remove(key);
-                    }
-
                     mIsSyncableMap.put(key, false);
                     break;
                 case 1:
@@ -141,7 +137,7 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg
         String key = createKey(account, authority);
         synchronized (mSyncableMapLock) {
             if (mIsSyncableMap.containsKey(key)) {
-                return mIsSyncableMap.containsKey(key) ? 1 : 0;
+                return mIsSyncableMap.get(key) ? 1 : 0;
             } else {
                 return -1;
             }