Merge "Prevent a deadlock in #abandon" into rvc-dev am: 69c5cac4c1 am: 1bf197cab3 am: 8dd5f4e4d0

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/11983819

Change-Id: I7d36a96c94f012d67657d6df1adac96332a8c348

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) {
         }
     }