From cb5e62cc500a5a1b770e2a48b6e5deacd0134db9 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: 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: Ia6d6dbc442fe28832f1ed70b35ca64278a871237 --- .../NotificationEntryManager.java | 54 +++++++++++-------- .../NotificationEntryManagerTest.java | 22 ++++++++ 2 files changed, 55 insertions(+), 21 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 d2517774ab2ee..5a7ed40c677cb 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,17 +562,24 @@ 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); + } + + abortExistingInflation(key, "addNotification"); + for (NotifCollectionListener listener : mNotifCollectionListeners) { listener.onEntryBind(entry, notification); } - mAllNotifications.add(entry); - - mLeakDetector.trackInstance(entry); for (NotifCollectionListener listener : mNotifCollectionListeners) { listener.onEntryInit(entry); @@ -581,7 +594,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/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationEntryManagerTest.java index bb7f73a3a9596..f4006c930c55f 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);