From 4dbc0607ee43d3b0eab5e115531c639ff1a1f339 Mon Sep 17 00:00:00 2001 From: Alex Buynytskyy Date: Tue, 12 May 2020 11:24:14 -0700 Subject: [PATCH] Don't try to recreate IncrementalFileStorages on re-commit. Bug: 156287164 Fixes 156287164 Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest Change-Id: I75e392a1fa84df8b6ac0b855233f9a2662998e96 --- .../os/incremental/IncrementalFileStorages.java | 14 ++++++++++---- .../android/server/pm/PackageInstallerSession.java | 13 ++++++++++++- services/incremental/IncrementalService.cpp | 8 +++++--- .../incremental/test/IncrementalServiceTest.cpp | 4 ++-- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/core/java/android/os/incremental/IncrementalFileStorages.java b/core/java/android/os/incremental/IncrementalFileStorages.java index 321dc9e2246e1..958c7fb4fc8d7 100644 --- a/core/java/android/os/incremental/IncrementalFileStorages.java +++ b/core/java/android/os/incremental/IncrementalFileStorages.java @@ -92,10 +92,7 @@ public final class IncrementalFileStorages { } } - if (!result.mDefaultStorage.startLoading()) { - // TODO(b/146080380): add incremental-specific error code - throw new IOException("Failed to start loading data for Incremental installation."); - } + result.startLoading(); return result; } @@ -143,6 +140,15 @@ public final class IncrementalFileStorages { } } + /** + * Starts or re-starts loading of data. + */ + public void startLoading() throws IOException { + if (!mDefaultStorage.startLoading()) { + throw new IOException("Failed to start loading data for Incremental installation."); + } + } + /** * Resets the states and unbinds storage instances for an installation session. * TODO(b/136132412): make sure unnecessary binds are removed but useful storages are kept diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index e0d057a8d7f39..448ce445f205a 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -395,7 +395,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private boolean mDataLoaderFinished = false; - // TODO(b/146080380): merge file list with Callback installation. private IncrementalFileStorages mIncrementalFileStorages; private static final FileFilter sAddedApkFilter = new FileFilter() { @@ -2682,6 +2681,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { /** * Makes sure files are present in staging location. + * @return if the image is ready for installation */ @GuardedBy("mLock") private boolean prepareDataLoaderLocked() @@ -2693,6 +2693,17 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return true; } + // Retrying commit. + if (mIncrementalFileStorages != null) { + try { + mIncrementalFileStorages.startLoading(); + } catch (IOException e) { + throw new PackageManagerException(INSTALL_FAILED_MEDIA_UNAVAILABLE, e.getMessage(), + e.getCause()); + } + return false; + } + final List addedFiles = new ArrayList<>(); final List removedFiles = new ArrayList<>(); diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp index a9dc92fca971c..78439dba27246 100644 --- a/services/incremental/IncrementalService.cpp +++ b/services/incremental/IncrementalService.cpp @@ -748,7 +748,7 @@ int IncrementalService::unbind(StorageId storage, std::string_view target) { return -EINVAL; } - LOG(INFO) << "Removing bind point " << target; + LOG(INFO) << "Removing bind point " << target << " for storage " << storage; // Here we should only look up by the exact target, not by a subdirectory of any existing mount, // otherwise there's a chance to unmount something completely unrelated @@ -1807,6 +1807,8 @@ bool IncrementalService::DataLoaderStub::fsmStep() { targetStatus = mTargetStatus; } + LOG(DEBUG) << "fsmStep: " << mId << ": " << currentStatus << " -> " << targetStatus; + if (currentStatus == targetStatus) { return true; } @@ -1868,8 +1870,8 @@ binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mount listener = mListener; if (mCurrentStatus == IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE) { - // For unavailable, reset target status. - setTargetStatusLocked(IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE); + // For unavailable, unbind from DataLoader to ensure proper re-commit. + setTargetStatusLocked(IDataLoaderStatusListener::DATA_LOADER_DESTROYED); } } diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp index 325218dfa6a5c..2e4625cf85a1f 100644 --- a/services/incremental/test/IncrementalServiceTest.cpp +++ b/services/incremental/test/IncrementalServiceTest.cpp @@ -677,10 +677,10 @@ TEST_F(IncrementalServiceTest, testStartDataLoaderRecreateOnPendingReads) { mDataLoaderManager->bindToDataLoaderSuccess(); mDataLoaderManager->getDataLoaderSuccess(); EXPECT_CALL(*mDataLoaderManager, bindToDataLoader(_, _, _, _)).Times(2); - EXPECT_CALL(*mDataLoaderManager, unbindFromDataLoader(_)).Times(1); + EXPECT_CALL(*mDataLoaderManager, unbindFromDataLoader(_)).Times(2); EXPECT_CALL(*mDataLoader, create(_, _, _, _)).Times(2); EXPECT_CALL(*mDataLoader, start(_)).Times(0); - EXPECT_CALL(*mDataLoader, destroy(_)).Times(1); + EXPECT_CALL(*mDataLoader, destroy(_)).Times(2); EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2); EXPECT_CALL(*mLooper, addFd(MockIncFs::kPendingReadsFd, _, _, _, _)).Times(1); EXPECT_CALL(*mLooper, removeFd(MockIncFs::kPendingReadsFd)).Times(1);