Merge "Make RoleUserState use its own lock."
diff --git a/services/core/java/com/android/server/role/RoleManagerService.java b/services/core/java/com/android/server/role/RoleManagerService.java
index 8ce3838..8711ddf 100644
--- a/services/core/java/com/android/server/role/RoleManagerService.java
+++ b/services/core/java/com/android/server/role/RoleManagerService.java
@@ -156,21 +156,16 @@
@MainThread
private void performInitialGrantsIfNecessary(@UserIdInt int userId) {
RoleUserState userState;
- synchronized (mLock) {
- userState = getUserStateLocked(userId);
- }
+ userState = getOrCreateUserState(userId);
String packagesHash = computeComponentStateHash(userId);
- String oldPackagesHash;
- synchronized (mLock) {
- oldPackagesHash = userState.getPackagesHashLocked();
- }
+ String oldPackagesHash = userState.getPackagesHash();
boolean needGrant = !Objects.equals(packagesHash, oldPackagesHash);
if (needGrant) {
// Some vital packages state has changed since last role grant
// Run grants again
Slog.i(LOG_TAG, "Granting default permissions...");
CompletableFuture<Void> result = new CompletableFuture<>();
- getControllerService(userId).onGrantDefaultRoles(
+ getOrCreateControllerService(userId).onGrantDefaultRoles(
new IRoleManagerCallback.Stub() {
@Override
public void onSuccess() {
@@ -183,9 +178,7 @@
});
try {
result.get(5, TimeUnit.SECONDS);
- synchronized (mLock) {
- userState.setPackagesHashLocked(packagesHash);
- }
+ userState.setPackagesHash(packagesHash);
} catch (InterruptedException | ExecutionException | TimeoutException e) {
Slog.e(LOG_TAG, "Failed to grant defaults for user " + userId, e);
}
@@ -225,35 +218,38 @@
return PackageUtils.computeSha256Digest(out.toByteArray());
}
- @GuardedBy("mLock")
@NonNull
- private RoleUserState getUserStateLocked(@UserIdInt int userId) {
- RoleUserState userState = mUserStates.get(userId);
- if (userState == null) {
- userState = RoleUserState.newInstanceLocked(userId);
- mUserStates.put(userId, userState);
+ private RoleUserState getOrCreateUserState(@UserIdInt int userId) {
+ synchronized (mLock) {
+ RoleUserState userState = mUserStates.get(userId);
+ if (userState == null) {
+ userState = new RoleUserState(userId);
+ mUserStates.put(userId, userState);
+ }
+ return userState;
}
- return userState;
}
- @GuardedBy("mLock")
@NonNull
- private RemoteRoleControllerService getControllerService(@UserIdInt int userId) {
- RemoteRoleControllerService controllerService = mControllerServices.get(userId);
- if (controllerService == null) {
- controllerService = new RemoteRoleControllerService(userId, getContext());
- mControllerServices.put(userId, controllerService);
+ private RemoteRoleControllerService getOrCreateControllerService(@UserIdInt int userId) {
+ synchronized (mLock) {
+ RemoteRoleControllerService controllerService = mControllerServices.get(userId);
+ if (controllerService == null) {
+ controllerService = new RemoteRoleControllerService(userId, getContext());
+ mControllerServices.put(userId, controllerService);
+ }
+ return controllerService;
}
- return controllerService;
}
private void onRemoveUser(@UserIdInt int userId) {
+ RoleUserState userState;
synchronized (mLock) {
mControllerServices.remove(userId);
- RoleUserState userState = mUserStates.removeReturnOld(userId);
- if (userState != null) {
- userState.destroyLocked();
- }
+ userState = mUserStates.removeReturnOld(userId);
+ }
+ if (userState != null) {
+ userState.destroy();
}
}
@@ -264,10 +260,8 @@
Preconditions.checkStringNotEmpty(roleName, "roleName cannot be null or empty");
int userId = UserHandle.getUserId(getCallingUid());
- synchronized (mLock) {
- RoleUserState userState = getUserStateLocked(userId);
- return userState.isRoleAvailableLocked(roleName);
- }
+ RoleUserState userState = getOrCreateUserState(userId);
+ return userState.isRoleAvailable(roleName);
}
@Override
@@ -307,10 +301,8 @@
@Nullable
private ArraySet<String> getRoleHoldersInternal(@NonNull String roleName,
@UserIdInt int userId) {
- synchronized (mLock) {
- RoleUserState userState = getUserStateLocked(userId);
- return userState.getRoleHoldersLocked(roleName);
- }
+ RoleUserState userState = getOrCreateUserState(userId);
+ return userState.getRoleHolders(roleName);
}
@Override
@@ -327,7 +319,7 @@
getContext().enforceCallingOrSelfPermission(Manifest.permission.MANAGE_ROLE_HOLDERS,
"addRoleHolderAsUser");
- getControllerService(userId).onAddRoleHolder(roleName, packageName, callback);
+ getOrCreateControllerService(userId).onAddRoleHolder(roleName, packageName, callback);
}
@Override
@@ -344,7 +336,7 @@
getContext().enforceCallingOrSelfPermission(Manifest.permission.MANAGE_ROLE_HOLDERS,
"removeRoleHolderAsUser");
- getControllerService(userId).onRemoveRoleHolder(roleName, packageName,
+ getOrCreateControllerService(userId).onRemoveRoleHolder(roleName, packageName,
callback);
}
@@ -361,7 +353,7 @@
getContext().enforceCallingOrSelfPermission(Manifest.permission.MANAGE_ROLE_HOLDERS,
"clearRoleHoldersAsUser");
- getControllerService(userId).onClearRoleHolders(roleName, callback);
+ getOrCreateControllerService(userId).onClearRoleHolders(roleName, callback);
}
@Override
@@ -372,10 +364,8 @@
"setRoleNamesFromController");
int userId = UserHandle.getCallingUserId();
- synchronized (mLock) {
- RoleUserState userState = getUserStateLocked(userId);
- userState.setRoleNamesLocked(roleNames);
- }
+ RoleUserState userState = getOrCreateUserState(userId);
+ userState.setRoleNames(roleNames);
}
@Override
@@ -388,10 +378,8 @@
"addRoleHolderFromController");
int userId = UserHandle.getCallingUserId();
- synchronized (mLock) {
- RoleUserState userState = getUserStateLocked(userId);
- return userState.addRoleHolderLocked(roleName, packageName);
- }
+ RoleUserState userState = getOrCreateUserState(userId);
+ return userState.addRoleHolder(roleName, packageName);
}
@Override
@@ -404,10 +392,8 @@
"removeRoleHolderFromController");
int userId = UserHandle.getCallingUserId();
- synchronized (mLock) {
- RoleUserState userState = getUserStateLocked(userId);
- return userState.removeRoleHolderLocked(roleName, packageName);
- }
+ RoleUserState userState = getOrCreateUserState(userId);
+ return userState.removeRoleHolder(roleName, packageName);
}
@CheckResult
@@ -440,16 +426,14 @@
dumpOutputStream = new DualDumpOutputStream(new IndentingPrintWriter(fout, " "));
}
- synchronized (mLock) {
- int[] userIds = mUserManagerInternal.getUserIds();
- int userIdsLength = userIds.length;
- for (int i = 0; i < userIdsLength; i++) {
- int userId = userIds[i];
+ int[] userIds = mUserManagerInternal.getUserIds();
+ int userIdsLength = userIds.length;
+ for (int i = 0; i < userIdsLength; i++) {
+ int userId = userIds[i];
- RoleUserState userState = getUserStateLocked(userId);
- userState.dumpLocked(dumpOutputStream, "user_states",
- RoleManagerServiceDumpProto.USER_STATES);
- }
+ RoleUserState userState = getOrCreateUserState(userId);
+ userState.dump(dumpOutputStream, "user_states",
+ RoleManagerServiceDumpProto.USER_STATES);
}
dumpOutputStream.flush();
diff --git a/services/core/java/com/android/server/role/RoleUserState.java b/services/core/java/com/android/server/role/RoleUserState.java
index 327debf..81adb11 100644
--- a/services/core/java/com/android/server/role/RoleUserState.java
+++ b/services/core/java/com/android/server/role/RoleUserState.java
@@ -74,54 +74,51 @@
@UserIdInt
private final int mUserId;
- @GuardedBy("RoleManagerService.mLock")
+ @NonNull
+ private final Object mLock = new Object();
+
+ @GuardedBy("mLock")
private int mVersion = VERSION_UNDEFINED;
- @GuardedBy("RoleManagerService.mLock")
+ @GuardedBy("mLock")
@Nullable
private String mPackagesHash;
/**
* Maps role names to its holders' package names. The values should never be null.
*/
- @GuardedBy("RoleManagerService.mLock")
+ @GuardedBy("mLock")
@NonNull
private ArrayMap<String, ArraySet<String>> mRoles = new ArrayMap<>();
- @GuardedBy("RoleManagerService.mLock")
+ @GuardedBy("mLock")
private long mWritePendingSinceMillis;
- @GuardedBy("RoleManagerService.mLock")
+ @GuardedBy("mLock")
private boolean mDestroyed;
@NonNull
private final Handler mWriteHandler = new Handler(BackgroundThread.getHandler().getLooper());
- private RoleUserState(@UserIdInt int userId) {
- mUserId = userId;
-
- readLocked();
- }
-
/**
* Create a new instance of user state, and read its state from disk if previously persisted.
*
* @param userId the user id for the new user state
- *
- * @return the new user state
*/
- @GuardedBy("RoleManagerService.mLock")
- public static RoleUserState newInstanceLocked(@UserIdInt int userId) {
- return new RoleUserState(userId);
+ public RoleUserState(@UserIdInt int userId) {
+ mUserId = userId;
+
+ readFile();
}
/**
* Get the version of this user state.
*/
- @GuardedBy("RoleManagerService.mLock")
- public int getVersionLocked() {
- throwIfDestroyedLocked();
- return mVersion;
+ public int getVersion() {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ return mVersion;
+ }
}
/**
@@ -129,14 +126,15 @@
*
* @param version the version to set
*/
- @GuardedBy("RoleManagerService.mLock")
- public void setVersionLocked(int version) {
- throwIfDestroyedLocked();
- if (mVersion == version) {
- return;
+ public void setVersion(int version) {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ if (mVersion == version) {
+ return;
+ }
+ mVersion = version;
+ scheduleWriteFileLocked();
}
- mVersion = version;
- writeAsyncLocked();
}
/**
@@ -144,9 +142,11 @@
*
* @return the hash representing the state of packages
*/
- @GuardedBy("RoleManagerService.mLock")
- public String getPackagesHashLocked() {
- return mPackagesHash;
+ @Nullable
+ public String getPackagesHash() {
+ synchronized (mLock) {
+ return mPackagesHash;
+ }
}
/**
@@ -154,14 +154,15 @@
*
* @param packagesHash the hash representing the state of packages
*/
- @GuardedBy("RoleManagerService.mLock")
- public void setPackagesHashLocked(@Nullable String packagesHash) {
- throwIfDestroyedLocked();
- if (Objects.equals(mPackagesHash, packagesHash)) {
- return;
+ public void setPackagesHash(@Nullable String packagesHash) {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ if (Objects.equals(mPackagesHash, packagesHash)) {
+ return;
+ }
+ mPackagesHash = packagesHash;
+ scheduleWriteFileLocked();
}
- mPackagesHash = packagesHash;
- writeAsyncLocked();
}
/**
@@ -171,10 +172,11 @@
*
* @return whether the role is available
*/
- @GuardedBy("RoleManagerService.mLock")
- public boolean isRoleAvailableLocked(@NonNull String roleName) {
- throwIfDestroyedLocked();
- return mRoles.containsKey(roleName);
+ public boolean isRoleAvailable(@NonNull String roleName) {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ return mRoles.containsKey(roleName);
+ }
}
/**
@@ -184,11 +186,12 @@
*
* @return the set of role holders. {@code null} should not be returned and indicates an issue.
*/
- @GuardedBy("RoleManagerService.mLock")
@Nullable
- public ArraySet<String> getRoleHoldersLocked(@NonNull String roleName) {
- throwIfDestroyedLocked();
- return mRoles.get(roleName);
+ public ArraySet<String> getRoleHolders(@NonNull String roleName) {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ return new ArraySet<>(mRoles.get(roleName));
+ }
}
/**
@@ -196,33 +199,35 @@
*
* @param roleNames the names of all the available roles
*/
- @GuardedBy("RoleManagerService.mLock")
- public void setRoleNamesLocked(@NonNull List<String> roleNames) {
- throwIfDestroyedLocked();
- boolean changed = false;
- for (int i = mRoles.size() - 1; i >= 0; i--) {
- String roleName = mRoles.keyAt(i);
- if (!roleNames.contains(roleName)) {
- ArraySet<String> packageNames = mRoles.valueAt(i);
- if (!packageNames.isEmpty()) {
- Slog.e(LOG_TAG, "Holders of a removed role should have been cleaned up, role: "
- + roleName + ", holders: " + packageNames);
+ public void setRoleNames(@NonNull List<String> roleNames) {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ boolean changed = false;
+ for (int i = mRoles.size() - 1; i >= 0; i--) {
+ String roleName = mRoles.keyAt(i);
+ if (!roleNames.contains(roleName)) {
+ ArraySet<String> packageNames = mRoles.valueAt(i);
+ if (!packageNames.isEmpty()) {
+ Slog.e(LOG_TAG,
+ "Holders of a removed role should have been cleaned up, role: "
+ + roleName + ", holders: " + packageNames);
+ }
+ mRoles.removeAt(i);
+ changed = true;
}
- mRoles.removeAt(i);
- changed = true;
}
- }
- int roleNamesSize = roleNames.size();
- for (int i = 0; i < roleNamesSize; i++) {
- String roleName = roleNames.get(i);
- if (!mRoles.containsKey(roleName)) {
- mRoles.put(roleName, new ArraySet<>());
- Slog.i(LOG_TAG, "Added new role: " + roleName);
- changed = true;
+ int roleNamesSize = roleNames.size();
+ for (int i = 0; i < roleNamesSize; i++) {
+ String roleName = roleNames.get(i);
+ if (!mRoles.containsKey(roleName)) {
+ mRoles.put(roleName, new ArraySet<>());
+ Slog.i(LOG_TAG, "Added new role: " + roleName);
+ changed = true;
+ }
}
- }
- if (changed) {
- writeAsyncLocked();
+ if (changed) {
+ scheduleWriteFileLocked();
+ }
}
}
@@ -236,20 +241,21 @@
* indicates an issue.
*/
@CheckResult
- @GuardedBy("RoleManagerService.mLock")
- public boolean addRoleHolderLocked(@NonNull String roleName, @NonNull String packageName) {
- throwIfDestroyedLocked();
- ArraySet<String> roleHolders = mRoles.get(roleName);
- if (roleHolders == null) {
- Slog.e(LOG_TAG, "Cannot add role holder for unknown role, role: " + roleName
- + ", package: " + packageName);
- return false;
+ public boolean addRoleHolder(@NonNull String roleName, @NonNull String packageName) {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ ArraySet<String> roleHolders = mRoles.get(roleName);
+ if (roleHolders == null) {
+ Slog.e(LOG_TAG, "Cannot add role holder for unknown role, role: " + roleName
+ + ", package: " + packageName);
+ return false;
+ }
+ boolean changed = roleHolders.add(packageName);
+ if (changed) {
+ scheduleWriteFileLocked();
+ }
+ return true;
}
- boolean changed = roleHolders.add(packageName);
- if (changed) {
- writeAsyncLocked();
- }
- return true;
}
/**
@@ -262,38 +268,30 @@
* indicates an issue.
*/
@CheckResult
- @GuardedBy("RoleManagerService.mLock")
- public boolean removeRoleHolderLocked(@NonNull String roleName, @NonNull String packageName) {
- throwIfDestroyedLocked();
- ArraySet<String> roleHolders = mRoles.get(roleName);
- if (roleHolders == null) {
- Slog.e(LOG_TAG, "Cannot remove role holder for unknown role, role: " + roleName
- + ", package: " + packageName);
- return false;
+ public boolean removeRoleHolder(@NonNull String roleName, @NonNull String packageName) {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ ArraySet<String> roleHolders = mRoles.get(roleName);
+ if (roleHolders == null) {
+ Slog.e(LOG_TAG, "Cannot remove role holder for unknown role, role: " + roleName
+ + ", package: " + packageName);
+ return false;
+ }
+ boolean changed = roleHolders.remove(packageName);
+ if (changed) {
+ scheduleWriteFileLocked();
+ }
+ return true;
}
- boolean changed = roleHolders.remove(packageName);
- if (changed) {
- writeAsyncLocked();
- }
- return true;
}
/**
* Schedule writing the state to file.
*/
- @GuardedBy("RoleManagerService.mLock")
- private void writeAsyncLocked() {
+ @GuardedBy("mLock")
+ private void scheduleWriteFileLocked() {
throwIfDestroyedLocked();
- ArrayMap<String, ArraySet<String>> roles = new ArrayMap<>();
- for (int i = 0, size = CollectionUtils.size(mRoles); i < size; ++i) {
- String roleName = mRoles.keyAt(i);
- ArraySet<String> roleHolders = mRoles.valueAt(i);
-
- roleHolders = new ArraySet<>(roleHolders);
- roles.put(roleName, roleHolders);
- }
-
long currentTimeMillis = System.currentTimeMillis();
long writeDelayMillis;
if (!mWriteHandler.hasMessagesOrCallbacks()) {
@@ -311,14 +309,26 @@
}
}
- mWriteHandler.sendMessageDelayed(PooledLambda.obtainMessage(RoleUserState::writeSync, this,
- mVersion, mPackagesHash, roles), writeDelayMillis);
+ mWriteHandler.sendMessageDelayed(PooledLambda.obtainMessage(RoleUserState::writeFile, this),
+ writeDelayMillis);
Slog.i(LOG_TAG, "Scheduled writing roles.xml");
}
@WorkerThread
- private void writeSync(int version, @Nullable String packagesHash,
- @NonNull ArrayMap<String, ArraySet<String>> roles) {
+ private void writeFile() {
+ int version;
+ String packagesHash;
+ ArrayMap<String, ArraySet<String>> roles;
+ synchronized (mLock) {
+ if (mDestroyed) {
+ return;
+ }
+
+ version = mVersion;
+ packagesHash = mPackagesHash;
+ roles = snapshotRolesLocked();
+ }
+
AtomicFile atomicFile = new AtomicFile(getFile(mUserId), "roles-" + mUserId);
FileOutputStream out = null;
try {
@@ -385,18 +395,19 @@
/**
* Read the state from file.
*/
- @GuardedBy("RoleManagerService.mLock")
- private void readLocked() {
- File file = getFile(mUserId);
- try (FileInputStream in = new AtomicFile(file).openRead()) {
- XmlPullParser parser = Xml.newPullParser();
- parser.setInput(in, null);
- parseXmlLocked(parser);
- Slog.i(LOG_TAG, "Read roles.xml successfully");
- } catch (FileNotFoundException e) {
- Slog.i(LOG_TAG, "roles.xml not found");
- } catch (XmlPullParserException | IOException e) {
- throw new IllegalStateException("Failed to parse roles.xml: " + file, e);
+ private void readFile() {
+ synchronized (mLock) {
+ File file = getFile(mUserId);
+ try (FileInputStream in = new AtomicFile(file).openRead()) {
+ XmlPullParser parser = Xml.newPullParser();
+ parser.setInput(in, null);
+ parseXmlLocked(parser);
+ Slog.i(LOG_TAG, "Read roles.xml successfully");
+ } catch (FileNotFoundException e) {
+ Slog.i(LOG_TAG, "roles.xml not found");
+ } catch (XmlPullParserException | IOException e) {
+ throw new IllegalStateException("Failed to parse roles.xml: " + file, e);
+ }
}
}
@@ -470,20 +481,28 @@
*
* @param dumpOutputStream the output stream to dump to
*/
- @GuardedBy("RoleManagerService.mLock")
- public void dumpLocked(@NonNull DualDumpOutputStream dumpOutputStream,
- @NonNull String fieldName, long fieldId) {
- throwIfDestroyedLocked();
+ public void dump(@NonNull DualDumpOutputStream dumpOutputStream, @NonNull String fieldName,
+ long fieldId) {
+ int version;
+ String packagesHash;
+ ArrayMap<String, ArraySet<String>> roles;
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+
+ version = mVersion;
+ packagesHash = mPackagesHash;
+ roles = snapshotRolesLocked();
+ }
long fieldToken = dumpOutputStream.start(fieldName, fieldId);
dumpOutputStream.write("user_id", RoleUserStateProto.USER_ID, mUserId);
- dumpOutputStream.write("version", RoleUserStateProto.VERSION, mVersion);
- dumpOutputStream.write("packages_hash", RoleUserStateProto.PACKAGES_HASH, mPackagesHash);
+ dumpOutputStream.write("version", RoleUserStateProto.VERSION, version);
+ dumpOutputStream.write("packages_hash", RoleUserStateProto.PACKAGES_HASH, packagesHash);
- int rolesSize = mRoles.size();
+ int rolesSize = roles.size();
for (int rolesIndex = 0; rolesIndex < rolesSize; rolesIndex++) {
- String roleName = mRoles.keyAt(rolesIndex);
- ArraySet<String> roleHolders = mRoles.valueAt(rolesIndex);
+ String roleName = roles.keyAt(rolesIndex);
+ ArraySet<String> roleHolders = roles.valueAt(rolesIndex);
long rolesToken = dumpOutputStream.start("roles", RoleUserStateProto.ROLES);
dumpOutputStream.write("name", RoleProto.NAME, roleName);
@@ -501,19 +520,33 @@
dumpOutputStream.end(fieldToken);
}
+ @GuardedBy("mLock")
+ private ArrayMap<String, ArraySet<String>> snapshotRolesLocked() {
+ ArrayMap<String, ArraySet<String>> roles = new ArrayMap<>();
+ for (int i = 0, size = CollectionUtils.size(mRoles); i < size; ++i) {
+ String roleName = mRoles.keyAt(i);
+ ArraySet<String> roleHolders = mRoles.valueAt(i);
+
+ roleHolders = new ArraySet<>(roleHolders);
+ roles.put(roleName, roleHolders);
+ }
+ return roles;
+ }
+
/**
* Destroy this state and delete the corresponding file. Any pending writes to the file will be
* cancelled and any future interaction with this state will throw an exception.
*/
- @GuardedBy("RoleManagerService.mLock")
- public void destroyLocked() {
- throwIfDestroyedLocked();
- mWriteHandler.removeCallbacksAndMessages(null);
- getFile(mUserId).delete();
- mDestroyed = true;
+ public void destroy() {
+ synchronized (mLock) {
+ throwIfDestroyedLocked();
+ mWriteHandler.removeCallbacksAndMessages(null);
+ getFile(mUserId).delete();
+ mDestroyed = true;
+ }
}
- @GuardedBy("RoleManagerService.mLock")
+ @GuardedBy("mLock")
private void throwIfDestroyedLocked() {
if (mDestroyed) {
throw new IllegalStateException("This RoleUserState has already been destroyed");