diff --git a/core/java/com/android/internal/os/TimeoutRecord.java b/core/java/com/android/internal/os/TimeoutRecord.java index 680f8fe6535f9..a587834e2df2d 100644 --- a/core/java/com/android/internal/os/TimeoutRecord.java +++ b/core/java/com/android/internal/os/TimeoutRecord.java @@ -40,7 +40,8 @@ public class TimeoutRecord { TimeoutKind.SERVICE_START, TimeoutKind.SERVICE_EXEC, TimeoutKind.CONTENT_PROVIDER, - TimeoutKind.APP_REGISTERED}) + TimeoutKind.APP_REGISTERED, + TimeoutKind.SHORT_FGS_TIMEOUT}) @Retention(RetentionPolicy.SOURCE) public @interface TimeoutKind { @@ -51,6 +52,7 @@ public class TimeoutRecord { int SERVICE_EXEC = 5; int CONTENT_PROVIDER = 6; int APP_REGISTERED = 7; + int SHORT_FGS_TIMEOUT = 8; } /** Kind of timeout, e.g. BROADCAST_RECEIVER, etc. */ @@ -144,4 +146,10 @@ public class TimeoutRecord { public static TimeoutRecord forApp(@NonNull String reason) { return TimeoutRecord.endingApproximatelyNow(TimeoutKind.APP_REGISTERED, reason); } + + /** Record for a "short foreground service" timeout. */ + @NonNull + public static TimeoutRecord forShortFgsTimeout(String reason) { + return TimeoutRecord.endingNow(TimeoutKind.SHORT_FGS_TIMEOUT, reason); + } } diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java index 114ed86953efa..4539e9eab424e 100644 --- a/services/core/java/com/android/server/am/ActiveServices.java +++ b/services/core/java/com/android/server/am/ActiveServices.java @@ -228,7 +228,8 @@ public final class ActiveServices { private static final boolean DEBUG_DELAYED_SERVICE = DEBUG_SERVICE; private static final boolean DEBUG_DELAYED_STARTS = DEBUG_DELAYED_SERVICE; - private static final boolean DEBUG_SHORT_SERVICE = DEBUG_SERVICE; + // STOPSHIP(b/260012573) turn it off. + private static final boolean DEBUG_SHORT_SERVICE = true; // DEBUG_SERVICE; private static final boolean LOG_SERVICE_START_STOP = DEBUG_SERVICE; @@ -1912,6 +1913,36 @@ public final class ActiveServices { "startForeground(SHORT_SERVICE) called on a service that's not" + " started."); } + // If the service is already an FGS, and the type is changing, then we + // may need to do some extra work here. + if (r.isForeground && (r.foregroundServiceType != foregroundServiceType)) { + // TODO(short-service): Consider transitions: + // A. Short -> other types: + // Apply the BG restriction again. Don't just allow it. + // i.e. unless the app is in a situation where it's allowed to start + // a FGS, this transition shouldn't be allowed. + // ... But think about it more, there may be a case this should be + // allowed. + // + // If the transition is allowed, stop the timeout. + // If the transition is _not_ allowed... keep the timeout? + // + // B. Short -> Short: + // Allowed, but the timeout won't reset. The original timeout is used. + // C. Other -> short: + // This should always be allowed. + // A timeout should start. + + // For now, let's just disallow transition from / to SHORT_SERVICE. + final boolean isNewTypeShortFgs = + foregroundServiceType == FOREGROUND_SERVICE_TYPE_SHORT_SERVICE; + if (r.isShortFgs() != isNewTypeShortFgs) { + // TODO(short-service): We should (probably) allow it. + throw new IllegalArgumentException( + "setForeground(): Changing foreground service type from / to " + + " SHORT_SERVICE is now allowed"); + } + } // If a valid short-service (which has to be "started"), happens to // also be bound, then we still _will_ apply a timeout, because it still has @@ -1952,32 +1983,6 @@ public final class ActiveServices { // on the same sarvice after it's created, regardless of whether // stopForeground() has been called or not. - // TODO(short-service): Consider transitions: - // A. Short -> other types: - // Apply the BG restriction again. Don't just allow it. - // i.e. unless the app is in a situation where it's allowed to start - // a FGS, this transition shouldn't be allowed. - // ... But think about it more, there may be a case this should be - // allowed. - // - // If the transition is allowed, stop the timeout. - // If the transition is _not_ allowed... keep the timeout? - // - // B. Short -> Short: - // Allowed, but the timeout won't reset. The original timeout is used. - // C. Other -> short: - // This should always be allowed. - // A timeout should start. - - // For now, let's just disallow transition from / to SHORT_SERVICE. - final boolean isNewTypeShortFgs = - foregroundServiceType == FOREGROUND_SERVICE_TYPE_SHORT_SERVICE; - if (r.isShortFgs() != isNewTypeShortFgs) { - // TODO(short-service): We should (probably) allow it. - throw new IllegalArgumentException("Changing FGS type from / to " - + " SHORT_SERVICE is now allowed"); - } - // The second or later time startForeground() is called after service is // started. Check for app state again. setFgsRestrictionLocked(r.serviceInfo.packageName, r.app.getPid(), @@ -2948,6 +2953,9 @@ public final class ActiveServices { * Stop the timeout for a ServiceRecord, if it's of a short-FGS. */ private void maybeStopShortFgsTimeoutLocked(ServiceRecord sr) { + if (!sr.isShortFgs()) { + return; + } if (DEBUG_SHORT_SERVICE) { Slog.i(TAG_SERVICE, "Stop short FGS timeout: " + sr); } @@ -2960,7 +2968,7 @@ public final class ActiveServices { if (!sr.shouldTriggerShortFgsTimeout()) { return; } - Slog.i(TAG_SERVICE, "Short FGS timed out: " + sr); + Slog.e(TAG_SERVICE, "Short FGS timed out: " + sr); try { sr.app.getThread().scheduleTimeoutService(sr, sr.getShortFgsInfo().getStartId()); } catch (RemoteException e) { @@ -2974,15 +2982,28 @@ public final class ActiveServices { } void onShortFgsAnrTimeout(ServiceRecord sr) { - // TODO(short-service): Implement ANR, and change the WTF to e(). - // Also make sure to call waitingOnAMSLockStarted(). + final String reason = "A foreground service of FOREGROUND_SERVICE_TYPE_SHORT_SERVICE" + + " did not stop within a timeout: " + sr.getComponentName(); + + final TimeoutRecord tr = TimeoutRecord.forShortFgsTimeout(reason); + + // TODO(short-service): TODO Add SHORT_FGS_TIMEOUT to AnrLatencyTracker + tr.mLatencyTracker.waitingOnAMSLockStarted(); synchronized (mAm) { + tr.mLatencyTracker.waitingOnAMSLockEnded(); + if (!sr.shouldTriggerShortFgsAnr()) { return; } - } - Slog.wtf(TAG_SERVICE, "Short FGS ANR'ed (NOT IMPLEMENTED YET): " + sr); + final String message = "Short FGS ANR'ed: " + sr; + if (DEBUG_SHORT_SERVICE) { + Slog.wtf(TAG_SERVICE, message); + } else { + Slog.e(TAG_SERVICE, message); + } + mAm.appNotResponding(sr.app, tr); + } } private void updateAllowlistManagerLocked(ProcessServiceRecord psr) { diff --git a/services/core/java/com/android/server/am/OomAdjuster.java b/services/core/java/com/android/server/am/OomAdjuster.java index 034c1b3d55ad9..66a8babe224eb 100644 --- a/services/core/java/com/android/server/am/OomAdjuster.java +++ b/services/core/java/com/android/server/am/OomAdjuster.java @@ -1821,25 +1821,28 @@ public class OomAdjuster { } else if (psr.hasForegroundServices()) { // If we get here, hasNonShortForegroundServices() must be false. - // TODO(short-service): If all the short-fgs (there may be multiple) within the - // process is post proc-state-demote time, then we need to skip this part. - // ... Or, should we just ANR take care of timed-out short-FGS and shouldn't bother - // lowering the procstate / oom-adj...?? (likely not.) + // TODO(short-service): Proactively run OomAjudster when the grace period finish. + if (psr.areAllShortForegroundServicesProcstateTimedOut(now)) { + // All the short-FGSes within this process are timed out. Don't promote to FGS. + // TODO(short-service): Should we set some unique oom-adj to make it detectable, + // in a long trace? + } else { + // For short FGS. + adjType = "fg-service-short"; + // We use MEDIUM_APP_ADJ + 1 so we can tell apart EJ + // (which uses MEDIUM_APP_ADJ + 1) + // from short-FGS. + // (We use +1 and +2, not +0 and +1, to be consistent with the following + // RECENT_FOREGROUND_APP_ADJ tweak) + newAdj = PERCEPTIBLE_MEDIUM_APP_ADJ + 1; - // For short FGS. - adjType = "fg-service-short"; - // We use MEDIUM_APP_ADJ + 1 so we can tell apart EJ (which uses MEDIUM_APP_ADJ + 1) - // from short-FGS. - // (We use +1 and +2, not +0 and +1, to be consistent with the following - // RECENT_FOREGROUND_APP_ADJ tweak) - newAdj = PERCEPTIBLE_MEDIUM_APP_ADJ + 1; + // Short-FGS gets a below-BFGS procstate, so it can't start another FGS from it. + newProcState = PROCESS_STATE_IMPORTANT_FOREGROUND; - // Short-FGS gets a below-BFGS procstate, so it can't start another FGS from it. - newProcState = PROCESS_STATE_IMPORTANT_FOREGROUND; - - // Same as EJ, we explicitly grant network access to short FGS, - // even when battery saver or data saver is enabled. - capabilityFromFGS |= PROCESS_CAPABILITY_NETWORK; + // Same as EJ, we explicitly grant network access to short FGS, + // even when battery saver or data saver is enabled. + capabilityFromFGS |= PROCESS_CAPABILITY_NETWORK; + } } if (adjType != null) { diff --git a/services/core/java/com/android/server/am/ProcessServiceRecord.java b/services/core/java/com/android/server/am/ProcessServiceRecord.java index 13264db01680e..df442e83dc9ef 100644 --- a/services/core/java/com/android/server/am/ProcessServiceRecord.java +++ b/services/core/java/com/android/server/am/ProcessServiceRecord.java @@ -175,6 +175,9 @@ final class ProcessServiceRecord { } } + /** + * @return true if this process has any foreground services (even timed-out short-FGS) + */ boolean hasForegroundServices() { return mHasForegroundServices; } @@ -224,6 +227,33 @@ final class ProcessServiceRecord { return mFgServiceTypes != ServiceInfo.FOREGROUND_SERVICE_TYPE_SHORT_SERVICE; } + /** + * @return if this process: + * - has at least one short-FGS + * - has no other types of FGS + * - and all the short-FGSes are procstate-timed out. + */ + boolean areAllShortForegroundServicesProcstateTimedOut(long nowUptime) { + if (!mHasForegroundServices) { // Process has no FGS? + return false; + } + if (hasNonShortForegroundServices()) { // Any non-short FGS running? + return false; + } + // Now we need to look at all short-FGS within the process and see if all of them are + // procstate-timed-out or not. + for (int i = mServices.size() - 1; i >= 0; i--) { + final ServiceRecord sr = mServices.valueAt(i); + if (!sr.isShortFgs() || !sr.hasShortFgsInfo()) { + continue; + } + if (sr.getShortFgsInfo().getProcStateDemoteTime() >= nowUptime) { + return false; + } + } + return true; + } + int getReportedForegroundServiceTypes() { return mRepFgServiceTypes; } diff --git a/services/core/java/com/android/server/am/ServiceRecord.java b/services/core/java/com/android/server/am/ServiceRecord.java index 4c0bd073a4de3..547c20b55a3cd 100644 --- a/services/core/java/com/android/server/am/ServiceRecord.java +++ b/services/core/java/com/android/server/am/ServiceRecord.java @@ -690,6 +690,32 @@ final class ServiceRecord extends Binder implements ComponentName.WithComponentN } } + /** Used only for tests */ + private ServiceRecord(ActivityManagerService ams) { + this.ams = ams; + name = null; + instanceName = null; + shortInstanceName = null; + definingPackageName = null; + definingUid = 0; + intent = null; + serviceInfo = null; + userId = 0; + packageName = null; + processName = null; + permission = null; + exported = false; + restarter = null; + createRealTime = 0; + isSdkSandbox = false; + sdkSandboxClientAppUid = 0; + sdkSandboxClientAppPackage = null; + } + + public static ServiceRecord newEmptyInstanceForTest(ActivityManagerService ams) { + return new ServiceRecord(ams); + } + ServiceRecord(ActivityManagerService ams, ComponentName name, ComponentName instanceName, String definingPackageName, int definingUid, Intent.FilterComparison intent, ServiceInfo sInfo, boolean callerIsFg, 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 55d116043f157..9234431accb99 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/MockingOomAdjusterTests.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/MockingOomAdjusterTests.java @@ -439,14 +439,25 @@ public class MockingOomAdjusterTests { @SuppressWarnings("GuardedBy") @Test public void testUpdateOomAdj_DoOne_FgService_ShortFgs() { + sService.mConstants.TOP_TO_FGS_GRACE_DURATION = 100_000; + sService.mConstants.mShortFgsProcStateExtraWaitDuration = 200_000; + + ServiceRecord s = ServiceRecord.newEmptyInstanceForTest(sService); + s.startRequested = true; + s.isForeground = true; + s.foregroundServiceType = ServiceInfo.FOREGROUND_SERVICE_TYPE_SHORT_SERVICE; + s.setShortFgsInfo(SystemClock.uptimeMillis()); + // SHORT_SERVICE FGS will get IMP_FG and a slightly different recent-adjustment. { ProcessRecord app = spy(makeDefaultProcessRecord(MOCKAPP_PID, MOCKAPP_UID, MOCKAPP_PROCESSNAME, MOCKAPP_PACKAGENAME, true)); + app.mServices.startService(s); app.mServices.setHasForegroundServices(true, ServiceInfo.FOREGROUND_SERVICE_TYPE_SHORT_SERVICE, /* hasNoneType=*/false); app.mState.setLastTopTime(SystemClock.uptimeMillis()); sService.mWakefulness.set(PowerManagerInternal.WAKEFULNESS_AWAKE); + sService.mOomAdjuster.updateOomAdjLocked(app, OomAdjuster.OOM_ADJ_REASON_NONE); assertProcStates(app, PROCESS_STATE_IMPORTANT_FOREGROUND, @@ -461,9 +472,11 @@ public class MockingOomAdjusterTests { MOCKAPP_PROCESSNAME, MOCKAPP_PACKAGENAME, true)); app.mServices.setHasForegroundServices(true, ServiceInfo.FOREGROUND_SERVICE_TYPE_SHORT_SERVICE, /* hasNoneType=*/false); + app.mServices.startService(s); app.mState.setLastTopTime(SystemClock.uptimeMillis() - sService.mConstants.TOP_TO_FGS_GRACE_DURATION); sService.mWakefulness.set(PowerManagerInternal.WAKEFULNESS_AWAKE); + sService.mOomAdjuster.updateOomAdjLocked(app, OomAdjuster.OOM_ADJ_REASON_NONE); assertProcStates(app, PROCESS_STATE_IMPORTANT_FOREGROUND, @@ -471,6 +484,33 @@ public class MockingOomAdjusterTests { // Still should get network access. assertTrue((app.mState.getSetCapability() & PROCESS_CAPABILITY_NETWORK) != 0); } + + // SHORT_SERVICE, timed out already. + s = ServiceRecord.newEmptyInstanceForTest(sService); + s.startRequested = true; + s.isForeground = true; + s.foregroundServiceType = ServiceInfo.FOREGROUND_SERVICE_TYPE_SHORT_SERVICE; + s.setShortFgsInfo(SystemClock.uptimeMillis() + - sService.mConstants.mShortFgsTimeoutDuration + - sService.mConstants.mShortFgsProcStateExtraWaitDuration); + { + ProcessRecord app = spy(makeDefaultProcessRecord(MOCKAPP_PID, MOCKAPP_UID, + MOCKAPP_PROCESSNAME, MOCKAPP_PACKAGENAME, true)); + app.mServices.setHasForegroundServices(true, + ServiceInfo.FOREGROUND_SERVICE_TYPE_SHORT_SERVICE, /* hasNoneType=*/false); + app.mServices.startService(s); + app.mState.setLastTopTime(SystemClock.uptimeMillis() + - sService.mConstants.TOP_TO_FGS_GRACE_DURATION); + sService.mWakefulness.set(PowerManagerInternal.WAKEFULNESS_AWAKE); + + sService.mOomAdjuster.updateOomAdjLocked(app, OomAdjuster.OOM_ADJ_REASON_NONE); + + // Procstate should be lower than FGS. (It should be SERVICE) + assertEquals(app.mState.getSetProcState(), PROCESS_STATE_SERVICE); + + // Shouldn't have the network capability now. + assertTrue((app.mState.getSetCapability() & PROCESS_CAPABILITY_NETWORK) == 0); + } } @SuppressWarnings("GuardedBy")