NetworkNotificationManager: correctly handle existing notifications

This patch corrects a regression added by commit fb2609d3eee1 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 fb2609d3eee1, 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

diff --git a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java b/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java
index 703e50a..0d935db 100644 (file)
--- 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 f201bc7..911347c 100644 (file)
--- 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());
+    }
 }