Merge "Make ManagedServices more thread-safe" into udc-dev

2 files changed, 116 insertions, 42 deletions

diff --git a/services/core/java/com/android/server/notification/ManagedServices.java b/services/core/java/com/android/server/notification/ManagedServices.java
index 73440b7f2eec..12fc263e1c8c 100644
--- a/services/core/java/com/android/server/notification/ManagedServices.java
+++ b/services/core/java/com/android/server/notification/ManagedServices.java
@@ -140,34 +140,39 @@ abstract public class ManagedServices {
      * The services that have been bound by us. If the service is also connected, it will also
      * be in {@link #mServices}.
      */
+    @GuardedBy("mMutex")
     private final ArrayList<Pair<ComponentName, Integer>> mServicesBound = new ArrayList<>();
+    @GuardedBy("mMutex")
     private final ArraySet<Pair<ComponentName, Integer>> mServicesRebinding = new ArraySet<>();
     // we need these packages to be protected because classes that inherit from it need to see it
     protected final Object mDefaultsLock = new Object();
+    @GuardedBy("mDefaultsLock")
     protected final ArraySet<ComponentName> mDefaultComponents = new ArraySet<>();
+    @GuardedBy("mDefaultsLock")
     protected final ArraySet<String> mDefaultPackages = new ArraySet<>();
 
     // lists the component names of all enabled (and therefore potentially connected)
     // app services for current profiles.
-    private ArraySet<ComponentName> mEnabledServicesForCurrentProfiles
-            = new ArraySet<>();
+    @GuardedBy("mMutex")
+    private final ArraySet<ComponentName> mEnabledServicesForCurrentProfiles = new ArraySet<>();
     // Just the packages from mEnabledServicesForCurrentProfiles
-    private ArraySet<String> mEnabledServicesPackageNames = new ArraySet<>();
+    @GuardedBy("mMutex")
+    private final ArraySet<String> mEnabledServicesPackageNames = new ArraySet<>();
     // Per user id, list of enabled packages that have nevertheless asked not to be run
-    private final android.util.SparseSetArray<ComponentName> mSnoozing =
-            new android.util.SparseSetArray<>();
+    @GuardedBy("mSnoozing")
+    private final SparseSetArray<ComponentName> mSnoozing = new SparseSetArray<>();
 
     // List of approved packages or components (by user, then by primary/secondary) that are
     // allowed to be bound as managed services. A package or component appearing in this list does
     // not mean that we are currently bound to said package/component.
+    @GuardedBy("mApproved")
     protected final ArrayMap<Integer, ArrayMap<Boolean, ArraySet<String>>> mApproved =
             new ArrayMap<>();
-
     // List of packages or components (by user) that are configured to be enabled/disabled
     // explicitly by the user
     @GuardedBy("mApproved")
     protected ArrayMap<Integer, ArraySet<String>> mUserSetServices = new ArrayMap<>();
-
+    @GuardedBy("mApproved")
     protected ArrayMap<Integer, Boolean> mIsUserChanged = new ArrayMap<>();
 
     // True if approved services are stored in xml, not settings.
@@ -262,20 +267,18 @@ abstract public class ManagedServices {
     @NonNull
     ArrayMap<Boolean, ArrayList<ComponentName>> resetComponents(String packageName, int userId) {
         // components that we want to enable
-        ArrayList<ComponentName> componentsToEnable =
-                new ArrayList<>(mDefaultComponents.size());
-
+        ArrayList<ComponentName> componentsToEnable;
         // components that were removed
-        ArrayList<ComponentName> disabledComponents =
-                new ArrayList<>(mDefaultComponents.size());
-
+        ArrayList<ComponentName> disabledComponents;
         // all components that are enabled now
-        ArraySet<ComponentName> enabledComponents =
-                new ArraySet<>(getAllowedComponents(userId));
+        ArraySet<ComponentName> enabledComponents = new ArraySet<>(getAllowedComponents(userId));
 
         boolean changed = false;
 
         synchronized (mDefaultsLock) {
+            componentsToEnable = new ArrayList<>(mDefaultComponents.size());
+            disabledComponents = new ArrayList<>(mDefaultComponents.size());
+
             // record all components that are enabled but should not be by default
             for (int i = 0; i < mDefaultComponents.size() && enabledComponents.size() > 0; i++) {
                 ComponentName currentDefault = mDefaultComponents.valueAt(i);
@@ -374,14 +377,14 @@ abstract public class ManagedServices {
             }
         }
 
-        pw.println("    All " + getCaption() + "s (" + mEnabledServicesForCurrentProfiles.size()
-                + ") enabled for current profiles:");
-        for (ComponentName cmpt : mEnabledServicesForCurrentProfiles) {
-            if (filter != null && !filter.matches(cmpt)) continue;
-            pw.println("      " + cmpt);
-        }
-
         synchronized (mMutex) {
+            pw.println("    All " + getCaption() + "s (" + mEnabledServicesForCurrentProfiles.size()
+                    + ") enabled for current profiles:");
+            for (ComponentName cmpt : mEnabledServicesForCurrentProfiles) {
+                if (filter != null && !filter.matches(cmpt)) continue;
+                pw.println("      " + cmpt);
+            }
+
             pw.println("    Live " + getCaption() + "s (" + mServices.size() + "):");
             for (ManagedServiceInfo info : mServices) {
                 if (filter != null && !filter.matches(info.component)) continue;
@@ -434,12 +437,12 @@ abstract public class ManagedServices {
             }
         }
 
-        for (ComponentName cmpt : mEnabledServicesForCurrentProfiles) {
-            if (filter != null && !filter.matches(cmpt)) continue;
-            cmpt.dumpDebug(proto, ManagedServicesProto.ENABLED);
-        }
 
         synchronized (mMutex) {
+            for (ComponentName cmpt : mEnabledServicesForCurrentProfiles) {
+                if (filter != null && !filter.matches(cmpt)) continue;
+                cmpt.dumpDebug(proto, ManagedServicesProto.ENABLED);
+            }
             for (ManagedServiceInfo info : mServices) {
                 if (filter != null && !filter.matches(info.component)) continue;
                 info.dumpDebug(proto, ManagedServicesProto.LIVE_SERVICES, this);
@@ -677,7 +680,9 @@ abstract public class ManagedServices {
                         if (isUserChanged == null) { //NLS
                             userSetComponent = TextUtils.emptyIfNull(userSetComponent);
                         } else { //NAS
-                            mIsUserChanged.put(resolvedUserId, Boolean.valueOf(isUserChanged));
+                            synchronized (mApproved) {
+                                mIsUserChanged.put(resolvedUserId, Boolean.valueOf(isUserChanged));
+                            }
                             userSetComponent = Boolean.valueOf(isUserChanged) ? approved : "";
                         }
                     } else {
@@ -688,7 +693,9 @@ abstract public class ManagedServices {
                             if (isUserChanged_Old != null && Boolean.valueOf(isUserChanged_Old)) {
                                 //user_set = true
                                 userSetComponent = approved;
-                                mIsUserChanged.put(resolvedUserId, true);
+                                synchronized (mApproved) {
+                                    mIsUserChanged.put(resolvedUserId, true);
+                                }
                                 needUpgradeUserset = false;
                             } else {
                                 userSetComponent = "";
@@ -724,8 +731,11 @@ abstract public class ManagedServices {
     }
 
     void upgradeDefaultsXmlVersion() {
-        // check if any defaults are loaded
-        int defaultsSize = mDefaultComponents.size() + mDefaultPackages.size();
+        int defaultsSize;
+        synchronized (mDefaultsLock) {
+            // check if any defaults are loaded
+            defaultsSize = mDefaultComponents.size() + mDefaultPackages.size();
+        }
         if (defaultsSize == 0) {
             // load defaults from current allowed
             if (this.mApprovalLevel == APPROVAL_BY_COMPONENT) {
@@ -741,8 +751,10 @@ abstract public class ManagedServices {
                 }
             }
         }
+        synchronized (mDefaultsLock) {
+            defaultsSize = mDefaultComponents.size() + mDefaultPackages.size();
+        }
         // if no defaults are loaded, then load from config
-        defaultsSize = mDefaultComponents.size() + mDefaultPackages.size();
         if (defaultsSize == 0) {
             loadDefaultsFromConfig();
         }
@@ -806,7 +818,9 @@ abstract public class ManagedServices {
     }
 
     protected boolean isComponentEnabledForPackage(String pkg) {
-        return mEnabledServicesPackageNames.contains(pkg);
+        synchronized (mMutex) {
+            return mEnabledServicesPackageNames.contains(pkg);
+        }
     }
 
     protected void setPackageOrComponentEnabled(String pkgOrComponent, int userId,
@@ -959,9 +973,13 @@ abstract public class ManagedServices {
     }
 
     public void onPackagesChanged(boolean removingPackage, String[] pkgList, int[] uidList) {
-        if (DEBUG) Slog.d(TAG, "onPackagesChanged removingPackage=" + removingPackage
-                + " pkgList=" + (pkgList == null ? null : Arrays.asList(pkgList))
-                + " mEnabledServicesPackageNames=" + mEnabledServicesPackageNames);
+        if (DEBUG) {
+            synchronized (mMutex) {
+                Slog.d(TAG, "onPackagesChanged removingPackage=" + removingPackage
+                        + " pkgList=" + (pkgList == null ? null : Arrays.asList(pkgList))
+                        + " mEnabledServicesPackageNames=" + mEnabledServicesPackageNames);
+            }
+        }
 
         if (pkgList != null && (pkgList.length > 0)) {
             boolean anyServicesInvolved = false;
@@ -975,7 +993,7 @@ abstract public class ManagedServices {
                 }
             }
             for (String pkgName : pkgList) {
-                if (mEnabledServicesPackageNames.contains(pkgName)) {
+                if (isComponentEnabledForPackage(pkgName)) {
                     anyServicesInvolved = true;
                 }
                 if (uidList != null && uidList.length > 0) {
@@ -1299,9 +1317,11 @@ abstract public class ManagedServices {
             }
 
             final Set<ComponentName> add = new HashSet<>(userComponents);
-            ArraySet<ComponentName> snoozed = mSnoozing.get(userId);
-            if (snoozed != null) {
-                add.removeAll(snoozed);
+            synchronized (mSnoozing) {
+                ArraySet<ComponentName> snoozed = mSnoozing.get(userId);
+                if (snoozed != null) {
+                    add.removeAll(snoozed);
+                }
             }
 
             componentsToBind.put(userId, add);
@@ -1605,9 +1625,12 @@ abstract public class ManagedServices {
         }
     }
 
+    @VisibleForTesting
     boolean isBound(ComponentName cn, int userId) {
         final Pair<ComponentName, Integer> servicesBindingTag = Pair.create(cn, userId);
-        return mServicesBound.contains(servicesBindingTag);
+        synchronized (mMutex) {
+            return mServicesBound.contains(servicesBindingTag);
+        }
     }
 
     protected boolean isBoundOrRebinding(final ComponentName cn, final int userId) {
@@ -1833,7 +1856,9 @@ abstract public class ManagedServices {
         public boolean isEnabledForCurrentProfiles() {
             if (this.isSystem) return true;
             if (this.connection == null) return false;
-            return mEnabledServicesForCurrentProfiles.contains(this.component);
+            synchronized (mMutex) {
+                return mEnabledServicesForCurrentProfiles.contains(this.component);
+            }
         }
 
         /**
@@ -1877,7 +1902,9 @@ abstract public class ManagedServices {
 
     /** convenience method for looking in mEnabledServicesForCurrentProfiles */
     public boolean isComponentEnabledForCurrentProfiles(ComponentName component) {
-        return mEnabledServicesForCurrentProfiles.contains(component);
+        synchronized (mMutex) {
+            return mEnabledServicesForCurrentProfiles.contains(component);
+        }
     }
 
     public static class UserProfiles {
diff --git a/services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java b/services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java
index 8fcbf2f9e97a..541739d50024 100644
--- a/services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java
+++ b/services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java
@@ -96,6 +96,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.CountDownLatch;
 
 public class ManagedServicesTest extends UiServiceTestCase {
 
@@ -1920,6 +1921,18 @@ public class ManagedServicesTest extends UiServiceTestCase {
         assertTrue(service.isBound(cn_disallowed, 0));
     }
 
+    @Test
+    public void isComponentEnabledForCurrentProfiles_isThreadSafe() throws InterruptedException {
+        for (UserInfo userInfo : mUm.getUsers()) {
+            mService.addApprovedList("pkg1/cmp1:pkg2/cmp2:pkg3/cmp3", userInfo.id, true);
+        }
+        testThreadSafety(() -> {
+            mService.rebindServices(false, 0);
+            assertThat(mService.isComponentEnabledForCurrentProfiles(
+                    new ComponentName("pkg1", "cmp1"))).isTrue();
+        }, 20, 30);
+    }
+
     private void mockServiceInfoWithMetaData(List<ComponentName> componentNames,
             ManagedServices service, ArrayMap<ComponentName, Bundle> metaDatas)
             throws RemoteException {
@@ -2298,4 +2311,38 @@ public class ManagedServicesTest extends UiServiceTestCase {
             return false;
         }
     }
+
+    /**
+     * 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();
+    }
 }