Merge "Refactor PackageWatchdog explicit health checks" into qt-dev (76194357) · Commits · e / os / android_frameworks_base

services/core/java/com/android/server/ExplicitHealthCheckController.java

+211 −105

Original line number	Diff line number	Diff line
		@@ -41,10 +41,14 @@ import android.util.Slog;
		import com.android.internal.annotations.GuardedBy;
		import com.android.internal.util.Preconditions;

		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;

		// TODO(b/120598832): Add tests
		/**
		* Controls the connections with {@link ExplicitHealthCheckService}.
		*/
		@@ -53,18 +57,30 @@ class ExplicitHealthCheckController {
		private final Object mLock = new Object();
		private final Context mContext;

		// 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.
		// To prevent deadlocks between the controller and watchdog threads, we have
		// a lock invariant to ALWAYS acquire the PackageWatchdog#mLock before #mLock in this class.
		// It's easier to just NOT hold #mLock when calling into watchdog code on this consumer.
		@GuardedBy("mLock") @Nullable private Consumer<String> mPassedConsumer;
		// Called everytime after a successful #syncRequest call, so the watchdog can receive packages
		// supporting health checks and update its internal state. In practice, should never be null
		// after it has been #setEnabled.
		// To prevent deadlocks between the controller and watchdog threads, we have
		// a lock invariant to ALWAYS acquire the PackageWatchdog#mLock before #mLock in this class.
		// It's easier to just NOT hold #mLock when calling into watchdog code on this consumer.
		@GuardedBy("mLock") @Nullable private Consumer<List<String>> mSupportedConsumer;
		// Called everytime we need to notify the watchdog to sync requests between itself and the
		// health check service. In practice, should never be null after it has been #setEnabled.
		// To prevent deadlocks between the controller and watchdog threads, we have
		// a lock invariant to ALWAYS acquire the PackageWatchdog#mLock before #mLock in this class.
		// It's easier to just NOT hold #mLock when calling into watchdog code on this runnable.
		@GuardedBy("mLock") @Nullable private Runnable mNotifySyncRunnable;
		// Actual binder object to the explicit health check service.
		@GuardedBy("mLock") @Nullable private IExplicitHealthCheckService mRemoteService;
		// 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
		// Connection to the explicit health check service, necessary to unbind.
		// We should only try to bind if mConnection is null, non-null indicates we
		// are connected or at least connecting.
		@GuardedBy("mLock") @Nullable private ServiceConnection mConnection;
		// Bind state of the explicit health check service.
		@GuardedBy("mLock") private boolean mEnabled;
		@@ -73,23 +89,117 @@ class ExplicitHealthCheckController {
		mContext = context;
		}

		/** Enables or disables explicit health checks. */
		public void setEnabled(boolean enabled) {
		synchronized (mLock) {
		Slog.i(TAG, "Explicit health checks " + (enabled ? "enabled." : "disabled."));
		mEnabled = enabled;
		}
		}

		/**
		* Sets callbacks to listen to important events from the controller.
		*
		* <p> Should be called once at initialization before any other calls to the controller to
		* ensure a happens-before relationship of the set parameters and visibility on other threads.
		*/
		public void setCallbacks(Consumer<String> passedConsumer,
		Consumer<List<String>> supportedConsumer, Runnable notifySyncRunnable) {
		synchronized (mLock) {
		if (mPassedConsumer != null \|\| mSupportedConsumer != null
		\|\| mNotifySyncRunnable != null) {
		Slog.wtf(TAG, "Resetting health check controller callbacks");
		}

		mPassedConsumer = Preconditions.checkNotNull(passedConsumer);
		mSupportedConsumer = Preconditions.checkNotNull(supportedConsumer);
		mNotifySyncRunnable = Preconditions.checkNotNull(notifySyncRunnable);
		}
		}

		/**
		* Calls the health check service to request or cancel packages based on
		* {@code newRequestedPackages}.
		*
		* <p> Supported packages in {@code newRequestedPackages} that have not been previously
		* requested will be requested while supported packages not in {@code newRequestedPackages}
		* but were previously requested will be cancelled.
		*
		* <p> This handles binding and unbinding to the health check service as required.
		*
		* <p> Note, calling this may modify {@code newRequestedPackages}.
		*
		* <p> Note, this method is not thread safe, all calls should be serialized.
		*/
		public void syncRequests(Set<String> newRequestedPackages) {
		boolean enabled;
		synchronized (mLock) {
		enabled = mEnabled;
		}

		if (!enabled) {
		Slog.i(TAG, "Health checks disabled, no supported packages");
		// Call outside lock
		mSupportedConsumer.accept(Collections.emptyList());
		return;
		}

		getSupportedPackages(supportedPackages -> {
		// Notify the watchdog without lock held
		mSupportedConsumer.accept(supportedPackages);
		getRequestedPackages(previousRequestedPackages -> {
		synchronized (mLock) {
		// Hold lock so requests and cancellations are sent atomically.
		// It is important we don't mix requests from multiple threads.

		// Note, this may modify newRequestedPackages
		newRequestedPackages.retainAll(supportedPackages);

		// Cancel packages no longer requested
		actOnDifference(previousRequestedPackages,
		newRequestedPackages, p -> cancel(p));
		// Request packages not yet requested
		actOnDifference(newRequestedPackages,
		previousRequestedPackages, p -> request(p));

		if (newRequestedPackages.isEmpty()) {
		Slog.i(TAG, "No more health check requests, unbinding...");
		unbindService();
		return;
		}
		}
		});
		});
		}

		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);
		}
		}
		}

		/**
		* Requests an explicit health check for {@code packageName}.
		* 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 {
		private void request(String packageName) {
		synchronized (mLock) {
		if (!mEnabled) {
		if (!prepareServiceLocked("request health check for " + packageName)) {
		return;
		}

		enforceServiceReadyLocked();

		Slog.i(TAG, "Requesting health check for package " + packageName);
		try {
		mRemoteService.request(packageName);
		} catch (RemoteException e) {
		Slog.w(TAG, "Failed to request health check for package " + packageName, e);
		}
		}
		}

		@@ -97,46 +207,46 @@ class ExplicitHealthCheckController {
		* Cancels all explicit health checks for {@code packageName}.
		* 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 {
		private void cancel(String packageName) {
		synchronized (mLock) {
		if (!mEnabled) {
		if (!prepareServiceLocked("cancel health check for " + packageName)) {
		return;
		}

		enforceServiceReadyLocked();

		Slog.i(TAG, "Cancelling health check for package " + packageName);
		try {
		mRemoteService.cancel(packageName);
		} catch (RemoteException e) {
		// Do nothing, if the service is down, when it comes up, we will sync requests,
		// if there's some other error, retrying wouldn't fix anyways.
		Slog.w(TAG, "Failed to cancel health check for package " + packageName, e);
		}
		}
		}

		/**
		* Returns the packages that we can request explicit health checks for.
		* The packages will be returned to the {@code consumer}.
		*
		* @throws IllegalStateException if the service is not started
		*/
		public void getSupportedPackages(Consumer<List<String>> consumer) throws RemoteException {
		private void getSupportedPackages(Consumer<List<String>> consumer) {
		synchronized (mLock) {
		if (!mEnabled) {
		consumer.accept(Collections.emptyList());
		if (!prepareServiceLocked("get health check supported packages")) {
		return;
		}

		enforceServiceReadyLocked();

		if (mSupportedPackages == null) {
		Slog.d(TAG, "Getting health check supported packages");
		try {
		mRemoteService.getSupportedPackages(new RemoteCallback(result -> {
		mSupportedPackages = result.getStringArrayList(EXTRA_SUPPORTED_PACKAGES);
		consumer.accept(mSupportedPackages);
		List<String> packages = result.getStringArrayList(EXTRA_SUPPORTED_PACKAGES);
		Slog.i(TAG, "Explicit health check supported packages " + packages);
		consumer.accept(packages);
		}));
		} else {
		Slog.d(TAG, "Getting cached health check supported packages");
		consumer.accept(mSupportedPackages);
		} catch (RemoteException e) {
		// Request failed, treat as if all observed packages are supported, if any packages
		// expire during this period, we may incorrectly treat it as failing health checks
		// even if we don't support health checks for the package.
		Slog.w(TAG, "Failed to get health check supported packages", e);
		}
		}
		}
		@@ -144,57 +254,42 @@ class ExplicitHealthCheckController {
		/**
		* Returns the packages for which health checks are currently in progress.
		* The packages will be returned to the {@code consumer}.
		*
		* @throws IllegalStateException if the service is not started
		*/
		public void getRequestedPackages(Consumer<List<String>> consumer) throws RemoteException {
		private void getRequestedPackages(Consumer<List<String>> consumer) {
		synchronized (mLock) {
		if (!mEnabled) {
		consumer.accept(Collections.emptyList());
		if (!prepareServiceLocked("get health check requested packages")) {
		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();
		try {
		mRemoteService.getRequestedPackages(new RemoteCallback(result -> {
		List<String> packages = result.getStringArrayList(EXTRA_REQUESTED_PACKAGES);
		Slog.i(TAG, "Explicit health check requested packages " + packages);
		consumer.accept(packages);
		}));
		} catch (RemoteException e) {
		// Request failed, treat as if we haven't requested any packages, if any packages
		// were actually requested, they will not be cancelled now. May be cancelled later
		Slog.w(TAG, "Failed to get health check requested packages", e);
		}
		}
		}

		/**
		* Sets callbacks to listen to important events from the controller.
		* Should be called at initialization.
		* Binds to the explicit health check service if the controller is enabled and
		* not already bound.
		*/
		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) {
		if (!mEnabled \|\| mConnection != null \|\| mRemoteService != null) {
		if (!mEnabled) {
		Slog.i(TAG, "Not binding to service, service disabled");
		} else if (mRemoteService != null) {
		Slog.i(TAG, "Not binding to service, service already connected");
		} else {
		Slog.i(TAG, "Not binding to service, service already connecting");
		}
		return;
		}
		ComponentName component = getServiceComponentNameLocked();
		@@ -205,14 +300,11 @@ class ExplicitHealthCheckController {

		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) {
		initState(service);
		Slog.i(TAG, "Explicit health check service is connected " + name);
		initState(service);
		}

		@Override
		@@ -221,20 +313,19 @@ class ExplicitHealthCheckController {
		// 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);
		synchronized (mLock) {
		mRemoteService = null;
		}
		}

		@Override
		public void onBindingDied(ComponentName name) {
		// Application hosting service probably got updated
		// Need to re-bind.
		synchronized (mLock) {
		if (mEnabled) {
		Slog.i(TAG, "Explicit health check service binding is dead. Rebind: " + name);
		unbindService();
		bindService();
		}
		}
		Slog.i(TAG, "Explicit health check service binding is dead " + name);
		}

		@Override
		public void onNullBinding(ComponentName name) {
		@@ -243,9 +334,9 @@ class ExplicitHealthCheckController {
		}
		};

		Slog.i(TAG, "Binding to explicit health service");
		mContext.bindServiceAsUser(intent, mConnection, Context.BIND_AUTO_CREATE,
		UserHandle.of(UserHandle.USER_SYSTEM));
		mContext.bindServiceAsUser(intent, mConnection,
		Context.BIND_AUTO_CREATE, UserHandle.of(UserHandle.USER_SYSTEM));
		Slog.i(TAG, "Explicit health check service is bound");
		}
		}

		@@ -253,10 +344,11 @@ class ExplicitHealthCheckController {
		private void unbindService() {
		synchronized (mLock) {
		if (mRemoteService != null) {
		Slog.i(TAG, "Unbinding from explicit health service");
		mContext.unbindService(mConnection);
		mRemoteService = null;
		mConnection = null;
		}
		Slog.i(TAG, "Explicit health check service is unbound");
		}
		}

		@@ -301,40 +393,54 @@ class ExplicitHealthCheckController {

		private void initState(IBinder service) {
		synchronized (mLock) {
		mSupportedPackages = null;
		if (!mEnabled) {
		Slog.w(TAG, "Attempting to connect disabled service?? Unbinding...");
		// Very unlikely, but we disabled the service after binding but before we connected
		unbindService();
		return;
		}
		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
		Slog.wtf(TAG, "Health check passed for package " + packageName
		+ "but no consumer registered.");
		} else {
		// Call without lock held
		mPassedConsumer.accept(packageName);
		}
		}
		} else {
		Slog.w(TAG, "Empty package passed explicit health check?");
		Slog.wtf(TAG, "Empty package passed explicit health check?");
		}
		}));
		if (mOnConnected == null) {
		Slog.w(TAG, "Health check service connected but no runnable registered.");
		} else {
		mOnConnected.run();
		}
		Slog.i(TAG, "Service initialized, syncing requests");
		} catch (RemoteException e) {
		Slog.wtf(TAG, "Could not setCallback on explicit health check service");
		}
		}
		// Calling outside lock
		mNotifySyncRunnable.run();
		}

		/**
		* Prepares the health check service to receive requests.
		*
		* @return {@code true} if it is ready and we can proceed with a request,
		* {@code false} otherwise. If it is not ready, and the service is enabled,
		* we will bind and the request should be automatically attempted later.
		*/
		@GuardedBy("mLock")
		private void enforceServiceReadyLocked() {
		if (mRemoteService == null) {
		// TODO: Try to bind to service
		throw new IllegalStateException("Explicit health check service not ready");
		private boolean prepareServiceLocked(String action) {
		if (mRemoteService != null && mEnabled) {
		return true;
		}
		Slog.i(TAG, "Service not ready to " + action
		+ (mEnabled ? ". Binding..." : ". Disabled"));
		if (mEnabled) {
		bindService();
		}
		return false;
		}
		}

services/core/java/com/android/server/PackageWatchdog.java

+104 −123

File changed.

Preview size limit exceeded, changes collapsed.

tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java

+19 −28

Original line number	Diff line number	Diff line
		@@ -26,7 +26,6 @@ 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;

		@@ -43,6 +42,7 @@ import java.util.ArrayList;
		import java.util.Arrays;
		import java.util.Collections;
		import java.util.List;
		import java.util.Set;
		import java.util.concurrent.TimeUnit;
		import java.util.function.Consumer;

		@@ -635,43 +635,34 @@ public class PackageWatchdogTest {
		private boolean mIsEnabled;
		private List<String> mSupportedPackages = new ArrayList<>();
		private List<String> mRequestedPackages = new ArrayList<>();
		private Runnable mStateChangedRunnable;
		private Consumer<String> mPassedConsumer;
		private Consumer<List<String>> mSupportedConsumer;
		private Runnable mNotifySyncRunnable;

		@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);
		public void setEnabled(boolean enabled) {
		mIsEnabled = enabled;
		if (!mIsEnabled) {
		mSupportedPackages.clear();
		}

		@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());
		public void setCallbacks(Consumer<String> passedConsumer,
		Consumer<List<String>> supportedConsumer, Runnable notifySyncRunnable) {
		mPassedConsumer = passedConsumer;
		mSupportedConsumer = supportedConsumer;
		mNotifySyncRunnable = notifySyncRunnable;
		}

		@Override
		public void setEnabled(boolean enabled) {
		mIsEnabled = enabled;
		mStateChangedRunnable.run();
		public void syncRequests(Set<String> packages) {
		mRequestedPackages.clear();
		if (mIsEnabled) {
		packages.retainAll(mSupportedPackages);
		mRequestedPackages.addAll(packages);
		}

		@Override
		public void setCallbacks(Runnable stateChangedRunnable, Consumer<String> passedConsumer) {
		mStateChangedRunnable = stateChangedRunnable;
		mPassedConsumer = passedConsumer;
		mSupportedConsumer.accept(mSupportedPackages);
		}

		public void setSupportedPackages(List<String> packages) {