diff options
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 |