Store object references along with child session Ids (1/n)

1. Null-check is no longer needed in removeChildSessionId()
   since we never store null in child sessions.

2. Simplify the lock model for we no longer need
   mSessionProvider.getSession() to retrieve a child session which
   must be called outside mLock.

Bug: 162386287
Test: atest StagedInstallTest

Change-Id: I05580968a6ed97647c841f7c47fc1cb1a94ead5a

2 files changed, 92 insertions, 86 deletions

diff --git a/services/core/java/com/android/server/pm/PackageInstallerService.java b/services/core/java/com/android/server/pm/PackageInstallerService.java
index 55e7ca8ca838..840645edcb82 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerService.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerService.java
@@ -442,7 +442,7 @@ public class PackageInstallerService extends IPackageInstaller.Stub implements
         // After reboot housekeeping.
         for (int i = 0; i < mSessions.size(); ++i) {
             PackageInstallerSession session = mSessions.valueAt(i);
-            session.onAfterSessionRead();
+            session.onAfterSessionRead(mSessions);
         }
     }
 
diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index 05026a0cafb6..b79903720761 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -122,7 +122,7 @@ import android.util.ArraySet;
 import android.util.ExceptionUtils;
 import android.util.MathUtils;
 import android.util.Slog;
-import android.util.SparseIntArray;
+import android.util.SparseArray;
 import android.util.apk.ApkSignatureVerifier;
 
 import com.android.internal.R;
@@ -336,7 +336,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     @GuardedBy("mLock")
     private PackageParser.SigningDetails mSigningDetails;
     @GuardedBy("mLock")
-    private SparseIntArray mChildSessionIds = new SparseIntArray();
+    private SparseArray<PackageInstallerSession> mChildSessions = new SparseArray<>();
     @GuardedBy("mLock")
     private int mParentSessionId;
 
@@ -589,7 +589,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         this.mShouldBeSealed = sealed;
         if (childSessionIds != null) {
             for (int childSessionId : childSessionIds) {
-                mChildSessionIds.put(childSessionId, 0);
+                // Null values will be resolved to actual object references in
+                // #onAfterSessionRead later.
+                mChildSessions.put(childSessionId, null);
             }
         }
         this.mParentSessionId = parentSessionId;
@@ -708,10 +710,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             info.isStaged = params.isStaged;
             info.rollbackDataPolicy = params.rollbackDataPolicy;
             info.parentSessionId = mParentSessionId;
-            info.childSessionIds = mChildSessionIds.copyKeys();
-            if (info.childSessionIds == null) {
-                info.childSessionIds = EMPTY_CHILD_SESSION_ARRAY;
-            }
+            info.childSessionIds = getChildSessionIdsLocked();
             info.isStagedSessionApplied = mStagedSessionApplied;
             info.isStagedSessionReady = mStagedSessionReady;
             info.isStagedSessionFailed = mStagedSessionFailed;
@@ -1159,27 +1158,22 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             return;
         }
         if (isMultiPackage()) {
-            final SparseIntArray remainingSessions;
-            final int[] childSessionIds;
             synchronized (mLock) {
-                remainingSessions = mChildSessionIds.clone();
-                childSessionIds = mChildSessionIds.copyKeys();
-            }
-            final IntentSender childIntentSender =
-                    new ChildStatusIntentReceiver(remainingSessions, statusReceiver)
-                            .getIntentSender();
-            boolean sealFailed = false;
-            for (int i = childSessionIds.length - 1; i >= 0; --i) {
-                final int childSessionId = childSessionIds[i];
-                // seal all children, regardless if any of them fail; we'll throw/return
-                // as appropriate once all children have been processed
-                if (!mSessionProvider.getSession(childSessionId)
-                        .markAsSealed(childIntentSender, forTransfer)) {
-                    sealFailed = true;
+                final IntentSender childIntentSender =
+                        new ChildStatusIntentReceiver(mChildSessions.clone(), statusReceiver)
+                                .getIntentSender();
+                boolean sealFailed = false;
+                for (int i = mChildSessions.size() - 1; i >= 0; --i) {
+                    // seal all children, regardless if any of them fail; we'll throw/return
+                    // as appropriate once all children have been processed
+                    if (!mChildSessions.valueAt(i)
+                            .markAsSealed(childIntentSender, forTransfer)) {
+                        sealFailed = true;
+                    }
+                }
+                if (sealFailed) {
+                    return;
                 }
-            }
-            if (sealFailed) {
-                return;
             }
         }
 
@@ -1218,21 +1212,20 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         }
 
         if (isMultiPackage()) {
-            final int[] childSessionIds;
+            final List<PackageInstallerSession> childSessions;
             synchronized (mLock) {
-                childSessionIds = mChildSessionIds.copyKeys();
+                childSessions = getChildSessionsLocked();
             }
-            int childCount = childSessionIds.length;
+            int childCount = childSessions.size();
 
             // This will contain all child sessions that do not encounter an unrecoverable failure
             ArrayList<PackageInstallerSession> nonFailingSessions = new ArrayList<>(childCount);
 
             for (int i = childCount - 1; i >= 0; --i) {
-                final int childSessionId = childSessionIds[i];
                 // commit all children, regardless if any of them fail; we'll throw/return
                 // as appropriate once all children have been processed
                 try {
-                    PackageInstallerSession session = mSessionProvider.getSession(childSessionId);
+                    PackageInstallerSession session = childSessions.get(i);
                     allSessionsReady &= session.streamValidateAndCommit();
                     nonFailingSessions.add(session);
                 } catch (PackageManagerException e) {
@@ -1293,7 +1286,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     }
 
     private class ChildStatusIntentReceiver {
-        private final SparseIntArray mChildSessionsRemaining;
+        private final SparseArray<PackageInstallerSession> mChildSessionsRemaining;
         private final IntentSender mStatusReceiver;
         private final IIntentSender.Stub mLocalSender = new IIntentSender.Stub() {
             @Override
@@ -1303,7 +1296,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             }
         };
 
-        private ChildStatusIntentReceiver(SparseIntArray remainingSessions,
+        private ChildStatusIntentReceiver(SparseArray<PackageInstallerSession> remainingSessions,
                 IntentSender statusReceiver) {
             this.mChildSessionsRemaining = remainingSessions;
             this.mStatusReceiver = statusReceiver;
@@ -1413,8 +1406,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) {
         Objects.requireNonNull(statusReceiver);
 
-        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
-
         synchronized (mLock) {
             assertCallerIsOwnerOrRootLocked();
             assertPreparedAndNotDestroyedLocked("commit");
@@ -1446,7 +1437,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             }
 
             try {
-                sealLocked(childSessions);
+                sealLocked(getChildSessionsLocked());
             } catch (PackageManagerException e) {
                 return false;
             }
@@ -1507,6 +1498,19 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         return childSessions;
     }
 
+    @GuardedBy("mLock")
+    private @Nullable List<PackageInstallerSession> getChildSessionsLocked() {
+        List<PackageInstallerSession> childSessions = null;
+        if (isMultiPackage()) {
+            int size = mChildSessions.size();
+            childSessions = new ArrayList<>(size);
+            for (int i = 0; i < size; ++i) {
+                childSessions.add(mChildSessions.valueAt(i));
+            }
+        }
+        return childSessions;
+    }
+
     /**
      * Assert multipackage install has consistent sessions.
      *
@@ -1657,17 +1661,32 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
      *
      * <p> This is meant to be called after all of the sessions are loaded and added to
      * PackageInstallerService
+     *
+     * @param allSessions All sessions loaded by PackageInstallerService, guaranteed to be
+     *                    immutable by the caller during the method call. Used to resolve child
+     *                    sessions Ids to actual object reference.
      */
-    void onAfterSessionRead() {
+    void onAfterSessionRead(SparseArray<PackageInstallerSession> allSessions) {
         synchronized (mLock) {
+            // Resolve null values to actual object references
+            for (int i = mChildSessions.size() - 1; i >= 0; --i) {
+                int childSessionId = mChildSessions.keyAt(i);
+                PackageInstallerSession childSession = allSessions.get(childSessionId);
+                if (childSession != null) {
+                    mChildSessions.setValueAt(i, childSession);
+                } else {
+                    Slog.e(TAG, "Child session not existed: " + childSessionId);
+                    mChildSessions.removeAt(i);
+                }
+            }
+
             if (!mShouldBeSealed || isStagedAndInTerminalState()) {
                 return;
             }
         }
-        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
         synchronized (mLock) {
             try {
-                sealLocked(childSessions);
+                sealLocked(getChildSessionsLocked());
 
                 if (isApexInstallation()) {
                     // APEX installations rely on certain fields to be populated after reboot.
@@ -1708,14 +1727,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             throw new SecurityException("Can only transfer sessions that use public options");
         }
 
-        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
-
         synchronized (mLock) {
             assertCallerIsOwnerOrRootLocked();
             assertPreparedAndNotSealedLocked("transfer");
 
             try {
-                sealLocked(childSessions);
+                sealLocked(getChildSessionsLocked());
             } catch (PackageManagerException e) {
                 throw new IllegalArgumentException("Package is not valid", e);
             }
@@ -1746,11 +1763,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             return;
         }
 
-        // For a multiPackage session, read the child sessions
-        // outside of the lock, because reading the child
-        // sessions with the lock held could lead to deadlock
-        // (b/123391593).
-        List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
+        final List<PackageInstallerSession> childSessions;
+        synchronized (mLock) {
+            childSessions = getChildSessionsLocked();
+        }
 
         try {
             verifyNonStaged(childSessions);
@@ -2741,15 +2757,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         }
     }
 
-    /**
-     * Adds a child session ID without any safety / sanity checks. This should only be used to
-     * build a session from XML or similar.
-     */
-    @GuardedBy("mLock")
-    void addChildSessionIdLocked(int sessionId) {
-        mChildSessionIds.put(sessionId, 0);
-    }
-
     public void open() throws IOException {
         if (mActiveCount.getAndIncrement() == 0) {
             mCallback.onSessionActiveChanged(this, true);
@@ -2804,7 +2811,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                             + getParentSessionId() +  " 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
@@ -2828,7 +2834,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                     mCallback.onStagedSessionChanged(this);
                     return;
                 }
-                cleanStageDir(childSessions);
+                cleanStageDir(getChildSessionsLocked());
             }
 
             if (mRelinquished) {
@@ -3149,8 +3155,15 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
 
     @GuardedBy("mLock")
     private int[] getChildSessionIdsLocked() {
-        final int[] childSessionIds = mChildSessionIds.copyKeys();
-        return childSessionIds != null ? childSessionIds : EMPTY_CHILD_SESSION_ARRAY;
+        int size = mChildSessions.size();
+        if (size == 0) {
+            return EMPTY_CHILD_SESSION_ARRAY;
+        }
+        final int[] childSessionIds = new int[size];
+        for (int i = 0; i < size; ++i) {
+            childSessionIds[i] = mChildSessions.keyAt(i);
+        }
+        return childSessionIds;
     }
 
     @Override
@@ -3205,12 +3218,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                 assertCallerIsOwnerOrRootLocked();
                 assertPreparedAndNotSealedLocked("addChildSessionId");
 
-                final int indexOfSession = mChildSessionIds.indexOfKey(childSessionId);
+                final int indexOfSession = mChildSessions.indexOfKey(childSessionId);
                 if (indexOfSession >= 0) {
                     return;
                 }
                 childSession.setParentSessionId(this.sessionId);
-                addChildSessionIdLocked(childSessionId);
+                mChildSessions.put(childSessionId, childSession);
             }
         } finally {
             releaseTransactionLock();
@@ -3220,30 +3233,23 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
 
     @Override
     public void removeChildSessionId(int sessionId) {
-        final PackageInstallerSession session = mSessionProvider.getSession(sessionId);
-        try {
-            acquireTransactionLock();
-            if (session != null) {
-                session.acquireTransactionLock();
-            }
-
-            synchronized (mLock) {
-                assertCallerIsOwnerOrRootLocked();
-                assertPreparedAndNotSealedLocked("removeChildSessionId");
+        synchronized (mLock) {
+            assertCallerIsOwnerOrRootLocked();
+            assertPreparedAndNotSealedLocked("removeChildSessionId");
 
-                final int indexOfSession = mChildSessionIds.indexOfKey(sessionId);
-                if (indexOfSession < 0) {
-                    // not added in the first place; no-op
-                    return;
-                }
-                if (session != null) {
-                    session.setParentSessionId(SessionInfo.INVALID_ID);
-                }
-                mChildSessionIds.removeAt(indexOfSession);
+            final int indexOfSession = mChildSessions.indexOfKey(sessionId);
+            if (indexOfSession < 0) {
+                // not added in the first place; no-op
+                return;
             }
-        } finally {
-            releaseTransactionLock();
-            if (session != null) {
+            PackageInstallerSession session = mChildSessions.valueAt(indexOfSession);
+            try {
+                acquireTransactionLock();
+                session.acquireTransactionLock();
+                session.setParentSessionId(SessionInfo.INVALID_ID);
+                mChildSessions.removeAt(indexOfSession);
+            } finally {
+                releaseTransactionLock();
                 session.releaseTransactionLock();
             }
         }
@@ -3504,7 +3510,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         pw.printPair("params.isMultiPackage", params.isMultiPackage);
         pw.printPair("params.isStaged", params.isStaged);
         pw.printPair("mParentSessionId", mParentSessionId);
-        pw.printPair("mChildSessionIds", mChildSessionIds);
+        pw.printPair("mChildSessionIds", getChildSessionIdsLocked());
         pw.printPair("mStagedSessionApplied", mStagedSessionApplied);
         pw.printPair("mStagedSessionFailed", mStagedSessionFailed);
         pw.printPair("mStagedSessionReady", mStagedSessionReady);