From bed0dd5e7254c686c12f1a963a4fdaebf0da7817 Mon Sep 17 00:00:00 2001 From: Kevin Han Date: Thu, 2 Sep 2021 23:42:05 -0700 Subject: [PATCH] Don't hold hibernation lock when calling other services Fix deadlock issues by ensuring that calls to activity manager and package manager are done without holding the hibernation lock. This will loosen some guarantees on the hibernation state (e.g. there may be a brief period of time where an app is hibernated but not force-stopped), but none of the callers currently need that strong guarantee. Bug: 196535754 Test: atest AppHibernationIntegrationTest Test: atest AppHibernationServiceTest Test: adb shell cmd jobscheduler run -u 0 -f com.google.android.permissioncontroller 2 Change-Id: Ic67d9a93565a935c077707e3c34a4c6605f041d1 (cherry picked from commit d41db18129f4119218c6eda21a7d51c0313b18c1) --- .../apphibernation/AppHibernationService.java | 72 ++++++++----------- .../AppHibernationServiceTest.java | 6 +- 2 files changed, 35 insertions(+), 43 deletions(-) diff --git a/services/core/java/com/android/server/apphibernation/AppHibernationService.java b/services/core/java/com/android/server/apphibernation/AppHibernationService.java index 5cfcb4c5f8a81..4d025c981ce94 100644 --- a/services/core/java/com/android/server/apphibernation/AppHibernationService.java +++ b/services/core/java/com/android/server/apphibernation/AppHibernationService.java @@ -277,16 +277,16 @@ public final class AppHibernationService extends SystemService { getContext().enforceCallingOrSelfPermission( android.Manifest.permission.MANAGE_APP_HIBERNATION, "Caller does not have MANAGE_APP_HIBERNATION permission."); - userId = handleIncomingUser(userId, methodName); - if (!checkUserStatesExist(userId, methodName)) { + final int realUserId = handleIncomingUser(userId, methodName); + if (!checkUserStatesExist(realUserId, methodName)) { return; } synchronized (mLock) { - final Map packageStates = mUserStates.get(userId); + final Map packageStates = mUserStates.get(realUserId); final UserLevelState pkgState = packageStates.get(packageName); if (pkgState == null) { Slog.e(TAG, String.format("Package %s is not installed for user %s", - packageName, userId)); + packageName, realUserId)); return; } @@ -294,13 +294,17 @@ public final class AppHibernationService extends SystemService { return; } + pkgState.hibernated = isHibernating; if (isHibernating) { - hibernatePackageForUser(packageName, userId, pkgState); + mBackgroundExecutor.execute(() -> hibernatePackageForUser(packageName, realUserId)); } else { - unhibernatePackageForUser(packageName, userId, pkgState); + mBackgroundExecutor.execute( + () -> unhibernatePackageForUser(packageName, realUserId)); + pkgState.lastUnhibernatedMs = System.currentTimeMillis(); } + final UserLevelState stateSnapshot = new UserLevelState(pkgState); - final int userIdSnapshot = userId; + final int userIdSnapshot = realUserId; mBackgroundExecutor.execute(() -> { FrameworkStatsLog.write( FrameworkStatsLog.USER_LEVEL_HIBERNATION_STATE_CHANGED, @@ -308,8 +312,8 @@ public final class AppHibernationService extends SystemService { userIdSnapshot, stateSnapshot.hibernated); }); - List states = new ArrayList<>(mUserStates.get(userId).values()); - mUserDiskStores.get(userId).scheduleWriteHibernationStates(states); + List states = new ArrayList<>(mUserStates.get(realUserId).values()); + mUserDiskStores.get(realUserId).scheduleWriteHibernationStates(states); } } @@ -334,10 +338,12 @@ public final class AppHibernationService extends SystemService { return; } if (state.hibernated != isHibernating) { + state.hibernated = isHibernating; if (isHibernating) { - hibernatePackageGlobally(packageName, state); + mBackgroundExecutor.execute(() -> hibernatePackageGlobally(packageName, state)); } else { - unhibernatePackageGlobally(packageName, state); + state.savedByte = 0; + state.lastUnhibernatedMs = System.currentTimeMillis(); } List states = new ArrayList<>(mGlobalHibernationStates.values()); mGlobalLevelHibernationDiskStore.scheduleWriteHibernationStates(states); @@ -374,20 +380,16 @@ public final class AppHibernationService extends SystemService { } /** - * Put an app into hibernation for a given user, allowing user-level optimizations to occur. - * - * @param pkgState package hibernation state + * Put an app into hibernation for a given user, allowing user-level optimizations to occur. Do + * not hold {@link #mLock} while calling this to avoid deadlock scenarios. */ - @GuardedBy("mLock") - private void hibernatePackageForUser(@NonNull String packageName, int userId, - @NonNull UserLevelState pkgState) { + private void hibernatePackageForUser(@NonNull String packageName, int userId) { Trace.traceBegin(Trace.TRACE_TAG_SYSTEM_SERVER, "hibernatePackage"); final long caller = Binder.clearCallingIdentity(); try { mIActivityManager.forceStopPackage(packageName, userId); mIPackageManager.deleteApplicationCacheFilesAsUser(packageName, userId, null /* observer */); - pkgState.hibernated = true; } catch (RemoteException e) { throw new IllegalStateException( "Failed to hibernate due to manager not being available", e); @@ -398,16 +400,11 @@ public final class AppHibernationService extends SystemService { } /** - * Remove a package from hibernation for a given user. - * - * @param pkgState package hibernation state + * Remove a package from hibernation for a given user. Do not hold {@link #mLock} while calling + * this. */ - @GuardedBy("mLock") - private void unhibernatePackageForUser(@NonNull String packageName, int userId, - UserLevelState pkgState) { + private void unhibernatePackageForUser(@NonNull String packageName, int userId) { Trace.traceBegin(Trace.TRACE_TAG_SYSTEM_SERVER, "unhibernatePackage"); - pkgState.hibernated = false; - pkgState.lastUnhibernatedMs = System.currentTimeMillis(); final long caller = Binder.clearCallingIdentity(); // Deliver LOCKED_BOOT_COMPLETE AND BOOT_COMPLETE broadcast so app can re-register // their alarms/jobs/etc. @@ -458,29 +455,20 @@ public final class AppHibernationService extends SystemService { } /** - * Put a package into global hibernation, optimizing its storage at a package / APK level. + * Put a package into global hibernation, optimizing its storage at a package / APK level. Do + * not hold {@link #mLock} while calling this. */ - @GuardedBy("mLock") private void hibernatePackageGlobally(@NonNull String packageName, GlobalLevelState state) { Trace.traceBegin(Trace.TRACE_TAG_SYSTEM_SERVER, "hibernatePackageGlobally"); + long savedBytes = 0; if (mOatArtifactDeletionEnabled) { - state.savedByte = Math.max( + savedBytes = Math.max( mPackageManagerInternal.deleteOatArtifactsOfPackage(packageName), 0); } - state.hibernated = true; - Trace.traceEnd(Trace.TRACE_TAG_SYSTEM_SERVER); - } - - /** - * Unhibernate a package from global hibernation. - */ - @GuardedBy("mLock") - private void unhibernatePackageGlobally(@NonNull String packageName, GlobalLevelState state) { - Trace.traceBegin(Trace.TRACE_TAG_SYSTEM_SERVER, "unhibernatePackageGlobally"); - state.hibernated = false; - state.savedByte = 0; - state.lastUnhibernatedMs = System.currentTimeMillis(); + synchronized (mLock) { + state.savedByte = savedBytes; + } Trace.traceEnd(Trace.TRACE_TAG_SYSTEM_SERVER); } diff --git a/services/tests/servicestests/src/com/android/server/apphibernation/AppHibernationServiceTest.java b/services/tests/servicestests/src/com/android/server/apphibernation/AppHibernationServiceTest.java index 5c7a580a25936..1c49e6e64dd88 100644 --- a/services/tests/servicestests/src/com/android/server/apphibernation/AppHibernationServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/apphibernation/AppHibernationServiceTest.java @@ -146,12 +146,15 @@ public final class AppHibernationServiceTest { } @Test - public void testSetHibernatingForUser_packageIsHibernating() { + public void testSetHibernatingForUser_packageIsHibernating() throws Exception { // WHEN we hibernate a package for a user mAppHibernationService.setHibernatingForUser(PACKAGE_NAME_1, USER_ID_1, true); // THEN the package is marked hibernating for the user assertTrue(mAppHibernationService.isHibernatingForUser(PACKAGE_NAME_1, USER_ID_1)); + verify(mIActivityManager).forceStopPackage(PACKAGE_NAME_1, USER_ID_1); + verify(mIPackageManager).deleteApplicationCacheFilesAsUser( + eq(PACKAGE_NAME_1), eq(USER_ID_1), any()); } @Test @@ -204,6 +207,7 @@ public final class AppHibernationServiceTest { // THEN the package is marked hibernating for the user assertTrue(mAppHibernationService.isHibernatingGlobally(PACKAGE_NAME_1)); + verify(mPackageManagerInternal).deleteOatArtifactsOfPackage(PACKAGE_NAME_1); } @Test