From 2a5f1edc4165ace418dd71c01ce978fe9c7283d3 Mon Sep 17 00:00:00 2001
From: Winson Chiu <chiuwinson@google.com>
Date: Thu, 12 May 2022 15:43:51 +0000
Subject: [PATCH] Fix PMS recordInitialState on retry

Need to recapture the initial state so that the retry on the
implementation lambda works as expected. Otherwise it will compare
a stale, always broken state on the retry.

Also adds a safety check in the commitStateMutation method to see
if the calling thread already has mLock. If it does, the state change
check can be skipped since no snapshot changes can come in while the
lock is held.

Bug: 232061163

Test: atest android.packageinstaller.install.cts.SessionTest#setAppCategory

Change-Id: I78cd6a32782ee64a705ba5faa6a72a7bf8a95f03
---
 .../server/pm/PackageManagerService.java      | 25 +++++++++++++------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java
index 37fa4d1c02a63..96f00414c494e 100644
--- a/services/core/java/com/android/server/pm/PackageManagerService.java
+++ b/services/core/java/com/android/server/pm/PackageManagerService.java
@@ -5384,10 +5384,8 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         @Override
         public void setApplicationCategoryHint(String packageName, int categoryHint,
                 String callerPackageName) {
-            final PackageStateMutator.InitialState initialState = recordInitialState();
-
-            final FunctionalUtils.ThrowingFunction<Computer, PackageStateMutator.Result>
-                    implementation = computer -> {
+            final FunctionalUtils.ThrowingBiFunction<PackageStateMutator.InitialState, Computer,
+                    PackageStateMutator.Result> implementation = (initialState, computer) -> {
                 if (computer.getInstantAppPackageName(Binder.getCallingUid()) != null) {
                     throw new SecurityException(
                             "Instant applications don't have access to this method");
@@ -5415,12 +5413,13 @@ public class PackageManagerService implements PackageSender, TestUtilityService
                 }
             };
 
-            PackageStateMutator.Result result = implementation.apply(snapshotComputer());
+            PackageStateMutator.Result result =
+                    implementation.apply(recordInitialState(), snapshotComputer());
             if (result != null && result.isStateChanged() && !result.isSpecificPackageNull()) {
                 // TODO: Specific return value of what state changed?
                 // The installer on record might have changed, retry with lock
                 synchronized (mPackageStateWriteLock) {
-                    result = implementation.apply(snapshotComputer());
+                    result = implementation.apply(recordInitialState(), snapshotComputer());
                 }
             }
 
@@ -7152,9 +7151,19 @@ public class PackageManagerService implements PackageSender, TestUtilityService
     public PackageStateMutator.Result commitPackageStateMutation(
             @Nullable PackageStateMutator.InitialState initialState, @NonNull String packageName,
             @NonNull Consumer<PackageStateWrite> consumer) {
+        PackageStateMutator.Result result = null;
+        if (Thread.holdsLock(mPackageStateWriteLock)) {
+            // If the thread is already holding the lock, this is likely a retry based on a prior
+            // failure, and re-calculating whether a state change occurred can be skipped.
+            result = PackageStateMutator.Result.SUCCESS;
+        }
         synchronized (mPackageStateWriteLock) {
-            final PackageStateMutator.Result result = mPackageStateMutator.generateResult(
-                    initialState, mChangedPackagesTracker.getSequenceNumber());
+            if (result == null) {
+                // If the thread wasn't previously holding, this is a first-try commit and so a
+                // state change may have happened.
+                result = mPackageStateMutator.generateResult(
+                        initialState, mChangedPackagesTracker.getSequenceNumber());
+            }
             if (result != PackageStateMutator.Result.SUCCESS) {
                 return result;
             }