From 900a3dbcc0b97b60fabd1db64f33943699050b4e Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Tue, 25 Jul 2017 21:57:51 +0900 Subject: [PATCH] NetworkNotificationManager: correctly handle existing notifications This patch corrects a regression added by commit fd350805559d that did not take into account the case of multiple notifications shown for a single network id. Given how network notifications are triggered, it can happen that NO_INTERNET and SIGN_IN notifications are both triggered for the same network when captive portal detection is slow. Contrary to the situation before commit fd350805559d, a notification priority order is introduced so that SIGN_IN always overrides NO_INTERNET, and NO_INTERNET is ignored if SIGN_IN is already present. Bug: 63676954 Bug: 62503737 Test: runtest frameworks-net, added new unit tests Merged-In: Ib8658601e8d4dc6c41b335ab7dd8caa0cccd9531 Merged-In: I4432f66067ea1ab02e1d2dfe42530bcdafa52df6 Merged-In: I74631b0bfd14daf18a1641ed7f2ec323d636ebbf Merged-In: I73cc879e910d503946facdba498b300337f349fd Merged-In: Ieed9e3e7755e0c5f89dc41ef66f47d4dbf4c66a9 Merged-In: I0aa590170f1bd4c37175c7e35e54d52f1fb21347 (cherry picked from commit 5fcd050e0ecd5985cf184f55ea3df4434da8f824) Change-Id: I41675768ab59e9b23ca4275edf297b82595e5730 --- .../NetworkNotificationManager.java | 30 +++++++++ .../NetworkNotificationManagerTest.java | 62 ++++++++++++++++--- 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java b/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java index 703e50a088..0d935dba22 100644 --- a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java +++ b/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java @@ -142,6 +142,18 @@ public class NetworkNotificationManager { extraInfo = null; } + // Clear any previous notification with lower priority, otherwise return. http://b/63676954. + // A new SIGN_IN notification with a new intent should override any existing one. + final int previousEventId = mNotificationTypeMap.get(id); + final NotificationType previousNotifyType = NotificationType.getFromId(previousEventId); + if (priority(previousNotifyType) > priority(notifyType)) { + Slog.d(TAG, String.format( + "ignoring notification %s for network %s with existing notification %s", + notifyType, id, previousNotifyType)); + return; + } + clearNotification(id); + if (DBG) { Slog.d(TAG, String.format( "showNotification tag=%s event=%s transport=%s extraInfo=%s highPrioriy=%s", @@ -274,4 +286,22 @@ public class NetworkNotificationManager { NotificationType t = NotificationType.getFromId(eventId); return (t != null) ? t.name() : "UNKNOWN"; } + + private static int priority(NotificationType t) { + if (t == null) { + return 0; + } + switch (t) { + case SIGN_IN: + return 4; + case NO_INTERNET: + return 3; + case NETWORK_SWITCH: + return 2; + case LOST_INTERNET: + return 1; + default: + return 0; + } + } } diff --git a/tests/net/java/com/android/server/connectivity/NetworkNotificationManagerTest.java b/tests/net/java/com/android/server/connectivity/NetworkNotificationManagerTest.java index f201bc7a7d..911347c134 100644 --- a/tests/net/java/com/android/server/connectivity/NetworkNotificationManagerTest.java +++ b/tests/net/java/com/android/server/connectivity/NetworkNotificationManagerTest.java @@ -16,6 +16,16 @@ package com.android.server.connectivity; +import static com.android.server.connectivity.NetworkNotificationManager.NotificationType.*; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyInt; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + import android.app.Notification; import android.app.NotificationManager; import android.content.Context; @@ -37,15 +47,6 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; -import static com.android.server.connectivity.NetworkNotificationManager.NotificationType.*; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.anyInt; -import static org.mockito.Mockito.eq; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - public class NetworkNotificationManagerTest extends TestCase { static final NetworkCapabilities CELL_CAPABILITIES = new NetworkCapabilities(); @@ -140,4 +141,47 @@ public class NetworkNotificationManagerTest extends TestCase { verify(mNotificationManager, never()).notifyAsUser(any(), anyInt(), any(), any()); } + + @SmallTest + public void testDuplicatedNotificationsNoInternetThenSignIn() { + final int id = 101; + final String tag = NetworkNotificationManager.tagFor(id); + + // Show first NO_INTERNET + mManager.showNotification(id, NO_INTERNET, mWifiNai, mCellNai, null, false); + verify(mNotificationManager, times(1)) + .notifyAsUser(eq(tag), eq(NO_INTERNET.eventId), any(), any()); + + // Captive portal detection triggers SIGN_IN a bit later, clearing the previous NO_INTERNET + mManager.showNotification(id, SIGN_IN, mWifiNai, mCellNai, null, false); + verify(mNotificationManager, times(1)) + .cancelAsUser(eq(tag), eq(NO_INTERNET.eventId), any()); + verify(mNotificationManager, times(1)) + .notifyAsUser(eq(tag), eq(SIGN_IN.eventId), any(), any()); + + // Network disconnects + mManager.clearNotification(id); + verify(mNotificationManager, times(1)).cancelAsUser(eq(tag), eq(SIGN_IN.eventId), any()); + } + + @SmallTest + public void testDuplicatedNotificationsSignInThenNoInternet() { + final int id = 101; + final String tag = NetworkNotificationManager.tagFor(id); + + // Show first SIGN_IN + mManager.showNotification(id, SIGN_IN, mWifiNai, mCellNai, null, false); + verify(mNotificationManager, times(1)) + .notifyAsUser(eq(tag), eq(SIGN_IN.eventId), any(), any()); + reset(mNotificationManager); + + // NO_INTERNET arrives after, but is ignored. + mManager.showNotification(id, NO_INTERNET, mWifiNai, mCellNai, null, false); + verify(mNotificationManager, never()).cancelAsUser(any(), anyInt(), any()); + verify(mNotificationManager, never()).notifyAsUser(any(), anyInt(), any(), any()); + + // Network disconnects + mManager.clearNotification(id); + verify(mNotificationManager, times(1)).cancelAsUser(eq(tag), eq(SIGN_IN.eventId), any()); + } }