Ensure PM watches all Watchables

Bug: 179388643

The bug occurs because two Watchables were not being observed by
PackageManagerService.  The two Watchables are mSettings and
mInstantAppRegistry.  The bug should have been detected by
Watchable.verifyWatchedAttributes() but that, too had a bug: the
PackageManagerService attributes were not visible to the Watchable
package so the verification code got an IllegalAccessException, which
caused the attributes to be silently ignored.

The following changes are made:
 1. PackageManagerService now registers with the missing Watchables.
 2. Registration is moved to a function that is called in two places.
 3. The verification code is enhanced to ensure fields are
    accessible.  A RuntimeException is thrown if the field is still
    not accessible.  Note that only fields annotated with @Watched can
    generate this exception.
 4. PackageManagerVerification is skipped (log-only on errors) if it
    appears to be part of a mockito test.
 4. Settings now registers with a missing Watchable.  The Watchable is
    not actually used in Settings (see the TODO at the attribute
    declaration) but this CL assumes that it might be used in the
    future.
 6. One import order violation was corrected.

In addition to automated tests, the changes were tested with an
instrumented PackageManagerService that enabled snapshots and skipped
registration with one or more observers.  All attributes were tested
one by one.

Test: atest
 * FrameworksServicesTests:UserSystemPackageInstallerTest
 * FrameworksServicesTests:PackageManagerSettingsTests
 * FrameworksServicesTests:PackageManagerServiceTest
 * FrameworksServicesTests:AppsFilterTest
 * FrameworksServicesTests:PackageInstallerSessionTest
 * FrameworksServicesTests:ScanTests
 * UserLifecycleTests#startUser
 * UserLifecycleTests#stopUser
 * UserLifecycleTests#switchUser
 * android.appsecurity.cts.EphemeralTest#testEphemeralStartExposed06
 * android.appsecurity.cts.InstantAppUserTest#testStartExposed06
 * com.android.server.pm.PackageManagerServiceBootTest

Change-Id: Ib2b14c4745bd5e5ab2882ed6fe953d7da2df4087

3 files changed, 59 insertions, 43 deletions

diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java
index 5d4fa1e1c123..587e625e053f 100644
--- a/services/core/java/com/android/server/pm/PackageManagerService.java
+++ b/services/core/java/com/android/server/pm/PackageManagerService.java
@@ -5940,6 +5940,21 @@ public class PackageManagerService extends IPackageManager.Stub
         }
     }
 
+    // Link watchables to the class
+    private void registerObserver() {
+        mPackages.registerObserver(mWatcher);
+        mSharedLibraries.registerObserver(mWatcher);
+        mStaticLibsByDeclaringPackage.registerObserver(mWatcher);
+        mInstrumentation.registerObserver(mWatcher);
+        mWebInstantAppsDisabled.registerObserver(mWatcher);
+        mAppsFilter.registerObserver(mWatcher);
+        mInstantAppRegistry.registerObserver(mWatcher);
+        mSettings.registerObserver(mWatcher);
+        // If neither "build" attribute is true then this may be a mockito test, and verification
+        // can fail as a false positive.
+        Watchable.verifyWatchedAttributes(this, mWatcher, !(mIsEngBuild || mIsUserDebugBuild));
+    }
+
     /**
      * A extremely minimal constructor designed to start up a PackageManagerService instance for
      * testing.
@@ -6023,15 +6038,7 @@ public class PackageManagerService extends IPackageManager.Stub
         sSnapshotCorked = true;
         mLiveComputer = createLiveComputer();
         mSnapshotComputer = mLiveComputer;
-
-        // Link up the watchers
-        mPackages.registerObserver(mWatcher);
-        mSharedLibraries.registerObserver(mWatcher);
-        mStaticLibsByDeclaringPackage.registerObserver(mWatcher);
-        mInstrumentation.registerObserver(mWatcher);
-        mWebInstantAppsDisabled.registerObserver(mWatcher);
-        mAppsFilter.registerObserver(mWatcher);
-        Watchable.verifyWatchedAttributes(this, mWatcher);
+        registerObserver();
 
         mPackages.putAll(testParams.packages);
         mEnableFreeCacheV2 = testParams.enableFreeCacheV2;
@@ -6185,15 +6192,6 @@ public class PackageManagerService extends IPackageManager.Stub
         mDomainVerificationManager = injector.getDomainVerificationManagerInternal();
         mDomainVerificationManager.setConnection(mDomainVerificationConnection);
 
-        // Link up the watchers
-        mPackages.registerObserver(mWatcher);
-        mSharedLibraries.registerObserver(mWatcher);
-        mStaticLibsByDeclaringPackage.registerObserver(mWatcher);
-        mInstrumentation.registerObserver(mWatcher);
-        mWebInstantAppsDisabled.registerObserver(mWatcher);
-        mAppsFilter.registerObserver(mWatcher);
-        Watchable.verifyWatchedAttributes(this, mWatcher);
-
         // Create the computer as soon as the state objects have been installed.  The
         // cached computer is the same as the live computer until the end of the
         // constructor, at which time the invalidation method updates it.  The cache is
@@ -6202,6 +6200,7 @@ public class PackageManagerService extends IPackageManager.Stub
         sSnapshotCorked = true;
         mLiveComputer = createLiveComputer();
         mSnapshotComputer = mLiveComputer;
+        registerObserver();
 
         // CHECKSTYLE:OFF IndentationCheck
         synchronized (mInstallLock) {
diff --git a/services/core/java/com/android/server/pm/Settings.java b/services/core/java/com/android/server/pm/Settings.java
index fb033e6594b8..b8040ec98fda 100644
--- a/services/core/java/com/android/server/pm/Settings.java
+++ b/services/core/java/com/android/server/pm/Settings.java
@@ -105,9 +105,6 @@ import com.android.permission.persistence.RuntimePermissionsState;
 import com.android.server.LocalServices;
 import com.android.server.backup.PreferredActivityBackupHelper;
 import com.android.server.pm.Installer.InstallerException;
-import com.android.server.pm.verify.domain.DomainVerificationLegacySettings;
-import com.android.server.pm.verify.domain.DomainVerificationManagerInternal;
-import com.android.server.pm.verify.domain.DomainVerificationPersistence;
 import com.android.server.pm.parsing.PackageInfoUtils;
 import com.android.server.pm.parsing.pkg.AndroidPackage;
 import com.android.server.pm.parsing.pkg.AndroidPackageUtils;
@@ -115,6 +112,9 @@ import com.android.server.pm.permission.LegacyPermissionDataProvider;
 import com.android.server.pm.permission.LegacyPermissionSettings;
 import com.android.server.pm.permission.LegacyPermissionState;
 import com.android.server.pm.permission.LegacyPermissionState.PermissionState;
+import com.android.server.pm.verify.domain.DomainVerificationLegacySettings;
+import com.android.server.pm.verify.domain.DomainVerificationManagerInternal;
+import com.android.server.pm.verify.domain.DomainVerificationPersistence;
 import com.android.server.utils.Snappable;
 import com.android.server.utils.TimingsTraceAndSlog;
 import com.android.server.utils.Watchable;
@@ -487,7 +487,7 @@ public final class Settings implements Watchable, Snappable {
     // App-link priority tracking, per-user
     @NonNull
     @Watched
-    final WatchedSparseIntArray mNextAppLinkGeneration = new WatchedSparseIntArray();
+    private final WatchedSparseIntArray mNextAppLinkGeneration = new WatchedSparseIntArray();
 
     final StringBuilder mReadMessages = new StringBuilder();
 
@@ -552,6 +552,7 @@ public final class Settings implements Watchable, Snappable {
         mAppIds.registerObserver(mObserver);
         mOtherAppIds.registerObserver(mObserver);
         mRenamedPackages.registerObserver(mObserver);
+        mNextAppLinkGeneration.registerObserver(mObserver);
         mDefaultBrowserApp.registerObserver(mObserver);
 
         Watchable.verifyWatchedAttributes(this, mObserver);
@@ -602,6 +603,7 @@ public final class Settings implements Watchable, Snappable {
         mAppIds.registerObserver(mObserver);
         mOtherAppIds.registerObserver(mObserver);
         mRenamedPackages.registerObserver(mObserver);
+        mNextAppLinkGeneration.registerObserver(mObserver);
         mDefaultBrowserApp.registerObserver(mObserver);
 
         Watchable.verifyWatchedAttributes(this, mObserver);
@@ -649,6 +651,7 @@ public final class Settings implements Watchable, Snappable {
         mPastSignatures.addAll(r.mPastSignatures);
         mKeySetRefs.putAll(r.mKeySetRefs);
         mRenamedPackages.snapshot(r.mRenamedPackages);
+        mNextAppLinkGeneration.snapshot(r.mNextAppLinkGeneration);
         mDefaultBrowserApp.snapshot(r.mDefaultBrowserApp);
         // mReadMessages
         mPendingPackages.addAll(r.mPendingPackages);
diff --git a/services/core/java/com/android/server/utils/Watchable.java b/services/core/java/com/android/server/utils/Watchable.java
index f936693bd621..44a74593cda2 100644
--- a/services/core/java/com/android/server/utils/Watchable.java
+++ b/services/core/java/com/android/server/utils/Watchable.java
@@ -19,8 +19,8 @@ package com.android.server.utils;
 import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.os.Build;
+import android.util.Log;
 
-import java.lang.annotation.Annotation;
 import java.lang.reflect.Field;
 
 /**
@@ -61,40 +61,54 @@ public interface Watchable {
     public void dispatchChange(@Nullable Watchable what);
 
     /**
-     * Return true if the field is tagged with @Watched
-     */
-    private static boolean isWatched(Field f) {
-        for (Annotation a : f.getDeclaredAnnotations()) {
-            if (a.annotationType().equals(Watched.class)) {
-                return true;
-            }
-        }
-        return false;
-    }
-
-    /**
      * Verify that all @Watched {@link Watchable} attributes are being watched by this
      * class.  This requires reflection and only runs in engineering or user debug
      * builds.
+     * @param base The object that contains watched attributes.
+     * @param observer The {@link Watcher} that should be watching these attributes.
+     * @param logOnly If true then log errors; if false then throw an RuntimeExecption on error.
      */
-    static void verifyWatchedAttributes(Object base, Watcher observer) {
-        if (Build.IS_ENG || Build.IS_USERDEBUG) {
-            for (Field f : base.getClass().getDeclaredFields()) {
+    static void verifyWatchedAttributes(Object base, Watcher observer, boolean logOnly) {
+        if (!(Build.IS_ENG || Build.IS_USERDEBUG)) {
+            return;
+        }
+        for (Field f : base.getClass().getDeclaredFields()) {
+            if (f.getAnnotation(Watched.class) != null) {
+                final String fn = base.getClass().getName() + "." + f.getName();
                 try {
-                    final boolean flagged = isWatched(f);
+                    f.setAccessible(true);
                     final Object o = f.get(base);
-                    final boolean watchable = o instanceof Watchable;
-                    if (flagged && watchable) {
-                        Watchable attr = (Watchable) f.get(base);
+                    if (o instanceof Watchable) {
+                        Watchable attr = (Watchable) (o);
                         if (attr != null && !attr.isRegisteredObserver(observer)) {
-                            throw new RuntimeException(f.getName() + " missing an observer");
+                            if (logOnly) {
+                                Log.e("Watchable", fn + " missing an observer");
+                            } else {
+                                throw new RuntimeException("Watchable " + fn
+                                                           + " missing an observer");
+                            }
                         }
                     }
                 } catch (IllegalAccessException e) {
                     // The field is protected; ignore it.  Other exceptions that may be thrown by
                     // Field.get() are allowed to roll up.
+                    if (logOnly) {
+                        Log.e("Watchable", fn + " not visible");
+                    } else {
+                        throw new RuntimeException("Watchable " + fn + " not visible");
+                    }
                 }
             }
         }
     }
+
+    /**
+     * Verify that all @Watched {@link Watchable} attributes are being watched by this
+     * class.  This calls verifyWatchedAttributes() with logOnly set to false.
+     * @param base The object that contains watched attributes.
+     * @param observer The {@link Watcher} that should be watching these attributes.
+     */
+    static void verifyWatchedAttributes(Object base, Watcher observer) {
+        verifyWatchedAttributes(base, observer, false);
+    }
 }