From 29f1852efd836b803d09002b41a1926871950379 Mon Sep 17 00:00:00 2001 From: Christopher Tate Date: Thu, 3 Jun 2021 09:17:18 -0700 Subject: [PATCH] Fix FGS enter/exit statslog semantics Logging wasn't quite right in the case of deferred FGS notification in various ordering scenarios. Bug: 189926836 Test: atest CtsStatsdAtomHostTestCases:android.cts.statsdatom.statsd.UidAtomTests#testForegroundServiceState Change-Id: I650730db9e01faa90a36a73b486464ab5b179fc6 --- .../android/app/ActivityManagerInternal.java | 8 ++--- .../com/android/server/am/ActiveServices.java | 30 ++++++++++++------- .../server/am/ActivityManagerService.java | 10 +++---- .../NotificationManagerService.java | 21 +++++++------ 4 files changed, 40 insertions(+), 29 deletions(-) diff --git a/core/java/android/app/ActivityManagerInternal.java b/core/java/android/app/ActivityManagerInternal.java index d962fa3bc316a..25fd254cd9f6f 100644 --- a/core/java/android/app/ActivityManagerInternal.java +++ b/core/java/android/app/ActivityManagerInternal.java @@ -486,11 +486,11 @@ public abstract class ActivityManagerInternal { /** * Callback from the notification subsystem that the given FGS notification has - * been shown or updated. This can happen after either Service.startForeground() - * or NotificationManager.notify(). + * been evaluated, and either shown or explicitly overlooked. This can happen + * after either Service.startForeground() or NotificationManager.notify(). */ - public abstract void onForegroundServiceNotificationUpdate(Notification notification, - int id, String pkg, @UserIdInt int userId); + public abstract void onForegroundServiceNotificationUpdate(boolean shown, + Notification notification, int id, String pkg, @UserIdInt int userId); /** * If the given app has any FGSs whose notifications are in the given channel, diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java index 9e42900988432..7500be8cfe33c 100644 --- a/services/core/java/com/android/server/am/ActiveServices.java +++ b/services/core/java/com/android/server/am/ActiveServices.java @@ -2216,8 +2216,9 @@ public final class ActiveServices { * visibility, starting with both Service.startForeground() and * NotificationManager.notify(). */ - public void onForegroundServiceNotificationUpdateLocked(Notification notification, - final int id, final String pkg, @UserIdInt final int userId) { + public void onForegroundServiceNotificationUpdateLocked(boolean shown, + Notification notification, final int id, final String pkg, + @UserIdInt final int userId) { // If this happens to be a Notification for an FGS still in its deferral period, // drop the deferral and make sure our content bookkeeping is up to date. for (int i = mPendingFgsNotifications.size() - 1; i >= 0; i--) { @@ -2225,17 +2226,26 @@ public final class ActiveServices { if (userId == sr.userId && id == sr.foregroundId && sr.appInfo.packageName.equals(pkg)) { - if (DEBUG_FOREGROUND_SERVICE) { - Slog.d(TAG_SERVICE, "Notification shown; canceling deferral of " - + sr); - } + // Found it. If 'shown' is false, it means that the notification + // subsystem will not be displaying it yet, so all we do is log + // the "fgs entered" transition noting deferral, then we're done. maybeLogFGSStateEnteredLocked(sr); - sr.mFgsNotificationShown = true; - sr.mFgsNotificationDeferred = false; - mPendingFgsNotifications.remove(i); + if (shown) { + if (DEBUG_FOREGROUND_SERVICE) { + Slog.d(TAG_SERVICE, "Notification shown; canceling deferral of " + + sr); + } + sr.mFgsNotificationShown = true; + sr.mFgsNotificationDeferred = false; + mPendingFgsNotifications.remove(i); + } else { + if (DEBUG_FOREGROUND_SERVICE) { + Slog.d(TAG_SERVICE, "FGS notification deferred for " + sr); + } + } } } - // And make sure to retain the latest notification content for the FGS + // In all cases, make sure to retain the latest notification content for the FGS ServiceMap smap = mServiceMap.get(userId); if (smap != null) { for (int i = 0; i < smap.mServicesByInstanceName.size(); i++) { diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 294762139f501..94cfa53c002c5 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -350,8 +350,6 @@ import com.android.internal.util.Preconditions; import com.android.internal.util.function.DecFunction; import com.android.internal.util.function.HeptFunction; import com.android.internal.util.function.HexFunction; -import com.android.internal.util.function.NonaFunction; -import com.android.internal.util.function.OctFunction; import com.android.internal.util.function.QuadFunction; import com.android.internal.util.function.QuintFunction; import com.android.internal.util.function.TriFunction; @@ -16085,11 +16083,11 @@ public class ActivityManagerService extends IActivityManager.Stub } @Override - public void onForegroundServiceNotificationUpdate(Notification notification, - int id, String pkg, @UserIdInt int userId) { + public void onForegroundServiceNotificationUpdate(boolean shown, + Notification notification, int id, String pkg, @UserIdInt int userId) { synchronized (ActivityManagerService.this) { - mServices.onForegroundServiceNotificationUpdateLocked(notification, - id, pkg, userId); + mServices.onForegroundServiceNotificationUpdateLocked(shown, + notification, id, pkg, userId); } } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 412eee840d4f0..5b5d5d403cb77 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -3052,17 +3052,19 @@ public class NotificationManagerService extends SystemService { } } - protected void maybeReportForegroundServiceUpdate(final NotificationRecord r) { + protected void reportForegroundServiceUpdate(boolean shown, + final Notification notification, final int id, final String pkg, final int userId) { + mHandler.post(() -> { + mAmi.onForegroundServiceNotificationUpdate(shown, notification, id, pkg, userId); + }); + } + + protected void maybeReportForegroundServiceUpdate(final NotificationRecord r, boolean shown) { if (r.isForegroundService()) { // snapshot live state for the asynchronous operation final StatusBarNotification sbn = r.getSbn(); - final Notification notification = sbn.getNotification(); - final int id = sbn.getId(); - final String pkg = sbn.getPackageName(); - final int userId = sbn.getUser().getIdentifier(); - mHandler.post(() -> { - mAmi.onForegroundServiceNotificationUpdate(notification, id, pkg, userId); - }); + reportForegroundServiceUpdate(shown, sbn.getNotification(), sbn.getId(), + sbn.getPackageName(), sbn.getUser().getIdentifier()); } } @@ -6194,6 +6196,7 @@ public class NotificationManagerService extends SystemService { // because the service lifecycle logic has retained responsibility for its // handling. if (!isNotificationShownInternal(pkg, tag, id, userId)) { + reportForegroundServiceUpdate(false, notification, id, pkg, userId); return; } } @@ -7121,7 +7124,7 @@ public class NotificationManagerService extends SystemService { maybeRecordInterruptionLocked(r); maybeRegisterMessageSent(r); - maybeReportForegroundServiceUpdate(r); + maybeReportForegroundServiceUpdate(r, true); // Log event to statsd mNotificationRecordLogger.maybeLogNotificationPosted(r, old, position,