NetworkNotificationManager: correctly handle existing notifications
This patch corrects a regression added by commit 51727428fcfa 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 51727428fcfa, 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
Change-Id: Ib8658601e8d4dc6c41b335ab7dd8caa0cccd9531
diff --git a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java b/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java
index 4ff6657..1bf9733 100644
--- a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java
+++ b/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java
@@ -140,6 +140,18 @@
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",
@@ -270,4 +282,22 @@
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 f201bc7..911347c 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.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 @@
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());
+ }
}