Fix deadlock on CompatConfig.mChanges
Call out to PackageManager before locking mChanges.
Bug: 187514238
Test: atest FrameworksServicesTests:CompatConfigTest
Merged-In: I72b23fb4651c6efece0afb53b5dc4ca8df63dddb
Change-Id: I72b23fb4651c6efece0afb53b5dc4ca8df63dddb
diff --git a/services/core/java/com/android/server/compat/CompatChange.java b/services/core/java/com/android/server/compat/CompatChange.java
index d29a0c7..0f97b90 100644
--- a/services/core/java/com/android/server/compat/CompatChange.java
+++ b/services/core/java/com/android/server/compat/CompatChange.java
@@ -26,9 +26,7 @@
import android.compat.annotation.Disabled;
import android.compat.annotation.EnabledSince;
import android.compat.annotation.Overridable;
-import android.content.Context;
import android.content.pm.ApplicationInfo;
-import android.content.pm.PackageManager;
import com.android.internal.compat.AndroidBuildClassifier;
import com.android.internal.compat.CompatibilityChangeInfo;
@@ -161,15 +159,17 @@
*
* @param packageName Package name to tentatively enable the change for.
* @param override The package override to be set
+ * @param allowedState Whether the override is allowed.
+ * @param versionCode The version code of the package.
*/
void addPackageOverride(String packageName, PackageOverride override,
- OverrideAllowedState allowedState, Context context) {
+ OverrideAllowedState allowedState, @Nullable Long versionCode) {
if (getLoggingOnly()) {
throw new IllegalArgumentException(
"Can't add overrides for a logging only change " + toString());
}
mRawOverrides.put(packageName, override);
- recheckOverride(packageName, allowedState, context);
+ recheckOverride(packageName, allowedState, versionCode);
}
/**
@@ -179,32 +179,24 @@
* overrides, check if they need to be demoted to deferred.</p>
*
* @param packageName Package name to apply deferred overrides for.
- * @param allowed Whether the override is allowed.
+ * @param allowedState Whether the override is allowed.
+ * @param versionCode The version code of the package.
*
* @return {@code true} if the recheck yielded a result that requires invalidating caches
* (a deferred override was consolidated or a regular override was removed).
*/
boolean recheckOverride(String packageName, OverrideAllowedState allowedState,
- Context context) {
+ @Nullable Long versionCode) {
boolean allowed = (allowedState.state == OverrideAllowedState.ALLOWED);
- Long version = null;
- try {
- ApplicationInfo applicationInfo = context.getPackageManager().getApplicationInfo(
- packageName, 0);
- version = applicationInfo.longVersionCode;
- } catch (PackageManager.NameNotFoundException e) {
- // Do nothing
- }
-
// If the app is not installed or no longer has raw overrides, evaluate to false
- if (version == null || !hasRawOverride(packageName) || !allowed) {
+ if (versionCode == null || !hasRawOverride(packageName) || !allowed) {
removePackageOverrideInternal(packageName);
return false;
}
// Evaluate the override based on its version
- int overrideValue = mRawOverrides.get(packageName).evaluate(version);
+ int overrideValue = mRawOverrides.get(packageName).evaluate(versionCode);
switch (overrideValue) {
case VALUE_UNDEFINED:
removePackageOverrideInternal(packageName);
@@ -229,11 +221,13 @@
* <p>Note, this method is not thread safe so callers must ensure thread safety.
*
* @param pname Package name to reset to defaults for.
+ * @param allowedState Whether the override is allowed.
+ * @param versionCode The version code of the package.
*/
boolean removePackageOverride(String pname, OverrideAllowedState allowedState,
- Context context) {
+ @Nullable Long versionCode) {
if (mRawOverrides.remove(pname) != null) {
- recheckOverride(pname, allowedState, context);
+ recheckOverride(pname, allowedState, versionCode);
return true;
}
return false;
diff --git a/services/core/java/com/android/server/compat/CompatConfig.java b/services/core/java/com/android/server/compat/CompatConfig.java
index 9247568..909ed11 100644
--- a/services/core/java/com/android/server/compat/CompatConfig.java
+++ b/services/core/java/com/android/server/compat/CompatConfig.java
@@ -16,11 +16,13 @@
package com.android.server.compat;
+import android.annotation.Nullable;
import android.app.compat.ChangeIdStateCache;
import android.app.compat.PackageOverride;
import android.compat.Compatibility.ChangeConfig;
import android.content.Context;
import android.content.pm.ApplicationInfo;
+import android.content.pm.PackageManager;
import android.os.Environment;
import android.text.TextUtils;
import android.util.LongArray;
@@ -51,7 +53,6 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.PrintWriter;
-import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
@@ -249,6 +250,7 @@
OverrideAllowedState allowedState =
mOverrideValidator.getOverrideAllowedState(changeId, packageName);
allowedState.enforce(changeId, packageName);
+ Long versionCode = getVersionCodeOrNull(packageName);
synchronized (mChanges) {
CompatChange c = mChanges.get(changeId);
if (c == null) {
@@ -256,7 +258,7 @@
c = new CompatChange(changeId);
addChange(c);
}
- c.addPackageOverride(packageName, overrides, allowedState, mContext);
+ c.addPackageOverride(packageName, overrides, allowedState, versionCode);
invalidateCache();
}
return alreadyKnown;
@@ -336,6 +338,7 @@
* It does not invalidate the cache nor save the overrides.
*/
private boolean removeOverrideUnsafe(long changeId, String packageName) {
+ Long versionCode = getVersionCodeOrNull(packageName);
synchronized (mChanges) {
CompatChange c = mChanges.get(changeId);
if (c != null) {
@@ -343,7 +346,7 @@
mOverrideValidator.getOverrideAllowedState(changeId, packageName);
if (c.hasPackageOverride(packageName)) {
allowedState.enforce(changeId, packageName);
- c.removePackageOverride(packageName, allowedState, mContext);
+ c.removePackageOverride(packageName, allowedState, versionCode);
invalidateCache();
return true;
}
@@ -653,26 +656,33 @@
* Rechecks all the existing overrides for a package.
*/
void recheckOverrides(String packageName) {
- // Local cache of compat changes. Holding a lock on mChanges for the whole duration of the
- // method will cause a deadlock.
- List<CompatChange> changes;
+ Long versionCode = getVersionCodeOrNull(packageName);
synchronized (mChanges) {
- changes = new ArrayList<>(mChanges.size());
+ boolean shouldInvalidateCache = false;
for (int idx = 0; idx < mChanges.size(); ++idx) {
- changes.add(mChanges.valueAt(idx));
+ CompatChange c = mChanges.valueAt(idx);
+ if (!c.hasPackageOverride(packageName)) {
+ continue;
+ }
+ OverrideAllowedState allowedState =
+ mOverrideValidator.getOverrideAllowedStateForRecheck(c.getId(),
+ packageName);
+ shouldInvalidateCache |= c.recheckOverride(packageName, allowedState, versionCode);
+ }
+ if (shouldInvalidateCache) {
+ invalidateCache();
}
}
- boolean shouldInvalidateCache = false;
- for (CompatChange c: changes) {
- if (!c.hasPackageOverride(packageName)) {
- continue;
- }
- OverrideAllowedState allowedState =
- mOverrideValidator.getOverrideAllowedStateForRecheck(c.getId(), packageName);
- shouldInvalidateCache |= c.recheckOverride(packageName, allowedState, mContext);
- }
- if (shouldInvalidateCache) {
- invalidateCache();
+ }
+
+ @Nullable
+ private Long getVersionCodeOrNull(String packageName) {
+ try {
+ ApplicationInfo applicationInfo = mContext.getPackageManager().getApplicationInfo(
+ packageName, 0);
+ return applicationInfo.longVersionCode;
+ } catch (PackageManager.NameNotFoundException e) {
+ return null;
}
}