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

4 files changed, 56 insertions, 27 deletions

diff --git a/services/core/java/com/android/server/pm/ShortcutService.java b/services/core/java/com/android/server/pm/ShortcutService.java
index d5767b4cad8a..c1fc7f114c67 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 @@ public class ShortcutService extends IShortcutService.Stub {
             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 @@ public class ShortcutService extends IShortcutService.Stub {
         }
         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 @@ public class ShortcutService extends IShortcutService.Stub {
         }
     }
 
-    // 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 @@ public class ShortcutService extends IShortcutService.Stub {
     // === 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 @@ public class ShortcutService extends IShortcutService.Stub {
         }
     }
 
-    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 e7f33457bb5f..1c7a138468cf 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 @@ public abstract class BaseShortcutManagerTest extends InstrumentationTestCase {
         }
 
         @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 75a34272d82b..253334eec9cf 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 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest {
             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 ad5bc7730833..d25923c019ca 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 class ShortcutManagerTest2 extends BaseShortcutManagerTest {
                     });
         });
     }
+
+    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));
+    }
 }