From 09843a687bc19f8e360b2f5b0493a1f378bb603b Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Mon, 5 Aug 2019 17:11:54 -0400 Subject: [PATCH] Force FGS notifications to show for a minimum time It's possible for a service to do a start/stop foreground and cause a couple of things to happen: NotificationManagerService will enqueue a EnqueueNotificationRunnable, post a PostNotificationRunnable (for the startForeground), and then also enqueue a CancelNotificationRunnable. There is some racy behavior here in that the cancel runnable can get triggered in between enqueue and post runnables. If the cancel happens first, then NotificationListenerServices will never get the message. This behavior is technically allowed, however for foreground services we want to ensure that there is a minmum amount of time that notification listeners are aware of the foreground service so that (for instance) the FGS notification can be shown. This CL does two things to mitigate this problem: 1. Introduce checking in the CancelNotificationRunnable such that it will not cancel until after PostNotificationRunnable has finished executing. 2. Introduce a NotificationLifetimeExtender method that will allow a lifetime extender to manage the lifetime of a notification that has been enqueued but not inflated yet. Bug: 119041698 Test: atest NotificationManagerServiceTest Test: atest ForegroundServiceNotificationListenerTest Change-Id: I0680034ed9315aa2c05282524d48faaed066ebd0 Merged-In: I0680034ed9315aa2c05282524d48faaed066ebd0 --- ...ForegroundServiceNotificationListener.java | 70 +++++++++++++++ .../NotificationLifetimeExtender.java | 12 +++ .../NotificationEntryManager.java | 20 ++++- .../phone/StatusBarNotificationPresenter.java | 2 +- ...groundServiceNotificationListenerTest.java | 86 +++++++++++++++++++ .../NotificationManagerService.java | 12 ++- .../NotificationManagerServiceTest.java | 14 +++ 7 files changed, 210 insertions(+), 6 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java diff --git a/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java b/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java index 96b62ac918ab4..46e08661cd707 100644 --- a/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java +++ b/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java @@ -16,14 +16,20 @@ package com.android.systemui; +import android.annotation.NonNull; import android.app.Notification; import android.app.NotificationManager; import android.content.Context; import android.os.Bundle; +import android.os.Handler; +import android.os.Looper; import android.service.notification.StatusBarNotification; +import android.util.ArraySet; import android.util.Log; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.statusbar.NotificationVisibility; +import com.android.systemui.statusbar.NotificationLifetimeExtender; import com.android.systemui.statusbar.notification.NotificationEntryListener; import com.android.systemui.statusbar.notification.NotificationEntryManager; import com.android.systemui.statusbar.notification.collection.NotificationEntry; @@ -66,6 +72,9 @@ public class ForegroundServiceNotificationListener { removeNotification(entry.notification); } }); + + notificationEntryManager.addNotificationLifetimeExtender( + new ForegroundServiceNotificationListener.ForegroundServiceLifetimeExtender()); } /** @@ -144,4 +153,65 @@ public class ForegroundServiceNotificationListener { }, true /* create if not found */); } + + /** + * Extends the lifetime of foreground notification services such that they show for at least + * five seconds + */ + public static class ForegroundServiceLifetimeExtender implements NotificationLifetimeExtender { + + private static final String TAG = "FGSLifetimeExtender"; + @VisibleForTesting + static final int MIN_FGS_TIME_MS = 5000; + + private NotificationSafeToRemoveCallback mNotificationSafeToRemoveCallback; + private ArraySet mManagedEntries = new ArraySet<>(); + private Handler mHandler = new Handler(Looper.getMainLooper()); + + public ForegroundServiceLifetimeExtender() { + } + + @Override + public void setCallback(@NonNull NotificationSafeToRemoveCallback callback) { + mNotificationSafeToRemoveCallback = callback; + } + + @Override + public boolean shouldExtendLifetime(@NonNull NotificationEntry entry) { + if ((entry.notification.getNotification().flags + & Notification.FLAG_FOREGROUND_SERVICE) == 0) { + return false; + } + + long currentTime = System.currentTimeMillis(); + return currentTime - entry.notification.getPostTime() < MIN_FGS_TIME_MS; + } + + @Override + public boolean shouldExtendLifetimeForPendingNotification( + @NonNull NotificationEntry entry) { + return shouldExtendLifetime(entry); + } + + @Override + public void setShouldManageLifetime( + @NonNull NotificationEntry entry, boolean shouldManage) { + if (!shouldManage) { + mManagedEntries.remove(entry); + return; + } + + mManagedEntries.add(entry); + + Runnable r = () -> { + if (mManagedEntries.contains(entry)) { + if (mNotificationSafeToRemoveCallback != null) { + mNotificationSafeToRemoveCallback.onSafeToRemove(entry.key); + } + mManagedEntries.remove(entry); + } + }; + mHandler.postDelayed(r, MIN_FGS_TIME_MS); + } + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java index 0f295ba75fe40..48e2923c97d99 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java @@ -26,6 +26,18 @@ public interface NotificationLifetimeExtender { */ boolean shouldExtendLifetime(@NonNull NotificationEntry entry); + /** + * It's possible that a notification was canceled before it ever became visible. This callback + * gives lifetime extenders a chance to make sure it shows up. For example if a foreground + * service is canceled too quickly but we still want to make sure a FGS notification shows. + * @param pendingEntry the canceled (but pending) entry + * @return true if the notification lifetime should be extended + */ + default boolean shouldExtendLifetimeForPendingNotification( + @NonNull NotificationEntry pendingEntry) { + return false; + } + /** * Sets whether or not the lifetime should be managed by the extender. In practice, if * shouldManage is true, this is where the extender starts managing the entry internally and is diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java index f8fef7d4778c8..a37367e4bb255 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java @@ -281,11 +281,25 @@ public class NotificationEntryManager implements } final NotificationEntry entry = mNotificationData.get(key); - - abortExistingInflation(key); - boolean lifetimeExtended = false; + // Notification was canceled before it got inflated + if (entry == null) { + NotificationEntry pendingEntry = mPendingNotifications.get(key); + if (pendingEntry != null) { + for (NotificationLifetimeExtender extender : mNotificationLifetimeExtenders) { + if (extender.shouldExtendLifetimeForPendingNotification(pendingEntry)) { + extendLifetime(pendingEntry, extender); + lifetimeExtended = true; + } + } + } + } + + if (!lifetimeExtended) { + abortExistingInflation(key); + } + if (entry != null) { // If a manager needs to keep the notification around for whatever reason, we // keep the notification diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java index a870590c08ac7..069219802cc3d 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java @@ -224,7 +224,7 @@ public class StatusBarNotificationPresenter implements NotificationPresenter, mVisualStabilityManager.setUpWithPresenter(this); mGutsManager.setUpWithPresenter(this, notifListContainer, mCheckSaveListener, mOnSettingsClickListener); - // ForegroundServiceControllerListener adds its listener in its constructor + // ForegroundServiceNotificationListener adds its listener in its constructor // but we need to request it here in order for it to be instantiated. // TODO: figure out how to do this correctly once Dependency.get() is gone. Dependency.get(ForegroundServiceNotificationListener.class); diff --git a/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java b/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java new file mode 100644 index 0000000000000..72a457e9bbbee --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java @@ -0,0 +1,86 @@ +/* + * Copyright (C) 2019 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 com.android.systemui; + +import static com.android.systemui.ForegroundServiceNotificationListener.ForegroundServiceLifetimeExtender.MIN_FGS_TIME_MS; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import android.app.Notification; +import android.service.notification.StatusBarNotification; + +import androidx.test.filters.SmallTest; +import androidx.test.runner.AndroidJUnit4; + +import com.android.systemui.statusbar.notification.collection.NotificationEntry; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +@RunWith(AndroidJUnit4.class) +@SmallTest +public class ForegroundServiceNotificationListenerTest extends SysuiTestCase { + private ForegroundServiceNotificationListener.ForegroundServiceLifetimeExtender mExtender = + new ForegroundServiceNotificationListener.ForegroundServiceLifetimeExtender(); + private StatusBarNotification mSbn; + private NotificationEntry mEntry; + private Notification mNotif; + + @Before + public void setup() { + mNotif = new Notification.Builder(mContext, "") + .setSmallIcon(R.drawable.ic_person) + .setContentTitle("Title") + .setContentText("Text") + .build(); + + mSbn = mock(StatusBarNotification.class); + when(mSbn.getNotification()).thenReturn(mNotif); + + mEntry = new NotificationEntry(mSbn); + } + + /** + * ForegroundServiceLifetimeExtenderTest + */ + @Test + public void testShouldExtendLifetime_should_foreground() { + // Extend the lifetime of a FGS notification iff it has not been visible + // for the minimum time + mNotif.flags |= Notification.FLAG_FOREGROUND_SERVICE; + when(mSbn.getPostTime()).thenReturn(System.currentTimeMillis()); + assertTrue(mExtender.shouldExtendLifetime(mEntry)); + } + + @Test + public void testShouldExtendLifetime_shouldNot_foreground() { + mNotif.flags |= Notification.FLAG_FOREGROUND_SERVICE; + when(mSbn.getPostTime()).thenReturn(System.currentTimeMillis() - MIN_FGS_TIME_MS - 1); + assertFalse(mExtender.shouldExtendLifetime(mEntry)); + } + + @Test + public void testShouldExtendLifetime_shouldNot_notForeground() { + mNotif.flags = 0; + when(mSbn.getPostTime()).thenReturn(System.currentTimeMillis() - MIN_FGS_TIME_MS - 1); + assertFalse(mExtender.shouldExtendLifetime(mEntry)); + } +} diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index d4b3138f38295..c0b26467da085 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -5236,8 +5236,16 @@ public class NotificationManagerService extends SystemService { } synchronized (mNotificationLock) { - // Look for the notification, searching both the posted and enqueued lists. - NotificationRecord r = findNotificationLocked(mPkg, mTag, mId, mUserId); + // If the notification is currently enqueued, repost this runnable so it has a + // chance to notify listeners + if ((findNotificationByListLocked(mEnqueuedNotifications, mPkg, mTag, mId, mUserId)) + != null) { + mHandler.post(this); + return; + } + // Look for the notification in the posted list, since we already checked enqueued. + NotificationRecord r = + findNotificationByListLocked(mNotificationList, mPkg, mTag, mId, mUserId); if (r != null) { // The notification was found, check if it should be removed. diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java index b5e4934f49bfd..5d9e0e21601b2 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -961,6 +961,20 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { assertEquals(0, mService.getNotificationRecordCount()); } + @Test + public void testCancelImmediatelyAfterEnqueueNotifiesListeners_ForegroundServiceFlag() + throws Exception { + final StatusBarNotification sbn = generateNotificationRecord(null).sbn; + sbn.getNotification().flags = + Notification.FLAG_ONGOING_EVENT | FLAG_FOREGROUND_SERVICE; + mBinderService.enqueueNotificationWithTag(PKG, PKG, "tag", + sbn.getId(), sbn.getNotification(), sbn.getUserId()); + mBinderService.cancelNotificationWithTag(PKG, "tag", sbn.getId(), sbn.getUserId()); + waitForIdle(); + verify(mListeners, times(1)).notifyPostedLocked(any(), any()); + verify(mListeners, times(1)).notifyRemovedLocked(any(), anyInt(), any()); + } + @Test public void testUserInitiatedClearAll_noLeak() throws Exception { final NotificationRecord n = generateNotificationRecord(