Adapt SyncTestRule to allow AndroidSyncSettings to depend on PSS

Problem:
AndroidSyncSettings (ASS) will soon depend on ProfileSyncService (PSS),
particularly in its constructor. Although this is is fine in production
code, SyncTestRule poses two issues for this setup on test code.
- ASS is created before a fake PSS provided by tests can be injected.
This is a problem because the initial state of PSS will be relevant to
the constructor logic of ASS, so we need to be able to fake this initial
state to write good tests.
- ASS is created before the JNI loads in startMainActivityForSyncTest().
For tests that don't provide a fake PSS, ASS will then attempt to talk
to the real PSS before JNI is loaded, causing an error.

Solution:
When SyncTestRule creates an ASS, the goal is actually just to fake one
of its internal dependencies, SyncContentResolverDelegate. In this
CL, we convert SyncContentResolverDelegate into a singleton. In this way,
a fake for this object can be injected without needing to early construct
ASS*. Furthermore, JNI is loaded earlier so that the rule can safely use
tools like FakeServer and FakeProfileSyncService.

* Note that constructing after startMainActivityForSyncTest() is not an
option because there's already code talking to ASS when this function
is running.

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

chrome/android/chrome_java_sources.gni

@@ -1459,7 +1459,6 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/sync/SyncController.java",
   "java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java",
   "java/src/org/chromium/chrome/browser/sync/SyncUserDataWiper.java",
-  "java/src/org/chromium/chrome/browser/sync/SystemSyncContentResolverDelegate.java",
   "java/src/org/chromium/chrome/browser/sync/TrustedVaultClient.java",
   "java/src/org/chromium/chrome/browser/sync/settings/AccountManagementFragment.java",
   "java/src/org/chromium/chrome/browser/sync/settings/ClearDataProgressDialog.java",

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

@@ -72,8 +72,7 @@ public class AndroidSyncSettings {
     public static AndroidSyncSettings get() {
         ThreadUtils.assertOnUiThread();
         if (sInstance == null) {
-            SyncContentResolverDelegate contentResolver = new SystemSyncContentResolverDelegate();
-            sInstance = new AndroidSyncSettings(contentResolver);
+            sInstance = new AndroidSyncSettings(getSyncAccount());
         }
         return sInstance;
     }
@@ -89,29 +88,17 @@ public class AndroidSyncSettings {
     }
 
     /**
-     * @param syncContentResolverDelegate an implementation of {@link SyncContentResolverDelegate}.
-     */
-    @VisibleForTesting
-    AndroidSyncSettings(SyncContentResolverDelegate syncContentResolverDelegate) {
-        this(syncContentResolverDelegate, getSyncAccount());
-    }
-
-    /**
-     * @param syncContentResolverDelegate an implementation of {@link SyncContentResolverDelegate}.
      * @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
-    AndroidSyncSettings(
-            SyncContentResolverDelegate syncContentResolverDelegate, @Nullable Account account) {
+    public AndroidSyncSettings(@Nullable Account account) {
+        ThreadUtils.assertOnUiThread();
         mContractAuthority = getContractAuthority();
-        mSyncContentResolverDelegate = syncContentResolverDelegate;
+        mSyncContentResolverDelegate = SyncContentResolverDelegate.get();
 
         mAccount = account;
         updateCachedSettings();

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

@@ -5,30 +5,74 @@
 package org.chromium.chrome.browser.sync;
 
 import android.accounts.Account;
+import android.content.ContentResolver;
 import android.content.SyncStatusObserver;
 import android.os.Bundle;
 
+import androidx.annotation.VisibleForTesting;
+
+import org.chromium.base.ThreadUtils;
+
 /**
  * Since the ContentResolver in Android has a lot of static methods, it is hard to
- * mock out for tests. This interface wraps all the sync-related methods we use from
+ * mock out for tests. This class wraps all the sync-related methods we use from
  * the Android 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.
  */
-interface SyncContentResolverDelegate {
-    Object addStatusChangeListener(int mask, SyncStatusObserver callback);
+class SyncContentResolverDelegate {
+    private static SyncContentResolverDelegate sInstance;
+
+    public static SyncContentResolverDelegate get() {
+        ThreadUtils.assertOnUiThread();
+        if (sInstance == null) {
+            sInstance = new SyncContentResolverDelegate();
+        }
+        return sInstance;
+    }
+
+    public Object addStatusChangeListener(int mask, SyncStatusObserver callback) {
+        return ContentResolver.addStatusChangeListener(mask, callback);
+    }
+
+    public void removeStatusChangeListener(Object handle) {
+        ContentResolver.removeStatusChangeListener(handle);
+    }
+
+    public void setMasterSyncAutomatically(boolean sync) {
+        ContentResolver.setMasterSyncAutomatically(sync);
+    }
 
-    void removeStatusChangeListener(Object handle);
+    public boolean getMasterSyncAutomatically() {
+        return ContentResolver.getMasterSyncAutomatically();
+    }
 
-    void setMasterSyncAutomatically(boolean sync);
+    public boolean getSyncAutomatically(Account account, String authority) {
+        return ContentResolver.getSyncAutomatically(account, authority);
+    }
 
-    boolean getMasterSyncAutomatically();
+    public void setSyncAutomatically(Account account, String authority, boolean sync) {
+        ContentResolver.setSyncAutomatically(account, authority, sync);
+    }
 
-    void setSyncAutomatically(Account account, String authority, boolean sync);
+    public void setIsSyncable(Account account, String authority, int syncable) {
+        ContentResolver.setIsSyncable(account, authority, syncable);
+    }
 
-    boolean getSyncAutomatically(Account account, String authority);
+    public int getIsSyncable(Account account, String authority) {
+        return ContentResolver.getIsSyncable(account, authority);
+    }
 
-    void setIsSyncable(Account account, String authority, int syncable);
+    public void removePeriodicSync(Account account, String authority, Bundle extras) {
+        ContentResolver.removePeriodicSync(account, authority, extras);
+    }
 
-    int getIsSyncable(Account account, String authority);
+    @VisibleForTesting
+    public static void overrideForTests(SyncContentResolverDelegate delegate) {
+        ThreadUtils.assertOnUiThread();
+        sInstance = delegate;
+    }
 
-    void removePeriodicSync(Account account, String authority, Bundle extras);
+    protected SyncContentResolverDelegate() {}
 }

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

-// Copyright 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-package org.chromium.chrome.browser.sync;
-
-import android.accounts.Account;
-import android.content.ContentResolver;
-import android.content.SyncStatusObserver;
-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.
- */
-class SystemSyncContentResolverDelegate implements SyncContentResolverDelegate {
-    @Override
-    public Object addStatusChangeListener(int mask, SyncStatusObserver callback) {
-        return ContentResolver.addStatusChangeListener(mask, callback);
-    }
-
-    @Override
-    public void removeStatusChangeListener(Object handle) {
-        ContentResolver.removeStatusChangeListener(handle);
-    }
-
-    @Override
-    public void setMasterSyncAutomatically(boolean sync) {
-        ContentResolver.setMasterSyncAutomatically(sync);
-    }
-
-    @Override
-    public boolean getMasterSyncAutomatically() {
-        return ContentResolver.getMasterSyncAutomatically();
-    }
-
-    @Override
-    public boolean getSyncAutomatically(Account account, String authority) {
-        return ContentResolver.getSyncAutomatically(account, authority);
-    }
-
-    @Override
-    public void setSyncAutomatically(Account account, String authority, boolean sync) {
-        ContentResolver.setSyncAutomatically(account, authority, sync);
-    }
-
-    @Override
-    public void setIsSyncable(Account account, String authority, int syncable) {
-        ContentResolver.setIsSyncable(account, authority, syncable);
-    }
-
-    @Override
-    public int getIsSyncable(Account account, String authority) {
-        return ContentResolver.getIsSyncable(account, authority);
-    }
-
-    @Override
-    public void removePeriodicSync(Account account, String authority, Bundle extras) {
-        ContentResolver.removePeriodicSync(account, authority, extras);
-    }
-}

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

@@ -129,6 +129,8 @@ public class AndroidSyncSettingsTest {
         FeatureList.setTestCanUseDefaultsForTesting();
 
         mSyncContentResolverDelegate = new CountingMockSyncContentResolverDelegate();
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> SyncContentResolverDelegate.overrideForTests(mSyncContentResolverDelegate));
 
         FakeAccountManagerFacade fakeAccountManagerFacade = new FakeAccountManagerFacade(null);
         AccountManagerFacadeProvider.setInstanceForTests(fakeAccountManagerFacade);
@@ -140,7 +142,9 @@ public class AndroidSyncSettingsTest {
 
     private void createAndroidSyncSettings() {
         TestThreadUtils.runOnUiThreadBlocking(() -> {
-            mAndroidSyncSettings = new AndroidSyncSettings(mSyncContentResolverDelegate, mAccount);
+            // TODO(crbug.com/1105795): Consider setting the fake account in the identity manager
+            // so there's no need to inject it here.
+            mAndroidSyncSettings = new AndroidSyncSettings(mAccount);
             AndroidSyncSettings.overrideForTests(mAndroidSyncSettings);
         });
 

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

@@ -23,18 +23,14 @@ public class AndroidSyncSettingsTestUtils {
     @CalledByNative
     @VisibleForTesting
     public static void setUpAndroidSyncSettingsForTesting() {
-        setUpAndroidSyncSettingsForTesting(new MockSyncContentResolverDelegate());
-    }
-
-    static AndroidSyncSettings setUpAndroidSyncSettingsForTesting(
-            SyncContentResolverDelegate delegate) {
+        MockSyncContentResolverDelegate delegate = new MockSyncContentResolverDelegate();
         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 settings = new AndroidSyncSettings(delegate, null);
-        AndroidSyncSettings.overrideForTests(settings);
-        return settings;
+        AndroidSyncSettings.overrideForTests(new AndroidSyncSettings(null));
     }
 
     public static boolean getIsSyncEnabledOnUiThread() {

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

@@ -24,20 +24,14 @@ import java.util.Set;
  * observers for the SYNC_OBSERVER_TYPE_SETTINGS type and it doesn't allow querying
  * settings for a null account.
  */
-class MockSyncContentResolverDelegate implements SyncContentResolverDelegate {
-    private final Set<String> mSyncAutomaticallySet;
-    private final Map<String, Boolean> mIsSyncableMap;
-    private final Set<SyncStatusObserver> mObservers;
+class MockSyncContentResolverDelegate extends SyncContentResolverDelegate {
+    private final Set<String> mSyncAutomaticallySet = new HashSet<String>();
+    private final Map<String, Boolean> mIsSyncableMap = new HashMap<String, Boolean>();
+    private final Set<SyncStatusObserver> mObservers = new HashSet<SyncStatusObserver>();
     private boolean mMasterSyncAutomatically;
 
     private Object mLock = new Object();
 
-    public MockSyncContentResolverDelegate() {
-        mSyncAutomaticallySet = new HashSet<String>();
-        mIsSyncableMap = new HashMap<String, Boolean>();
-        mObservers = new HashSet<SyncStatusObserver>();
-    }
-
     @Override
     public Object addStatusChangeListener(int mask, SyncStatusObserver observer) {
         if (mask != ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS) {

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

@@ -38,6 +38,7 @@ import org.chromium.components.sync.protocol.AutofillWalletSpecifics;
 import org.chromium.components.sync.protocol.EntitySpecifics;
 import org.chromium.components.sync.protocol.SyncEntity;
 import org.chromium.components.sync.protocol.WalletMaskedCreditCard;
+import org.chromium.content_public.browser.test.NativeLibraryTestUtils;
 import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 
@@ -305,30 +306,28 @@ public class SyncTestRule extends ChromeActivityTestRule<ChromeActivity> {
         final Statement base = super.apply(new Statement() {
             @Override
             public void evaluate() throws Throwable {
-                TestThreadUtils.runOnUiThreadBlocking(() -> {
-                    mSyncContentResolver = new MockSyncContentResolverDelegate();
-                    AndroidSyncSettingsTestUtils.setUpAndroidSyncSettingsForTesting(
-                            mSyncContentResolver);
-                });
+                mSyncContentResolver = new MockSyncContentResolverDelegate();
+                mSyncContentResolver.setMasterSyncAutomatically(true);
+                TestThreadUtils.runOnUiThreadBlocking(
+                        () -> SyncContentResolverDelegate.overrideForTests(mSyncContentResolver));
 
                 TrustedVaultClient.setInstanceForTesting(
                         new TrustedVaultClient(FakeTrustedVaultClientBackend.get()));
 
-                startMainActivityForSyncTest();
-                mContext = InstrumentationRegistry.getTargetContext();
+                // Load native since the FakeServer needs it and possibly ProfileSyncService as well
+                // (depends on what fake is provided by |createProfileSyncService()|).
+                NativeLibraryTestUtils.loadNativeLibraryAndInitBrowserProcess();
 
-                TestThreadUtils.runOnUiThreadBlocking(() -> {
-                    // Ensure SyncController is registered with the new AndroidSyncSettings.
-                    AndroidSyncSettings.get().registerObserver(SyncController.get());
-                    mFakeServerHelper = FakeServerHelper.get();
-                });
-                FakeServerHelper.useFakeServer(mContext);
                 TestThreadUtils.runOnUiThreadBlocking(() -> {
                     ProfileSyncService profileSyncService = createProfileSyncService();
                     if (profileSyncService != null) {
                         ProfileSyncService.overrideForTests(profileSyncService);
                     }
                     mProfileSyncService = ProfileSyncService.get();
+
+                    mContext = InstrumentationRegistry.getTargetContext();
+                    FakeServerHelper.useFakeServer(mContext);
+                    mFakeServerHelper = FakeServerHelper.get();
                 });
 
                 UniqueIdentificationGeneratorFactory.registerGenerator(
@@ -340,6 +339,12 @@ public class SyncTestRule extends ChromeActivityTestRule<ChromeActivity> {
                             }
                         },
                         true);
+
+                startMainActivityForSyncTest();
+
+                // Ensure SyncController is created.
+                TestThreadUtils.runOnUiThreadBlocking(() -> SyncController.get());
+
                 statement.evaluate();
             }
         }, desc);