From 64f88fa17344ecea8c9503c9f0ba95a8f9e754bb Mon Sep 17 00:00:00 2001 From: Jing Ji Date: Fri, 19 Feb 2021 14:05:24 -0800 Subject: [PATCH] Handle the potential recursions within updateOomAdj Some code paths within the updateOomAdj* would potentially call into the updateOomAdj* again and result in undefined behavior. This CL adds the handling for it - in case of recursion is encountered, the oomAdj update request will be enqueued, and at the end of the current pass of updateOomAdj*, we'll do another updateOomAdj* to handle those requests. Bug: 167773591 Bug: 162476856 Test: atest CtsAppTestCases Test: atest MockingOomAdjusterTests Change-Id: I4788ba12f06744204c68382b399216f232059138 --- .../com/android/server/am/OomAdjuster.java | 129 +++++++++++++- .../server/am/MockingOomAdjusterTests.java | 162 +++++++++++++++++- 2 files changed, 276 insertions(+), 15 deletions(-) diff --git a/services/core/java/com/android/server/am/OomAdjuster.java b/services/core/java/com/android/server/am/OomAdjuster.java index d79fb8a265e88..0a8016cf05561 100644 --- a/services/core/java/com/android/server/am/OomAdjuster.java +++ b/services/core/java/com/android/server/am/OomAdjuster.java @@ -72,6 +72,7 @@ import static com.android.server.am.AppProfiler.TAG_PSS; import static com.android.server.am.ProcessList.TAG_PROCESS_OBSERVERS; import static com.android.server.wm.ActivityTaskManagerDebugConfig.DEBUG_SWITCH; +import android.annotation.Nullable; import android.app.ActivityManager; import android.app.ActivityThread; import android.app.ApplicationExitInfo; @@ -229,6 +230,22 @@ public final class OomAdjuster { private final ArrayDeque mTmpQueue; private final ArraySet mPendingProcessSet = new ArraySet<>(); + /** + * Flag to mark if there is an ongoing oomAdjUpdate: potentially the oomAdjUpdate + * could be called recursively because of the indirect calls during the update; + * however the oomAdjUpdate itself doesn't support recursion - in this case we'd + * have to queue up the new targets found during the update, and perform another + * round of oomAdjUpdate at the end of last update. + */ + @GuardedBy("mService") + private boolean mOomAdjUpdateOngoing = false; + + /** + * Flag to mark if there is a pending full oomAdjUpdate. + */ + @GuardedBy("mService") + private boolean mPendingFullOomAdjUpdate = false; + private final PlatformCompatCache mPlatformCompatCache; private static class PlatformCompatCache { @@ -439,6 +456,23 @@ public final class OomAdjuster { if (oomAdjAll && mConstants.OOMADJ_UPDATE_QUICK) { return updateOomAdjLSP(app, oomAdjReason); } + if (checkAndEnqueueOomAdjTargetLocked(app)) { + // Simply return true as there is an oomAdjUpdate ongoing + return true; + } + try { + mOomAdjUpdateOngoing = true; + return performUpdateOomAdjLSP(app, oomAdjAll, oomAdjReason); + } finally { + // Kick off the handling of any pending targets enqueued during the above update + mOomAdjUpdateOngoing = false; + updateOomAdjPendingTargetsLocked(oomAdjReason); + } + } + + @GuardedBy({"mService", "mProcLock"}) + private boolean performUpdateOomAdjLSP(ProcessRecord app, boolean oomAdjAll, + String oomAdjReason) { final ProcessRecord topApp = mService.getTopApp(); final ProcessStateRecord state = app.mState; final boolean wasCached = state.isCached(); @@ -453,20 +487,21 @@ public final class OomAdjuster { ? state.getCurRawAdj() : ProcessList.UNKNOWN_ADJ; // Check if this process is in the pending list too, remove from pending list if so. mPendingProcessSet.remove(app); - boolean success = updateOomAdjLSP(app, cachedAdj, topApp, false, + boolean success = performUpdateOomAdjLSP(app, cachedAdj, topApp, false, SystemClock.uptimeMillis()); if (oomAdjAll && (wasCached != state.isCached() || state.getCurRawAdj() == ProcessList.UNKNOWN_ADJ)) { // Changed to/from cached state, so apps after it in the LRU // list may also be changed. - updateOomAdjLSP(oomAdjReason); + performUpdateOomAdjLSP(oomAdjReason); } + return success; } @GuardedBy({"mService", "mProcLock"}) - private boolean updateOomAdjLSP(ProcessRecord app, int cachedAdj, + private boolean performUpdateOomAdjLSP(ProcessRecord app, int cachedAdj, ProcessRecord TOP_APP, boolean doingAll, long now) { if (app.getThread() == null) { return false; @@ -519,6 +554,22 @@ public final class OomAdjuster { @GuardedBy({"mService", "mProcLock"}) private void updateOomAdjLSP(String oomAdjReason) { + if (checkAndEnqueueOomAdjTargetLocked(null)) { + // Simply return as there is an oomAdjUpdate ongoing + return; + } + try { + mOomAdjUpdateOngoing = true; + performUpdateOomAdjLSP(oomAdjReason); + } finally { + // Kick off the handling of any pending targets enqueued during the above update + mOomAdjUpdateOngoing = false; + updateOomAdjPendingTargetsLocked(oomAdjReason); + } + } + + @GuardedBy({"mService", "mProcLock"}) + private void performUpdateOomAdjLSP(String oomAdjReason) { final ProcessRecord topApp = mService.getTopApp(); // Clear any pending ones because we are doing a full update now. mPendingProcessSet.clear(); @@ -548,6 +599,23 @@ public final class OomAdjuster { return true; } + if (checkAndEnqueueOomAdjTargetLocked(app)) { + // Simply return true as there is an oomAdjUpdate ongoing + return true; + } + + try { + mOomAdjUpdateOngoing = true; + return performUpdateOomAdjLSP(app, oomAdjReason); + } finally { + // Kick off the handling of any pending targets enqueued during the above update + mOomAdjUpdateOngoing = false; + updateOomAdjPendingTargetsLocked(oomAdjReason); + } + } + + @GuardedBy({"mService", "mProcLock"}) + private boolean performUpdateOomAdjLSP(ProcessRecord app, String oomAdjReason) { final ProcessRecord topApp = mService.getTopApp(); Trace.traceBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, oomAdjReason); @@ -567,7 +635,7 @@ public final class OomAdjuster { state.resetCachedInfo(); // Check if this process is in the pending list too, remove from pending list if so. mPendingProcessSet.remove(app); - boolean success = updateOomAdjLSP(app, cachedAdj, topApp, false, + boolean success = performUpdateOomAdjLSP(app, cachedAdj, topApp, false, SystemClock.uptimeMillis()); if (!success || (wasCached == state.isCached() && oldAdj != ProcessList.INVALID_ADJ && wasBackground == ActivityManager.isProcStateBackground( @@ -695,15 +763,60 @@ public final class OomAdjuster { } } + /** + * Check if there is an ongoing oomAdjUpdate, enqueue the given process record + * to {@link #mPendingProcessSet} if there is one. + * + * @param app The target app to get an oomAdjUpdate, or a full oomAdjUpdate if it's null. + * @return {@code true} if there is an ongoing oomAdjUpdate. + */ + @GuardedBy("mService") + private boolean checkAndEnqueueOomAdjTargetLocked(@Nullable ProcessRecord app) { + if (!mOomAdjUpdateOngoing) { + return false; + } + if (app != null) { + mPendingProcessSet.add(app); + } else { + mPendingFullOomAdjUpdate = true; + } + return true; + } + /** * Kick off an oom adj update pass for the pending targets which are enqueued via * {@link #enqueueOomAdjTargetLocked}. */ @GuardedBy("mService") void updateOomAdjPendingTargetsLocked(String oomAdjReason) { + // First check if there is pending full update + if (mPendingFullOomAdjUpdate) { + mPendingFullOomAdjUpdate = false; + mPendingProcessSet.clear(); + updateOomAdjLocked(oomAdjReason); + return; + } if (mPendingProcessSet.isEmpty()) { return; } + + if (mOomAdjUpdateOngoing) { + // There's another oomAdjUpdate ongoing, return from here now; + // that ongoing update would call us again at the end of it. + return; + } + try { + mOomAdjUpdateOngoing = true; + performUpdateOomAdjPendingTargetsLocked(oomAdjReason); + } finally { + // Kick off the handling of any pending targets enqueued during the above update + mOomAdjUpdateOngoing = false; + updateOomAdjPendingTargetsLocked(oomAdjReason); + } + } + + @GuardedBy("mService") + private void performUpdateOomAdjPendingTargetsLocked(String oomAdjReason) { final ProcessRecord topApp = mService.getTopApp(); Trace.traceBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, oomAdjReason); @@ -1846,8 +1959,8 @@ public final class OomAdjuster { computeOomAdjLSP(client, cachedAdj, topApp, doingAll, now, cycleReEval, true); } else { - cstate.setCurRawAdj(cstate.getSetAdj()); - cstate.setCurRawProcState(cstate.getSetProcState()); + cstate.setCurRawAdj(cstate.getCurAdj()); + cstate.setCurRawProcState(cstate.getCurProcState()); } int clientAdj = cstate.getCurRawAdj(); @@ -2130,8 +2243,8 @@ public final class OomAdjuster { if (computeClients) { computeOomAdjLSP(client, cachedAdj, topApp, doingAll, now, cycleReEval, true); } else { - cstate.setCurRawAdj(cstate.getSetAdj()); - cstate.setCurRawProcState(cstate.getSetProcState()); + cstate.setCurRawAdj(cstate.getCurAdj()); + cstate.setCurRawProcState(cstate.getCurProcState()); } if (shouldSkipDueToCycle(state, cstate, procState, adj, cycleReEval)) { diff --git a/services/tests/mockingservicestests/src/com/android/server/am/MockingOomAdjusterTests.java b/services/tests/mockingservicestests/src/com/android/server/am/MockingOomAdjusterTests.java index 27825a461b40b..a382e85538cd5 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/MockingOomAdjusterTests.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/MockingOomAdjusterTests.java @@ -64,6 +64,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.mockito.AdditionalAnswers.answer; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyBoolean; import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.anyLong; import static org.mockito.Mockito.doAnswer; @@ -73,6 +74,8 @@ import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import android.app.ActivityManager; +import android.app.AppOpsManager; import android.app.IApplicationThread; import android.app.IServiceConnection; import android.content.ComponentName; @@ -178,11 +181,20 @@ public class MockingOomAdjusterTests { setFieldValue(ActivityManagerService.class, sService, "mAppProfiler", profiler); setFieldValue(ActivityManagerService.class, sService, "mProcLock", new ActivityManagerProcLock()); + setFieldValue(ActivityManagerService.class, sService, "mServices", + spy(new ActiveServices(sService))); + setFieldValue(ActivityManagerService.class, sService, "mInternal", + mock(ActivityManagerService.LocalService.class)); + setFieldValue(ActivityManagerService.class, sService, "mBatteryStatsService", + mock(BatteryStatsService.class)); + doReturn(mock(AppOpsManager.class)).when(sService).getAppOpsManager(); + doCallRealMethod().when(sService).enqueueOomAdjTargetLocked(any(ProcessRecord.class)); + doCallRealMethod().when(sService).updateOomAdjPendingTargetsLocked(any(String.class)); setFieldValue(AppProfiler.class, profiler, "mProfilerLock", new Object()); doReturn(new ActivityManagerService.ProcessChangeItem()).when(pr) .enqueueProcessChangeItemLocked(anyInt(), anyInt()); sService.mOomAdjuster = new OomAdjuster(sService, sService.mProcessList, - mock(ActiveUids.class)); + new ActiveUids(sService, false)); sService.mOomAdjuster.mAdjSeq = 10000; sService.mWakefulness = new AtomicInteger(PowerManagerInternal.WAKEFULNESS_AWAKE); } @@ -1313,6 +1325,115 @@ public class MockingOomAdjusterTests { SCHED_GROUP_DEFAULT); } + @SuppressWarnings("GuardedBy") + @Test + public void testUpdateOomAdj_UidIdle_StopService() { + final ProcessRecord app1 = spy(makeDefaultProcessRecord(MOCKAPP_PID, MOCKAPP_UID, + MOCKAPP_PROCESSNAME, MOCKAPP_PACKAGENAME, false)); + final ProcessRecord app2 = spy(makeDefaultProcessRecord(MOCKAPP2_PID, MOCKAPP2_UID, + MOCKAPP2_PROCESSNAME, MOCKAPP2_PACKAGENAME, false)); + final ProcessRecord client1 = spy(makeDefaultProcessRecord(MOCKAPP3_PID, MOCKAPP3_UID, + MOCKAPP3_PROCESSNAME, MOCKAPP3_PACKAGENAME, false)); + final ProcessRecord client2 = spy(makeDefaultProcessRecord(MOCKAPP4_PID, MOCKAPP3_UID, + MOCKAPP4_PROCESSNAME, MOCKAPP3_PACKAGENAME, false)); + final ProcessRecord app3 = spy(makeDefaultProcessRecord(MOCKAPP5_PID, MOCKAPP5_UID, + MOCKAPP5_PROCESSNAME, MOCKAPP5_PACKAGENAME, false)); + final UidRecord app1UidRecord = new UidRecord(MOCKAPP_UID, sService); + final UidRecord app2UidRecord = new UidRecord(MOCKAPP2_UID, sService); + final UidRecord app3UidRecord = new UidRecord(MOCKAPP5_UID, sService); + final UidRecord clientUidRecord = new UidRecord(MOCKAPP3_UID, sService); + app1.setUidRecord(app1UidRecord); + app2.setUidRecord(app2UidRecord); + app3.setUidRecord(app3UidRecord); + client1.setUidRecord(clientUidRecord); + client2.setUidRecord(clientUidRecord); + + client1.mServices.setHasForegroundServices(true, 0); + client2.mState.setForcingToImportant(new Object()); + ArrayList lru = sService.mProcessList.getLruProcessesLOSP(); + lru.clear(); + lru.add(app1); + lru.add(app2); + lru.add(app3); + lru.add(client1); + lru.add(client2); + sService.mWakefulness.set(PowerManagerInternal.WAKEFULNESS_AWAKE); + + final ComponentName cn1 = ComponentName.unflattenFromString( + MOCKAPP_PACKAGENAME + "/.TestService"); + final ServiceRecord s1 = bindService(app1, client1, null, 0, mock(IBinder.class)); + setFieldValue(ServiceRecord.class, s1, "name", cn1); + s1.startRequested = true; + + final ComponentName cn2 = ComponentName.unflattenFromString( + MOCKAPP2_PACKAGENAME + "/.TestService"); + final ServiceRecord s2 = bindService(app2, client2, null, 0, mock(IBinder.class)); + setFieldValue(ServiceRecord.class, s2, "name", cn2); + s2.startRequested = true; + + final ComponentName cn3 = ComponentName.unflattenFromString( + MOCKAPP5_PACKAGENAME + "/.TestService"); + final ServiceRecord s3 = bindService(app3, client1, null, 0, mock(IBinder.class)); + setFieldValue(ServiceRecord.class, s3, "name", cn3); + s3.startRequested = true; + + final ComponentName cn4 = ComponentName.unflattenFromString( + MOCKAPP3_PACKAGENAME + "/.TestService"); + final ServiceRecord c2s = makeServiceRecord(client2); + setFieldValue(ServiceRecord.class, c2s, "name", cn4); + c2s.startRequested = true; + + try { + sService.mOomAdjuster.mActiveUids.put(MOCKAPP_UID, app1UidRecord); + sService.mOomAdjuster.mActiveUids.put(MOCKAPP2_UID, app2UidRecord); + sService.mOomAdjuster.mActiveUids.put(MOCKAPP5_UID, app3UidRecord); + sService.mOomAdjuster.mActiveUids.put(MOCKAPP3_UID, clientUidRecord); + + setServiceMap(s1, MOCKAPP_UID, cn1); + setServiceMap(s2, MOCKAPP2_UID, cn2); + setServiceMap(s3, MOCKAPP5_UID, cn3); + setServiceMap(c2s, MOCKAPP3_UID, cn4); + app2UidRecord.setIdle(false); + sService.mOomAdjuster.updateOomAdjLocked(OomAdjuster.OOM_ADJ_REASON_NONE); + + assertProcStates(app1, PROCESS_STATE_FOREGROUND_SERVICE, PERCEPTIBLE_APP_ADJ, + SCHED_GROUP_DEFAULT); + assertProcStates(app3, PROCESS_STATE_FOREGROUND_SERVICE, PERCEPTIBLE_APP_ADJ, + SCHED_GROUP_DEFAULT); + assertProcStates(client1, PROCESS_STATE_FOREGROUND_SERVICE, PERCEPTIBLE_APP_ADJ, + SCHED_GROUP_DEFAULT); + assertEquals(PROCESS_STATE_TRANSIENT_BACKGROUND, app2.mState.getSetProcState()); + assertEquals(PROCESS_STATE_TRANSIENT_BACKGROUND, client2.mState.getSetProcState()); + + client1.mServices.setHasForegroundServices(false, 0); + client2.mState.setForcingToImportant(null); + app1UidRecord.reset(); + app2UidRecord.reset(); + app3UidRecord.reset(); + clientUidRecord.reset(); + app1UidRecord.setIdle(true); + app2UidRecord.setIdle(true); + app3UidRecord.setIdle(true); + clientUidRecord.setIdle(true); + doReturn(ActivityManager.APP_START_MODE_DELAYED).when(sService) + .getAppStartModeLOSP(anyInt(), any(String.class), anyInt(), + anyInt(), anyBoolean(), anyBoolean(), anyBoolean()); + doNothing().when(sService.mServices) + .scheduleServiceTimeoutLocked(any(ProcessRecord.class)); + sService.mOomAdjuster.updateOomAdjLocked(client1, OomAdjuster.OOM_ADJ_REASON_NONE); + + assertEquals(PROCESS_STATE_CACHED_EMPTY, client1.mState.getSetProcState()); + assertEquals(PROCESS_STATE_SERVICE, app1.mState.getSetProcState()); + assertEquals(PROCESS_STATE_SERVICE, client2.mState.getSetProcState()); + } finally { + doCallRealMethod().when(sService) + .getAppStartModeLOSP(anyInt(), any(String.class), anyInt(), + anyInt(), anyBoolean(), anyBoolean(), anyBoolean()); + sService.mServices.mServiceMap.clear(); + sService.mOomAdjuster.mActiveUids.clear(); + } + } + @SuppressWarnings("GuardedBy") @Test public void testUpdateOomAdj_DoAll_Unbound() { @@ -1815,15 +1936,42 @@ public class MockingOomAdjusterTests { return app; } + private ServiceRecord makeServiceRecord(ProcessRecord app) { + final ServiceRecord record = mock(ServiceRecord.class); + record.app = app; + setFieldValue(ServiceRecord.class, record, "connections", + new ArrayMap>()); + doCallRealMethod().when(record).getConnections(); + setFieldValue(ServiceRecord.class, record, "packageName", app.info.packageName); + app.mServices.startService(record); + record.appInfo = app.info; + setFieldValue(ServiceRecord.class, record, "bindings", new ArrayMap<>()); + setFieldValue(ServiceRecord.class, record, "pendingStarts", new ArrayList<>()); + return record; + } + + private void setServiceMap(ServiceRecord s, int uid, ComponentName cn) { + ActiveServices.ServiceMap serviceMap = sService.mServices.mServiceMap.get( + UserHandle.getUserId(uid)); + if (serviceMap == null) { + serviceMap = mock(ActiveServices.ServiceMap.class); + setFieldValue(ActiveServices.ServiceMap.class, serviceMap, "mServicesByInstanceName", + new ArrayMap<>()); + setFieldValue(ActiveServices.ServiceMap.class, serviceMap, "mActiveForegroundApps", + new ArrayMap<>()); + setFieldValue(ActiveServices.ServiceMap.class, serviceMap, "mServicesByIntent", + new ArrayMap<>()); + setFieldValue(ActiveServices.ServiceMap.class, serviceMap, "mDelayedStartList", + new ArrayList<>()); + sService.mServices.mServiceMap.put(UserHandle.getUserId(uid), serviceMap); + } + serviceMap.mServicesByInstanceName.put(cn, s); + } + private ServiceRecord bindService(ProcessRecord service, ProcessRecord client, ServiceRecord record, int bindFlags, IBinder binder) { if (record == null) { - record = mock(ServiceRecord.class); - record.app = service; - setFieldValue(ServiceRecord.class, record, "connections", - new ArrayMap>()); - service.mServices.startService(record); - doCallRealMethod().when(record).getConnections(); + record = makeServiceRecord(service); } AppBindRecord binding = new AppBindRecord(record, null, client); ConnectionRecord cr = spy(new ConnectionRecord(binding,