Proper retrying DL installation sessions.

Plus more robust handling of broken DLs.

Bug: 190012477
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest com.google.android.packageinstallerv2proxy.host.gts.IncrementalInstallerHostTest
Change-Id: I5cb037d49cd2b140bed1045c99f072112495acfc

5 files changed, 50 insertions, 21 deletions

diff --git a/core/java/android/os/incremental/IncrementalFileStorages.java b/core/java/android/os/incremental/IncrementalFileStorages.java
index 6e25968192ae..8ebc0814a3dc 100644
--- a/core/java/android/os/incremental/IncrementalFileStorages.java
+++ b/core/java/android/os/incremental/IncrementalFileStorages.java
@@ -160,7 +160,7 @@ public final class IncrementalFileStorages {
     /**
      * Starts or re-starts loading of data.
      */
-    void startLoading(
+    public void startLoading(
             @NonNull DataLoaderParams dataLoaderParams,
             @Nullable IDataLoaderStatusListener statusListener,
             @Nullable StorageHealthCheckParams healthCheckParams,
diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index 8202587ecf53..4b0eb6546888 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -3760,11 +3760,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             return true;
         }
 
-        // Retrying commit.
-        if (mIncrementalFileStorages != null) {
-            return false;
-        }
-
         final List<InstallationFileParcel> addedFiles = new ArrayList<>();
         final List<String> removedFiles = new ArrayList<>();
 
@@ -3925,18 +3920,24 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                         (pkgInfo != null && pkgInfo.applicationInfo != null) ? new File(
                                 pkgInfo.applicationInfo.getCodePath()).getParentFile() : null;
 
-                mIncrementalFileStorages = IncrementalFileStorages.initialize(mContext, stageDir,
-                        inheritedDir, params, statusListener, healthCheckParams, healthListener,
-                        addedFiles, perUidReadTimeouts,
-                        new IPackageLoadingProgressCallback.Stub() {
-                            @Override
-                            public void onPackageLoadingProgressChanged(float progress) {
-                                synchronized (mProgressLock) {
-                                    mIncrementalProgress = progress;
-                                    computeProgressLocked(true);
+                if (mIncrementalFileStorages == null) {
+                    mIncrementalFileStorages = IncrementalFileStorages.initialize(mContext,
+                            stageDir, inheritedDir, params, statusListener, healthCheckParams,
+                            healthListener, addedFiles, perUidReadTimeouts,
+                            new IPackageLoadingProgressCallback.Stub() {
+                                @Override
+                                public void onPackageLoadingProgressChanged(float progress) {
+                                    synchronized (mProgressLock) {
+                                        mIncrementalProgress = progress;
+                                        computeProgressLocked(true);
+                                    }
                                 }
-                            }
-                        });
+                            });
+                } else {
+                    // Retrying commit.
+                    mIncrementalFileStorages.startLoading(params, statusListener, healthCheckParams,
+                            healthListener, perUidReadTimeouts);
+                }
                 return false;
             } catch (IOException e) {
                 throw new PackageManagerException(INSTALL_FAILED_MEDIA_UNAVAILABLE, e.getMessage(),
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 47d59b250e29..757c9dec06ed 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -89,6 +89,11 @@ struct Constants {
 
     // Max interval after system invoked the DL when readlog collection can be enabled.
     static constexpr auto readLogsMaxInterval = 2h;
+
+    // How long should we wait till dataLoader reports destroyed.
+    static constexpr auto destroyTimeout = 60s;
+
+    static constexpr auto anyStatus = INT_MIN;
 };
 
 static const Constants& constants() {
@@ -2554,7 +2559,7 @@ void IncrementalService::DataLoaderStub::cleanupResources() {
         mControl = {};
         mHealthControl = {};
         mHealthListener = {};
-        mStatusCondition.wait_until(lock, now + 60s, [this] {
+        mStatusCondition.wait_until(lock, now + Constants::destroyTimeout, [this] {
             return mCurrentStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED;
         });
         mStatusListener = {};
@@ -2754,8 +2759,16 @@ bool IncrementalService::DataLoaderStub::fsmStep() {
     switch (targetStatus) {
         case IDataLoaderStatusListener::DATA_LOADER_DESTROYED: {
             switch (currentStatus) {
+                case IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE:
+                case IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE:
+                    destroy();
+                    // DataLoader is broken, just assume it's destroyed.
+                    compareAndSetCurrentStatus(currentStatus,
+                                               IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+                    return true;
                 case IDataLoaderStatusListener::DATA_LOADER_BINDING:
-                    setCurrentStatus(IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+                    compareAndSetCurrentStatus(currentStatus,
+                                               IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
                     return true;
                 default:
                     return destroy();
@@ -2776,7 +2789,11 @@ bool IncrementalService::DataLoaderStub::fsmStep() {
                 case IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE:
                     // Before binding need to make sure we are unbound.
                     // Otherwise we'll get stuck binding.
-                    return destroy();
+                    destroy();
+                    // DataLoader is broken, just assume it's destroyed.
+                    compareAndSetCurrentStatus(currentStatus,
+                                               IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+                    return true;
                 case IDataLoaderStatusListener::DATA_LOADER_DESTROYED:
                 case IDataLoaderStatusListener::DATA_LOADER_BINDING:
                     return bind();
@@ -2815,6 +2832,11 @@ binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mount
 }
 
 void IncrementalService::DataLoaderStub::setCurrentStatus(int newStatus) {
+    compareAndSetCurrentStatus(Constants::anyStatus, newStatus);
+}
+
+void IncrementalService::DataLoaderStub::compareAndSetCurrentStatus(int expectedStatus,
+                                                                    int newStatus) {
     int oldStatus, oldTargetStatus, newTargetStatus;
     DataLoaderStatusListener listener;
     {
@@ -2822,6 +2844,9 @@ void IncrementalService::DataLoaderStub::setCurrentStatus(int newStatus) {
         if (mCurrentStatus == newStatus) {
             return;
         }
+        if (expectedStatus != Constants::anyStatus && expectedStatus != mCurrentStatus) {
+            return;
+        }
 
         oldStatus = mCurrentStatus;
         oldTargetStatus = mTargetStatus;
diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index 5de7325b3d28..b81e1b1b071c 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -255,6 +255,7 @@ private:
         binder::Status onStatusChanged(MountId mount, int newStatus) final;
 
         void setCurrentStatus(int newStatus);
+        void compareAndSetCurrentStatus(int expectedStatus, int newStatus);
 
         sp<content::pm::IDataLoader> getDataLoader();
 
diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp
index 094c5fb36eea..59d96d2393ef 100644
--- a/services/incremental/test/IncrementalServiceTest.cpp
+++ b/services/incremental/test/IncrementalServiceTest.cpp
@@ -808,7 +808,9 @@ public:
                                                     METRICS_MILLIS_SINCE_OLDEST_PENDING_READ()
                                                             .c_str()),
                                    &millisSinceOldestPendingRead));
-        ASSERT_EQ(expectedMillisSinceOldestPendingRead, millisSinceOldestPendingRead);
+        // Allow 10ms.
+        ASSERT_LE(expectedMillisSinceOldestPendingRead, millisSinceOldestPendingRead);
+        ASSERT_GE(expectedMillisSinceOldestPendingRead + 10, millisSinceOldestPendingRead);
         int storageHealthStatusCode = -1;
         ASSERT_TRUE(
                 result.getInt(String16(BnIncrementalService::METRICS_STORAGE_HEALTH_STATUS_CODE()