Merge "Fix ConcurrentModificationException in SyncManager." (5f8c6f88) · Commits · e / os / android_frameworks_base

services/core/java/com/android/server/content/SyncManager.java

+20 −26

Original line number	Original line	Diff line number	Diff line
	@@ -1216,7 +1216,7 @@ public class SyncManager {
	for (SyncOperation op: ops) {		for (SyncOperation op: ops) {
	if (op.isPeriodic && op.target.matchesSpec(target)) {		if (op.isPeriodic && op.target.matchesSpec(target)) {
	periodicSyncs.add(new PeriodicSync(op.target.account, op.target.provider,		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.");		Slog.e(TAG, "Can't schedule null sync operation.");
	return;		return;
	}		}
	if (!syncOperation.ignoreBackoff()) {		if (!syncOperation.hasIgnoreBackoff()) {
	Pair<Long, Long> backoff = mSyncStorageEngine.getBackoff(syncOperation.target);		Pair<Long, Long> backoff = mSyncStorageEngine.getBackoff(syncOperation.target);
	if (backoff == null) {		if (backoff == null) {
	Slog.e(TAG, "Couldn't find backoff values for "		Slog.e(TAG, "Couldn't find backoff values for "
	@@ -1631,7 +1631,7 @@ public class SyncManager {
	getSyncStorageEngine().markPending(syncOperation.target, true);		getSyncStorageEngine().markPending(syncOperation.target, true);
	}		}

	if (syncOperation.extras.getBoolean(ContentResolver.SYNC_EXTRAS_REQUIRE_CHARGING)) {		if (syncOperation.hasRequireCharging()) {
	b.setRequiresCharging(true);		b.setRequiresCharging(true);
	}		}

	@@ -1686,7 +1686,7 @@ public class SyncManager {
	List<SyncOperation> ops = getAllPendingSyncs();		List<SyncOperation> ops = getAllPendingSyncs();
	for (SyncOperation op: ops) {		for (SyncOperation op: ops) {
	if (!op.isPeriodic && op.target.matchesSpec(info)		if (!op.isPeriodic && op.target.matchesSpec(info)
	&& syncExtrasEquals(extras, op.extras, false)) {		&& op.areExtrasEqual(extras, /includeSyncSettings=/ false)) {
	cancelJob(op, "cancelScheduledSyncOperation");		cancelJob(op, "cancelScheduledSyncOperation");
	}		}
	}		}
	@@ -1704,15 +1704,9 @@ public class SyncManager {
	Log.d(TAG, "encountered error(s) during the sync: " + syncResult + ", " + operation);		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		operation.enableBackoff();
	// 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);
	}

	if (operation.extras.getBoolean(ContentResolver.SYNC_EXTRAS_DO_NOT_RETRY, false)		if (operation.hasDoNotRetry() && !syncResult.syncAlreadyInProgress) {
	&& !syncResult.syncAlreadyInProgress) {
	// syncAlreadyInProgress flag is set by AbstractThreadedSyncAdapter. The sync adapter		// 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		// has no way of knowing that a sync error occured. So we DO retry if the error is
	// syncAlreadyInProgress.		// syncAlreadyInProgress.
	@@ -1720,10 +1714,9 @@ public class SyncManager {
	Log.d(TAG, "not retrying sync operation because SYNC_EXTRAS_DO_NOT_RETRY was specified "		Log.d(TAG, "not retrying sync operation because SYNC_EXTRAS_DO_NOT_RETRY was specified "
	+ operation);		+ operation);
	}		}
	} else if (operation.extras.getBoolean(ContentResolver.SYNC_EXTRAS_UPLOAD, false)		} else if (operation.isUpload() && !syncResult.syncAlreadyInProgress) {
	&& !syncResult.syncAlreadyInProgress) {
	// If this was an upward sync then schedule a two-way sync immediately.		// If this was an upward sync then schedule a two-way sync immediately.
	operation.extras.remove(ContentResolver.SYNC_EXTRAS_UPLOAD);		operation.enableTwoWaySync();
	if (isLoggable) {		if (isLoggable) {
	Log.d(TAG, "retrying sync operation as a two-way sync because an upload-only sync "		Log.d(TAG, "retrying sync operation as a two-way sync because an upload-only sync "
	+ "encountered an error: " + operation);		+ "encountered an error: " + operation);
	@@ -3326,7 +3319,7 @@ public class SyncManager {
	List<SyncOperation> ops = getAllPendingSyncs();		List<SyncOperation> ops = getAllPendingSyncs();
	for (SyncOperation op: ops) {		for (SyncOperation op: ops) {
	if (op.isPeriodic && op.target.matchesSpec(target)		if (op.isPeriodic && op.target.matchesSpec(target)
	&& syncExtrasEquals(op.extras, extras, true /* includeSyncSettings */)) {		&& op.areExtrasEqual(extras, /includeSyncSettings=/ true)) {
	maybeUpdateSyncPeriodH(op, pollFrequencyMillis, flexMillis);		maybeUpdateSyncPeriodH(op, pollFrequencyMillis, flexMillis);
	return;		return;
	}		}
	@@ -3408,7 +3401,7 @@ public class SyncManager {
	List<SyncOperation> ops = getAllPendingSyncs();		List<SyncOperation> ops = getAllPendingSyncs();
	for (SyncOperation op: ops) {		for (SyncOperation op: ops) {
	if (op.isPeriodic && op.target.matchesSpec(target)		if (op.isPeriodic && op.target.matchesSpec(target)
	&& syncExtrasEquals(op.extras, extras, true /* includeSyncSettings */)) {		&& op.areExtrasEqual(extras, /includeSyncSettings=/ true)) {
	removePeriodicSyncInternalH(op, why);		removePeriodicSyncInternalH(op, why);
	}		}
	}		}
	@@ -3559,16 +3552,18 @@ public class SyncManager {
	activeSyncContext.mIsLinkedToDeath = true;		activeSyncContext.mIsLinkedToDeath = true;
	syncAdapter.linkToDeath(activeSyncContext, 0);		syncAdapter.linkToDeath(activeSyncContext, 0);

			if (mLogger.enabled()) {
	mLogger.log("Sync start: account=" + syncOperation.target.account,		mLogger.log("Sync start: account=" + syncOperation.target.account,
	" authority=", syncOperation.target.provider,		" authority=", syncOperation.target.provider,
	" reason=", SyncOperation.reasonToString(null, syncOperation.reason),		" reason=", SyncOperation.reasonToString(null, syncOperation.reason),
	" extras=", SyncOperation.extrasToString(syncOperation.extras),		" extras=", syncOperation.getExtrasAsString(),
	" adapter=", activeSyncContext.mSyncAdapter);		" adapter=", activeSyncContext.mSyncAdapter);
			}

	activeSyncContext.mSyncAdapter = ISyncAdapter.Stub.asInterface(syncAdapter);		activeSyncContext.mSyncAdapter = ISyncAdapter.Stub.asInterface(syncAdapter);
	activeSyncContext.mSyncAdapter		activeSyncContext.mSyncAdapter
	.startSync(activeSyncContext, syncOperation.target.provider,		.startSync(activeSyncContext, syncOperation.target.provider,
	syncOperation.target.account, syncOperation.extras);		syncOperation.target.account, syncOperation.getClonedExtras());

	mLogger.log("Sync is running now...");		mLogger.log("Sync is running now...");
	} catch (RemoteException remoteExc) {		} catch (RemoteException remoteExc) {
	@@ -3602,9 +3597,8 @@ public class SyncManager {
	continue;		continue;
	}		}
	if (extras != null &&		if (extras != null &&
	!syncExtrasEquals(activeSyncContext.mSyncOperation.extras,		!activeSyncContext.mSyncOperation.areExtrasEqual(extras,
	extras,		/includeSyncSettings=/ false)) {
	false /* no config settings */)) {
	continue;		continue;
	}		}
	SyncJobService.callJobFinished(activeSyncContext.mSyncOperation.jobId, false,		SyncJobService.callJobFinished(activeSyncContext.mSyncOperation.jobId, false,

services/core/java/com/android/server/content/SyncOperation.java

+90 −19

Original line number	Original line	Diff line number	Diff line
	@@ -74,7 +74,14 @@ public class SyncOperation {
	/** Where this sync was initiated. */		/** Where this sync was initiated. */
	public final int syncSource;		public final int syncSource;
	public final boolean allowParallelSyncs;		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;		public final boolean isPeriodic;
	/** jobId of the periodic SyncOperation that initiated this one */		/** jobId of the periodic SyncOperation that initiated this one */
	public final int sourcePeriodicId;		public final int sourcePeriodicId;
	@@ -118,12 +125,13 @@ public class SyncOperation {

	public SyncOperation(SyncOperation op, long periodMillis, long flexMillis) {		public SyncOperation(SyncOperation op, long periodMillis, long flexMillis) {
	this(op.target, op.owningUid, op.owningPackage, op.reason, op.syncSource,		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);		periodMillis, flexMillis, ContentResolver.SYNC_EXEMPTION_NONE);
	}		}

	public SyncOperation(SyncStorageEngine.EndPoint info, int owningUid, String owningPackage,		public SyncOperation(SyncStorageEngine.EndPoint info, int owningUid, String owningPackage,
	int reason, int source, Bundle extras, boolean allowParallelSyncs,		int reason, int source, Bundle extras,
			boolean allowParallelSyncs,
	boolean isPeriodic, int sourcePeriodicId, long periodMillis,		boolean isPeriodic, int sourcePeriodicId, long periodMillis,
	long flexMillis, @SyncExemption int syncExemptionFlag) {		long flexMillis, @SyncExemption int syncExemptionFlag) {
	this.target = info;		this.target = info;
	@@ -131,7 +139,7 @@ public class SyncOperation {
	this.owningPackage = owningPackage;		this.owningPackage = owningPackage;
	this.reason = reason;		this.reason = reason;
	this.syncSource = source;		this.syncSource = source;
	this.extras = new Bundle(extras);		this.mImmutableExtras = new Bundle(extras);
	this.allowParallelSyncs = allowParallelSyncs;		this.allowParallelSyncs = allowParallelSyncs;
	this.isPeriodic = isPeriodic;		this.isPeriodic = isPeriodic;
	this.sourcePeriodicId = sourcePeriodicId;		this.sourcePeriodicId = sourcePeriodicId;
	@@ -148,7 +156,7 @@ public class SyncOperation {
	return null;		return null;
	}		}
	SyncOperation op = new SyncOperation(target, owningUid, owningPackage, reason, syncSource,		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);		periodMillis, flexMillis, ContentResolver.SYNC_EXEMPTION_NONE);
	return op;		return op;
	}		}
	@@ -160,7 +168,10 @@ public class SyncOperation {
	reason = other.reason;		reason = other.reason;
	syncSource = other.syncSource;		syncSource = other.syncSource;
	allowParallelSyncs = other.allowParallelSyncs;		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();		wakeLockName = other.wakeLockName();
	isPeriodic = other.isPeriodic;		isPeriodic = other.isPeriodic;
	sourcePeriodicId = other.sourcePeriodicId;		sourcePeriodicId = other.sourcePeriodicId;
	@@ -173,7 +184,8 @@ public class SyncOperation {
	/**		/**
	* All fields are stored in a corresponding key in the persistable bundle.		* 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		* 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		* 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		* PersistableBundle containing account information at key 'ACCOUNT:key'. The Account object
	@@ -188,6 +200,8 @@ public class SyncOperation {
	PersistableBundle jobInfoExtras = new PersistableBundle();		PersistableBundle jobInfoExtras = new PersistableBundle();

	PersistableBundle syncExtrasBundle = new PersistableBundle();		PersistableBundle syncExtrasBundle = new PersistableBundle();

			final Bundle extras = mImmutableExtras;
	for (String key : extras.keySet()) {		for (String key : extras.keySet()) {
	Object value = extras.get(key);		Object value = extras.get(key);
	if (value instanceof Account) {		if (value instanceof Account) {
	@@ -327,7 +341,7 @@ public class SyncOperation {

	boolean matchesPeriodicOperation(SyncOperation other) {		boolean matchesPeriodicOperation(SyncOperation other) {
	return target.matchesSpec(other.target)		return target.matchesSpec(other.target)
	&& SyncManager.syncExtrasEquals(extras, other.extras, true)		&& SyncManager.syncExtrasEquals(mImmutableExtras, other.mImmutableExtras, true)
	&& periodMillis == other.periodMillis && flexMillis == other.flexMillis;		&& periodMillis == other.periodMillis && flexMillis == other.flexMillis;
	}		}

	@@ -345,6 +359,7 @@ public class SyncOperation {
	}		}

	private String toKey() {		private String toKey() {
			final Bundle extras = mImmutableExtras;
	StringBuilder sb = new StringBuilder();		StringBuilder sb = new StringBuilder();
	sb.append("provider: ").append(target.provider);		sb.append("provider: ").append(target.provider);
	sb.append(" account {name=" + target.account.name		sb.append(" account {name=" + target.account.name
	@@ -372,6 +387,7 @@ public class SyncOperation {

	String dump(PackageManager pm, boolean shorter, SyncAdapterStateFetcher appStates,		String dump(PackageManager pm, boolean shorter, SyncAdapterStateFetcher appStates,
	boolean logSafe) {		boolean logSafe) {
			final Bundle extras = mImmutableExtras;
	StringBuilder sb = new StringBuilder();		StringBuilder sb = new StringBuilder();
	sb.append("JobId=").append(jobId)		sb.append("JobId=").append(jobId)
	.append(" ")		.append(" ")
	@@ -468,33 +484,67 @@ public class SyncOperation {
	}		}

	boolean isInitialization() {		boolean isInitialization() {
	return extras.getBoolean(ContentResolver.SYNC_EXTRAS_INITIALIZE, false);		return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_INITIALIZE, false);
	}		}

	boolean isExpedited() {		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);
			}

			/**
			* 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 ignoreBackoff() {		boolean hasDoNotRetry() {
	return extras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false);		return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_DO_NOT_RETRY, false);
	}		}

	boolean isNotAllowedOnMetered() {		boolean isNotAllowedOnMetered() {
	return extras.getBoolean(ContentResolver.SYNC_EXTRAS_DISALLOW_METERED, false);		return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_DISALLOW_METERED, false);
	}		}

	boolean isManual() {		boolean isManual() {
	return extras.getBoolean(ContentResolver.SYNC_EXTRAS_MANUAL, false);		return mImmutableExtras.getBoolean(ContentResolver.SYNC_EXTRAS_MANUAL, false);
	}		}

	boolean isIgnoreSettings() {		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() {		boolean isAppStandbyExempted() {
	return syncExemptionFlag != ContentResolver.SYNC_EXEMPTION_NONE;		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) {		static void extrasToStringBuilder(Bundle bundle, StringBuilder sb) {
	if (bundle == null) {		if (bundle == null) {
	sb.append("null");		sb.append("null");
	@@ -507,7 +557,7 @@ public class SyncOperation {
	sb.append("]");		sb.append("]");
	}		}

	static String extrasToString(Bundle bundle) {		private static String extrasToString(Bundle bundle) {
	final StringBuilder sb = new StringBuilder();		final StringBuilder sb = new StringBuilder();
	extrasToStringBuilder(bundle, sb);		extrasToStringBuilder(bundle, sb);
	return sb.toString();		return sb.toString();
	@@ -531,4 +581,25 @@ public class SyncOperation {
	logArray[3] = target.account.name.hashCode();		logArray[3] = target.account.name.hashCode();
	return logArray;		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);
			}
	}		}

services/core/java/com/android/server/content/SyncStorageEngine.java

+3 −3

Original line number	Original line	Diff line number	Diff line
	@@ -137,7 +137,7 @@ public class SyncStorageEngine {
	/**		/**
	* String names for the sync source types.		* 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 = {		public static final String[] SOURCES = {
	"OTHER",		"OTHER",
	@@ -1117,7 +1117,7 @@ public class SyncStorageEngine {
	Slog.v(TAG, "setActiveSync: account="		Slog.v(TAG, "setActiveSync: account="
	+ " auth=" + activeSyncContext.mSyncOperation.target		+ " auth=" + activeSyncContext.mSyncOperation.target
	+ " src=" + activeSyncContext.mSyncOperation.syncSource		+ " src=" + activeSyncContext.mSyncOperation.syncSource
	+ " extras=" + activeSyncContext.mSyncOperation.extras);		+ " extras=" + activeSyncContext.mSyncOperation.getExtrasAsString());
	}		}
	final EndPoint info = activeSyncContext.mSyncOperation.target;		final EndPoint info = activeSyncContext.mSyncOperation.target;
	AuthorityInfo authorityInfo = getOrCreateAuthorityLocked(		AuthorityInfo authorityInfo = getOrCreateAuthorityLocked(
	@@ -1179,7 +1179,7 @@ public class SyncStorageEngine {
	item.eventTime = now;		item.eventTime = now;
	item.source = op.syncSource;		item.source = op.syncSource;
	item.reason = op.reason;		item.reason = op.reason;
	item.extras = op.extras;		item.extras = op.getClonedExtras();
	item.event = EVENT_START;		item.event = EVENT_START;
	item.syncExemptionFlag = op.syncExemptionFlag;		item.syncExemptionFlag = op.syncExemptionFlag;
	mSyncHistory.add(0, item);		mSyncHistory.add(0, item);

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

+3 −2

Original line number	Original line	Diff line number	Diff line
	@@ -124,8 +124,9 @@ public class SyncOperationTest extends AndroidTestCase {
	SyncOperation op2 = SyncOperation.maybeCreateFromJobExtras(pb);		SyncOperation op2 = SyncOperation.maybeCreateFromJobExtras(pb);

	assertTrue("Account fields in extras not persisted.",		assertTrue("Account fields in extras not persisted.",
	account1.equals(op2.extras.get("acc")));		account1.equals(op2.getClonedExtras().get("acc")));
	assertTrue("Fields in extras not persisted", "String".equals(op2.extras.getString("str")));		assertTrue("Fields in extras not persisted", "String".equals(
			op2.getClonedExtras().getString("str")));
	}		}

	@SmallTest		@SmallTest