Fix short-service TODOs

Test: atest CtsShortFgsTestCases
Bug: 270963079

Change-Id: Ib958b7ebc07c2759b7387f8bce15edf207219839

4 files changed, 87 insertions, 22 deletions

diff --git a/core/api/current.txt b/core/api/current.txt
index 8a6bb7b7a08b..fb722cda0ca9 100644
--- a/core/api/current.txt
+++ b/core/api/current.txt
@@ -7269,6 +7269,13 @@ package android.app {
     method public void onSharedElementsReady();
   }
 
+  public final class StartForegroundCalledOnStoppedServiceException extends java.lang.IllegalStateException implements android.os.Parcelable {
+    ctor public StartForegroundCalledOnStoppedServiceException(@NonNull String);
+    method public int describeContents();
+    method public void writeToParcel(@NonNull android.os.Parcel, int);
+    field @NonNull public static final android.os.Parcelable.Creator<android.app.StartForegroundCalledOnStoppedServiceException> CREATOR;
+  }
+
   public class StatusBarManager {
     method @RequiresPermission(android.Manifest.permission.LAUNCH_CAPTURE_CONTENT_ACTIVITY_FOR_NOTE) public boolean canLaunchCaptureContentActivityForNote(@NonNull android.app.Activity);
     method public void requestAddTileService(@NonNull android.content.ComponentName, @NonNull CharSequence, @NonNull android.graphics.drawable.Icon, @NonNull java.util.concurrent.Executor, @NonNull java.util.function.Consumer<java.lang.Integer>);
diff --git a/core/java/android/app/StartForegroundCalledOnStoppedServiceException.java b/core/java/android/app/StartForegroundCalledOnStoppedServiceException.java
new file mode 100644
index 000000000000..55885a65a265
--- /dev/null
+++ b/core/java/android/app/StartForegroundCalledOnStoppedServiceException.java
@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package android.app;
+
+import android.annotation.NonNull;
+import android.os.Parcel;
+import android.os.Parcelable;
+
+/**
+ * Exception thrown when {@link Service#startForeground} is called on a service that's not
+ * actually started.
+ */
+public final class StartForegroundCalledOnStoppedServiceException
+        extends IllegalStateException implements Parcelable {
+    /**
+     * Constructor.
+     */
+    public StartForegroundCalledOnStoppedServiceException(@NonNull String message) {
+        super(message);
+    }
+
+    StartForegroundCalledOnStoppedServiceException(@NonNull Parcel source) {
+        super(source.readString());
+    }
+
+    @Override
+    public int describeContents() {
+        return 0;
+    }
+
+    @Override
+    public void writeToParcel(@NonNull Parcel dest, int flags) {
+        dest.writeString(getMessage());
+    }
+
+    public static final @NonNull Creator<StartForegroundCalledOnStoppedServiceException>
+            CREATOR = new Creator<StartForegroundCalledOnStoppedServiceException>() {
+                @NonNull
+                public StartForegroundCalledOnStoppedServiceException createFromParcel(
+                        Parcel source) {
+                    return new StartForegroundCalledOnStoppedServiceException(source);
+                }
+
+                @NonNull
+                public StartForegroundCalledOnStoppedServiceException[] newArray(int size) {
+                    return new StartForegroundCalledOnStoppedServiceException[size];
+                }
+            };
+}
diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java
index 70304c55067e..653dd561faea 100644
--- a/services/core/java/com/android/server/am/ActiveServices.java
+++ b/services/core/java/com/android/server/am/ActiveServices.java
@@ -137,6 +137,7 @@ import android.app.PendingIntent;
 import android.app.RemoteServiceException.ForegroundServiceDidNotStartInTimeException;
 import android.app.Service;
 import android.app.ServiceStartArgs;
+import android.app.StartForegroundCalledOnStoppedServiceException;
 import android.app.admin.DevicePolicyEventLogger;
 import android.app.compat.CompatChanges;
 import android.app.usage.UsageEvents;
@@ -800,13 +801,17 @@ public final class ActiveServices {
                     ? res.permission : "private to package");
         }
 
-
-        // TODO(short-service): This is inside startService() / startForegroundService().
-        // Consider if there's anything special we have to do if these are called on an already-
-        // running short-FGS... But given these APIs shouldn't change the FGS type, we likely
-        // don't need to do anything. (If they would change the FGS type, we'd have to stop
-        // the timeout)
         ServiceRecord r = res.record;
+        // Note, when startService() or startForegroundService() is called on an already
+        // running SHORT_SERVICE FGS, the call will succeed (i.e. we won't throw
+        // ForegroundServiceStartNotAllowedException), even when the service is alerady timed
+        // out. This is because these APIs will essnetially only change the "started" state
+        // of the service, and it won't afect "the foreground-ness" of the service, or the type
+        // of the FGS.
+        // However, this call will still _not_ extend the SHORT_SERVICE timeout either.
+        // Also, if the app tries to change the type of the FGS later (using
+        // Service.startForeground()), at that point we will consult the BFSL check and the timeout
+        // and make the necessary decisions.
         setFgsRestrictionLocked(callingPackage, callingPid, callingUid, service, r, userId,
                 backgroundStartPrivileges, false /* isBindService */);
 
@@ -2035,8 +2040,7 @@ public final class ActiveServices {
                         // suddenly disallow it.
                         // However, this would be very problematic if used with a short-FGS, so we
                         // explicitly disallow this combination.
-                        // TODO(short-service): Change to another exception type?
-                        throw new IllegalStateException(
+                        throw new StartForegroundCalledOnStoppedServiceException(
                                 "startForeground(SHORT_SERVICE) called on a service that's not"
                                 + " started.");
                     }
@@ -2241,10 +2245,6 @@ public final class ActiveServices {
                         cancelForegroundNotificationLocked(r);
                         r.foregroundId = id;
                     }
-
-                    // TODO(short-service): Stop the short service timeout, if the type is changing
-                    // from short to non-short. (should we do it earlier?)
-
                     notification.flags |= Notification.FLAG_FOREGROUND_SERVICE;
                     r.foregroundNoti = notification;
                     r.foregroundServiceType = foregroundServiceType;
@@ -3190,7 +3190,7 @@ public final class ActiveServices {
             try {
                 sr.app.getThread().scheduleTimeoutService(sr, sr.getShortFgsInfo().getStartId());
             } catch (RemoteException e) {
-                // TODO(short-service): Anything to do here?
+                Slog.w(TAG_SERVICE, "Exception from scheduleTimeoutService: " + e.toString());
             }
             // Schedule the procstate demotion timeout and ANR timeout.
             {
@@ -3262,8 +3262,9 @@ public final class ActiveServices {
             }
             mAm.appNotResponding(sr.app, tr);
 
-            // TODO(short-service): Make sure, if the FGS stops after this, the ANR dialog
-            // disappears.
+            // TODO: Can we close the ANR dialog here, if it's still shown? Currently, the ANR
+            // dialog really doesn't remember the "cause" (especially if there have been multiple
+            // ANRs), so it's not doable.
         }
     }
 
@@ -3278,8 +3279,8 @@ public final class ActiveServices {
         }
     }
 
-    // TODO(short-service): Hmm what is it? Should we stop the timeout here?
     private void stopServiceAndUpdateAllowlistManagerLocked(ServiceRecord service) {
+        maybeStopShortFgsTimeoutLocked(service);
         final ProcessServiceRecord psr = service.app.mServices;
         psr.stopService(service);
         psr.updateBoundClientUids();
@@ -5406,8 +5407,6 @@ public final class ActiveServices {
 
         // Check to see if the service had been started as foreground, but being
         // brought down before actually showing a notification.  That is not allowed.
-        // TODO(short-service): This is unlikely related to short-FGS, but I'm curious why it's
-        // not allowed. Look into it.
         if (r.fgRequired) {
             Slog.w(TAG_SERVICE, "Bringing down service while still waiting for start foreground: "
                     + r);
@@ -5478,6 +5477,7 @@ public final class ActiveServices {
         cancelForegroundNotificationLocked(r);
         final boolean exitingFg = r.isForeground;
         if (exitingFg) {
+            maybeStopShortFgsTimeoutLocked(r);
             decActiveForegroundAppLocked(smap, r);
             synchronized (mAm.mProcessStats.mLock) {
                 ServiceState stracker = r.getTracker();
@@ -5501,8 +5501,6 @@ public final class ActiveServices {
                 mFGSLogger.logForegroundServiceStop(r.appInfo.uid, r);
             }
             mAm.updateForegroundServiceUsageStats(r.name, r.userId, false);
-
-            // TODO(short-service): Make sure we stop the timeout by here.
         }
 
         r.isForeground = false;
diff --git a/services/core/java/com/android/server/am/OomAdjuster.java b/services/core/java/com/android/server/am/OomAdjuster.java
index 0c366268604d..2b930b561eeb 100644
--- a/services/core/java/com/android/server/am/OomAdjuster.java
+++ b/services/core/java/com/android/server/am/OomAdjuster.java
@@ -2184,8 +2184,6 @@ public class OomAdjuster {
                 }
             }
 
-            // TODO(short-service): While-in-user permissions. Do we need any change here for
-            // short-FGS? (Likely not)
             if (s.isForeground) {
                 final int fgsType = s.foregroundServiceType;
                 if (s.mAllowWhileInUsePermissionInFgs) {