Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix ConcurrentModificationException in SyncManager.

Browse Source 
Change-Id: Iae304fa7d64beac1b4aeab0956022d2e952b2bb7
Fix: 149070594
Test: atest CtsSyncManagerTestsCases
This commit is contained in:
Makoto Onuki
2020-02-07 15:51:13 -08:00
parent 08b20c4b0b
commit 2690838de9
4 changed files with 116 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
services/core/java/com/android/server/content/SyncManager.java
View File

@@ -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,

109
services/core/java/com/android/server/content/SyncOperation.java
View File

@@ -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 ignoreBackoff() {
return extras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false);
boolean isUpload() {
return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_UPLOAD, 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);
}
}

6
services/core/java/com/android/server/content/SyncStorageEngine.java
View File

@@ -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);

5
services/tests/servicestests/src/com/android/server/content/SyncOperationTest.java
View File

@@ -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