From a4f3f81eadbba638f2da9780437150769463e13d Mon Sep 17 00:00:00 2001 From: Beverly Date: Wed, 13 May 2020 12:30:20 -0400 Subject: [PATCH] Don't readd pending notifs to NEM's allNotifs list Instead, we reuse the pending notification entry so we don't unnecessarily create and retain mutliple notification entries for the same notification. Additionally: - dump all notifications kept by NEM in a dumpsys/bugreport - move mLeakDetector.trackGarbage(entry) to after NEM.listeners are told to remove their references to the entry - trackGarbage(entry) on pending notifications that are removed Test: receive phone call, see HUN still appears Test: post high priority notification (HUN) and then send immediate update, still see HUN Test: atest HeadUpNotificationTests Test: atest NotificationEntryManagerTest Test: adb shell dumpsys activity service com.android.systemui/.SystemUIService dependency DumpController NotificationEntryManager Test: check memory usage of com.android.systemui before and after running NexusLauncherTests (observe view count doesn't incrementally get worse) Fixes: 156301621 Change-Id: I60ec7847f0b8cc3f3a3ab3fbc0004991e816dc16 --- .../NotificationEntryManager.java | 60 +++++++++++-------- .../collection/NotifCollection.java | 2 +- .../NotificationEntryManagerTest.java | 22 +++++++ 3 files changed, 58 insertions(+), 26 deletions(-) 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 d6471243e0539..adb51a5d959a0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java @@ -151,6 +151,16 @@ public class NotificationEntryManager implements @Override public void dump(FileDescriptor fd, PrintWriter pw, String[] args) { pw.println("NotificationEntryManager state:"); + pw.println(" mAllNotifications="); + if (mAllNotifications.size() == 0) { + pw.println("null"); + } else { + int i = 0; + for (NotificationEntry entry : mAllNotifications) { + dumpEntry(pw, " ", i, entry); + i++; + } + } pw.print(" mPendingNotifications="); if (mPendingNotifications.size() == 0) { pw.println("null"); @@ -350,8 +360,8 @@ public class NotificationEntryManager implements private final NotificationHandler mNotifListener = new NotificationHandler() { @Override public void onNotificationPosted(StatusBarNotification sbn, RankingMap rankingMap) { - final boolean isUpdate = mActiveNotifications.containsKey(sbn.getKey()); - if (isUpdate) { + final boolean isUpdateToInflatedNotif = mActiveNotifications.containsKey(sbn.getKey()); + if (isUpdateToInflatedNotif) { updateNotification(sbn, rankingMap); } else { addNotification(sbn, rankingMap); @@ -442,16 +452,12 @@ public class NotificationEntryManager implements } if (!lifetimeExtended) { // At this point, we are guaranteed the notification will be removed + abortExistingInflation(key, "removeNotification"); mAllNotifications.remove(pendingEntry); + mLeakDetector.trackGarbage(pendingEntry); } } - } - - if (!lifetimeExtended) { - abortExistingInflation(key, "removeNotification"); - } - - if (entry != null) { + } else { // If a manager needs to keep the notification around for whatever reason, we // keep the notification boolean entryDismissed = entry.isRowDismissed(); @@ -469,6 +475,8 @@ public class NotificationEntryManager implements if (!lifetimeExtended) { // At this point, we are guaranteed the notification will be removed + abortExistingInflation(key, "removeNotification"); + mAllNotifications.remove(entry); // Ensure any managers keeping the lifetime extended stop managing the entry cancelLifetimeExtension(entry); @@ -477,13 +485,10 @@ public class NotificationEntryManager implements entry.removeRow(); } - mAllNotifications.remove(entry); - // Let's remove the children if this was a summary handleGroupSummaryRemoved(key); removeVisibleNotification(key); updateNotifications("removeNotificationInternal"); - mLeakDetector.trackGarbage(entry); removedByUser |= entryDismissed; mLogger.logNotifRemoved(entry.getKey(), removedByUser); @@ -497,6 +502,7 @@ public class NotificationEntryManager implements for (NotifCollectionListener listener : mNotifCollectionListeners) { listener.onEntryCleanUp(entry); } + mLeakDetector.trackGarbage(entry); } } } @@ -556,21 +562,26 @@ public class NotificationEntryManager implements Ranking ranking = new Ranking(); rankingMap.getRanking(key, ranking); - NotificationEntry entry = new NotificationEntry( - notification, - ranking, - mFgsFeatureController.isForegroundServiceDismissalEnabled(), - SystemClock.uptimeMillis()); + NotificationEntry entry = mPendingNotifications.get(key); + if (entry != null) { + entry.setSbn(notification); + } else { + entry = new NotificationEntry( + notification, + ranking, + mFgsFeatureController.isForegroundServiceDismissalEnabled(), + SystemClock.uptimeMillis()); + mAllNotifications.add(entry); + mLeakDetector.trackInstance(entry); + + for (NotifCollectionListener listener : mNotifCollectionListeners) { + listener.onEntryInit(entry); + } + } + for (NotifCollectionListener listener : mNotifCollectionListeners) { listener.onEntryBind(entry, notification); } - mAllNotifications.add(entry); - - mLeakDetector.trackInstance(entry); - - for (NotifCollectionListener listener : mNotifCollectionListeners) { - listener.onEntryInit(entry); - } // Construct the expanded view. if (!mFeatureFlags.isNewNotifPipelineRenderingEnabled()) { @@ -581,7 +592,6 @@ public class NotificationEntryManager implements mInflationCallback); } - abortExistingInflation(key, "addNotification"); mPendingNotifications.put(key, entry); mLogger.logNotifAdded(entry.getKey()); for (NotificationEntryListener listener : mNotificationEntryListeners) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java index a3621b6cabf60..0576833295124 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java @@ -390,11 +390,11 @@ public class NotifCollection implements Dumpable { if (entry == null) { // A new notification! entry = new NotificationEntry(sbn, ranking, SystemClock.uptimeMillis()); + mEventQueue.add(new InitEntryEvent(entry)); mEventQueue.add(new BindEntryEvent(entry, sbn)); mNotificationSet.put(sbn.getKey(), entry); mLogger.logNotifPosted(sbn.getKey()); - mEventQueue.add(new InitEntryEvent(entry)); mEventQueue.add(new EntryAddedEvent(entry)); } else { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java index d583048fbb268..a5a5f81bdffe2 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java @@ -209,6 +209,28 @@ public class NotificationEntryManagerTest extends SysuiTestCase { setUserSentiment(mSbn.getKey(), Ranking.USER_SENTIMENT_NEUTRAL); } + @Test + public void testAddNotification_noDuplicateEntriesCreated() { + // GIVEN a notification has been added + mEntryManager.addNotification(mSbn, mRankingMap); + + // WHEN the same notification is added multiple times before the previous entry (with + // the same key) didn't finish inflating + mEntryManager.addNotification(mSbn, mRankingMap); + mEntryManager.addNotification(mSbn, mRankingMap); + mEntryManager.addNotification(mSbn, mRankingMap); + + // THEN getAllNotifs() only contains exactly one notification with this key + int count = 0; + for (NotificationEntry entry : mEntryManager.getAllNotifs()) { + if (entry.getKey().equals(mSbn.getKey())) { + count++; + } + } + assertEquals("Should only be one entry with key=" + mSbn.getKey() + " in mAllNotifs. " + + "Instead there are " + count, 1, count); + } + @Test public void testAddNotification_setsUserSentiment() { mEntryManager.addNotification(mSbn, mRankingMap);