Merge "Add session cleanup tests for multi-package sessions (2/n)"

3 files changed, 55 insertions, 15 deletions

diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index c47ca4b45ab3..803a2833dc3c 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -558,10 +558,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                 Slog.d(TAG, "Marking session " + sessionId + " as failed: " + errorMessage);
                 childSessions = getChildSessionsLocked();
             }
-            destroyInternal();
-            for (PackageInstallerSession child : childSessions) {
-                child.destroyInternal();
-            }
+            destroy();
             mCallback.onStagedSessionChanged(PackageInstallerSession.this);
         }
 
@@ -579,10 +576,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                 Slog.d(TAG, "Marking session " + sessionId + " as applied");
                 childSessions = getChildSessionsLocked();
             }
-            destroyInternal();
-            for (PackageInstallerSession child : childSessions) {
-                child.destroyInternal();
-            }
+            destroy();
             mCallback.onStagedSessionChanged(PackageInstallerSession.this);
         }
 
@@ -2111,18 +2105,14 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     private void onSessionVerificationFailure(int error, String msg) {
         final String msgWithErrorCode = PackageManager.installStatusToString(error, msg);
         Slog.e(TAG, "Failed to verify session " + sessionId + " [" + msgWithErrorCode + "]");
-        // Session is sealed and committed but could not be verified, we need to destroy it.
-        destroyInternal();
-        if (isMultiPackage()) {
-            for (PackageInstallerSession childSession : getChildSessions()) {
-                childSession.destroyInternal();
-            }
-        }
         if (isStaged()) {
+            // This will clean up the session when it reaches the terminal state
             mStagedSession.setSessionFailed(
                     SessionInfo.STAGED_SESSION_VERIFICATION_FAILED, msgWithErrorCode);
             mStagedSession.notifyEndPreRebootVerification();
         } else {
+            // Session is sealed and committed but could not be verified, we need to destroy it.
+            destroy();
             // Dispatch message to remove session from PackageInstallerService.
             dispatchSessionFinished(error, msg, null);
         }
@@ -4265,6 +4255,28 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         return params.isStaged ? mStagedSession.getSessionErrorMessage() : "";
     }
 
+    /**
+     * Free up storage used by this session and its children.
+     * Must not be called on a child session.
+     */
+    private void destroy() {
+        // TODO(b/173194203): destroy() is called indirectly by
+        //  PackageInstallerService#restoreAndApplyStagedSessionIfNeeded on an orphan child session.
+        //  Enable this assertion when we figure out a better way to clean up orphan sessions.
+        // assertNotChild("destroy");
+
+        // TODO(b/173194203): destroyInternal() should be used by destroy() only.
+        //  For the sake of consistency, a session should be destroyed as a whole. The caller
+        //  should always call destroy() for cleanup without knowing it has child sessions or not.
+        destroyInternal();
+        for (PackageInstallerSession child : getChildSessions()) {
+            child.destroyInternal();
+        }
+    }
+
+    /**
+     * Free up storage used by this session.
+     */
     private void destroyInternal() {
         final IncrementalFileStorages incrementalFileStorages;
         synchronized (mLock) {
diff --git a/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java b/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java
index e4e535d3240d..da5468e48f19 100644
--- a/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java
+++ b/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java
@@ -157,8 +157,18 @@ public class StagedInstallInternalTest {
 
     @Test
     public void testStagedSessionShouldCleanUpOnVerificationFailure() throws Exception {
+        // APEX verification
         InstallUtils.commitExpectingFailure(AssertionError.class, "apexd verification failed",
                 Install.single(APEX_WRONG_SHA_V2).setStaged());
+        InstallUtils.commitExpectingFailure(AssertionError.class, "apexd verification failed",
+                Install.multi(APEX_WRONG_SHA_V2, TestApp.A1).setStaged());
+        // APK verification
+        Install.single(TestApp.A2).commit();
+        assertThat(InstallUtils.getInstalledVersion(TestApp.A)).isEqualTo(2);
+        InstallUtils.commitExpectingFailure(AssertionError.class, "Downgrade detected",
+                Install.single(TestApp.A1).setStaged());
+        InstallUtils.commitExpectingFailure(AssertionError.class, "Downgrade detected",
+                Install.multi(TestApp.A1, TestApp.B1).setStaged());
     }
 
     @Test
@@ -176,6 +186,12 @@ public class StagedInstallInternalTest {
     }
 
     @Test
+    public void testStagedSessionShouldCleanUpOnOnSuccessMultiPackage_Commit() throws Exception {
+        int sessionId = Install.multi(TestApp.A1, TestApp.Apex2).setStaged().commit();
+        storeSessionId(sessionId);
+    }
+
+    @Test
     public void testStagedInstallationShouldCleanUpOnValidationFailure() throws Exception {
         InstallUtils.commitExpectingFailure(AssertionError.class, "INSTALL_FAILED_INVALID_APK",
                 Install.single(TestApp.AIncompleteSplit).setStaged());
diff --git a/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java b/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java
index 78cf9aca66c0..926bf1b88fee 100644
--- a/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java
+++ b/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java
@@ -301,6 +301,18 @@ public class StagedInstallInternalTest extends BaseHostJUnit4Test {
     }
 
     @Test
+    @LargeTest
+    public void testStagedSessionShouldCleanUpOnOnSuccessMultiPackage() throws Exception {
+        List<String> before = getStagingDirectories();
+        runPhase("testStagedSessionShouldCleanUpOnOnSuccessMultiPackage_Commit");
+        assertThat(getStagingDirectories()).isNotEqualTo(before);
+        getDevice().reboot();
+        runPhase("testStagedSessionShouldCleanUpOnOnSuccess_Verify");
+        List<String> after = getStagingDirectories();
+        assertThat(after).isEqualTo(before);
+    }
+
+    @Test
     public void testStagedInstallationShouldCleanUpOnValidationFailure() throws Exception {
         List<String> before = getStagingDirectories();
         runPhase("testStagedInstallationShouldCleanUpOnValidationFailure");