diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index bad74a5c3de7f..a2ada46f2f971 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -402,6 +402,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private boolean mDataLoaderFinished = false; + // TODO(b/159663586): should be protected by mLock private IncrementalFileStorages mIncrementalFileStorages; private static final FileFilter sAddedApkFilter = new FileFilter() { @@ -1355,7 +1356,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) { Objects.requireNonNull(statusReceiver); - List childSessions = getChildSessions(); + List childSessions = getChildSessionsNotLocked(); synchronized (mLock) { assertCallerIsOwnerOrRootLocked(); @@ -1438,7 +1439,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { * *

This method is handy to prevent potential deadlocks (b/123391593) */ - private @Nullable List getChildSessions() { + private @Nullable List getChildSessionsNotLocked() { + if (Thread.holdsLock(mLock)) { + Slog.wtf(TAG, "Calling thread " + Thread.currentThread().getName() + + " is holding mLock", new Throwable()); + } List childSessions = null; if (isMultiPackage()) { final int[] childSessionIds = getChildSessionIds(); @@ -1607,7 +1612,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return; } } - List childSessions = getChildSessions(); + List childSessions = getChildSessionsNotLocked(); synchronized (mLock) { try { sealLocked(childSessions); @@ -1651,7 +1656,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throw new SecurityException("Can only transfer sessions that use public options"); } - List childSessions = getChildSessions(); + List childSessions = getChildSessionsNotLocked(); synchronized (mLock) { assertCallerIsOwnerOrRootLocked(); @@ -1703,7 +1708,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // outside of the lock, because reading the child // sessions with the lock held could lead to deadlock // (b/123391593). - List childSessions = getChildSessions(); + List childSessions = getChildSessionsNotLocked(); try { synchronized (mLock) { @@ -2603,6 +2608,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { "Session " + sessionId + " is a child of multi-package session " + mParentSessionId + " and may not be abandoned directly."); } + + List childSessions = getChildSessionsNotLocked(); synchronized (mLock) { if (params.isStaged && mDestroyed) { // If a user abandons staged session in an unsafe state, then system will try to @@ -2626,7 +2633,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mCallback.onStagedSessionChanged(this); return; } - cleanStageDir(); + cleanStageDir(childSessions); } if (mRelinquished) { @@ -3056,7 +3063,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mStagedSessionErrorMessage = errorMessage; Slog.d(TAG, "Marking session " + sessionId + " as failed: " + errorMessage); } - cleanStageDir(); + cleanStageDirNotLocked(); mCallback.onStagedSessionChanged(this); } @@ -3071,7 +3078,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mStagedSessionErrorMessage = ""; Slog.d(TAG, "Marking session " + sessionId + " as applied"); } - cleanStageDir(); + cleanStageDirNotLocked(); mCallback.onStagedSessionChanged(this); } @@ -3129,20 +3136,37 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } - private void cleanStageDir() { - if (isMultiPackage()) { - for (int childSessionId : getChildSessionIds()) { - mSessionProvider.getSession(childSessionId).cleanStageDir(); + /** + * must not hold {@link #mLock} + */ + private void cleanStageDirNotLocked() { + if (Thread.holdsLock(mLock)) { + Slog.wtf(TAG, "Calling thread " + Thread.currentThread().getName() + + " is holding mLock", new Throwable()); + } + cleanStageDir(getChildSessionsNotLocked()); + } + + private void cleanStageDir(List childSessions) { + if (childSessions != null) { + for (PackageInstallerSession childSession : childSessions) { + if (childSession != null) { + childSession.cleanStageDir(); + } } } else { - if (mIncrementalFileStorages != null) { - mIncrementalFileStorages.cleanUp(); - mIncrementalFileStorages = null; - } - try { - mPm.mInstaller.rmPackageDir(stageDir.getAbsolutePath()); - } catch (InstallerException ignored) { - } + cleanStageDir(); + } + } + + private void cleanStageDir() { + if (mIncrementalFileStorages != null) { + mIncrementalFileStorages.cleanUp(); + mIncrementalFileStorages = null; + } + try { + mPm.mInstaller.rmPackageDir(stageDir.getAbsolutePath()); + } catch (InstallerException ignored) { } }