From d978d634942b14659d89bdad16c7c608055d9fdd 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 Merged-In: 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 06ff69176bb72..d1d1eb00b1148 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 PackageParser.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 5817a80ec6c10..a8b493d68a748 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -26366,7 +26366,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 67dd0556e0985..dc745cdc0c84f 100644 --- a/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/AppsFilterTest.java @@ -83,9 +83,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; @@ -318,6 +326,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 =