From 16b79cfdfb7023f2cbb8fdf626d851db4bc269e9 Mon Sep 17 00:00:00 2001 From: ykhung Date: Wed, 15 Nov 2023 10:20:57 +0800 Subject: [PATCH] Add protection for the ConcurrentModificationException Fix: 310120803 Test: presubmit Change-Id: I8526ed0c93570ba3e183b675b29bd9e9e1582f5a --- .../batteryusage/BatteryDiffEntry.java | 80 +++++++++++++------ 1 file changed, 55 insertions(+), 25 deletions(-) diff --git a/src/com/android/settings/fuelgauge/batteryusage/BatteryDiffEntry.java b/src/com/android/settings/fuelgauge/batteryusage/BatteryDiffEntry.java index a8be398d144..bad1b76de50 100644 --- a/src/com/android/settings/fuelgauge/batteryusage/BatteryDiffEntry.java +++ b/src/com/android/settings/fuelgauge/batteryusage/BatteryDiffEntry.java @@ -27,10 +27,12 @@ import android.util.ArrayMap; import android.util.Log; import android.util.Pair; +import androidx.annotation.GuardedBy; import androidx.annotation.VisibleForTesting; import com.android.settings.R; import com.android.settings.fuelgauge.BatteryUtils; +import com.android.settings.fuelgauge.batteryusage.BatteryEntry.NameAndIcon; import com.android.settingslib.utils.StringUtil; import java.util.Comparator; @@ -40,16 +42,23 @@ import java.util.Map; /** A container class to carry battery data in a specific time slot. */ public class BatteryDiffEntry { private static final String TAG = "BatteryDiffEntry"; + private static final Object sResourceCacheLock = new Object(); + private static final Object sPackageNameAndUidCacheLock = new Object(); + private static final Object sValidForRestrictionLock = new Object(); static Locale sCurrentLocale = null; + // Caches app label and icon to improve loading performance. - static final Map sResourceCache = new ArrayMap<>(); + @GuardedBy("sResourceCacheLock") + static final Map sResourceCache = new ArrayMap<>(); // Caches package name and uid to improve loading performance. + @GuardedBy("sPackageNameAndUidCacheLock") static final Map sPackageNameAndUidCache = new ArrayMap<>(); // Whether a specific item is valid to launch restriction page? @VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE) + @GuardedBy("sValidForRestrictionLock") static final Map sValidForRestriction = new ArrayMap<>(); /** A comparator for {@link BatteryDiffEntry} based on the sorting key. */ @@ -304,12 +313,16 @@ public class BatteryDiffEntry { } private int getPackageUid(String packageName) { - if (sPackageNameAndUidCache.containsKey(packageName)) { - return sPackageNameAndUidCache.get(packageName); + synchronized (sPackageNameAndUidCacheLock) { + if (sPackageNameAndUidCache.containsKey(packageName)) { + return sPackageNameAndUidCache.get(packageName); + } } int uid = BatteryUtils.getInstance(mContext).getPackageUid(packageName); - sPackageNameAndUidCache.put(packageName, uid); + synchronized (sPackageNameAndUidCacheLock) { + sPackageNameAndUidCache.put(packageName, uid); + } return uid; } @@ -318,13 +331,16 @@ public class BatteryDiffEntry { return; } // Checks whether we have cached data or not first before fetching. - final BatteryEntry.NameAndIcon nameAndIcon = getCache(); + final NameAndIcon nameAndIcon = getCache(); if (nameAndIcon != null) { mAppLabel = nameAndIcon.mName; mAppIcon = nameAndIcon.mIcon; mAppIconId = nameAndIcon.mIconId; } - final Boolean validForRestriction = sValidForRestriction.get(getKey()); + Boolean validForRestriction = null; + synchronized (sValidForRestrictionLock) { + validForRestriction = sValidForRestriction.get(getKey()); + } if (validForRestriction != null) { mValidForRestriction = validForRestriction; } @@ -336,33 +352,34 @@ public class BatteryDiffEntry { // Configures whether we can launch restriction page or not. updateRestrictionFlagState(); - sValidForRestriction.put(getKey(), Boolean.valueOf(mValidForRestriction)); + synchronized (sValidForRestrictionLock) { + sValidForRestriction.put(getKey(), Boolean.valueOf(mValidForRestriction)); + } if (getKey() != null && SPECIAL_ENTRY_MAP.containsKey(getKey())) { Pair pair = SPECIAL_ENTRY_MAP.get(getKey()); mAppLabel = mContext.getString(pair.first); mAppIconId = pair.second; mAppIcon = mContext.getDrawable(mAppIconId); - sResourceCache.put( - getKey(), new BatteryEntry.NameAndIcon(mAppLabel, mAppIcon, mAppIconId)); + putResourceCache(getKey(), new NameAndIcon(mAppLabel, mAppIcon, mAppIconId)); return; } // Loads application icon and label based on consumer type. switch (mConsumerType) { case ConvertUtils.CONSUMER_TYPE_USER_BATTERY: - final BatteryEntry.NameAndIcon nameAndIconForUser = + final NameAndIcon nameAndIconForUser = BatteryEntry.getNameAndIconFromUserId(mContext, (int) mUserId); if (nameAndIconForUser != null) { mAppIcon = nameAndIconForUser.mIcon; mAppLabel = nameAndIconForUser.mName; - sResourceCache.put( + putResourceCache( getKey(), - new BatteryEntry.NameAndIcon(mAppLabel, mAppIcon, /* iconId= */ 0)); + new NameAndIcon(mAppLabel, mAppIcon, /* iconId= */ 0)); } break; case ConvertUtils.CONSUMER_TYPE_SYSTEM_BATTERY: - final BatteryEntry.NameAndIcon nameAndIconForSystem = + final NameAndIcon nameAndIconForSystem = BatteryEntry.getNameAndIconFromPowerComponent(mContext, mComponentId); if (nameAndIconForSystem != null) { mAppLabel = nameAndIconForSystem.mName; @@ -370,9 +387,8 @@ public class BatteryDiffEntry { mAppIconId = nameAndIconForSystem.mIconId; mAppIcon = mContext.getDrawable(nameAndIconForSystem.mIconId); } - sResourceCache.put( - getKey(), - new BatteryEntry.NameAndIcon(mAppLabel, mAppIcon, mAppIconId)); + putResourceCache( + getKey(), new NameAndIcon(mAppLabel, mAppIcon, mAppIconId)); } break; case ConvertUtils.CONSUMER_TYPE_UID_BATTERY: @@ -384,9 +400,9 @@ public class BatteryDiffEntry { // Adds badge icon into app icon for work profile. mAppIcon = getBadgeIconForUser(mAppIcon); if (mAppLabel != null || mAppIcon != null) { - sResourceCache.put( + putResourceCache( getKey(), - new BatteryEntry.NameAndIcon(mAppLabel, mAppIcon, /* iconId= */ 0)); + new NameAndIcon(mAppLabel, mAppIcon, /* iconId= */ 0)); } break; } @@ -429,7 +445,7 @@ public class BatteryDiffEntry { } } - private BatteryEntry.NameAndIcon getCache() { + private NameAndIcon getCache() { final Locale locale = Locale.getDefault(); if (sCurrentLocale != locale) { Log.d( @@ -440,7 +456,9 @@ public class BatteryDiffEntry { sCurrentLocale = locale; clearCache(); } - return sResourceCache.get(getKey()); + synchronized (sResourceCacheLock) { + return sResourceCache.get(getKey()); + } } private void loadNameAndIconForUid() { @@ -469,13 +487,13 @@ public class BatteryDiffEntry { final String[] packages = packageManager.getPackagesForUid(uid); // Loads special defined application label and icon if available. if (packages == null || packages.length == 0) { - final BatteryEntry.NameAndIcon nameAndIcon = + final NameAndIcon nameAndIcon = BatteryEntry.getNameAndIconFromUid(mContext, mAppLabel, uid); mAppLabel = nameAndIcon.mName; mAppIcon = nameAndIcon.mIcon; } - final BatteryEntry.NameAndIcon nameAndIcon = + final NameAndIcon nameAndIcon = BatteryEntry.loadNameAndIcon( mContext, uid, /* batteryEntry= */ null, packageName, mAppLabel, mAppIcon); // Clears BatteryEntry internal cache since we will have another one. @@ -544,9 +562,21 @@ public class BatteryDiffEntry { /** Clears all cache data. */ public static void clearCache() { - sResourceCache.clear(); - sValidForRestriction.clear(); - sPackageNameAndUidCache.clear(); + synchronized (sResourceCacheLock) { + sResourceCache.clear(); + } + synchronized (sValidForRestrictionLock) { + sValidForRestriction.clear(); + } + synchronized (sPackageNameAndUidCacheLock) { + sPackageNameAndUidCache.clear(); + } + } + + private static void putResourceCache(String key, NameAndIcon nameAndIcon) { + synchronized (sResourceCacheLock) { + sResourceCache.put(key, nameAndIcon); + } } private Drawable getBadgeIconForUser(Drawable icon) {