From 14b4c8d7dd42785efe8790283631b4b5994238a3 Mon Sep 17 00:00:00 2001 From: Alex Buynytskyy Date: Tue, 30 Mar 2021 21:06:15 -0700 Subject: [PATCH] Remove deadlock/lock contention on Incremental progress update. Incremental progress is updated separately, and can create deadlock or simply wait for mLock while session is doing some heavy processing. This in turn blocks progress update or binder thread, causing long delays for all other packages. Bug: 160635296 Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest PackageManagerServiceTest ChecksumsTest Change-Id: I6120443a03ec22d1ca672e5a1acf8b3752e1c3ee Merged-In: I6120443a03ec22d1ca672e5a1acf8b3752e1c3ee --- .../server/pm/PackageInstallerSession.java | 81 ++++++++++--------- 1 file changed, 42 insertions(+), 39 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 7b71be98834c2..4eafe5143c8ff 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -309,23 +309,24 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private final String mOriginalInstallerPackageName; /** Uid of the owner of the installer session */ - @GuardedBy("mLock") - private int mInstallerUid; + private volatile int mInstallerUid; /** Where this install request came from */ @GuardedBy("mLock") private InstallSource mInstallSource; - @GuardedBy("mLock") + private final Object mProgressLock = new Object(); + + @GuardedBy("mProgressLock") private float mClientProgress = 0; - @GuardedBy("mLock") + @GuardedBy("mProgressLock") private float mInternalProgress = 0; - @GuardedBy("mLock") + @GuardedBy("mProgressLock") private float mProgress = 0; - @GuardedBy("mLock") + @GuardedBy("mProgressLock") private float mReportedProgress = -1; - @GuardedBy("mLock") + @GuardedBy("mProgressLock") private float mIncrementalProgress = 0; /** State of the session. */ @@ -571,7 +572,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { */ @Override public void installSession(IntentSender statusReceiver) { - assertCallerIsOwnerOrRootOrSystemLocked(); + assertCallerIsOwnerOrRootOrSystem(); assertNotChildLocked("StagedSession#installSession"); Preconditions.checkArgument(isCommitted() && isSessionReady()); @@ -670,7 +671,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { final Runnable r; synchronized (mLock) { assertNotChildLocked("StagedSession#abandon"); - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); if (isInTerminalState()) { // We keep the session in the database if it's in a finalized state. It will be // removed by PackageInstallerService when the last update time is old enough. @@ -744,7 +745,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { */ @Override public void verifySession() { - assertCallerIsOwnerOrRootOrSystemLocked(); + assertCallerIsOwnerOrRootOrSystem(); Preconditions.checkArgument(isCommitted()); Preconditions.checkArgument(!mSessionApplied && !mSessionFailed); verify(); @@ -1229,7 +1230,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } - @GuardedBy("mLock") + @GuardedBy("mProgressLock") private void setClientProgressLocked(float progress) { // Always publish first staging movement final boolean forcePublish = (mClientProgress == 0); @@ -1239,21 +1240,21 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @Override public void setClientProgress(float progress) { - synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); + synchronized (mProgressLock) { setClientProgressLocked(progress); } } @Override public void addClientProgress(float progress) { - synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); + synchronized (mProgressLock) { setClientProgressLocked(mClientProgress + progress); } } - @GuardedBy("mLock") + @GuardedBy("mProgressLock") private void computeProgressLocked(boolean forcePublish) { if (!mCommitted) { mProgress = MathUtils.constrain(mClientProgress * 0.8f, 0f, 0.8f) @@ -1278,8 +1279,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @Override public String[] getNames() { + assertCallerIsOwnerOrRoot(); synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotCommittedOrDestroyedLocked("getNames"); return getNamesLocked(); @@ -1362,8 +1363,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } + assertCallerIsOwnerOrRoot(); synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotCommittedOrDestroyedLocked("addChecksums"); if (mChecksums.containsKey(name)) { @@ -1384,8 +1385,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throw new IllegalStateException("Must specify package name to remove a split"); } + assertCallerIsOwnerOrRoot(); synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotCommittedOrDestroyedLocked("removeSplit"); try { @@ -1431,8 +1432,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throw new IllegalStateException( "Cannot write regular files in a data loader installation session."); } + assertCallerIsOwnerOrRoot(); synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotSealedLocked("assertCanWrite"); } if (reverseMode) { @@ -1605,8 +1606,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throw new IllegalStateException( "Cannot read regular files in a data loader installation session."); } + assertCallerIsOwnerOrRoot(); synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotCommittedOrDestroyedLocked("openRead"); try { return openReadInternalLocked(name); @@ -1634,8 +1635,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { * Check if the caller is the owner of this session. Otherwise throw a * {@link SecurityException}. */ - @GuardedBy("mLock") - private void assertCallerIsOwnerOrRootLocked() { + private void assertCallerIsOwnerOrRoot() { final int callingUid = Binder.getCallingUid(); if (callingUid != Process.ROOT_UID && callingUid != mInstallerUid) { throw new SecurityException("Session does not belong to uid " + callingUid); @@ -1646,8 +1646,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { * Check if the caller is the owner of this session. Otherwise throw a * {@link SecurityException}. */ - @GuardedBy("mLock") - private void assertCallerIsOwnerOrRootOrSystemLocked() { + private void assertCallerIsOwnerOrRootOrSystem() { final int callingUid = Binder.getCallingUid(); if (callingUid != Process.ROOT_UID && callingUid != mInstallerUid && callingUid != Process.SYSTEM_UID) { @@ -1929,9 +1928,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { */ private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) { Objects.requireNonNull(statusReceiver); + assertCallerIsOwnerOrRoot(); synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotDestroyedLocked("commit of session " + sessionId); assertNoWriteFileTransfersOpenLocked(); @@ -2004,9 +2003,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { "Session destroyed"); } if (!isIncrementalInstallation()) { - // For non-incremental installs, client staging is fully done at this point - mClientProgress = 1f; - computeProgressLocked(true); + synchronized (mProgressLock) { + // For non-incremental installs, client staging is fully done at this point + mClientProgress = 1f; + computeProgressLocked(true); + } } // This ongoing commit should keep session active, even though client @@ -2191,7 +2192,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); assertPreparedAndNotSealedLocked("transfer"); try { @@ -2376,8 +2377,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { PackageLite result = parseApkLite(); if (result != null) { mPackageLite = result; - mInternalProgress = 0.5f; - computeProgressLocked(true); + synchronized (mProgressLock) { + mInternalProgress = 0.5f; + computeProgressLocked(true); + } extractNativeLibraries( mPackageLite, stageDir, params.abiOverride, mayInheritNativeLibs()); @@ -3554,7 +3557,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { int activeCount; synchronized (mLock) { if (checkCaller) { - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); } activeCount = mActiveCount.decrementAndGet(); @@ -3593,7 +3596,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private void abandonNonStaged() { synchronized (mLock) { assertNotChildLocked("abandonNonStaged"); - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); if (mRelinquished) { if (LOGD) Slog.d(TAG, "Ignoring abandon after commit relinquished control"); return; @@ -3659,7 +3662,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); assertPreparedAndNotSealedLocked("addFile"); if (!mFiles.add(new FileEntry(mFiles.size(), @@ -3680,7 +3683,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); assertPreparedAndNotSealedLocked("removeFile"); if (!mFiles.add(new FileEntry(mFiles.size(), @@ -3893,7 +3896,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { new IPackageLoadingProgressCallback.Stub() { @Override public void onPackageLoadingProgressChanged(float progress) { - synchronized (mLock) { + synchronized (mProgressLock) { mIncrementalProgress = progress; computeProgressLocked(true); } @@ -3993,7 +3996,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { + " as it is in an invalid state."); } synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); assertPreparedAndNotSealedLocked("addChildSessionId"); final int indexOfSession = mChildSessions.indexOfKey(childSessionId); @@ -4012,7 +4015,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @Override public void removeChildSessionId(int sessionId) { synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); + assertCallerIsOwnerOrRoot(); assertPreparedAndNotSealedLocked("removeChildSessionId"); final int indexOfSession = mChildSessions.indexOfKey(sessionId);