Fix two shortcut manager issues
- isUserUnlocked check is still racy
We used a local copy of each user state in mUnlockedUsers, and updated
it in the service lifecycle events. However because
SystemService.onUnlockUser() is called on Handler, there was a brief
window between when the user was actually unlocked and the shortcut
manager thought the user was unlocked.
So now check with activity manager for the latest state too. We still
check with the local copy first, because we want to consider STOPPING
as "unlocked".
- Messenger loses all bitmap icons.
Because we delay-save the shortcuts.xml file, if the device shut down
before we save the XML file but after removing the bitmap files,
we'd lose the bitmaps.
(Apparently SystemService.onCleanupUser() may not be called even when
a device is cleanly shutting down.)
So don't remove bitmap files synchronously, ever, and instead after
saving the XML just run the dangling file cleanup.
Bug 30784267
Bug 30730471
Change-Id: Ie58656efba2dca2b00582e145613bc56266a091e
diff --git a/services/core/java/com/android/server/pm/ShortcutService.java b/services/core/java/com/android/server/pm/ShortcutService.java
index d5767b4..c1fc7f11 100644
--- a/services/core/java/com/android/server/pm/ShortcutService.java
+++ b/services/core/java/com/android/server/pm/ShortcutService.java
@@ -878,6 +878,9 @@
saveUserInternalLocked(userId, os, /* forBackup= */ false);
file.finishWrite(os);
+
+ // Remove all dangling bitmap files.
+ cleanupDanglingBitmapDirectoriesLocked(userId);
} catch (XmlPullParserException | IOException e) {
Slog.e(TAG, "Failed to write to file " + file.getBaseFile(), e);
file.failWrite(os);
@@ -929,7 +932,6 @@
}
try {
final ShortcutUser ret = loadUserInternal(userId, in, /* forBackup= */ false);
- cleanupDanglingBitmapDirectoriesLocked(userId, ret);
return ret;
} catch (IOException | XmlPullParserException e) {
Slog.e(TAG, "Failed to read file " + file.getBaseFile(), e);
@@ -1062,9 +1064,22 @@
}
}
- // Requires mLock held, but "Locked" prefix would look weired so we jsut say "L".
+ // Requires mLock held, but "Locked" prefix would look weired so we just say "L".
protected boolean isUserUnlockedL(@UserIdInt int userId) {
- return mUnlockedUsers.get(userId);
+ // First, check the local copy.
+ if (mUnlockedUsers.get(userId)) {
+ return true;
+ }
+ // If the local copy says the user is locked, check with AM for the actual state, since
+ // the user might just have been unlocked.
+ // Note we just don't use isUserUnlockingOrUnlocked() here, because it'll return false
+ // when the user is STOPPING, which we still want to consider as "unlocked".
+ final long token = injectClearCallingIdentity();
+ try {
+ return mUserManager.isUserUnlockingOrUnlocked(userId);
+ } finally {
+ injectRestoreCallingIdentity(token);
+ }
}
// Requires mLock held, but "Locked" prefix would look weired so we jsut say "L".
@@ -1125,14 +1140,8 @@
// === Caller validation ===
void removeIcon(@UserIdInt int userId, ShortcutInfo shortcut) {
- if (shortcut.getBitmapPath() != null) {
- if (DEBUG) {
- Slog.d(TAG, "Removing " + shortcut.getBitmapPath());
- }
- new File(shortcut.getBitmapPath()).delete();
-
- shortcut.setBitmapPath(null);
- }
+ // Do not remove the actual bitmap file yet, because if the device crashes before saving
+ // he XML we'd lose the icon. We just remove all dangling files after saving the XML.
shortcut.setIconResourceId(0);
shortcut.setIconResName(null);
shortcut.clearFlags(ShortcutInfo.FLAG_HAS_ICON_FILE | ShortcutInfo.FLAG_HAS_ICON_RES);
@@ -1148,13 +1157,14 @@
}
}
- private void cleanupDanglingBitmapDirectoriesLocked(
- @UserIdInt int userId, @NonNull ShortcutUser user) {
+ private void cleanupDanglingBitmapDirectoriesLocked(@UserIdInt int userId) {
if (DEBUG) {
Slog.d(TAG, "cleanupDanglingBitmaps: userId=" + userId);
}
final long start = injectElapsedRealtime();
+ final ShortcutUser user = getUserShortcutsLocked(userId);
+
final File bitmapDir = getUserBitmapFilePath(userId);
final File[] children = bitmapDir.listFiles();
if (children == null) {
diff --git a/services/tests/servicestests/src/com/android/server/pm/BaseShortcutManagerTest.java b/services/tests/servicestests/src/com/android/server/pm/BaseShortcutManagerTest.java
index e7f3345..1c7a138 100644
--- a/services/tests/servicestests/src/com/android/server/pm/BaseShortcutManagerTest.java
+++ b/services/tests/servicestests/src/com/android/server/pm/BaseShortcutManagerTest.java
@@ -247,20 +247,6 @@
}
@Override
- protected boolean isUserUnlockedL(@UserIdInt int userId) {
- // Note due to a late change, now ShortcutManager doesn't use
- // UserManager.isUserUnlockingOrUnlocked(). But all unit tests are still using it,
- // so we convert here.
-
- final long token = injectClearCallingIdentity();
- try {
- return mMockUserManager.isUserUnlockingOrUnlocked(userId);
- } finally {
- injectRestoreCallingIdentity(token);
- }
- }
-
- @Override
int injectDipToPixel(int dip) {
return dip;
}
diff --git a/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest1.java b/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest1.java
index 75a3427..253334e 100644
--- a/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest1.java
+++ b/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest1.java
@@ -5863,6 +5863,7 @@
assertEmpty(mManager.getPinnedShortcuts());
});
// Send add broadcast, but the user is not running, so should be ignored.
+ mService.handleCleanupUser(USER_10);
mRunningUsers.put(USER_10, false);
mUnlockedUsers.put(USER_10, false);
diff --git a/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest2.java b/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest2.java
index ad5bc77..d25923c 100644
--- a/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest2.java
+++ b/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest2.java
@@ -2005,4 +2005,36 @@
});
});
}
+
+ public void testIsUserUnlocked() {
+ mRunningUsers.clear();
+ mUnlockedUsers.clear();
+
+ assertFalse(mService.isUserUnlockedL(USER_0));
+ assertFalse(mService.isUserUnlockedL(USER_10));
+
+ // Start user 0, still locked.
+ mRunningUsers.put(USER_0, true);
+ assertFalse(mService.isUserUnlockedL(USER_0));
+ assertFalse(mService.isUserUnlockedL(USER_10));
+
+ // Unlock user.
+ mUnlockedUsers.put(USER_0, true);
+ assertTrue(mService.isUserUnlockedL(USER_0));
+ assertFalse(mService.isUserUnlockedL(USER_10));
+
+ // Clear again.
+ mRunningUsers.clear();
+ mUnlockedUsers.clear();
+
+ // Directly call the lifecycle event. Now also locked.
+ mService.handleUnlockUser(USER_0);
+ assertTrue(mService.isUserUnlockedL(USER_0));
+ assertFalse(mService.isUserUnlockedL(USER_10));
+
+ // Directly call the stop lifecycle event. Goes back to the initial state.
+ mService.handleCleanupUser(USER_0);
+ assertFalse(mService.isUserUnlockedL(USER_0));
+ assertFalse(mService.isUserUnlockedL(USER_10));
+ }
}
