From 1bd36ec14a6d27a010f7892c5ea52776bb88d454 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Fri, 9 Apr 2021 10:20:10 -0700 Subject: [PATCH] Only clear UID-based structures when UID is fully gone. Check to make sure that there are no remaining packages in a UID before we remove data structures based on UID-only keys. Bug: 184098842 Test: atest frameworks/base/services/tests/servicestests/src/com/android/server/job Test: atest frameworks/base/services/tests/mockingservicestests/src/com/android/server/job Test: atest CtsJobSchedulerTestCases Change-Id: Ia38130c1b6fcbf6c3851fe6890726bcab71e15b1 --- .../server/job/JobSchedulerService.java | 58 +++++++++++ .../controllers/ConnectivityController.java | 14 ++- .../job/controllers/QuotaController.java | 95 +++---------------- .../job/controllers/QuotaControllerTest.java | 14 +-- 4 files changed, 90 insertions(+), 91 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java index 8b9eca6f31dc5..d72f565be4a0f 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java @@ -21,6 +21,7 @@ import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_DISABLED import static android.text.format.DateUtils.MINUTE_IN_MILLIS; import android.annotation.NonNull; +import android.annotation.Nullable; import android.annotation.UserIdInt; import android.app.Activity; import android.app.ActivityManager; @@ -67,15 +68,18 @@ import android.os.WorkSource; import android.provider.DeviceConfig; import android.text.format.DateUtils; import android.util.ArrayMap; +import android.util.ArraySet; import android.util.IndentingPrintWriter; import android.util.Log; import android.util.Slog; import android.util.SparseArray; import android.util.SparseIntArray; +import android.util.SparseSetArray; import android.util.TimeUtils; import android.util.proto.ProtoOutputStream; import com.android.internal.R; +import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.app.IBatteryStats; import com.android.internal.util.ArrayUtils; @@ -316,6 +320,9 @@ public class JobSchedulerService extends com.android.server.SystemService */ final ArrayMap mDebuggableApps = new ArrayMap<>(); + /** Cached mapping of UIDs (for all users) to a list of packages in the UID. */ + private final SparseSetArray mUidToPackageCache = new SparseSetArray<>(); + /** * Named indices into standby bucket arrays, for clarity in referring to * specific buckets' bookkeeping. @@ -785,12 +792,20 @@ public class JobSchedulerService extends com.android.server.SystemService } else { Slog.w(TAG, "PACKAGE_CHANGED for " + pkgName + " / uid " + pkgUid); } + } else if (Intent.ACTION_PACKAGE_ADDED.equals(action)) { + if (!intent.getBooleanExtra(Intent.EXTRA_REPLACING, false)) { + final int uid = intent.getIntExtra(Intent.EXTRA_UID, -1); + synchronized (mLock) { + mUidToPackageCache.remove(uid); + } + } } else if (Intent.ACTION_PACKAGE_FULLY_REMOVED.equals(action)) { int uidRemoved = intent.getIntExtra(Intent.EXTRA_UID, -1); if (DEBUG) { Slog.d(TAG, "Removing jobs for uid: " + uidRemoved); } synchronized (mLock) { + mUidToPackageCache.remove(uidRemoved); // There's no guarantee that the process has been stopped by the time we // get here, but since this is generally a user-initiated action, it should // be fine to just put USER instead of UNINSTALL or DISABLED. @@ -815,6 +830,7 @@ public class JobSchedulerService extends com.android.server.SystemService Slog.d(TAG, "Removing jobs for user: " + userId); } synchronized (mLock) { + mUidToPackageCache.clear(); cancelJobsForUserLocked(userId); for (int c = 0; c < mControllers.size(); ++c) { mControllers.get(c).onUserRemovedLocked(userId); @@ -904,6 +920,27 @@ public class JobSchedulerService extends com.android.server.SystemService return WorkSource.isChainedBatteryAttributionEnabled(getContext()); } + @Nullable + @GuardedBy("mLock") + public ArraySet getPackagesForUidLocked(final int uid) { + ArraySet packages = mUidToPackageCache.get(uid); + if (packages == null) { + try { + String[] pkgs = AppGlobals.getPackageManager() + .getPackagesForUid(uid); + if (pkgs != null) { + for (String pkg : pkgs) { + mUidToPackageCache.add(uid, pkg); + } + packages = mUidToPackageCache.get(uid); + } + } catch (RemoteException e) { + // Shouldn't happen. + } + } + return packages; + } + @Override public void onUserUnlocked(@NonNull TargetUser user) { synchronized (mLock) { @@ -1484,6 +1521,7 @@ public class JobSchedulerService extends com.android.server.SystemService // Register br for package removals and user removals. final IntentFilter filter = new IntentFilter(); filter.addAction(Intent.ACTION_PACKAGE_FULLY_REMOVED); + filter.addAction(Intent.ACTION_PACKAGE_ADDED); filter.addAction(Intent.ACTION_PACKAGE_CHANGED); filter.addAction(Intent.ACTION_PACKAGE_RESTARTED); filter.addAction(Intent.ACTION_QUERY_PACKAGE_RESTART); @@ -3285,6 +3323,26 @@ public class JobSchedulerService extends com.android.server.SystemService pw.decreaseIndent(); } + boolean uidMapPrinted = false; + for (int i = 0; i < mUidToPackageCache.size(); ++i) { + final int uid = mUidToPackageCache.keyAt(i); + if (filterUid != -1 && filterUid != uid) { + continue; + } + if (!uidMapPrinted) { + uidMapPrinted = true; + pw.println(); + pw.println("Cached UID->package map:"); + pw.increaseIndent(); + } + pw.print(uid); + pw.print(": "); + pw.println(mUidToPackageCache.get(uid)); + } + if (uidMapPrinted) { + pw.decreaseIndent(); + } + boolean backingPrinted = false; for (int i = 0; i < mBackingUpUids.size(); i++) { int uid = mBackingUpUids.keyAt(i); diff --git a/apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java b/apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java index b4e167a97b4c9..33724e85a7e2d 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java +++ b/apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java @@ -452,11 +452,15 @@ public final class ConnectivityController extends RestrictingController implemen @GuardedBy("mLock") @Override public void onAppRemovedLocked(String pkgName, int uid) { - mTrackedJobs.delete(uid); - UidStats uidStats = mUidStats.removeReturnOld(uid); - unregisterDefaultNetworkCallbackLocked(uid, sElapsedRealtimeClock.millis()); - mSortedStats.remove(uidStats); - registerPendingUidCallbacksLocked(); + if (mService.getPackagesForUidLocked(uid) == null) { + // All packages in the UID have been removed. It's safe to remove things based on + // UID alone. + mTrackedJobs.delete(uid); + UidStats uidStats = mUidStats.removeReturnOld(uid); + unregisterDefaultNetworkCallbackLocked(uid, sElapsedRealtimeClock.millis()); + mSortedStats.remove(uidStats); + registerPendingUidCallbacksLocked(); + } } @GuardedBy("mLock") diff --git a/apex/jobscheduler/service/java/com/android/server/job/controllers/QuotaController.java b/apex/jobscheduler/service/java/com/android/server/job/controllers/QuotaController.java index 158a0b9d9a5d8..d4ce437328648 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/controllers/QuotaController.java +++ b/apex/jobscheduler/service/java/com/android/server/job/controllers/QuotaController.java @@ -340,9 +340,6 @@ public final class QuotaController extends StateController { /** List of UIDs currently in the foreground. */ private final SparseBooleanArray mForegroundUids = new SparseBooleanArray(); - /** Cached mapping of UIDs (for all users) to a list of packages in the UID. */ - private final SparseSetArray mUidToPackageCache = new SparseSetArray<>(); - /** * List of jobs that started while the UID was in the TOP state. There will be no more than * 16 ({@link JobSchedulerService#MAX_JOB_CONTEXTS_COUNT}) running at once, so an ArraySet is @@ -449,22 +446,6 @@ public final class QuotaController extends StateController { } } - private final BroadcastReceiver mPackageAddedReceiver = new BroadcastReceiver() { - @Override - public void onReceive(Context context, Intent intent) { - if (intent == null) { - return; - } - if (intent.getBooleanExtra(Intent.EXTRA_REPLACING, false)) { - return; - } - final int uid = intent.getIntExtra(Intent.EXTRA_UID, -1); - synchronized (mLock) { - mUidToPackageCache.remove(uid); - } - } - }; - /** * The rolling window size for each standby bucket. Within each window, an app will have 10 * minutes to run its jobs. @@ -611,9 +592,6 @@ public final class QuotaController extends StateController { mBackgroundJobsController = backgroundJobsController; mConnectivityController = connectivityController; - final IntentFilter filter = new IntentFilter(Intent.ACTION_PACKAGE_ADDED); - mContext.registerReceiverAsUser(mPackageAddedReceiver, UserHandle.ALL, filter, null, null); - // Set up the app standby bucketing tracker AppStandbyInternal appStandby = LocalServices.getService(AppStandbyInternal.class); appStandby.addListener(new StandbyTracker()); @@ -730,13 +708,15 @@ public final class QuotaController extends StateController { return; } clearAppStatsLocked(UserHandle.getUserId(uid), packageName); - mForegroundUids.delete(uid); - mUidToPackageCache.remove(uid); - mTempAllowlistCache.delete(uid); - mTempAllowlistGraceCache.delete(uid); - mTopAppCache.delete(uid); - mTopAppTrackers.delete(uid); - mTopAppGraceCache.delete(uid); + if (mService.getPackagesForUidLocked(uid) == null) { + // All packages in the UID have been removed. It's safe to remove things based on + // UID alone. + mForegroundUids.delete(uid); + mTempAllowlistCache.delete(uid); + mTempAllowlistGraceCache.delete(uid); + mTopAppCache.delete(uid); + mTopAppGraceCache.delete(uid); + } } @Override @@ -754,8 +734,8 @@ public final class QuotaController extends StateController { mInQuotaAlarmListener.removeAlarmsLocked(userId); mExecutionStatsCache.delete(userId); mEJStats.delete(userId); - mUidToPackageCache.clear(); mSystemInstallers.remove(userId); + mTopAppTrackers.delete(userId); } /** Drop all historical stats and stop tracking any active sessions for the specified app. */ @@ -780,6 +760,7 @@ public final class QuotaController extends StateController { mInQuotaAlarmListener.removeAlarmLocked(userId, packageName); mExecutionStatsCache.delete(userId, packageName); mEJStats.delete(userId, packageName); + mTopAppTrackers.delete(userId, packageName); } private void cacheInstallerPackagesLocked(int userId) { @@ -2450,7 +2431,7 @@ public final class QuotaController extends StateController { synchronized (mLock) { final long nowElapsed = sElapsedRealtimeClock.millis(); mTempAllowlistCache.put(uid, true); - final ArraySet packages = getPackagesForUidLocked(uid); + final ArraySet packages = mService.getPackagesForUidLocked(uid); if (packages != null) { final int userId = UserHandle.getUserId(uid); for (int i = packages.size() - 1; i >= 0; --i) { @@ -2505,26 +2486,6 @@ public final class QuotaController extends StateController { // getRemainingEJExecutionTimeLocked(). } - @Nullable - private ArraySet getPackagesForUidLocked(final int uid) { - ArraySet packages = mUidToPackageCache.get(uid); - if (packages == null) { - try { - String[] pkgs = AppGlobals.getPackageManager() - .getPackagesForUid(uid); - if (pkgs != null) { - for (String pkg : pkgs) { - mUidToPackageCache.add(uid, pkg); - } - packages = mUidToPackageCache.get(uid); - } - } catch (RemoteException e) { - // Shouldn't happen. - } - } - return packages; - } - private class QcHandler extends Handler { QcHandler(Looper looper) { @@ -2655,7 +2616,8 @@ public final class QuotaController extends StateController { // Update Timers first. if (mPkgTimers.indexOfKey(userId) >= 0 || mEJPkgTimers.indexOfKey(userId) >= 0) { - final ArraySet packages = getPackagesForUidLocked(uid); + final ArraySet packages = + mService.getPackagesForUidLocked(uid); if (packages != null) { for (int i = packages.size() - 1; i >= 0; --i) { Timer t = mEJPkgTimers.get(userId, packages.valueAt(i)); @@ -2740,7 +2702,7 @@ public final class QuotaController extends StateController { } mTempAllowlistGraceCache.delete(uid); mTopAppGraceCache.delete(uid); - final ArraySet packages = getPackagesForUidLocked(uid); + final ArraySet packages = mService.getPackagesForUidLocked(uid); if (packages != null) { final int userId = UserHandle.getUserId(uid); for (int i = packages.size() - 1; i >= 0; --i) { @@ -4109,17 +4071,6 @@ public final class QuotaController extends StateController { pw.println(mTempAllowlistGraceCache.toString()); pw.println(); - pw.println("Cached UID->package map:"); - pw.increaseIndent(); - for (int i = 0; i < mUidToPackageCache.size(); ++i) { - final int uid = mUidToPackageCache.keyAt(i); - pw.print(uid); - pw.print(": "); - pw.println(mUidToPackageCache.get(uid)); - } - pw.decreaseIndent(); - pw.println(); - pw.println("Special apps:"); pw.increaseIndent(); pw.print("System installers", mSystemInstallers.toString()); @@ -4278,22 +4229,6 @@ public final class QuotaController extends StateController { mForegroundUids.keyAt(i)); } - for (int i = 0; i < mUidToPackageCache.size(); ++i) { - final long upToken = proto.start( - StateControllerProto.QuotaController.UID_TO_PACKAGE_CACHE); - - final int uid = mUidToPackageCache.keyAt(i); - ArraySet packages = mUidToPackageCache.get(uid); - - proto.write(StateControllerProto.QuotaController.UidPackageMapping.UID, uid); - for (int j = 0; j < packages.size(); ++j) { - proto.write(StateControllerProto.QuotaController.UidPackageMapping.PACKAGE_NAMES, - packages.valueAt(j)); - } - - proto.end(upToken); - } - mTrackedJobs.forEach((jobs) -> { for (int j = 0; j < jobs.size(); j++) { final JobStatus js = jobs.valueAt(j); diff --git a/services/tests/mockingservicestests/src/com/android/server/job/controllers/QuotaControllerTest.java b/services/tests/mockingservicestests/src/com/android/server/job/controllers/QuotaControllerTest.java index 4bab8e51874c7..64687908dc9b9 100644 --- a/services/tests/mockingservicestests/src/com/android/server/job/controllers/QuotaControllerTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/job/controllers/QuotaControllerTest.java @@ -81,6 +81,7 @@ import android.os.RemoteException; import android.os.SystemClock; import android.platform.test.annotations.LargeTest; import android.provider.DeviceConfig; +import android.util.ArraySet; import android.util.SparseBooleanArray; import androidx.test.runner.AndroidJUnit4; @@ -254,6 +255,9 @@ public class QuotaControllerTest { any()); mUidObserver = uidObserverCaptor.getValue(); mSourceUid = AppGlobals.getPackageManager().getPackageUid(SOURCE_PACKAGE, 0, 0); + // Need to do this since we're using a mock JS and not a real object. + doReturn(new ArraySet<>(new String[]{SOURCE_PACKAGE})) + .when(mJobSchedulerService).getPackagesForUidLocked(mSourceUid); } catch (RemoteException e) { fail(e.getMessage()); } @@ -1803,12 +1807,10 @@ public class QuotaControllerTest { setStandbyBucket(ACTIVE_INDEX, fgStateChanger); setProcessState(ActivityManager.PROCESS_STATE_BACKUP, fgChangerUid); - IPackageManager packageManager = AppGlobals.getPackageManager(); - spyOn(packageManager); - doReturn(new String[]{unaffectedPkgName}) - .when(packageManager).getPackagesForUid(unaffectedUid); - doReturn(new String[]{fgChangerPkgName}) - .when(packageManager).getPackagesForUid(fgChangerUid); + doReturn(new ArraySet<>(new String[]{unaffectedPkgName})) + .when(mJobSchedulerService).getPackagesForUidLocked(unaffectedUid); + doReturn(new ArraySet<>(new String[]{fgChangerPkgName})) + .when(mJobSchedulerService).getPackagesForUidLocked(fgChangerUid); synchronized (mQuotaController.mLock) { mQuotaController.maybeStartTrackingJobLocked(unaffected, null);