From cc44c874cf9945d0190e4b1ceacdc77c103708cf Mon Sep 17 00:00:00 2001 From: Patrick Baumann Date: Wed, 9 Jun 2021 12:48:49 -0700 Subject: [PATCH] Only update cache for new user on user create This change optimizes our handling of user additions to only compute the cache for the newly added user and its interactions with existing users. Prior to this change, we'd recompute the entire cache on user add. Test: atest AppsFilterTest AppEnumerationTests Fixes: 187853334 Change-Id: I09cb9f98b34d86ae678373d3f61dd5fdd334cfb7 --- .../com/android/server/pm/AppsFilter.java | 94 +++++++++++++------ .../server/pm/PackageManagerService.java | 2 +- .../com/android/server/pm/AppsFilterTest.java | 53 ++++++++++- 3 files changed, 118 insertions(+), 31 deletions(-) diff --git a/services/core/java/com/android/server/pm/AppsFilter.java b/services/core/java/com/android/server/pm/AppsFilter.java index d4af254f6d9a4..c78dc9c94a604 100644 --- a/services/core/java/com/android/server/pm/AppsFilter.java +++ b/services/core/java/com/android/server/pm/AppsFilter.java @@ -17,6 +17,7 @@ package com.android.server.pm; import static android.os.Trace.TRACE_TAG_PACKAGE_MANAGER; +import static android.os.UserHandle.USER_ALL; import static android.provider.DeviceConfig.NAMESPACE_PACKAGE_MANAGER_SERVICE; import static com.android.internal.annotations.VisibleForTesting.Visibility.PRIVATE; @@ -685,7 +686,7 @@ public class AppsFilter implements Watchable, Snappable { synchronized (mCacheLock) { if (mShouldFilterCache != null) { updateShouldFilterCacheForPackage(mShouldFilterCache, null, newPkgSetting, - settings, users, settings.size()); + settings, users, USER_ALL, settings.size()); if (additionalChangedPackages != null) { for (int index = 0; index < additionalChangedPackages.size(); index++) { String changedPackage = additionalChangedPackages.valueAt(index); @@ -698,7 +699,8 @@ public class AppsFilter implements Watchable, Snappable { } updateShouldFilterCacheForPackage(mShouldFilterCache, null, - changedPkgSetting, settings, users, settings.size()); + changedPkgSetting, settings, users, USER_ALL, + settings.size()); } } } // else, rebuild entire cache when system is ready @@ -830,24 +832,51 @@ public class AppsFilter implements Watchable, Snappable { } } } - private void updateEntireShouldFilterCache() { + updateEntireShouldFilterCache(USER_ALL); + } + + private void updateEntireShouldFilterCache(int subjectUserId) { mStateProvider.runWithState((settings, users) -> { + int userId = subjectUserId; + if (!ArrayUtils.contains(users, subjectUserId)) { + Slog.e(TAG, "We encountered a new user that isn't a member of known users, " + + "updating the whole cache"); + userId = USER_ALL; + } WatchedSparseBooleanMatrix cache = - updateEntireShouldFilterCacheInner(settings, users); + updateEntireShouldFilterCacheInner(settings, users, userId); synchronized (mCacheLock) { + if (userId != USER_ALL) { + // if we're only updating a single user id, we need to copy over the prior + // cached values for the other users. + int[] uids = mShouldFilterCache.keys(); + for (int i = 0; i < uids.length; i++) { + int uid1 = uids[i]; + if (UserHandle.getUserId(uid1) == userId) { + continue; + } + for (int j = 0; j < uids.length; j++) { + int uid2 = uids[j]; + if (UserHandle.getUserId(uid2) == userId) { + continue; + } + cache.setValueAt(uid1, uid2, mShouldFilterCache.valueAt(uid1, uid2)); + } + } + } mShouldFilterCache = cache; } }); } private WatchedSparseBooleanMatrix updateEntireShouldFilterCacheInner( - ArrayMap settings, UserInfo[] users) { + ArrayMap settings, UserInfo[] users, int subjectUserId) { WatchedSparseBooleanMatrix cache = new WatchedSparseBooleanMatrix(users.length * settings.size()); for (int i = settings.size() - 1; i >= 0; i--) { updateShouldFilterCacheForPackage(cache, - null /*skipPackage*/, settings.valueAt(i), settings, users, i); + null /*skipPackage*/, settings.valueAt(i), settings, users, subjectUserId, i); } return cache; } @@ -868,8 +897,8 @@ public class AppsFilter implements Watchable, Snappable { packagesCache.put(settings.keyAt(i), pkg); } }); - WatchedSparseBooleanMatrix cache = - updateEntireShouldFilterCacheInner(settingsCopy, usersRef[0]); + WatchedSparseBooleanMatrix cache = updateEntireShouldFilterCacheInner( + settingsCopy, usersRef[0], USER_ALL); boolean[] changed = new boolean[1]; // We have a cache, let's make sure the world hasn't changed out from under us. mStateProvider.runWithState((settings, users) -> { @@ -899,10 +928,10 @@ public class AppsFilter implements Watchable, Snappable { }); } - public void onUsersChanged() { + public void onUserCreated(int newUserId) { synchronized (mCacheLock) { if (mShouldFilterCache != null) { - updateEntireShouldFilterCache(); + updateEntireShouldFilterCache(newUserId); onChanged(); } } @@ -913,7 +942,7 @@ public class AppsFilter implements Watchable, Snappable { if (mShouldFilterCache != null) { mStateProvider.runWithState((settings, users) -> { updateShouldFilterCacheForPackage(mShouldFilterCache, null /* skipPackage */, - settings.get(packageName), settings, users, + settings.get(packageName), settings, users, USER_ALL, settings.size() /*maxIndex*/); }); } @@ -922,7 +951,7 @@ public class AppsFilter implements Watchable, Snappable { private void updateShouldFilterCacheForPackage(WatchedSparseBooleanMatrix cache, @Nullable String skipPackageName, PackageSetting subjectSetting, ArrayMap allSettings, UserInfo[] allUsers, int maxIndex) { + PackageSetting> allSettings, UserInfo[] allUsers, int subjectUserId, int maxIndex) { for (int i = Math.min(maxIndex, allSettings.size() - 1); i >= 0; i--) { PackageSetting otherSetting = allSettings.valueAt(i); if (subjectSetting.appId == otherSetting.appId) { @@ -932,25 +961,34 @@ public class AppsFilter implements Watchable, Snappable { if (subjectSetting.name == skipPackageName || otherSetting.name == skipPackageName) { continue; } - final int userCount = allUsers.length; - final int appxUidCount = userCount * allSettings.size(); - for (int su = 0; su < userCount; su++) { - int subjectUser = allUsers[su].id; - for (int ou = 0; ou < userCount; ou++) { - int otherUser = allUsers[ou].id; - int subjectUid = UserHandle.getUid(subjectUser, subjectSetting.appId); - int otherUid = UserHandle.getUid(otherUser, otherSetting.appId); - cache.put(subjectUid, otherUid, - shouldFilterApplicationInternal( - subjectUid, subjectSetting, otherSetting, otherUser)); - cache.put(otherUid, subjectUid, - shouldFilterApplicationInternal( - otherUid, otherSetting, subjectSetting, subjectUser)); + if (subjectUserId == USER_ALL) { + for (int su = 0; su < allUsers.length; su++) { + updateShouldFilterCacheForUser(cache, subjectSetting, allUsers, otherSetting, + allUsers[su].id); } + } else { + updateShouldFilterCacheForUser(cache, subjectSetting, allUsers, otherSetting, + subjectUserId); } } } + private void updateShouldFilterCacheForUser(WatchedSparseBooleanMatrix cache, + PackageSetting subjectSetting, UserInfo[] allUsers, PackageSetting otherSetting, + int subjectUserId) { + for (int ou = 0; ou < allUsers.length; ou++) { + int otherUser = allUsers[ou].id; + int subjectUid = UserHandle.getUid(subjectUserId, subjectSetting.appId); + int otherUid = UserHandle.getUid(otherUser, otherSetting.appId); + cache.put(subjectUid, otherUid, + shouldFilterApplicationInternal( + subjectUid, subjectSetting, otherSetting, otherUser)); + cache.put(otherUid, subjectUid, + shouldFilterApplicationInternal( + otherUid, otherSetting, subjectSetting, subjectUserId)); + } + } + private static boolean isSystemSigned(@NonNull SigningDetails sysSigningDetails, PackageSetting pkgSetting) { return pkgSetting.isSystem() @@ -1145,7 +1183,7 @@ public class AppsFilter implements Watchable, Snappable { continue; } updateShouldFilterCacheForPackage(mShouldFilterCache, setting.name, - siblingSetting, settings, users, settings.size()); + siblingSetting, settings, users, USER_ALL, settings.size()); } } @@ -1162,7 +1200,7 @@ public class AppsFilter implements Watchable, Snappable { } updateShouldFilterCacheForPackage(mShouldFilterCache, null, - changedPkgSetting, settings, users, settings.size()); + changedPkgSetting, settings, users, USER_ALL, settings.size()); } } } diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index c350a747c69b0..02fb3e28c43a9 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -26300,7 +26300,7 @@ public class PackageManagerService extends IPackageManager.Stub synchronized (mLock) { scheduleWritePackageRestrictionsLocked(userId); scheduleWritePackageListLocked(userId); - mAppsFilter.onUsersChanged(); + mAppsFilter.onUserCreated(userId); } } diff --git a/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java b/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java index be942d6738af6..e7171ff1deff8 100644 --- a/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java @@ -82,9 +82,17 @@ public class AppsFilterTest { private static final int DUMMY_OVERLAY_APPID = 10756; private static final int SYSTEM_USER = 0; private static final int SECONDARY_USER = 10; + private static final int ADDED_USER = 11; private static final int[] USER_ARRAY = {SYSTEM_USER, SECONDARY_USER}; - private static final UserInfo[] USER_INFO_LIST = Arrays.stream(USER_ARRAY).mapToObj( - id -> new UserInfo(id, Integer.toString(id), 0)).toArray(UserInfo[]::new); + private static final int[] USER_ARRAY_WITH_ADDED = {SYSTEM_USER, SECONDARY_USER, ADDED_USER}; + private static final UserInfo[] USER_INFO_LIST = toUserInfos(USER_ARRAY); + private static final UserInfo[] USER_INFO_LIST_WITH_ADDED = toUserInfos(USER_ARRAY_WITH_ADDED); + + private static UserInfo[] toUserInfos(int[] userIds) { + return Arrays.stream(userIds) + .mapToObj(id -> new UserInfo(id, Integer.toString(id), 0)) + .toArray(UserInfo[]::new); + } @Mock AppsFilter.FeatureConfig mFeatureConfigMock; @@ -317,6 +325,47 @@ public class AppsFilterTest { watcher.verifyNoChangeReported("shouldFilterApplication"); } + @Test + public void testOnUserCreated_FilterMatches() throws Exception { + final AppsFilter appsFilter = + new AppsFilter(mStateProvider, mFeatureConfigMock, new String[]{}, false, null, + mMockExecutor); + simulateAddBasicAndroid(appsFilter); + + appsFilter.onSystemReady(); + + PackageSetting target = simulateAddPackage(appsFilter, + pkgWithProvider("com.some.package", "com.some.authority"), DUMMY_TARGET_APPID); + PackageSetting calling = simulateAddPackage(appsFilter, + pkgQueriesProvider("com.some.other.package", "com.some.authority"), + DUMMY_CALLING_APPID); + + for (int subjectUserId : USER_ARRAY) { + for (int otherUserId : USER_ARRAY) { + assertFalse(appsFilter.shouldFilterApplication( + UserHandle.getUid(DUMMY_CALLING_APPID, subjectUserId), calling, target, + otherUserId)); + } + } + + // adds new user + doAnswer(invocation -> { + ((AppsFilter.StateProvider.CurrentStateCallback) invocation.getArgument(0)) + .currentState(mExisting, USER_INFO_LIST_WITH_ADDED); + return new Object(); + }).when(mStateProvider) + .runWithState(any(AppsFilter.StateProvider.CurrentStateCallback.class)); + appsFilter.onUserCreated(ADDED_USER); + + for (int subjectUserId : USER_ARRAY_WITH_ADDED) { + for (int otherUserId : USER_ARRAY_WITH_ADDED) { + assertFalse(appsFilter.shouldFilterApplication( + UserHandle.getUid(DUMMY_CALLING_APPID, subjectUserId), calling, target, + otherUserId)); + } + } + } + @Test public void testQueriesDifferentProvider_Filters() throws Exception { final AppsFilter appsFilter =