Do not load xml metadata for unchanged packages in RegisteredServicesCache

RegisteredServicesCache caches services in an xml, so the system doesn't
have to scan all the apks. Whenever a package is changed
(add/remove/replaced), the broadcast receiver in RegisteredServicesCache
will refresh this cache by quering all matching services and reading
their
xml metadata. There is extra work done here, because only the changed
package
will have services added/removed.

The edge case is after a cache is invalidated, we want to query package
manager for everything, regardless of what changedUids is (we could
have arrived here if invalidateCache is called, and then a package
change event fires, before a getAllServices is called to rescan
everything).

Add a new test to verify that the optimization takes care of the case
when the cache is invalidated.

Bug: 117755076
Bug: 122912184
Test: atest RegisteredServicesCache
Test: dumpsys content # check sync adapters
Change-Id: I5c1f57108c4b67d24b198000d57216c63d35290a
(cherry picked from commit dc66ef5ed383a889779adcd455826dcd1534ccf9)

diff --git a/core/java/android/content/pm/RegisteredServicesCache.java b/core/java/android/content/pm/RegisteredServicesCache.java
index a8c3b88..d7c4439 100644 (file)
--- a/core/java/android/content/pm/RegisteredServicesCache.java
+++ b/core/java/android/content/pm/RegisteredServicesCache.java
@@ -17,6 +17,7 @@
 package android.content.pm;
 
 import android.Manifest;
+import android.annotation.Nullable;
 import android.annotation.UnsupportedAppUsage;
 import android.content.BroadcastReceiver;
 import android.content.ComponentName;
@@ -176,7 +177,8 @@ public abstract class RegisteredServicesCache<V> {
         mContext.registerReceiver(mUserRemovedReceiver, userFilter);
     }
 
-    private final void handlePackageEvent(Intent intent, int userId) {
+    @VisibleForTesting
+    protected void handlePackageEvent(Intent intent, int userId) {
         // Don't regenerate the services map when the package is removed or its
         // ASEC container unmounted as a step in replacement.  The subsequent
         // _ADDED / _AVAILABLE call will regenerate the map in the final state.
@@ -238,6 +240,9 @@ public abstract class RegisteredServicesCache<V> {
 
     public void invalidateCache(int userId) {
         synchronized (mServicesLock) {
+            if (DEBUG) {
+                Slog.d(TAG, "invalidating cache for " + userId + " " + mInterfaceName);
+            }
             final UserServices<V> user = findOrCreateUserLocked(userId);
             user.services = null;
             onServicesChangedLocked(userId);
@@ -465,34 +470,48 @@ public abstract class RegisteredServicesCache<V> {
      *                    or null to assume that everything is affected.
      * @param userId the user for whom to update the services map.
      */
-    private void generateServicesMap(int[] changedUids, int userId) {
+    private void generateServicesMap(@Nullable int[] changedUids, int userId) {
         if (DEBUG) {
             Slog.d(TAG, "generateServicesMap() for " + userId + ", changed UIDs = "
                     + Arrays.toString(changedUids));
         }
 
-        final ArrayList<ServiceInfo<V>> serviceInfos = new ArrayList<>();
-        final List<ResolveInfo> resolveInfos = queryIntentServices(userId);
-        for (ResolveInfo resolveInfo : resolveInfos) {
-            try {
-                ServiceInfo<V> info = parseServiceInfo(resolveInfo);
-                if (info == null) {
-                    Log.w(TAG, "Unable to load service info " + resolveInfo.toString());
-                    continue;
-                }
-                serviceInfos.add(info);
-            } catch (XmlPullParserException|IOException e) {
-                Log.w(TAG, "Unable to load service info " + resolveInfo.toString(), e);
-            }
-        }
-
         synchronized (mServicesLock) {
             final UserServices<V> user = findOrCreateUserLocked(userId);
-            final boolean firstScan = user.services == null;
-            if (firstScan) {
+            final boolean cacheInvalid = user.services == null;
+            if (cacheInvalid) {
                 user.services = Maps.newHashMap();
             }
 
+            final ArrayList<ServiceInfo<V>> serviceInfos = new ArrayList<>();
+            final List<ResolveInfo> resolveInfos = queryIntentServices(userId);
+
+            for (ResolveInfo resolveInfo : resolveInfos) {
+                try {
+                    // when changedUids == null, we want to do a rescan of everything, this means
+                    // it's the initial scan, and containsUid will trivially return true
+                    // when changedUids != null, we got here because a package changed, but
+                    // invalidateCache could have been called (thus user.services == null), and we
+                    // should query from PackageManager again
+                    if (!cacheInvalid
+                            && !containsUid(
+                                    changedUids, resolveInfo.serviceInfo.applicationInfo.uid)) {
+                        if (DEBUG) {
+                            Slog.d(TAG, "Skipping parseServiceInfo for " + resolveInfo);
+                        }
+                        continue;
+                    }
+                    ServiceInfo<V> info = parseServiceInfo(resolveInfo);
+                    if (info == null) {
+                        Log.w(TAG, "Unable to load service info " + resolveInfo.toString());
+                        continue;
+                    }
+                    serviceInfos.add(info);
+                } catch (XmlPullParserException | IOException e) {
+                    Log.w(TAG, "Unable to load service info " + resolveInfo.toString(), e);
+                }
+            }
+
             StringBuilder changes = new StringBuilder();
             boolean changed = false;
             for (ServiceInfo<V> info : serviceInfos) {
@@ -513,7 +532,7 @@ public abstract class RegisteredServicesCache<V> {
                     changed = true;
                     user.services.put(info.type, info);
                     user.persistentServices.put(info.type, info.uid);
-                    if (!(user.mPersistentServicesFileDidNotExist && firstScan)) {
+                    if (!(user.mPersistentServicesFileDidNotExist && cacheInvalid)) {
                         notifyListener(info.type, userId, false /* removed */);
                     }
                 } else if (previousUid == info.uid) {

diff --git a/core/tests/coretests/src/android/content/pm/RegisteredServicesCacheTest.java b/core/tests/coretests/src/android/content/pm/RegisteredServicesCacheTest.java
index d3d1f22..c8b449e 100644 (file)
--- a/core/tests/coretests/src/android/content/pm/RegisteredServicesCacheTest.java
+++ b/core/tests/coretests/src/android/content/pm/RegisteredServicesCacheTest.java
@@ -16,6 +16,7 @@
 
 package android.content.pm;
 
+import android.content.Intent;
 import android.content.res.Resources;
 import android.os.FileUtils;
 import android.os.Parcel;
@@ -188,6 +189,36 @@ public class RegisteredServicesCacheTest extends AndroidTestCase {
         assertEquals(0, cache.getPersistentServicesSize(u1));
     }
 
+    /**
+     * Check that an optimization to skip a call to PackageManager handles an invalidated cache.
+     *
+     * We added an optimization in generateServicesMap to only query PackageManager for packages
+     * that have been changed, because if a package is unchanged, we have already cached the
+     * services info for it, so we can save a query to PackageManager (and save some memory).
+     * However, if invalidateCache was called, we cannot optimize, and must do a full query.
+     * The initial optimization was buggy because it failed to check for an invalidated cache, and
+     * only scanned the changed packages, given in the ACTION_PACKAGE_CHANGED intent (b/122912184).
+     */
+    public void testParseServiceInfoOptimizationHandlesInvalidatedCache() {
+        TestServicesCache cache = new TestServicesCache();
+        cache.addServiceForQuerying(U0, r1, newServiceInfo(t1, UID1));
+        cache.addServiceForQuerying(U0, r2, newServiceInfo(t2, UID2));
+        assertEquals(2, cache.getAllServicesSize(U0));
+
+        // simulate the client of the cache invalidating it
+        cache.invalidateCache(U0);
+
+        // there should be 0 services (userServices.services == null ) at this point, but we don't
+        // call getAllServicesSize since that would force a full scan of packages,
+        // instead we trigger a package change in a package that is in the list of services
+        Intent intent = new Intent(Intent.ACTION_PACKAGE_CHANGED);
+        intent.putExtra(Intent.EXTRA_UID, UID1);
+        cache.handlePackageEvent(intent, U0);
+
+        // check that the optimization does a full query and caches both services
+        assertEquals(2, cache.getAllServicesSize(U0));
+    }
+
     private static RegisteredServicesCache.ServiceInfo<TestServiceType> newServiceInfo(
             TestServiceType type, int uid) {
         final ComponentInfo info = new ComponentInfo();
@@ -265,6 +296,11 @@ public class RegisteredServicesCacheTest extends AndroidTestCase {
                 map = new HashMap<>();
                 mServices.put(userId, map);
             }
+            // in actual cases, resolveInfo should always have a serviceInfo, since we specifically
+            // query for intent services
+            resolveInfo.serviceInfo = new android.content.pm.ServiceInfo();
+            resolveInfo.serviceInfo.applicationInfo =
+                new ApplicationInfo(serviceInfo.componentInfo.applicationInfo);
             map.put(resolveInfo, serviceInfo);
         }
 
@@ -303,6 +339,11 @@ public class RegisteredServicesCacheTest extends AndroidTestCase {
         public void onUserRemoved(int userId) {
             super.onUserRemoved(userId);
         }
+
+        @Override
+        public void handlePackageEvent(Intent intent, int userId) {
+            super.handlePackageEvent(intent, userId);
+        }
     }
 
     static class TestSerializer implements XmlSerializerAndParser<TestServiceType> {