Prevent a deadlock in #abandon

See b/159568595#comment1 for analysis.

1. rename cleanStageDir to cleanStageDirNotLocked to show it must be
   called without mLock held.
2. rename getChildSessions to getChildSessionsNotLocked to show it must
   be called without mLock held.
3. add cleanStageDir(List<PackageInstallerSession>) which is callable
   when mLock is held.

Bug: 159568595
Test: atest StagedInstallTest
Merged-In: I512ea07a4f8750431b306fa6e3c47a600a50b64f
Change-Id: I512ea07a4f8750431b306fa6e3c47a600a50b64f
(cherry picked from commit 4ce1273f926851bb58d44f59b5643373d4567c81)

1 files changed, 44 insertions, 20 deletions

diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index de8ad6b7db13..994cec2b1e59 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -401,6 +401,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
 
     private boolean mDataLoaderFinished = false;
 
+    // TODO(b/159663586): should be protected by mLock
     private IncrementalFileStorages mIncrementalFileStorages;
 
     private static final FileFilter sAddedApkFilter = new FileFilter() {
@@ -1353,7 +1354,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) {
         Objects.requireNonNull(statusReceiver);
 
-        List<PackageInstallerSession> childSessions = getChildSessions();
+        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
 
         synchronized (mLock) {
             assertCallerIsOwnerOrRootLocked();
@@ -1436,7 +1437,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
      *
      * <p> This method is handy to prevent potential deadlocks (b/123391593)
      */
-    private @Nullable List<PackageInstallerSession> getChildSessions() {
+    private @Nullable List<PackageInstallerSession> getChildSessionsNotLocked() {
+        if (Thread.holdsLock(mLock)) {
+            Slog.wtf(TAG, "Calling thread " + Thread.currentThread().getName()
+                    + " is holding mLock", new Throwable());
+        }
         List<PackageInstallerSession> childSessions = null;
         if (isMultiPackage()) {
             final int[] childSessionIds = getChildSessionIds();
@@ -1605,7 +1610,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                 return;
             }
         }
-        List<PackageInstallerSession> childSessions = getChildSessions();
+        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
         synchronized (mLock) {
             try {
                 sealLocked(childSessions);
@@ -1649,7 +1654,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             throw new SecurityException("Can only transfer sessions that use public options");
         }
 
-        List<PackageInstallerSession> childSessions = getChildSessions();
+        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
 
         synchronized (mLock) {
             assertCallerIsOwnerOrRootLocked();
@@ -1701,7 +1706,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         // outside of the lock, because reading the child
         // sessions with the lock held could lead to deadlock
         // (b/123391593).
-        List<PackageInstallerSession> childSessions = getChildSessions();
+        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
 
         try {
             synchronized (mLock) {
@@ -2602,6 +2607,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                     "Session " + sessionId + " is a child of multi-package session "
                             + mParentSessionId +  " and may not be abandoned directly.");
         }
+
+        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
         synchronized (mLock) {
             if (params.isStaged && mDestroyed) {
                 // If a user abandons staged session in an unsafe state, then system will try to
@@ -2625,7 +2632,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                     mCallback.onStagedSessionChanged(this);
                     return;
                 }
-                cleanStageDir();
+                cleanStageDir(childSessions);
             }
 
             if (mRelinquished) {
@@ -3055,7 +3062,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             mStagedSessionErrorMessage = errorMessage;
             Slog.d(TAG, "Marking session " + sessionId + " as failed: " + errorMessage);
         }
-        cleanStageDir();
+        cleanStageDirNotLocked();
         mCallback.onStagedSessionChanged(this);
     }
 
@@ -3070,7 +3077,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             mStagedSessionErrorMessage = "";
             Slog.d(TAG, "Marking session " + sessionId + " as applied");
         }
-        cleanStageDir();
+        cleanStageDirNotLocked();
         mCallback.onStagedSessionChanged(this);
     }
 
@@ -3128,20 +3135,37 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         }
     }
 
-    private void cleanStageDir() {
-        if (isMultiPackage()) {
-            for (int childSessionId : getChildSessionIds()) {
-                mSessionProvider.getSession(childSessionId).cleanStageDir();
+    /**
+     * <b>must not hold {@link #mLock}</b>
+     */
+    private void cleanStageDirNotLocked() {
+        if (Thread.holdsLock(mLock)) {
+            Slog.wtf(TAG, "Calling thread " + Thread.currentThread().getName()
+                    + " is holding mLock", new Throwable());
+        }
+        cleanStageDir(getChildSessionsNotLocked());
+    }
+
+    private void cleanStageDir(List<PackageInstallerSession> childSessions) {
+        if (childSessions != null) {
+            for (PackageInstallerSession childSession : childSessions) {
+                if (childSession != null) {
+                    childSession.cleanStageDir();
+                }
             }
         } else {
-            if (mIncrementalFileStorages != null) {
-                mIncrementalFileStorages.cleanUp();
-                mIncrementalFileStorages = null;
-            }
-            try {
-                mPm.mInstaller.rmPackageDir(stageDir.getAbsolutePath());
-            } catch (InstallerException ignored) {
-            }
+            cleanStageDir();
+        }
+    }
+
+    private void cleanStageDir() {
+        if (mIncrementalFileStorages != null) {
+            mIncrementalFileStorages.cleanUp();
+            mIncrementalFileStorages = null;
+        }
+        try {
+            mPm.mInstaller.rmPackageDir(stageDir.getAbsolutePath());
+        } catch (InstallerException ignored) {
         }
     }