Fix ConcurrentModificationException in SyncManager.

Change-Id: Iae304fa7d64beac1b4aeab0956022d2e952b2bb7
Fix: 149070594
Test: atest CtsSyncManagerTestsCases

4 files changed, 116 insertions, 50 deletions

diff --git a/services/core/java/com/android/server/content/SyncManager.java b/services/core/java/com/android/server/content/SyncManager.java
index 6d130d91f639..4a1afb248174 100644
--- a/services/core/java/com/android/server/content/SyncManager.java
+++ b/services/core/java/com/android/server/content/SyncManager.java
@@ -1216,7 +1216,7 @@ public class SyncManager {
         for (SyncOperation op: ops) {
             if (op.isPeriodic && op.target.matchesSpec(target)) {
                 periodicSyncs.add(new PeriodicSync(op.target.account, op.target.provider,
-                        op.extras, op.periodMillis / 1000, op.flexMillis / 1000));
+                        op.getClonedExtras(), op.periodMillis / 1000, op.flexMillis / 1000));
             }
         }
 
@@ -1478,7 +1478,7 @@ public class SyncManager {
             Slog.e(TAG, "Can't schedule null sync operation.");
             return;
         }
-        if (!syncOperation.ignoreBackoff()) {
+        if (!syncOperation.hasIgnoreBackoff()) {
             Pair<Long, Long> backoff = mSyncStorageEngine.getBackoff(syncOperation.target);
             if (backoff == null) {
                 Slog.e(TAG, "Couldn't find backoff values for "
@@ -1631,7 +1631,7 @@ public class SyncManager {
             getSyncStorageEngine().markPending(syncOperation.target, true);
         }
 
-        if (syncOperation.extras.getBoolean(ContentResolver.SYNC_EXTRAS_REQUIRE_CHARGING)) {
+        if (syncOperation.hasRequireCharging()) {
             b.setRequiresCharging(true);
         }
 
@@ -1686,7 +1686,7 @@ public class SyncManager {
         List<SyncOperation> ops = getAllPendingSyncs();
         for (SyncOperation op: ops) {
             if (!op.isPeriodic && op.target.matchesSpec(info)
-                    && syncExtrasEquals(extras, op.extras, false)) {
+                    && op.areExtrasEqual(extras, /*includeSyncSettings=*/ false)) {
                 cancelJob(op, "cancelScheduledSyncOperation");
             }
         }
@@ -1704,15 +1704,9 @@ public class SyncManager {
             Log.d(TAG, "encountered error(s) during the sync: " + syncResult + ", " + operation);
         }
 
-        // The SYNC_EXTRAS_IGNORE_BACKOFF only applies to the first attempt to sync a given
-        // request. Retries of the request will always honor the backoff, so clear the
-        // flag in case we retry this request.
-        if (operation.extras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false)) {
-            operation.extras.remove(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF);
-        }
+        operation.enableBackoff();
 
-        if (operation.extras.getBoolean(ContentResolver.SYNC_EXTRAS_DO_NOT_RETRY, false)
-                && !syncResult.syncAlreadyInProgress) {
+        if (operation.hasDoNotRetry() && !syncResult.syncAlreadyInProgress) {
             // syncAlreadyInProgress flag is set by AbstractThreadedSyncAdapter. The sync adapter
             // has no way of knowing that a sync error occured. So we DO retry if the error is
             // syncAlreadyInProgress.
@@ -1720,10 +1714,9 @@ public class SyncManager {
                 Log.d(TAG, "not retrying sync operation because SYNC_EXTRAS_DO_NOT_RETRY was specified "
                         + operation);
             }
-        } else if (operation.extras.getBoolean(ContentResolver.SYNC_EXTRAS_UPLOAD, false)
-                && !syncResult.syncAlreadyInProgress) {
+        } else if (operation.isUpload() && !syncResult.syncAlreadyInProgress) {
             // If this was an upward sync then schedule a two-way sync immediately.
-            operation.extras.remove(ContentResolver.SYNC_EXTRAS_UPLOAD);
+            operation.enableTwoWaySync();
             if (isLoggable) {
                 Log.d(TAG, "retrying sync operation as a two-way sync because an upload-only sync "
                         + "encountered an error: " + operation);
@@ -3326,7 +3319,7 @@ public class SyncManager {
             List<SyncOperation> ops = getAllPendingSyncs();
             for (SyncOperation op: ops) {
                 if (op.isPeriodic && op.target.matchesSpec(target)
-                        && syncExtrasEquals(op.extras, extras, true /* includeSyncSettings */)) {
+                        && op.areExtrasEqual(extras, /*includeSyncSettings=*/ true)) {
                     maybeUpdateSyncPeriodH(op, pollFrequencyMillis, flexMillis);
                     return;
                 }
@@ -3408,7 +3401,7 @@ public class SyncManager {
             List<SyncOperation> ops = getAllPendingSyncs();
             for (SyncOperation op: ops) {
                 if (op.isPeriodic && op.target.matchesSpec(target)
-                        && syncExtrasEquals(op.extras, extras, true /* includeSyncSettings */)) {
+                        && op.areExtrasEqual(extras, /*includeSyncSettings=*/ true)) {
                     removePeriodicSyncInternalH(op, why);
                 }
             }
@@ -3559,16 +3552,18 @@ public class SyncManager {
                 activeSyncContext.mIsLinkedToDeath = true;
                 syncAdapter.linkToDeath(activeSyncContext, 0);
 
-                mLogger.log("Sync start: account=" + syncOperation.target.account,
-                        " authority=", syncOperation.target.provider,
-                        " reason=", SyncOperation.reasonToString(null, syncOperation.reason),
-                        " extras=", SyncOperation.extrasToString(syncOperation.extras),
-                        " adapter=", activeSyncContext.mSyncAdapter);
+                if (mLogger.enabled()) {
+                    mLogger.log("Sync start: account=" + syncOperation.target.account,
+                            " authority=", syncOperation.target.provider,
+                            " reason=", SyncOperation.reasonToString(null, syncOperation.reason),
+                            " extras=", syncOperation.getExtrasAsString(),
+                            " adapter=", activeSyncContext.mSyncAdapter);
+                }
 
                 activeSyncContext.mSyncAdapter = ISyncAdapter.Stub.asInterface(syncAdapter);
                 activeSyncContext.mSyncAdapter
                         .startSync(activeSyncContext, syncOperation.target.provider,
-                                syncOperation.target.account, syncOperation.extras);
+                                syncOperation.target.account, syncOperation.getClonedExtras());
 
                 mLogger.log("Sync is running now...");
             } catch (RemoteException remoteExc) {
@@ -3602,9 +3597,8 @@ public class SyncManager {
                         continue;
                     }
                     if (extras != null &&
-                            !syncExtrasEquals(activeSyncContext.mSyncOperation.extras,
-                                    extras,
-                                    false /* no config settings */)) {
+                            !activeSyncContext.mSyncOperation.areExtrasEqual(extras,
+                                    /*includeSyncSettings=*/ false)) {
                         continue;
                     }
                     SyncJobService.callJobFinished(activeSyncContext.mSyncOperation.jobId, false,
diff --git a/services/core/java/com/android/server/content/SyncOperation.java b/services/core/java/com/android/server/content/SyncOperation.java
index 2abc2e60a47b..09b7828cbfa2 100644
--- a/services/core/java/com/android/server/content/SyncOperation.java
+++ b/services/core/java/com/android/server/content/SyncOperation.java
@@ -74,7 +74,14 @@ public class SyncOperation {
     /** Where this sync was initiated. */
     public final int syncSource;
     public final boolean allowParallelSyncs;
-    public final Bundle extras;
+
+    /**
+     * Sync extras. Note, DO NOT modify this bundle directly. When changing the content, always
+     * create a copy, update it, set it in this field. This is to avoid concurrent modifications
+     * when other threads are reading it.
+     */
+    private volatile Bundle mImmutableExtras;
+
     public final boolean isPeriodic;
     /** jobId of the periodic SyncOperation that initiated this one */
     public final int sourcePeriodicId;
@@ -118,20 +125,21 @@ public class SyncOperation {
 
     public SyncOperation(SyncOperation op, long periodMillis, long flexMillis) {
         this(op.target, op.owningUid, op.owningPackage, op.reason, op.syncSource,
-                new Bundle(op.extras), op.allowParallelSyncs, op.isPeriodic, op.sourcePeriodicId,
+                op.mImmutableExtras, op.allowParallelSyncs, op.isPeriodic, op.sourcePeriodicId,
                 periodMillis, flexMillis, ContentResolver.SYNC_EXEMPTION_NONE);
     }
 
     public SyncOperation(SyncStorageEngine.EndPoint info, int owningUid, String owningPackage,
-                         int reason, int source, Bundle extras, boolean allowParallelSyncs,
-                         boolean isPeriodic, int sourcePeriodicId, long periodMillis,
-                         long flexMillis, @SyncExemption int syncExemptionFlag) {
+            int reason, int source, Bundle extras,
+            boolean allowParallelSyncs,
+            boolean isPeriodic, int sourcePeriodicId, long periodMillis,
+            long flexMillis, @SyncExemption int syncExemptionFlag) {
         this.target = info;
         this.owningUid = owningUid;
         this.owningPackage = owningPackage;
         this.reason = reason;
         this.syncSource = source;
-        this.extras = new Bundle(extras);
+        this.mImmutableExtras = new Bundle(extras);
         this.allowParallelSyncs = allowParallelSyncs;
         this.isPeriodic = isPeriodic;
         this.sourcePeriodicId = sourcePeriodicId;
@@ -148,7 +156,7 @@ public class SyncOperation {
             return null;
         }
         SyncOperation op = new SyncOperation(target, owningUid, owningPackage, reason, syncSource,
-                new Bundle(extras), allowParallelSyncs, false, jobId /* sourcePeriodicId */,
+                mImmutableExtras, allowParallelSyncs, false, jobId /* sourcePeriodicId */,
                 periodMillis, flexMillis, ContentResolver.SYNC_EXEMPTION_NONE);
         return op;
     }
@@ -160,7 +168,10 @@ public class SyncOperation {
         reason = other.reason;
         syncSource = other.syncSource;
         allowParallelSyncs = other.allowParallelSyncs;
-        extras = new Bundle(other.extras);
+
+        // Since we treat this field as immutable, it's okay to use a shallow copy here.
+        // No need to create a copy.
+        mImmutableExtras = other.mImmutableExtras;
         wakeLockName = other.wakeLockName();
         isPeriodic = other.isPeriodic;
         sourcePeriodicId = other.sourcePeriodicId;
@@ -173,7 +184,8 @@ public class SyncOperation {
     /**
      * All fields are stored in a corresponding key in the persistable bundle.
      *
-     * {@link #extras} is a Bundle and can contain parcelable objects. But only the type Account
+     * {@link #mImmutableExtras} is a Bundle and can contain parcelable objects.
+     * But only the type Account
      * is allowed {@link ContentResolver#validateSyncExtrasBundle(Bundle)} that can't be stored in
      * a PersistableBundle. For every value of type Account with key 'key', we store a
      * PersistableBundle containing account information at key 'ACCOUNT:key'. The Account object
@@ -188,7 +200,9 @@ public class SyncOperation {
         PersistableBundle jobInfoExtras = new PersistableBundle();
 
         PersistableBundle syncExtrasBundle = new PersistableBundle();
-        for (String key: extras.keySet()) {
+
+        final Bundle extras = mImmutableExtras;
+        for (String key : extras.keySet()) {
             Object value = extras.get(key);
             if (value instanceof Account) {
                 Account account = (Account) value;
@@ -327,7 +341,7 @@ public class SyncOperation {
 
     boolean matchesPeriodicOperation(SyncOperation other) {
         return target.matchesSpec(other.target)
-                && SyncManager.syncExtrasEquals(extras, other.extras, true)
+                && SyncManager.syncExtrasEquals(mImmutableExtras, other.mImmutableExtras, true)
                 && periodMillis == other.periodMillis && flexMillis == other.flexMillis;
     }
 
@@ -345,6 +359,7 @@ public class SyncOperation {
     }
 
     private String toKey() {
+        final Bundle extras = mImmutableExtras;
         StringBuilder sb = new StringBuilder();
         sb.append("provider: ").append(target.provider);
         sb.append(" account {name=" + target.account.name
@@ -372,6 +387,7 @@ public class SyncOperation {
 
     String dump(PackageManager pm, boolean shorter, SyncAdapterStateFetcher appStates,
             boolean logSafe) {
+        final Bundle extras = mImmutableExtras;
         StringBuilder sb = new StringBuilder();
         sb.append("JobId=").append(jobId)
                 .append(" ")
@@ -468,33 +484,67 @@ public class SyncOperation {
     }
 
     boolean isInitialization() {
-        return extras.getBoolean(ContentResolver.SYNC_EXTRAS_INITIALIZE, false);
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_INITIALIZE, false);
     }
 
     boolean isExpedited() {
-        return extras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false);
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false);
+    }
+
+    boolean isUpload() {
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_UPLOAD, false);
     }
 
-    boolean ignoreBackoff() {
-        return extras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false);
+    /**
+     * Disable SYNC_EXTRAS_UPLOAD, so it will be a two-way (normal) sync.
+     */
+    void enableTwoWaySync() {
+        removeExtra(ContentResolver.SYNC_EXTRAS_UPLOAD);
+    }
+
+    boolean hasIgnoreBackoff() {
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false);
+    }
+
+    /**
+     * Disable SYNC_EXTRAS_IGNORE_BACKOFF.
+     *
+     * The SYNC_EXTRAS_IGNORE_BACKOFF only applies to the first attempt to sync a given
+     * request. Retries of the request will always honor the backoff, so clear the
+     * flag in case we retry this request.
+     */
+    void enableBackoff() {
+        removeExtra(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF);
+    }
+
+    boolean hasDoNotRetry() {
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_DO_NOT_RETRY, false);
     }
 
     boolean isNotAllowedOnMetered() {
-        return extras.getBoolean(ContentResolver.SYNC_EXTRAS_DISALLOW_METERED, false);
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_DISALLOW_METERED, false);
     }
 
     boolean isManual() {
-        return extras.getBoolean(ContentResolver.SYNC_EXTRAS_MANUAL, false);
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_MANUAL, false);
     }
 
     boolean isIgnoreSettings() {
-        return extras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_SETTINGS, false);
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_SETTINGS, false);
+    }
+
+    boolean hasRequireCharging() {
+        return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_REQUIRE_CHARGING, false);
     }
 
     boolean isAppStandbyExempted() {
         return syncExemptionFlag != ContentResolver.SYNC_EXEMPTION_NONE;
     }
 
+    boolean areExtrasEqual(Bundle other, boolean includeSyncSettings) {
+        return SyncManager.syncExtrasEquals(mImmutableExtras, other, includeSyncSettings);
+    }
+
     static void extrasToStringBuilder(Bundle bundle, StringBuilder sb) {
         if (bundle == null) {
             sb.append("null");
@@ -507,7 +557,7 @@ public class SyncOperation {
         sb.append("]");
     }
 
-    static String extrasToString(Bundle bundle) {
+    private static String extrasToString(Bundle bundle) {
         final StringBuilder sb = new StringBuilder();
         extrasToStringBuilder(bundle, sb);
         return sb.toString();
@@ -531,4 +581,25 @@ public class SyncOperation {
         logArray[3] = target.account.name.hashCode();
         return logArray;
     }
+
+    /**
+     * Removes a sync extra. Note do not call it from multiple threads simultaneously.
+     */
+    private void removeExtra(String key) {
+        final Bundle b = mImmutableExtras;
+        if (!b.containsKey(key)) {
+            return;
+        }
+        final Bundle clone = new Bundle(b);
+        clone.remove(key);
+        mImmutableExtras = clone;
+    }
+
+    public Bundle getClonedExtras() {
+        return new Bundle(mImmutableExtras);
+    }
+
+    public String getExtrasAsString() {
+        return extrasToString(mImmutableExtras);
+    }
 }
diff --git a/services/core/java/com/android/server/content/SyncStorageEngine.java b/services/core/java/com/android/server/content/SyncStorageEngine.java
index dea47db62b9c..0bc181ce0a11 100644
--- a/services/core/java/com/android/server/content/SyncStorageEngine.java
+++ b/services/core/java/com/android/server/content/SyncStorageEngine.java
@@ -137,7 +137,7 @@ public class SyncStorageEngine {
     /**
      * String names for the sync source types.
      *
-     * KEEP THIS AND {@link SyncStatusInfo#SOURCE_COUNT} IN SYNC.
+     * KEEP THIS AND {@link SyncStatusInfo}.SOURCE_COUNT IN SYNC.
      */
     public static final String[] SOURCES = {
             "OTHER",
@@ -1117,7 +1117,7 @@ public class SyncStorageEngine {
                 Slog.v(TAG, "setActiveSync: account="
                         + " auth=" + activeSyncContext.mSyncOperation.target
                         + " src=" + activeSyncContext.mSyncOperation.syncSource
-                        + " extras=" + activeSyncContext.mSyncOperation.extras);
+                        + " extras=" + activeSyncContext.mSyncOperation.getExtrasAsString());
             }
             final EndPoint info = activeSyncContext.mSyncOperation.target;
             AuthorityInfo authorityInfo = getOrCreateAuthorityLocked(
@@ -1179,7 +1179,7 @@ public class SyncStorageEngine {
             item.eventTime = now;
             item.source = op.syncSource;
             item.reason = op.reason;
-            item.extras = op.extras;
+            item.extras = op.getClonedExtras();
             item.event = EVENT_START;
             item.syncExemptionFlag = op.syncExemptionFlag;
             mSyncHistory.add(0, item);
diff --git a/services/tests/servicestests/src/com/android/server/content/SyncOperationTest.java b/services/tests/servicestests/src/com/android/server/content/SyncOperationTest.java
index e37ed7976a86..2cbc3f381909 100644
--- a/services/tests/servicestests/src/com/android/server/content/SyncOperationTest.java
+++ b/services/tests/servicestests/src/com/android/server/content/SyncOperationTest.java
@@ -124,8 +124,9 @@ public class SyncOperationTest extends AndroidTestCase {
         SyncOperation op2 = SyncOperation.maybeCreateFromJobExtras(pb);
 
         assertTrue("Account fields in extras not persisted.",
-                account1.equals(op2.extras.get("acc")));
-        assertTrue("Fields in extras not persisted", "String".equals(op2.extras.getString("str")));
+                account1.equals(op2.getClonedExtras().get("acc")));
+        assertTrue("Fields in extras not persisted", "String".equals(
+                op2.getClonedExtras().getString("str")));
     }
 
     @SmallTest