Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove deadlock/lock contention on Incremental progress update.

Browse Source 
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.

(This is a cherry-pick to resolve merging conflict)

Bug: 160635296
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest PackageManagerServiceTest ChecksumsTest
Change-Id: I6120443a03ec22d1ca672e5a1acf8b3752e1c3ee
Merged-In: I6120443a03ec22d1ca672e5a1acf8b3752e1c3ee
This commit is contained in:
Alex Buynytskyy
2021-03-30 21:06:15 -07:00
parent 9a601a015a
commit c9e4cf30f0
1 changed files with 42 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

81
services/core/java/com/android/server/pm/PackageInstallerSession.java
View File

@@ -309,23 +309,24 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
private final String mOriginalInstallerPackageName; private final String mOriginalInstallerPackageName;
/** Uid of the owner of the installer session */ /** Uid of the owner of the installer session */
@GuardedBy("mLock") private volatile int mInstallerUid;
private int mInstallerUid;
/** Where this install request came from */ /** Where this install request came from */
@GuardedBy("mLock") @GuardedBy("mLock")
private InstallSource mInstallSource; private InstallSource mInstallSource;
@GuardedBy("mLock") private final Object mProgressLock = new Object();
@GuardedBy("mProgressLock")
private float mClientProgress = 0; private float mClientProgress = 0;
@GuardedBy("mLock") @GuardedBy("mProgressLock")
private float mInternalProgress = 0; private float mInternalProgress = 0;
@GuardedBy("mLock") @GuardedBy("mProgressLock")
private float mProgress = 0; private float mProgress = 0;
@GuardedBy("mLock") @GuardedBy("mProgressLock")
private float mReportedProgress = -1; private float mReportedProgress = -1;
@GuardedBy("mLock") @GuardedBy("mProgressLock")
private float mIncrementalProgress = 0; private float mIncrementalProgress = 0;
/** State of the session. */ /** State of the session. */
@@ -571,7 +572,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
*/ */
@Override @Override
public void installSession(IntentSender statusReceiver) { public void installSession(IntentSender statusReceiver) {
assertCallerIsOwnerOrRootOrSystemLocked(); assertCallerIsOwnerOrRootOrSystem();
assertNotChildLocked("StagedSession#installSession"); assertNotChildLocked("StagedSession#installSession");
Preconditions.checkArgument(isCommitted() && isSessionReady()); Preconditions.checkArgument(isCommitted() && isSessionReady());
@@ -670,7 +671,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
final Runnable r; final Runnable r;
synchronized (mLock) { synchronized (mLock) {
assertNotChildLocked("StagedSession#abandon"); assertNotChildLocked("StagedSession#abandon");
assertCallerIsOwnerOrRootLocked(); assertCallerIsOwnerOrRoot();
if (isInTerminalState()) { if (isInTerminalState()) {
// We keep the session in the database if it's in a finalized state. It will be // 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. // removed by PackageInstallerService when the last update time is old enough.
@@ -738,7 +739,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
*/ */
@Override @Override
public void verifySession() { public void verifySession() {
assertCallerIsOwnerOrRootOrSystemLocked(); assertCallerIsOwnerOrRootOrSystem();
Preconditions.checkArgument(isCommitted()); Preconditions.checkArgument(isCommitted());
Preconditions.checkArgument(!mSessionApplied && !mSessionFailed); Preconditions.checkArgument(!mSessionApplied && !mSessionFailed);
notifyStartPreRebootVerification(); notifyStartPreRebootVerification();
@@ -1224,7 +1225,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
} }
} }
@GuardedBy("mLock") @GuardedBy("mProgressLock")
private void setClientProgressLocked(float progress) { private void setClientProgressLocked(float progress) {
// Always publish first staging movement // Always publish first staging movement
final boolean forcePublish = (mClientProgress == 0); final boolean forcePublish = (mClientProgress == 0);
@@ -1234,21 +1235,21 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
@Override @Override
public void setClientProgress(float progress) { public void setClientProgress(float progress) {
synchronized (mLock) { assertCallerIsOwnerOrRoot();
assertCallerIsOwnerOrRootLocked(); synchronized (mProgressLock) {
setClientProgressLocked(progress); setClientProgressLocked(progress);
} }
} }
@Override @Override
public void addClientProgress(float progress) { public void addClientProgress(float progress) {
synchronized (mLock) { assertCallerIsOwnerOrRoot();
assertCallerIsOwnerOrRootLocked(); synchronized (mProgressLock) {
setClientProgressLocked(mClientProgress + progress); setClientProgressLocked(mClientProgress + progress);
} }
} }
@GuardedBy("mLock") @GuardedBy("mProgressLock")
private void computeProgressLocked(boolean forcePublish) { private void computeProgressLocked(boolean forcePublish) {
if (!mCommitted) { if (!mCommitted) {
mProgress = MathUtils.constrain(mClientProgress * 0.8f, 0f, 0.8f) mProgress = MathUtils.constrain(mClientProgress * 0.8f, 0f, 0.8f)
@@ -1273,8 +1274,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
@Override @Override
public String[] getNames() { public String[] getNames() {
assertCallerIsOwnerOrRoot();
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked();
assertPreparedAndNotCommittedOrDestroyedLocked("getNames"); assertPreparedAndNotCommittedOrDestroyedLocked("getNames");
return getNamesLocked(); return getNamesLocked();
@@ -1357,8 +1358,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
} }
} }
assertCallerIsOwnerOrRoot();
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked();
assertPreparedAndNotCommittedOrDestroyedLocked("addChecksums"); assertPreparedAndNotCommittedOrDestroyedLocked("addChecksums");
if (mChecksums.containsKey(name)) { if (mChecksums.containsKey(name)) {
@@ -1379,8 +1380,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
throw new IllegalStateException("Must specify package name to remove a split"); throw new IllegalStateException("Must specify package name to remove a split");
} }
assertCallerIsOwnerOrRoot();
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked();
assertPreparedAndNotCommittedOrDestroyedLocked("removeSplit"); assertPreparedAndNotCommittedOrDestroyedLocked("removeSplit");
try { try {
@@ -1426,8 +1427,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
throw new IllegalStateException( throw new IllegalStateException(
"Cannot write regular files in a data loader installation session."); "Cannot write regular files in a data loader installation session.");
} }
assertCallerIsOwnerOrRoot();
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked();
assertPreparedAndNotSealedLocked("assertCanWrite"); assertPreparedAndNotSealedLocked("assertCanWrite");
} }
if (reverseMode) { if (reverseMode) {
@@ -1600,8 +1601,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
throw new IllegalStateException( throw new IllegalStateException(
"Cannot read regular files in a data loader installation session."); "Cannot read regular files in a data loader installation session.");
} }
assertCallerIsOwnerOrRoot();
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked();
assertPreparedAndNotCommittedOrDestroyedLocked("openRead"); assertPreparedAndNotCommittedOrDestroyedLocked("openRead");
try { try {
return openReadInternalLocked(name); return openReadInternalLocked(name);
@@ -1629,8 +1630,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
* Check if the caller is the owner of this session. Otherwise throw a * Check if the caller is the owner of this session. Otherwise throw a
* {@link SecurityException}. * {@link SecurityException}.
*/ */
@GuardedBy("mLock") private void assertCallerIsOwnerOrRoot() {
private void assertCallerIsOwnerOrRootLocked() {
final int callingUid = Binder.getCallingUid(); final int callingUid = Binder.getCallingUid();
if (callingUid != Process.ROOT_UID && callingUid != mInstallerUid) { if (callingUid != Process.ROOT_UID && callingUid != mInstallerUid) {
throw new SecurityException("Session does not belong to uid " + callingUid); throw new SecurityException("Session does not belong to uid " + callingUid);
@@ -1641,8 +1641,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
* Check if the caller is the owner of this session. Otherwise throw a * Check if the caller is the owner of this session. Otherwise throw a
* {@link SecurityException}. * {@link SecurityException}.
*/ */
@GuardedBy("mLock") private void assertCallerIsOwnerOrRootOrSystem() {
private void assertCallerIsOwnerOrRootOrSystemLocked() {
final int callingUid = Binder.getCallingUid(); final int callingUid = Binder.getCallingUid();
if (callingUid != Process.ROOT_UID && callingUid != mInstallerUid if (callingUid != Process.ROOT_UID && callingUid != mInstallerUid
&& callingUid != Process.SYSTEM_UID) { && callingUid != Process.SYSTEM_UID) {
@@ -1924,9 +1923,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
*/ */
private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) { private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) {
Objects.requireNonNull(statusReceiver); Objects.requireNonNull(statusReceiver);
assertCallerIsOwnerOrRoot();
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked();
assertPreparedAndNotDestroyedLocked("commit of session " + sessionId); assertPreparedAndNotDestroyedLocked("commit of session " + sessionId);
assertNoWriteFileTransfersOpenLocked(); assertNoWriteFileTransfersOpenLocked();
@@ -1999,9 +1998,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
"Session destroyed"); "Session destroyed");
} }
if (!isIncrementalInstallation()) { if (!isIncrementalInstallation()) {
// For non-incremental installs, client staging is fully done at this point synchronized (mProgressLock) {
mClientProgress = 1f; // For non-incremental installs, client staging is fully done at this point
computeProgressLocked(true); mClientProgress = 1f;
computeProgressLocked(true);
}
} }
// This ongoing commit should keep session active, even though client // This ongoing commit should keep session active, even though client
@@ -2184,7 +2185,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
} }
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked(); assertCallerIsOwnerOrRoot();
assertPreparedAndNotSealedLocked("transfer"); assertPreparedAndNotSealedLocked("transfer");
try { try {
@@ -2364,8 +2365,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
if (result != null) { if (result != null) {
mPackageLite = result; mPackageLite = result;
if (!isApexSession()) { if (!isApexSession()) {
mInternalProgress = 0.5f; synchronized (mProgressLock) {
computeProgressLocked(true); mInternalProgress = 0.5f;
computeProgressLocked(true);
}
extractNativeLibraries( extractNativeLibraries(
mPackageLite, stageDir, params.abiOverride, mayInheritNativeLibs()); mPackageLite, stageDir, params.abiOverride, mayInheritNativeLibs());
@@ -3549,7 +3552,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
int activeCount; int activeCount;
synchronized (mLock) { synchronized (mLock) {
if (checkCaller) { if (checkCaller) {
assertCallerIsOwnerOrRootLocked(); assertCallerIsOwnerOrRoot();
} }
activeCount = mActiveCount.decrementAndGet(); activeCount = mActiveCount.decrementAndGet();
@@ -3588,7 +3591,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
private void abandonNonStaged() { private void abandonNonStaged() {
synchronized (mLock) { synchronized (mLock) {
assertNotChildLocked("abandonNonStaged"); assertNotChildLocked("abandonNonStaged");
assertCallerIsOwnerOrRootLocked(); assertCallerIsOwnerOrRoot();
if (mRelinquished) { if (mRelinquished) {
if (LOGD) Slog.d(TAG, "Ignoring abandon after commit relinquished control"); if (LOGD) Slog.d(TAG, "Ignoring abandon after commit relinquished control");
return; return;
@@ -3654,7 +3657,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
} }
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked(); assertCallerIsOwnerOrRoot();
assertPreparedAndNotSealedLocked("addFile"); assertPreparedAndNotSealedLocked("addFile");
if (!mFiles.add(new FileEntry(mFiles.size(), if (!mFiles.add(new FileEntry(mFiles.size(),
@@ -3675,7 +3678,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
} }
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked(); assertCallerIsOwnerOrRoot();
assertPreparedAndNotSealedLocked("removeFile"); assertPreparedAndNotSealedLocked("removeFile");
if (!mFiles.add(new FileEntry(mFiles.size(), if (!mFiles.add(new FileEntry(mFiles.size(),
@@ -3888,7 +3891,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
new IPackageLoadingProgressCallback.Stub() { new IPackageLoadingProgressCallback.Stub() {
@Override @Override
public void onPackageLoadingProgressChanged(float progress) { public void onPackageLoadingProgressChanged(float progress) {
synchronized (mLock) { synchronized (mProgressLock) {
mIncrementalProgress = progress; mIncrementalProgress = progress;
computeProgressLocked(true); computeProgressLocked(true);
} }
@@ -3988,7 +3991,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
+ " as it is in an invalid state."); + " as it is in an invalid state.");
} }
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked(); assertCallerIsOwnerOrRoot();
assertPreparedAndNotSealedLocked("addChildSessionId"); assertPreparedAndNotSealedLocked("addChildSessionId");
final int indexOfSession = mChildSessions.indexOfKey(childSessionId); final int indexOfSession = mChildSessions.indexOfKey(childSessionId);
@@ -4007,7 +4010,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
@Override @Override
public void removeChildSessionId(int sessionId) { public void removeChildSessionId(int sessionId) {
synchronized (mLock) { synchronized (mLock) {
assertCallerIsOwnerOrRootLocked(); assertCallerIsOwnerOrRoot();
assertPreparedAndNotSealedLocked("removeChildSessionId"); assertPreparedAndNotSealedLocked("removeChildSessionId");
final int indexOfSession = mChildSessions.indexOfKey(sessionId); final int indexOfSession = mChildSessions.indexOfKey(sessionId);