Merge "Fix deadlock on CompatConfig.mChanges"

2 files changed, 41 insertions, 37 deletions

diff --git a/services/core/java/com/android/server/compat/CompatChange.java b/services/core/java/com/android/server/compat/CompatChange.java
index d29a0c715e95..0f97b9042ebe 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.ChangeId;
 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 @@ public final class CompatChange extends CompatibilityChangeInfo {
      *
      * @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 @@ public final class CompatChange extends CompatibilityChangeInfo {
      * 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 @@ public final class CompatChange extends CompatibilityChangeInfo {
      * <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 924756889408..909ed11e9d69 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.FileInputStream;
 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 @@ final class CompatConfig {
         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 @@ final class CompatConfig {
                 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 @@ final class CompatConfig {
      * 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 @@ final class CompatConfig {
                         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 @@ final class CompatConfig {
      * 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);
             }
-        }
-        boolean shouldInvalidateCache = false;
-        for (CompatChange c: changes) {
-            if (!c.hasPackageOverride(packageName)) {
-                continue;
+            if (shouldInvalidateCache) {
+                invalidateCache();
             }
-            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;
         }
     }