Merge "Fix thread-safety issues with mRequestedNotificationListeners" into udc-dev am: ca08538cd3

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/22673942

Change-Id: I497eef91c9f951ea0ea76cf24611a505a2df96fc
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

services/core/java/com/android/server/notification/NotificationManagerService.java

@@ -10796,7 +10796,8 @@ public class NotificationManagerService extends SystemService {
         static final String FLAG_SEPARATOR = "\\|";
 
         private final ArraySet<ManagedServiceInfo> mLightTrimListeners = new ArraySet<>();
-        ArrayMap<Pair<ComponentName, Integer>, NotificationListenerFilter>
+        @GuardedBy("mRequestedNotificationListeners")
+        private final ArrayMap<Pair<ComponentName, Integer>, NotificationListenerFilter>
                 mRequestedNotificationListeners = new ArrayMap<>();
         private final boolean mIsHeadlessSystemUserMode;
 
@@ -10914,17 +10915,20 @@ public class NotificationManagerService extends SystemService {
         @Override
         public void onUserRemoved(int user) {
             super.onUserRemoved(user);
+            synchronized (mRequestedNotificationListeners) {
                 for (int i = mRequestedNotificationListeners.size() - 1; i >= 0; i--) {
                     if (mRequestedNotificationListeners.keyAt(i).second == user) {
                         mRequestedNotificationListeners.removeAt(i);
                     }
                 }
             }
+        }
 
         @Override
         public void onPackagesChanged(boolean removingPackage, String[] pkgList, int[] uidList) {
             super.onPackagesChanged(removingPackage, pkgList, uidList);
 
+            synchronized (mRequestedNotificationListeners) {
                 // Since the default behavior is to allow everything, we don't need to explicitly
                 // handle package add or update. they will be added to the xml file on next boot or
                 // when the user tries to change the settings.
@@ -10933,7 +10937,8 @@ public class NotificationManagerService extends SystemService {
                         String pkg = pkgList[i];
                         int userId = UserHandle.getUserId(uidList[i]);
                         for (int j = mRequestedNotificationListeners.size() - 1; j >= 0; j--) {
-                        Pair<ComponentName, Integer> key = mRequestedNotificationListeners.keyAt(j);
+                            Pair<ComponentName, Integer> key =
+                                    mRequestedNotificationListeners.keyAt(j);
                             if (key.second == userId && key.first.getPackageName().equals(pkg)) {
                                 mRequestedNotificationListeners.removeAt(j);
                             }
@@ -10944,15 +10949,16 @@ public class NotificationManagerService extends SystemService {
                 // clean up anything in the disallowed pkgs list
                 for (int i = 0; i < pkgList.length; i++) {
                     String pkg = pkgList[i];
-                int userId = UserHandle.getUserId(uidList[i]);
                     for (int j = mRequestedNotificationListeners.size() - 1; j >= 0; j--) {
-                    NotificationListenerFilter nlf = mRequestedNotificationListeners.valueAt(j);
+                        NotificationListenerFilter nlf =
+                                mRequestedNotificationListeners.valueAt(j);
 
                         VersionedPackage ai = new VersionedPackage(pkg, uidList[i]);
                         nlf.removePackage(ai);
                     }
                 }
             }
+        }
 
         @Override
         protected String getRequiredPermission() {
@@ -10997,15 +11003,19 @@ public class NotificationManagerService extends SystemService {
                     }
                     NotificationListenerFilter nlf =
                             new NotificationListenerFilter(approved, disallowedPkgs);
+                    synchronized (mRequestedNotificationListeners) {
                         mRequestedNotificationListeners.put(Pair.create(cn, userId), nlf);
                     }
                 }
             }
+        }
 
         @Override
         protected void writeExtraXmlTags(TypedXmlSerializer out) throws IOException {
             out.startTag(null, TAG_REQUESTED_LISTENERS);
-            for (Pair<ComponentName, Integer> listener : mRequestedNotificationListeners.keySet()) {
+            synchronized (mRequestedNotificationListeners) {
+                for (Pair<ComponentName, Integer> listener :
+                        mRequestedNotificationListeners.keySet()) {
                     NotificationListenerFilter nlf = mRequestedNotificationListeners.get(listener);
                     out.startTag(null, TAG_REQUESTED_LISTENER);
                     XmlUtils.writeStringAttribute(
@@ -11027,23 +11037,29 @@ public class NotificationManagerService extends SystemService {
 
                     out.endTag(null, TAG_REQUESTED_LISTENER);
                 }
+            }
 
             out.endTag(null, TAG_REQUESTED_LISTENERS);
         }
 
-        protected @Nullable NotificationListenerFilter getNotificationListenerFilter(
+        @Nullable protected NotificationListenerFilter getNotificationListenerFilter(
                 Pair<ComponentName, Integer> pair) {
+            synchronized (mRequestedNotificationListeners) {
                 return mRequestedNotificationListeners.get(pair);
             }
+        }
 
         protected void setNotificationListenerFilter(Pair<ComponentName, Integer> pair,
                 NotificationListenerFilter nlf) {
+            synchronized (mRequestedNotificationListeners) {
                 mRequestedNotificationListeners.put(pair, nlf);
             }
+        }
 
         @Override
         protected void ensureFilters(ServiceInfo si, int userId) {
-            Pair listener = Pair.create(si.getComponentName(), userId);
+            Pair<ComponentName, Integer> listener = Pair.create(si.getComponentName(), userId);
+            synchronized (mRequestedNotificationListeners) {
                 NotificationListenerFilter existingNlf =
                         mRequestedNotificationListeners.get(listener);
                 if (si.metaData != null) {
@@ -11075,6 +11091,7 @@ public class NotificationManagerService extends SystemService {
                     }
                 }
             }
+        }
 
         private int getTypesFromStringList(String typeList) {
             int types = 0;

services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java

@@ -71,10 +71,10 @@ import android.service.notification.StatusBarNotification;
 import android.testing.TestableContext;
 import android.util.ArraySet;
 import android.util.Pair;
-import com.android.modules.utils.TypedXmlPullParser;
-import com.android.modules.utils.TypedXmlSerializer;
 import android.util.Xml;
 
+import com.android.modules.utils.TypedXmlPullParser;
+import com.android.modules.utils.TypedXmlSerializer;
 import com.android.server.UiServiceTestCase;
 
 import com.google.common.collect.ImmutableList;
@@ -92,6 +92,7 @@ import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.util.Arrays;
 import java.util.List;
+import java.util.concurrent.CountDownLatch;
 
 public class NotificationListenersTest extends UiServiceTestCase {
 
@@ -626,6 +627,58 @@ public class NotificationListenersTest extends UiServiceTestCase {
                 .onNotificationChannelGroupModification(anyString(), any(), any(), anyInt());
     }
 
+    @Test
+    public void testNotificationListenerFilter_threadSafety() throws Exception {
+        testThreadSafety(() -> {
+            mListeners.setNotificationListenerFilter(
+                    new Pair<>(new ComponentName("pkg1", "cls1"), 0),
+                    new NotificationListenerFilter());
+            mListeners.setNotificationListenerFilter(
+                    new Pair<>(new ComponentName("pkg2", "cls2"), 10),
+                    new NotificationListenerFilter());
+            mListeners.setNotificationListenerFilter(
+                    new Pair<>(new ComponentName("pkg3", "cls3"), 11),
+                    new NotificationListenerFilter());
+
+            mListeners.onUserRemoved(10);
+            mListeners.onPackagesChanged(true, new String[]{"pkg1", "pkg2"}, new int[]{0, 0});
+        }, 20, 50);
+    }
+
+    /**
+     * Helper method to test the thread safety of some operations.
+     *
+     * <p>Runs the supplied {@code operationToTest}, {@code nRunsPerThread} times,
+     * concurrently using {@code nThreads} threads, and waits for all of them to finish.
+     */
+    private static void testThreadSafety(Runnable operationToTest, int nThreads,
+            int nRunsPerThread) throws InterruptedException {
+        final CountDownLatch startLatch = new CountDownLatch(1);
+        final CountDownLatch doneLatch = new CountDownLatch(nThreads);
+
+        for (int i = 0; i < nThreads; i++) {
+            Runnable threadRunnable = () -> {
+                try {
+                    startLatch.await();
+                    for (int j = 0; j < nRunsPerThread; j++) {
+                        operationToTest.run();
+                    }
+                } catch (InterruptedException e) {
+                    e.printStackTrace();
+                } finally {
+                    doneLatch.countDown();
+                }
+            };
+            new Thread(threadRunnable, "Test Thread #" + i).start();
+        }
+
+        // Ready set go
+        startLatch.countDown();
+
+        // Wait for all test threads to be done.
+        doneLatch.await();
+    }
+
     private ManagedServices.ManagedServiceInfo getParcelingListener(
             final NotificationChannelGroup toParcel)
             throws RemoteException {