diff options
4 files changed, 563 insertions, 168 deletions
diff --git a/services/core/java/com/android/server/ExplicitHealthCheckController.java b/services/core/java/com/android/server/ExplicitHealthCheckController.java index f50364d70bc7..164837ab98dd 100644 --- a/services/core/java/com/android/server/ExplicitHealthCheckController.java +++ b/services/core/java/com/android/server/ExplicitHealthCheckController.java @@ -39,7 +39,9 @@ import android.text.TextUtils; import android.util.Slog; import com.android.internal.annotations.GuardedBy; +import com.android.internal.util.Preconditions; +import java.util.Collections; import java.util.List; import java.util.function.Consumer; @@ -50,10 +52,22 @@ class ExplicitHealthCheckController { private static final String TAG = "ExplicitHealthCheckController"; private final Object mLock = new Object(); private final Context mContext; - @GuardedBy("mLock") @Nullable private StateCallback mStateCallback; + + // Called everytime the service is connected, so the watchdog can sync it's state with + // the health check service. In practice, should never be null after it has been #setEnabled. + @GuardedBy("mLock") @Nullable private Runnable mOnConnected; + // Called everytime a package passes the health check, so the watchdog is notified of the + // passing check. In practice, should never be null after it has been #setEnabled. + @GuardedBy("mLock") @Nullable private Consumer<String> mPassedConsumer; + // Actual binder object to the explicit health check service. @GuardedBy("mLock") @Nullable private IExplicitHealthCheckService mRemoteService; - @GuardedBy("mLock") @Nullable private ServiceConnection mConnection; + // Cache for packages supporting explicit health checks. This cache should not change while + // the health check service is running. @GuardedBy("mLock") @Nullable private List<String> mSupportedPackages; + // Connection to the explicit health check service, necessary to unbind + @GuardedBy("mLock") @Nullable private ServiceConnection mConnection; + // Bind state of the explicit health check service. + @GuardedBy("mLock") private boolean mEnabled; ExplicitHealthCheckController(Context context) { mContext = context; @@ -61,28 +75,40 @@ class ExplicitHealthCheckController { /** * Requests an explicit health check for {@code packageName}. - * After this request, the callback registered on {@link startService} can receive explicit + * After this request, the callback registered on {@link #setCallbacks} can receive explicit * health check passed results. * * @throws IllegalStateException if the service is not started */ public void request(String packageName) throws RemoteException { synchronized (mLock) { + if (!mEnabled) { + return; + } + enforceServiceReadyLocked(); + + Slog.i(TAG, "Requesting health check for package " + packageName); mRemoteService.request(packageName); } } /** * Cancels all explicit health checks for {@code packageName}. - * After this request, the callback registered on {@link startService} can no longer receive + * After this request, the callback registered on {@link #setCallbacks} can no longer receive * explicit health check passed results. * * @throws IllegalStateException if the service is not started */ public void cancel(String packageName) throws RemoteException { synchronized (mLock) { + if (!mEnabled) { + return; + } + enforceServiceReadyLocked(); + + Slog.i(TAG, "Cancelling health check for package " + packageName); mRemoteService.cancel(packageName); } } @@ -95,13 +121,21 @@ class ExplicitHealthCheckController { */ public void getSupportedPackages(Consumer<List<String>> consumer) throws RemoteException { synchronized (mLock) { + if (!mEnabled) { + consumer.accept(Collections.emptyList()); + return; + } + enforceServiceReadyLocked(); + if (mSupportedPackages == null) { + Slog.d(TAG, "Getting health check supported packages"); mRemoteService.getSupportedPackages(new RemoteCallback(result -> { mSupportedPackages = result.getStringArrayList(EXTRA_SUPPORTED_PACKAGES); consumer.accept(mSupportedPackages); })); } else { + Slog.d(TAG, "Getting cached health check supported packages"); consumer.accept(mSupportedPackages); } } @@ -115,95 +149,113 @@ class ExplicitHealthCheckController { */ public void getRequestedPackages(Consumer<List<String>> consumer) throws RemoteException { synchronized (mLock) { + if (!mEnabled) { + consumer.accept(Collections.emptyList()); + return; + } + enforceServiceReadyLocked(); + + Slog.d(TAG, "Getting health check requested packages"); mRemoteService.getRequestedPackages(new RemoteCallback( result -> consumer.accept( result.getStringArrayList(EXTRA_REQUESTED_PACKAGES)))); } } + /** Enables or disables explicit health checks. */ + public void setEnabled(boolean enabled) { + synchronized (mLock) { + if (enabled == mEnabled) { + return; + } + + Slog.i(TAG, "Setting explicit health checks enabled " + enabled); + mEnabled = enabled; + if (enabled) { + bindService(); + } else { + unbindService(); + } + } + } + /** - * Starts the explicit health check service. - * - * @param stateCallback will receive important state changes changes - * @param passedConsumer will accept packages that pass explicit health checks - * - * @throws IllegalStateException if the service is already started + * Sets callbacks to listen to important events from the controller. + * Should be called at initialization. */ - public void startService(StateCallback stateCallback, Consumer<String> passedConsumer) { + public void setCallbacks(Runnable onConnected, Consumer<String> passedConsumer) { + Preconditions.checkNotNull(onConnected); + Preconditions.checkNotNull(passedConsumer); + mOnConnected = onConnected; + mPassedConsumer = passedConsumer; + } + + /** Binds to the explicit health check service. */ + private void bindService() { synchronized (mLock) { if (mRemoteService != null) { - throw new IllegalStateException("Explicit health check service already started."); + return; + } + ComponentName component = getServiceComponentNameLocked(); + if (component == null) { + Slog.wtf(TAG, "Explicit health check service not found"); + return; } - mStateCallback = stateCallback; + + Intent intent = new Intent(); + intent.setComponent(component); + // TODO: Fix potential race conditions during mConnection state transitions. + // E.g after #onServiceDisconected, the mRemoteService object is invalid until + // we get an #onServiceConnected. mConnection = new ServiceConnection() { @Override public void onServiceConnected(ComponentName name, IBinder service) { - synchronized (mLock) { - mRemoteService = IExplicitHealthCheckService.Stub.asInterface(service); - try { - mRemoteService.setCallback(new RemoteCallback(result -> { - String packageName = - result.getString(EXTRA_HEALTH_CHECK_PASSED_PACKAGE); - if (!TextUtils.isEmpty(packageName)) { - passedConsumer.accept(packageName); - } else { - Slog.w(TAG, "Empty package passed explicit health check?"); - } - })); - mStateCallback.onStart(); - Slog.i(TAG, "Explicit health check service is connected " + name); - } catch (RemoteException e) { - Slog.wtf(TAG, "Coud not setCallback on explicit health check service"); - } - } + initState(service); + Slog.i(TAG, "Explicit health check service is connected " + name); } @Override @MainThread public void onServiceDisconnected(ComponentName name) { - resetState(); + // Service crashed or process was killed, #onServiceConnected will be called. + // Don't need to re-bind. Slog.i(TAG, "Explicit health check service is disconnected " + name); } @Override public void onBindingDied(ComponentName name) { - resetState(); + // Application hosting service probably got updated + // Need to re-bind. + synchronized (mLock) { + if (mEnabled) { + unbindService(); + bindService(); + } + } Slog.i(TAG, "Explicit health check service binding is dead " + name); } @Override public void onNullBinding(ComponentName name) { - resetState(); - Slog.i(TAG, "Explicit health check service binding is null " + name); + // Should never happen. Service returned null from #onBind. + Slog.wtf(TAG, "Explicit health check service binding is null?? " + name); } }; - ComponentName component = getServiceComponentNameLocked(); - if (component != null) { - Intent intent = new Intent(); - intent.setComponent(component); - mContext.bindServiceAsUser(intent, mConnection, Context.BIND_AUTO_CREATE, - UserHandle.of(UserHandle.USER_SYSTEM)); - } + Slog.i(TAG, "Binding to explicit health service"); + mContext.bindServiceAsUser(intent, mConnection, Context.BIND_AUTO_CREATE, + UserHandle.of(UserHandle.USER_SYSTEM)); } } - // TODO: Differentiate between expected vs unexpected stop? - /** Callback to receive important {@link ExplicitHealthCheckController} state changes. */ - abstract static class StateCallback { - /** The controller is ready and we can request explicit health checks for packages */ - public void onStart() {} - - /** The controller is not ready and we cannot request explicit health checks for packages */ - public void onStop() {} - } - - /** Stops the explicit health check service. */ - public void stopService() { + /** Unbinds the explicit health check service. */ + private void unbindService() { synchronized (mLock) { if (mRemoteService != null) { + Slog.i(TAG, "Unbinding from explicit health service"); mContext.unbindService(mConnection); + mRemoteService = null; } } } @@ -247,19 +299,41 @@ class ExplicitHealthCheckController { return name; } - private void resetState() { + private void initState(IBinder service) { synchronized (mLock) { - mStateCallback.onStop(); - mStateCallback = null; mSupportedPackages = null; - mRemoteService = null; - mConnection = null; + mRemoteService = IExplicitHealthCheckService.Stub.asInterface(service); + try { + mRemoteService.setCallback(new RemoteCallback(result -> { + String packageName = result.getString(EXTRA_HEALTH_CHECK_PASSED_PACKAGE); + if (!TextUtils.isEmpty(packageName)) { + synchronized (mLock) { + if (mPassedConsumer == null) { + Slog.w(TAG, "Health check passed for package " + packageName + + "but no consumer registered."); + } else { + mPassedConsumer.accept(packageName); + } + } + } else { + Slog.w(TAG, "Empty package passed explicit health check?"); + } + })); + if (mOnConnected == null) { + Slog.w(TAG, "Health check service connected but no runnable registered."); + } else { + mOnConnected.run(); + } + } catch (RemoteException e) { + Slog.wtf(TAG, "Could not setCallback on explicit health check service"); + } } } @GuardedBy("mLock") private void enforceServiceReadyLocked() { if (mRemoteService == null) { + // TODO: Try to bind to service throw new IllegalStateException("Explicit health check service not ready"); } } diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java index 660109cf6114..2ba4d975a6b0 100644 --- a/services/core/java/com/android/server/PackageWatchdog.java +++ b/services/core/java/com/android/server/PackageWatchdog.java @@ -26,11 +26,12 @@ import android.content.pm.VersionedPackage; import android.os.Environment; import android.os.Handler; import android.os.Looper; +import android.os.RemoteException; import android.os.SystemClock; import android.text.TextUtils; import android.util.ArrayMap; +import android.util.ArraySet; import android.util.AtomicFile; -import android.util.Log; import android.util.Slog; import android.util.Xml; @@ -54,10 +55,12 @@ import java.io.InputStream; import java.lang.annotation.Retention; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Set; +import java.util.function.Consumer; /** * Monitors the health of packages on the system and notifies interested observers when packages @@ -84,10 +87,10 @@ public class PackageWatchdog { private final Object mLock = new Object(); // System server context private final Context mContext; - // Handler to run package cleanup runnables - private final Handler mTimerHandler; - // Handler for processing IO and observer actions - private final Handler mWorkerHandler; + // Handler to run short running tasks + private final Handler mShortTaskHandler; + // Handler for processing IO and long running tasks + private final Handler mLongTaskHandler; // Contains (observer-name -> observer-handle) that have ever been registered from // previous boots. Observers with all packages expired are periodically pruned. // It is saved to disk on system shutdown and repouplated on startup so it survives reboots. @@ -97,6 +100,12 @@ public class PackageWatchdog { private final AtomicFile mPolicyFile; // Runnable to prune monitored packages that have expired private final Runnable mPackageCleanup; + private final ExplicitHealthCheckController mHealthCheckController; + // Flag to control whether explicit health checks are supported or not + @GuardedBy("mLock") + private boolean mIsHealthCheckEnabled = true; + @GuardedBy("mLock") + private boolean mIsPackagesReady; // Last SystemClock#uptimeMillis a package clean up was executed. // 0 if mPackageCleanup not running. private long mUptimeAtLastRescheduleMs; @@ -104,32 +113,30 @@ public class PackageWatchdog { // 0 if mPackageCleanup not running. private long mDurationAtLastReschedule; - // TODO(b/120598832): Remove redundant context param private PackageWatchdog(Context context) { - mContext = context; - mPolicyFile = new AtomicFile(new File(new File(Environment.getDataDirectory(), "system"), - "package-watchdog.xml")); - mTimerHandler = new Handler(Looper.myLooper()); - mWorkerHandler = BackgroundThread.getHandler(); - mPackageCleanup = this::rescheduleCleanup; - loadFromFile(); + // Needs to be constructed inline + this(context, new AtomicFile( + new File(new File(Environment.getDataDirectory(), "system"), + "package-watchdog.xml")), + new Handler(Looper.myLooper()), BackgroundThread.getHandler(), + new ExplicitHealthCheckController(context)); } /** - * Creates a PackageWatchdog for testing that uses the same {@code looper} for all handlers - * and creates package-watchdog.xml in an apps data directory. + * Creates a PackageWatchdog that allows injecting dependencies. */ @VisibleForTesting - PackageWatchdog(Context context, Looper looper) { + PackageWatchdog(Context context, AtomicFile policyFile, Handler shortTaskHandler, + Handler longTaskHandler, ExplicitHealthCheckController controller) { mContext = context; - mPolicyFile = new AtomicFile(new File(context.getFilesDir(), "package-watchdog.xml")); - mTimerHandler = new Handler(looper); - mWorkerHandler = mTimerHandler; + mPolicyFile = policyFile; + mShortTaskHandler = shortTaskHandler; + mLongTaskHandler = longTaskHandler; mPackageCleanup = this::rescheduleCleanup; + mHealthCheckController = controller; loadFromFile(); } - /** Creates or gets singleton instance of PackageWatchdog. */ public static PackageWatchdog getInstance(Context context) { synchronized (PackageWatchdog.class) { @@ -141,6 +148,20 @@ public class PackageWatchdog { } /** + * Called during boot to notify when packages are ready on the device so we can start + * binding. + */ + public void onPackagesReady() { + synchronized (mLock) { + mIsPackagesReady = true; + mHealthCheckController.setCallbacks(this::updateHealthChecks, + packageName -> onHealthCheckPassed(packageName)); + // Controller is disabled at creation until here where we may enable it + mHealthCheckController.setEnabled(mIsHealthCheckEnabled); + } + } + + /** * Registers {@code observer} to listen for package failures * * <p>Observers are expected to call this on boot. It does not specify any packages but @@ -163,40 +184,63 @@ public class PackageWatchdog { * Starts observing the health of the {@code packages} for {@code observer} and notifies * {@code observer} of any package failures within the monitoring duration. * - * <p>If monitoring a package with {@code withExplicitHealthCheck}, at the end of the monitoring - * duration if {@link #onExplicitHealthCheckPassed} was never called, + * <p>If monitoring a package supporting explicit health check, at the end of the monitoring + * duration if {@link #onHealthCheckPassed} was never called, * {@link PackageHealthObserver#execute} will be called as if the package failed. * * <p>If {@code observer} is already monitoring a package in {@code packageNames}, * the monitoring window of that package will be reset to {@code durationMs} and the health - * check state will be reset to a default depending on {@code withExplictHealthCheck}. + * check state will be reset to a default depending on if the package is contained in + * {@link mPackagesWithExplicitHealthCheckEnabled}. * * @throws IllegalArgumentException if {@code packageNames} is empty * or {@code durationMs} is less than 1 */ - public void startObservingHealth(PackageHealthObserver observer, List<String> packageNames, - long durationMs, boolean withExplicitHealthCheck) { - if (packageNames.isEmpty() || durationMs < 1) { + public void startObservingHealth(PackageHealthObserver observer, List<String> packages, + long durationMs) { + if (packages.isEmpty() || durationMs < 1) { throw new IllegalArgumentException("Observation not started, no packages specified" + "or invalid duration"); } + if (!mIsPackagesReady) { + // TODO: Queue observation requests when packages are not ready + Slog.w(TAG, "Attempt to observe when packages not ready"); + return; + } + + try { + Slog.i(TAG, "Getting packages supporting explicit health check"); + mHealthCheckController.getSupportedPackages(supportedPackages -> + startObservingInner(observer, packages, durationMs, supportedPackages)); + } catch (RemoteException e) { + Slog.wtf(TAG, "Failed to fetch supported explicit health check packages"); + } + } + + private void startObservingInner(PackageHealthObserver observer, + List<String> packageNames, long durationMs, + List<String> healthCheckSupportedPackages) { + Slog.i(TAG, "Start observing packages " + packageNames + + ". Explicit health check supported packages " + healthCheckSupportedPackages); List<MonitoredPackage> packages = new ArrayList<>(); for (int i = 0; i < packageNames.size(); i++) { - // When observing packages withExplicitHealthCheck, - // MonitoredPackage#mHasExplicitHealthCheckPassed will be false initially. - packages.add(new MonitoredPackage(packageNames.get(i), durationMs, - !withExplicitHealthCheck)); + String packageName = packageNames.get(i); + boolean shouldEnableHealthCheck = healthCheckSupportedPackages.contains(packageName); + // If we should enable explicit health check for a package, + // MonitoredPackage#mHasHealthCheckPassed will be false + // until PackageWatchdog#onHealthCheckPassed + packages.add(new MonitoredPackage(packageName, durationMs, !shouldEnableHealthCheck)); } synchronized (mLock) { ObserverInternal oldObserver = mAllObservers.get(observer.getName()); if (oldObserver == null) { - Slog.d(TAG, observer.getName() + " started monitoring health of packages " - + packageNames); + Slog.d(TAG, observer.getName() + " started monitoring health " + + "of packages " + packageNames); mAllObservers.put(observer.getName(), new ObserverInternal(observer.getName(), packages)); } else { - Slog.d(TAG, observer.getName() + " added the following packages to monitor " - + packageNames); + Slog.d(TAG, observer.getName() + " added the following " + + "packages to monitor " + packageNames); oldObserver.updatePackages(packages); } } @@ -204,9 +248,97 @@ public class PackageWatchdog { // Always reschedule because we may need to expire packages // earlier than we are already scheduled for rescheduleCleanup(); + updateHealthChecks(); saveToFileAsync(); } + private void requestCheck(String packageName) { + try { + Slog.d(TAG, "Requesting explicit health check for " + packageName); + mHealthCheckController.request(packageName); + } catch (RemoteException e) { + Slog.wtf(TAG, "Failed to request explicit health check for " + packageName, e); + } + } + + private void cancelCheck(String packageName) { + try { + Slog.d(TAG, "Cancelling explicit health check for " + packageName); + mHealthCheckController.cancel(packageName); + } catch (RemoteException e) { + Slog.wtf(TAG, "Failed to cancel explicit health check for " + packageName, e); + } + } + + private void actOnDifference(Collection<String> collection1, Collection<String> collection2, + Consumer<String> action) { + Iterator<String> iterator = collection1.iterator(); + while (iterator.hasNext()) { + String packageName = iterator.next(); + if (!collection2.contains(packageName)) { + action.accept(packageName); + } + } + } + + private void updateChecksInner(List<String> supportedPackages, + List<String> previousRequestedPackages) { + boolean shouldUpdateFile = false; + + synchronized (mLock) { + Slog.i(TAG, "Updating explicit health checks. Supported packages: " + supportedPackages + + ". Requested packages: " + previousRequestedPackages); + Set<String> newRequestedPackages = new ArraySet<>(); + Iterator<ObserverInternal> oit = mAllObservers.values().iterator(); + while (oit.hasNext()) { + ObserverInternal observer = oit.next(); + Iterator<MonitoredPackage> pit = + observer.mPackages.values().iterator(); + while (pit.hasNext()) { + MonitoredPackage monitoredPackage = pit.next(); + String packageName = monitoredPackage.mName; + if (!monitoredPackage.mHasPassedHealthCheck) { + if (supportedPackages.contains(packageName)) { + newRequestedPackages.add(packageName); + } else { + shouldUpdateFile = true; + monitoredPackage.mHasPassedHealthCheck = true; + } + } + } + } + // TODO: Support ending the binding if newRequestedPackages is empty. + // Will have to re-bind when we #startObservingHealth. + + // Cancel packages no longer requested + actOnDifference(previousRequestedPackages, newRequestedPackages, p -> cancelCheck(p)); + // Request packages not yet requested + actOnDifference(newRequestedPackages, previousRequestedPackages, p -> requestCheck(p)); + } + + if (shouldUpdateFile) { + saveToFileAsync(); + } + } + + private void updateHealthChecks() { + mShortTaskHandler.post(() -> { + try { + Slog.i(TAG, "Updating explicit health checks for all available packages"); + mHealthCheckController.getSupportedPackages(supported -> { + try { + mHealthCheckController.getRequestedPackages( + requested -> updateChecksInner(supported, requested)); + } catch (RemoteException e) { + Slog.e(TAG, "Failed to get requested health check packages", e); + } + }); + } catch (RemoteException e) { + Slog.e(TAG, "Failed to get supported health check package", e); + } + }); + } + /** * Unregisters {@code observer} from listening to package failure. * Additionally, this stops observing any packages that may have previously been observed @@ -250,7 +382,7 @@ public class PackageWatchdog { * <p>This method could be called frequently if there is a severe problem on the device. */ public void onPackageFailure(List<VersionedPackage> packages) { - mWorkerHandler.post(() -> { + mLongTaskHandler.post(() -> { synchronized (mLock) { if (mAllObservers.isEmpty()) { return; @@ -286,49 +418,32 @@ public class PackageWatchdog { }); } - /** - * Updates the observers monitoring {@code packageName} that explicit health check has passed. - * - * <p> This update is strictly for registered observers at the time of the call - * Observers that register after this signal will have no knowledge of prior signals and will - * effectively behave as if the explicit health check hasn't passed for {@code packageName}. - * - * <p> {@code packageName} can still be considered failed if reported by - * {@link #onPackageFailure} before the package expires. - * - * <p> Triggered by components outside the system server when they are fully functional after an - * update. - */ - public void onExplicitHealthCheckPassed(String packageName) { - Slog.i(TAG, "Health check passed for package: " + packageName); - boolean shouldUpdateFile = false; - synchronized (mLock) { - for (int observerIdx = 0; observerIdx < mAllObservers.size(); observerIdx++) { - ObserverInternal observer = mAllObservers.valueAt(observerIdx); - MonitoredPackage monitoredPackage = observer.mPackages.get(packageName); - if (monitoredPackage != null && !monitoredPackage.mHasPassedHealthCheck) { - monitoredPackage.mHasPassedHealthCheck = true; - shouldUpdateFile = true; - } - } - } - if (shouldUpdateFile) { - saveToFileAsync(); - } - } - // TODO(b/120598832): Optimize write? Maybe only write a separate smaller file? // This currently adds about 7ms extra to shutdown thread /** Writes the package information to file during shutdown. */ public void writeNow() { if (!mAllObservers.isEmpty()) { - mWorkerHandler.removeCallbacks(this::saveToFile); + mLongTaskHandler.removeCallbacks(this::saveToFile); pruneObservers(SystemClock.uptimeMillis() - mUptimeAtLastRescheduleMs); saveToFile(); Slog.i(TAG, "Last write to update package durations"); } } + // TODO(b/120598832): Set depending on DeviceConfig flag + /** + * Enables or disables explicit health checks. + * <p> If explicit health checks are enabled, the health check service is started. + * <p> If explicit health checks are disabled, pending explicit health check requests are + * passed and the health check service is stopped. + */ + public void setExplicitHealthCheckEnabled(boolean enabled) { + synchronized (mLock) { + mIsHealthCheckEnabled = enabled; + mHealthCheckController.setEnabled(enabled); + } + } + /** Possible severity values of the user impact of a {@link PackageHealthObserver#execute}. */ @Retention(SOURCE) @IntDef(value = {PackageHealthObserverImpact.USER_IMPACT_NONE, @@ -371,6 +486,37 @@ public class PackageWatchdog { String getName(); } + /** + * Updates the observers monitoring {@code packageName} that explicit health check has passed. + * + * <p> This update is strictly for registered observers at the time of the call + * Observers that register after this signal will have no knowledge of prior signals and will + * effectively behave as if the explicit health check hasn't passed for {@code packageName}. + * + * <p> {@code packageName} can still be considered failed if reported by + * {@link #onPackageFailure} before the package expires. + * + * <p> Triggered by components outside the system server when they are fully functional after an + * update. + */ + private void onHealthCheckPassed(String packageName) { + Slog.i(TAG, "Health check passed for package: " + packageName); + boolean shouldUpdateFile = false; + synchronized (mLock) { + for (int observerIdx = 0; observerIdx < mAllObservers.size(); observerIdx++) { + ObserverInternal observer = mAllObservers.valueAt(observerIdx); + MonitoredPackage monitoredPackage = observer.mPackages.get(packageName); + if (monitoredPackage != null && !monitoredPackage.mHasPassedHealthCheck) { + monitoredPackage.mHasPassedHealthCheck = true; + shouldUpdateFile = true; + } + } + } + if (shouldUpdateFile) { + saveToFileAsync(); + } + } + /** Reschedules handler to prune expired packages from observers. */ private void rescheduleCleanup() { synchronized (mLock) { @@ -393,8 +539,8 @@ public class PackageWatchdog { || nextDurationToScheduleMs < remainingDurationMs) { // First schedule or an earlier reschedule pruneObservers(elapsedDurationMs); - mTimerHandler.removeCallbacks(mPackageCleanup); - mTimerHandler.postDelayed(mPackageCleanup, nextDurationToScheduleMs); + mShortTaskHandler.removeCallbacks(mPackageCleanup); + mShortTaskHandler.postDelayed(mPackageCleanup, nextDurationToScheduleMs); mDurationAtLastReschedule = nextDurationToScheduleMs; mUptimeAtLastRescheduleMs = uptimeMs; } @@ -437,7 +583,7 @@ public class PackageWatchdog { List<MonitoredPackage> failedPackages = observer.updateMonitoringDurations(elapsedMs); if (!failedPackages.isEmpty()) { - onExplicitHealthCheckFailed(observer, failedPackages); + onHealthCheckFailed(observer, failedPackages); } if (observer.mPackages.isEmpty()) { Slog.i(TAG, "Discarding observer " + observer.mName + ". All packages expired"); @@ -445,12 +591,13 @@ public class PackageWatchdog { } } } + updateHealthChecks(); saveToFileAsync(); } - private void onExplicitHealthCheckFailed(ObserverInternal observer, + private void onHealthCheckFailed(ObserverInternal observer, List<MonitoredPackage> failedPackages) { - mWorkerHandler.post(() -> { + mLongTaskHandler.post(() -> { synchronized (mLock) { PackageHealthObserver registeredObserver = observer.mRegisteredObserver; if (registeredObserver != null) { @@ -458,6 +605,7 @@ public class PackageWatchdog { for (int i = 0; i < failedPackages.size(); i++) { String packageName = failedPackages.get(i).mName; long versionCode = 0; + Slog.i(TAG, "Explicit health check failed for package " + packageName); try { versionCode = pm.getPackageInfo( packageName, 0 /* flags */).getLongVersionCode(); @@ -498,7 +646,7 @@ public class PackageWatchdog { } catch (FileNotFoundException e) { // Nothing to monitor } catch (IOException | NumberFormatException | XmlPullParserException e) { - Log.wtf(TAG, "Unable to read monitored packages, deleting file", e); + Slog.wtf(TAG, "Unable to read monitored packages, deleting file", e); mPolicyFile.delete(); } finally { IoUtils.closeQuietly(infile); @@ -542,8 +690,8 @@ public class PackageWatchdog { } private void saveToFileAsync() { - mWorkerHandler.removeCallbacks(this::saveToFile); - mWorkerHandler.post(this::saveToFile); + mLongTaskHandler.removeCallbacks(this::saveToFile); + mLongTaskHandler.post(this::saveToFile); } /** diff --git a/services/core/java/com/android/server/rollback/RollbackPackageHealthObserver.java b/services/core/java/com/android/server/rollback/RollbackPackageHealthObserver.java index 9348806c0e59..d8f07feb1ddb 100644 --- a/services/core/java/com/android/server/rollback/RollbackPackageHealthObserver.java +++ b/services/core/java/com/android/server/rollback/RollbackPackageHealthObserver.java @@ -166,8 +166,7 @@ public final class RollbackPackageHealthObserver implements PackageHealthObserve * This may cause {@code packages} to be rolled back if they crash too freqeuntly. */ public void startObservingHealth(List<String> packages, long durationMs) { - PackageWatchdog.getInstance(mContext).startObservingHealth(this, packages, durationMs, - false /* withExplicitHealthCheck */); + PackageWatchdog.getInstance(mContext).startObservingHealth(this, packages, durationMs); } /** Verifies the rollback state after a reboot. */ diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java index d0c261279c9d..28af7cee8f38 100644 --- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java +++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java @@ -19,11 +19,16 @@ package com.android.server; import static com.android.server.PackageWatchdog.TRIGGER_FAILURE_COUNT; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import android.content.Context; import android.content.pm.VersionedPackage; +import android.os.Handler; +import android.os.RemoteException; import android.os.test.TestLooper; +import android.util.AtomicFile; import androidx.test.InstrumentationRegistry; @@ -36,11 +41,14 @@ import org.junit.Test; import java.io.File; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; -// TODO(zezeozue): Write test without using PackageWatchdog#getPackages. Just rely on +// TODO: Write test without using PackageWatchdog#getPackages. Just rely on // behavior of observers receiving crash notifications or not to determine if it's registered +// TODO: Use Truth in tests. /** * Test PackageWatchdog. */ @@ -77,12 +85,11 @@ public class PackageWatchdogTest { TestObserver observer3 = new TestObserver(OBSERVER_NAME_3); // Start observing for observer1 which will be unregistered - watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION, false); + watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); // Start observing for observer2 which will expire - watchdog.startObservingHealth(observer2, Arrays.asList(APP_A, APP_B), SHORT_DURATION, - false); + watchdog.startObservingHealth(observer2, Arrays.asList(APP_A, APP_B), SHORT_DURATION); // Start observing for observer3 which will have expiry duration reduced - watchdog.startObservingHealth(observer3, Arrays.asList(APP_A), LONG_DURATION, false); + watchdog.startObservingHealth(observer3, Arrays.asList(APP_A), LONG_DURATION); // Verify packages observed at start // 1 @@ -145,9 +152,8 @@ public class PackageWatchdogTest { TestObserver observer1 = new TestObserver(OBSERVER_NAME_1); TestObserver observer2 = new TestObserver(OBSERVER_NAME_2); - watchdog1.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION, false); - watchdog1.startObservingHealth(observer2, Arrays.asList(APP_A, APP_B), SHORT_DURATION, - false); + watchdog1.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); + watchdog1.startObservingHealth(observer2, Arrays.asList(APP_A, APP_B), SHORT_DURATION); // Verify 2 observers are registered and saved internally // 1 @@ -193,8 +199,8 @@ public class PackageWatchdogTest { TestObserver observer1 = new TestObserver(OBSERVER_NAME_1); TestObserver observer2 = new TestObserver(OBSERVER_NAME_2); - watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION, false); - watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION, false); + watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION); + watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); // Then fail APP_A below the threshold for (int i = 0; i < TRIGGER_FAILURE_COUNT - 1; i++) { @@ -220,8 +226,8 @@ public class PackageWatchdogTest { TestObserver observer2 = new TestObserver(OBSERVER_NAME_2); - watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION, false); - watchdog.startObservingHealth(observer1, Arrays.asList(APP_B), SHORT_DURATION, false); + watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION); + watchdog.startObservingHealth(observer1, Arrays.asList(APP_B), SHORT_DURATION); // Then fail APP_C (not observed) above the threshold for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { @@ -255,7 +261,7 @@ public class PackageWatchdogTest { } }; - watchdog.startObservingHealth(observer, Arrays.asList(APP_A), SHORT_DURATION, false); + watchdog.startObservingHealth(observer, Arrays.asList(APP_A), SHORT_DURATION); // Then fail APP_A (different version) above the threshold for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { @@ -288,13 +294,13 @@ public class PackageWatchdogTest { // Start observing for all impact observers watchdog.startObservingHealth(observerNone, Arrays.asList(APP_A, APP_B, APP_C, APP_D), - SHORT_DURATION, false); + SHORT_DURATION); watchdog.startObservingHealth(observerHigh, Arrays.asList(APP_A, APP_B, APP_C), - SHORT_DURATION, false); + SHORT_DURATION); watchdog.startObservingHealth(observerMid, Arrays.asList(APP_A, APP_B), - SHORT_DURATION, false); + SHORT_DURATION); watchdog.startObservingHealth(observerLow, Arrays.asList(APP_A), - SHORT_DURATION, false); + SHORT_DURATION); // Then fail all apps above the threshold for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { @@ -346,8 +352,8 @@ public class PackageWatchdogTest { PackageHealthObserverImpact.USER_IMPACT_MEDIUM); // Start observing for observerFirst and observerSecond with failure handling - watchdog.startObservingHealth(observerFirst, Arrays.asList(APP_A), LONG_DURATION, false); - watchdog.startObservingHealth(observerSecond, Arrays.asList(APP_A), LONG_DURATION, false); + watchdog.startObservingHealth(observerFirst, Arrays.asList(APP_A), LONG_DURATION); + watchdog.startObservingHealth(observerSecond, Arrays.asList(APP_A), LONG_DURATION); // Then fail APP_A above the threshold for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { @@ -424,8 +430,8 @@ public class PackageWatchdogTest { PackageHealthObserverImpact.USER_IMPACT_HIGH); // Start observing for observer1 and observer2 with failure handling - watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION, false); - watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION, false); + watchdog.startObservingHealth(observer2, Arrays.asList(APP_A), SHORT_DURATION); + watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); // Then fail APP_A above the threshold for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { @@ -442,11 +448,12 @@ public class PackageWatchdogTest { } /** - * Test explicit health check status determines package failure or success on expiry + * Test package passing explicit health checks does not fail and vice versa. */ @Test - public void testPackageFailureExplicitHealthCheck() throws Exception { - PackageWatchdog watchdog = createWatchdog(); + public void testExplicitHealthChecks() throws Exception { + TestController controller = new TestController(); + PackageWatchdog watchdog = createWatchdog(controller, true /* withPackagesReady */); TestObserver observer1 = new TestObserver(OBSERVER_NAME_1, PackageHealthObserverImpact.USER_IMPACT_HIGH); TestObserver observer2 = new TestObserver(OBSERVER_NAME_2, @@ -457,21 +464,36 @@ public class PackageWatchdogTest { // Start observing with explicit health checks for APP_A and APP_B respectively // with observer1 and observer2 - watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION, true); - watchdog.startObservingHealth(observer2, Arrays.asList(APP_B), SHORT_DURATION, true); - // Explicit health check passed for APP_A (observer1 is aware) - watchdog.onExplicitHealthCheckPassed(APP_A); - // Start observing APP_A with explicit health checks for observer3. + controller.setSupportedPackages(Arrays.asList(APP_A, APP_B)); + watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); + watchdog.startObservingHealth(observer2, Arrays.asList(APP_B), SHORT_DURATION); + + // Run handler so requests are dispatched to the controller + mTestLooper.dispatchAll(); + + // Verify we requested health checks for APP_A and APP_B + List<String> requestedPackages = controller.getRequestedPackages(); + assertEquals(2, requestedPackages.size()); + assertEquals(APP_A, requestedPackages.get(0)); + assertEquals(APP_B, requestedPackages.get(1)); + + // Then health check passed for APP_A (observer1 is aware) + controller.setPackagePassed(APP_A); + + // Then start observing APP_A with explicit health checks for observer3. // Observer3 didn't exist when we got the explicit health check above, so // it starts out with a non-passing explicit health check and has to wait for a pass // otherwise it would be notified of APP_A failure on expiry - watchdog.startObservingHealth(observer3, Arrays.asList(APP_A), SHORT_DURATION, true); + watchdog.startObservingHealth(observer3, Arrays.asList(APP_A), SHORT_DURATION); // Then expire observers Thread.sleep(SHORT_DURATION); // Run handler so package failures are dispatched to observers mTestLooper.dispatchAll(); + // Verify we cancelled all requests on expiry + assertEquals(0, controller.getRequestedPackages().size()); + // Verify observer1 is not notified assertEquals(0, observer1.mFailedPackages.size()); @@ -484,9 +506,96 @@ public class PackageWatchdogTest { assertEquals(APP_A, observer3.mFailedPackages.get(0)); } + /** + * Test explicit health check state can be disabled and enabled correctly. + */ + @Test + public void testExplicitHealthCheckStateChanges() throws Exception { + TestController controller = new TestController(); + PackageWatchdog watchdog = createWatchdog(controller, true /* withPackagesReady */); + TestObserver observer = new TestObserver(OBSERVER_NAME_1, + PackageHealthObserverImpact.USER_IMPACT_MEDIUM); + + // Start observing with explicit health checks for APP_A and APP_B + controller.setSupportedPackages(Arrays.asList(APP_A, APP_B, APP_C)); + watchdog.startObservingHealth(observer, Arrays.asList(APP_A), SHORT_DURATION); + watchdog.startObservingHealth(observer, Arrays.asList(APP_B), LONG_DURATION); + + // Run handler so requests are dispatched to the controller + mTestLooper.dispatchAll(); + + // Verify we requested health checks for APP_A and APP_B + List<String> requestedPackages = controller.getRequestedPackages(); + assertEquals(2, requestedPackages.size()); + assertEquals(APP_A, requestedPackages.get(0)); + assertEquals(APP_B, requestedPackages.get(1)); + + // Disable explicit health checks (marks APP_A and APP_B as passed) + watchdog.setExplicitHealthCheckEnabled(false); + + // Run handler so requests/cancellations are dispatched to the controller + mTestLooper.dispatchAll(); + + // Verify all checks are cancelled + assertEquals(0, controller.getRequestedPackages().size()); + + // Then expire APP_A + Thread.sleep(SHORT_DURATION); + mTestLooper.dispatchAll(); + + // Verify APP_A is not failed (APP_B) is not expired yet + assertEquals(0, observer.mFailedPackages.size()); + + // Re-enable explicit health checks + watchdog.setExplicitHealthCheckEnabled(true); + + // Run handler so requests/cancellations are dispatched to the controller + mTestLooper.dispatchAll(); + + // Verify no requests are made cos APP_A is expired and APP_B was marked as passed + assertEquals(0, controller.getRequestedPackages().size()); + + // Then set new supported packages + controller.setSupportedPackages(Arrays.asList(APP_C)); + // Start observing APP_A and APP_C; only APP_C has support for explicit health checks + watchdog.startObservingHealth(observer, Arrays.asList(APP_A, APP_C), SHORT_DURATION); + + // Run handler so requests/cancellations are dispatched to the controller + mTestLooper.dispatchAll(); + + // Verify requests are only made for APP_C + requestedPackages = controller.getRequestedPackages(); + assertEquals(1, requestedPackages.size()); + assertEquals(APP_C, requestedPackages.get(0)); + + // Then expire APP_A and APP_C + Thread.sleep(SHORT_DURATION); + mTestLooper.dispatchAll(); + + // Verify only APP_C is failed because explicit health checks was not supported for APP_A + assertEquals(1, observer.mFailedPackages.size()); + assertEquals(APP_C, observer.mFailedPackages.get(0)); + } + private PackageWatchdog createWatchdog() { - return new PackageWatchdog(InstrumentationRegistry.getContext(), - mTestLooper.getLooper()); + return createWatchdog(new TestController(), true /* withPackagesReady */); + } + + private PackageWatchdog createWatchdog(TestController controller, boolean withPackagesReady) { + Context context = InstrumentationRegistry.getContext(); + AtomicFile policyFile = + new AtomicFile(new File(context.getFilesDir(), "package-watchdog.xml")); + Handler handler = new Handler(mTestLooper.getLooper()); + PackageWatchdog watchdog = + new PackageWatchdog(context, policyFile, handler, handler, controller); + // Verify controller is not automatically started + assertFalse(controller.mIsEnabled); + if (withPackagesReady) { + watchdog.onPackagesReady(); + // Verify controller by default is started when packages are ready + assertTrue(controller.mIsEnabled); + } + return watchdog; } private static class TestObserver implements PackageHealthObserver { @@ -517,4 +626,69 @@ public class PackageWatchdogTest { return mName; } } + + private static class TestController extends ExplicitHealthCheckController { + TestController() { + super(null /* controller */); + } + + private boolean mIsEnabled; + private List<String> mSupportedPackages = new ArrayList<>(); + private List<String> mRequestedPackages = new ArrayList<>(); + private Runnable mStateChangedRunnable; + private Consumer<String> mPassedConsumer; + + @Override + public void request(String packageName) throws RemoteException { + if (!mRequestedPackages.contains(packageName)) { + mRequestedPackages.add(packageName); + } + } + + @Override + public void cancel(String packageName) throws RemoteException { + mRequestedPackages.remove(packageName); + } + + @Override + public void getSupportedPackages(Consumer<List<String>> consumer) throws RemoteException { + consumer.accept(mIsEnabled ? mSupportedPackages : Collections.emptyList()); + } + + @Override + public void getRequestedPackages(Consumer<List<String>> consumer) throws RemoteException { + // Pass copy to prevent ConcurrentModificationException during test + consumer.accept( + mIsEnabled ? new ArrayList<>(mRequestedPackages) : Collections.emptyList()); + } + + @Override + public void setEnabled(boolean enabled) { + mIsEnabled = enabled; + mStateChangedRunnable.run(); + } + + @Override + public void setCallbacks(Runnable stateChangedRunnable, Consumer<String> passedConsumer) { + mStateChangedRunnable = stateChangedRunnable; + mPassedConsumer = passedConsumer; + } + + public void setSupportedPackages(List<String> packages) { + mSupportedPackages.clear(); + mSupportedPackages.addAll(packages); + } + + public void setPackagePassed(String packageName) { + mPassedConsumer.accept(packageName); + } + + public List<String> getRequestedPackages() { + if (mIsEnabled) { + return mRequestedPackages; + } else { + return Collections.emptyList(); + } + } + } } |