Prevent a deadlock in #abandon
See b/159568595#comment1 for analysis.
1. rename cleanStageDir to cleanStageDirNotLocked to show it must be
called without mLock held.
2. rename getChildSessions to getChildSessionsNotLocked to show it must
be called without mLock held.
3. add cleanStageDir(List<PackageInstallerSession>) which is callable
when mLock is held.
Bug: 159568595
Test: atest StagedInstallTest
Merged-In: I512ea07a4f8750431b306fa6e3c47a600a50b64f
Change-Id: I512ea07a4f8750431b306fa6e3c47a600a50b64f
(cherry picked from commit 4ce1273f926851bb58d44f59b5643373d4567c81)
diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index de8ad6b..994cec2 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -401,6 +401,7 @@
private boolean mDataLoaderFinished = false;
+ // TODO(b/159663586): should be protected by mLock
private IncrementalFileStorages mIncrementalFileStorages;
private static final FileFilter sAddedApkFilter = new FileFilter() {
@@ -1353,7 +1354,7 @@
private boolean markAsSealed(@NonNull IntentSender statusReceiver, boolean forTransfer) {
Objects.requireNonNull(statusReceiver);
- List<PackageInstallerSession> childSessions = getChildSessions();
+ List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
synchronized (mLock) {
assertCallerIsOwnerOrRootLocked();
@@ -1436,7 +1437,11 @@
*
* <p> This method is handy to prevent potential deadlocks (b/123391593)
*/
- private @Nullable List<PackageInstallerSession> getChildSessions() {
+ private @Nullable List<PackageInstallerSession> getChildSessionsNotLocked() {
+ if (Thread.holdsLock(mLock)) {
+ Slog.wtf(TAG, "Calling thread " + Thread.currentThread().getName()
+ + " is holding mLock", new Throwable());
+ }
List<PackageInstallerSession> childSessions = null;
if (isMultiPackage()) {
final int[] childSessionIds = getChildSessionIds();
@@ -1605,7 +1610,7 @@
return;
}
}
- List<PackageInstallerSession> childSessions = getChildSessions();
+ List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
synchronized (mLock) {
try {
sealLocked(childSessions);
@@ -1649,7 +1654,7 @@
throw new SecurityException("Can only transfer sessions that use public options");
}
- List<PackageInstallerSession> childSessions = getChildSessions();
+ List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
synchronized (mLock) {
assertCallerIsOwnerOrRootLocked();
@@ -1701,7 +1706,7 @@
// outside of the lock, because reading the child
// sessions with the lock held could lead to deadlock
// (b/123391593).
- List<PackageInstallerSession> childSessions = getChildSessions();
+ List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
try {
synchronized (mLock) {
@@ -2602,6 +2607,8 @@
"Session " + sessionId + " is a child of multi-package session "
+ mParentSessionId + " and may not be abandoned directly.");
}
+
+ List<PackageInstallerSession> childSessions = getChildSessionsNotLocked();
synchronized (mLock) {
if (params.isStaged && mDestroyed) {
// If a user abandons staged session in an unsafe state, then system will try to
@@ -2625,7 +2632,7 @@
mCallback.onStagedSessionChanged(this);
return;
}
- cleanStageDir();
+ cleanStageDir(childSessions);
}
if (mRelinquished) {
@@ -3055,7 +3062,7 @@
mStagedSessionErrorMessage = errorMessage;
Slog.d(TAG, "Marking session " + sessionId + " as failed: " + errorMessage);
}
- cleanStageDir();
+ cleanStageDirNotLocked();
mCallback.onStagedSessionChanged(this);
}
@@ -3070,7 +3077,7 @@
mStagedSessionErrorMessage = "";
Slog.d(TAG, "Marking session " + sessionId + " as applied");
}
- cleanStageDir();
+ cleanStageDirNotLocked();
mCallback.onStagedSessionChanged(this);
}
@@ -3128,20 +3135,37 @@
}
}
- private void cleanStageDir() {
- if (isMultiPackage()) {
- for (int childSessionId : getChildSessionIds()) {
- mSessionProvider.getSession(childSessionId).cleanStageDir();
+ /**
+ * <b>must not hold {@link #mLock}</b>
+ */
+ 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<PackageInstallerSession> 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) {
}
}