Refactor PackageWatchdog explicit health checks (cb148b2c) · Commits · e / os / android_frameworks_base

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

+211 −105

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

			import java.util.Collection;
	import java.util.Collections;		import java.util.Collections;
			import java.util.Iterator;
	import java.util.List;		import java.util.List;
			import java.util.Set;
	import java.util.function.Consumer;		import java.util.function.Consumer;

			// TODO(b/120598832): Add tests
	/**		/**
	* Controls the connections with {@link ExplicitHealthCheckService}.		* Controls the connections with {@link ExplicitHealthCheckService}.
	*/		*/
	@@ -53,18 +57,30 @@ class ExplicitHealthCheckController {
	private final Object mLock = new Object();		private final Object mLock = new Object();
	private final Context mContext;		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		// 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.		// 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;		@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.		// Actual binder object to the explicit health check service.
	@GuardedBy("mLock") @Nullable private IExplicitHealthCheckService mRemoteService;		@GuardedBy("mLock") @Nullable private IExplicitHealthCheckService mRemoteService;
	// Cache for packages supporting explicit health checks. This cache should not change while		// Connection to the explicit health check service, necessary to unbind.
	// the health check service is running.		// We should only try to bind if mConnection is null, non-null indicates we
	@GuardedBy("mLock") @Nullable private List<String> mSupportedPackages;		// are connected or at least connecting.
	// Connection to the explicit health check service, necessary to unbind
	@GuardedBy("mLock") @Nullable private ServiceConnection mConnection;		@GuardedBy("mLock") @Nullable private ServiceConnection mConnection;
	// Bind state of the explicit health check service.		// Bind state of the explicit health check service.
	@GuardedBy("mLock") private boolean mEnabled;		@GuardedBy("mLock") private boolean mEnabled;
	@@ -73,23 +89,117 @@ class ExplicitHealthCheckController {
	mContext = context;		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}.		* Requests an explicit health check for {@code packageName}.
	* After this request, the callback registered on {@link #setCallbacks} can receive explicit		* After this request, the callback registered on {@link #setCallbacks} can receive explicit
	* health check passed results.		* 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) {		synchronized (mLock) {
	if (!mEnabled) {		if (!prepareServiceLocked("request health check for " + packageName)) {
	return;		return;
	}		}

	enforceServiceReadyLocked();

	Slog.i(TAG, "Requesting health check for package " + packageName);		Slog.i(TAG, "Requesting health check for package " + packageName);
			try {
	mRemoteService.request(packageName);		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}.		* Cancels all explicit health checks for {@code packageName}.
	* After this request, the callback registered on {@link #setCallbacks} can no longer receive		* After this request, the callback registered on {@link #setCallbacks} can no longer receive
	* explicit health check passed results.		* 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) {		synchronized (mLock) {
	if (!mEnabled) {		if (!prepareServiceLocked("cancel health check for " + packageName)) {
	return;		return;
	}		}

	enforceServiceReadyLocked();

	Slog.i(TAG, "Cancelling health check for package " + packageName);		Slog.i(TAG, "Cancelling health check for package " + packageName);
			try {
	mRemoteService.cancel(packageName);		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.		* Returns the packages that we can request explicit health checks for.
	* The packages will be returned to the {@code consumer}.		* 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) {		synchronized (mLock) {
	if (!mEnabled) {		if (!prepareServiceLocked("get health check supported packages")) {
	consumer.accept(Collections.emptyList());
	return;		return;
	}		}

	enforceServiceReadyLocked();

	if (mSupportedPackages == null) {
	Slog.d(TAG, "Getting health check supported packages");		Slog.d(TAG, "Getting health check supported packages");
			try {
	mRemoteService.getSupportedPackages(new RemoteCallback(result -> {		mRemoteService.getSupportedPackages(new RemoteCallback(result -> {
	mSupportedPackages = result.getStringArrayList(EXTRA_SUPPORTED_PACKAGES);		List<String> packages = result.getStringArrayList(EXTRA_SUPPORTED_PACKAGES);
	consumer.accept(mSupportedPackages);		Slog.i(TAG, "Explicit health check supported packages " + packages);
			consumer.accept(packages);
	}));		}));
	} else {		} catch (RemoteException e) {
	Slog.d(TAG, "Getting cached health check supported packages");		// Request failed, treat as if all observed packages are supported, if any packages
	consumer.accept(mSupportedPackages);		// 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.		* Returns the packages for which health checks are currently in progress.
	* The packages will be returned to the {@code consumer}.		* 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) {		synchronized (mLock) {
	if (!mEnabled) {		if (!prepareServiceLocked("get health check requested packages")) {
	consumer.accept(Collections.emptyList());
	return;		return;
	}		}

	enforceServiceReadyLocked();

	Slog.d(TAG, "Getting health check requested packages");		Slog.d(TAG, "Getting health check requested packages");
	mRemoteService.getRequestedPackages(new RemoteCallback(		try {
	result -> consumer.accept(		mRemoteService.getRequestedPackages(new RemoteCallback(result -> {
	result.getStringArrayList(EXTRA_REQUESTED_PACKAGES))));		List<String> packages = result.getStringArrayList(EXTRA_REQUESTED_PACKAGES);
	}		Slog.i(TAG, "Explicit health check requested packages " + packages);
	}		consumer.accept(packages);
			}));
	/** Enables or disables explicit health checks. */		} catch (RemoteException e) {
	public void setEnabled(boolean enabled) {		// Request failed, treat as if we haven't requested any packages, if any packages
	synchronized (mLock) {		// were actually requested, they will not be cancelled now. May be cancelled later
	if (enabled == mEnabled) {		Slog.w(TAG, "Failed to get health check requested packages", e);
	return;
	}

	Slog.i(TAG, "Setting explicit health checks enabled " + enabled);
	mEnabled = enabled;
	if (enabled) {
	bindService();
	} else {
	unbindService();
	}		}
	}		}
	}		}

	/**		/**
	* Sets callbacks to listen to important events from the controller.		* Binds to the explicit health check service if the controller is enabled and
	* Should be called at initialization.		* 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() {		private void bindService() {
	synchronized (mLock) {		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;		return;
	}		}
	ComponentName component = getServiceComponentNameLocked();		ComponentName component = getServiceComponentNameLocked();
	@@ -205,14 +300,11 @@ class ExplicitHealthCheckController {

	Intent intent = new Intent();		Intent intent = new Intent();
	intent.setComponent(component);		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() {		mConnection = new ServiceConnection() {
	@Override		@Override
	public void onServiceConnected(ComponentName name, IBinder service) {		public void onServiceConnected(ComponentName name, IBinder service) {
	initState(service);
	Slog.i(TAG, "Explicit health check service is connected " + name);		Slog.i(TAG, "Explicit health check service is connected " + name);
			initState(service);
	}		}

	@Override		@Override
	@@ -221,20 +313,19 @@ class ExplicitHealthCheckController {
	// Service crashed or process was killed, #onServiceConnected will be called.		// Service crashed or process was killed, #onServiceConnected will be called.
	// Don't need to re-bind.		// Don't need to re-bind.
	Slog.i(TAG, "Explicit health check service is disconnected " + name);		Slog.i(TAG, "Explicit health check service is disconnected " + name);
			synchronized (mLock) {
			mRemoteService = null;
			}
	}		}

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

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

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

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

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

	private void initState(IBinder service) {		private void initState(IBinder service) {
	synchronized (mLock) {		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);		mRemoteService = IExplicitHealthCheckService.Stub.asInterface(service);
	try {		try {
	mRemoteService.setCallback(new RemoteCallback(result -> {		mRemoteService.setCallback(new RemoteCallback(result -> {
	String packageName = result.getString(EXTRA_HEALTH_CHECK_PASSED_PACKAGE);		String packageName = result.getString(EXTRA_HEALTH_CHECK_PASSED_PACKAGE);
	if (!TextUtils.isEmpty(packageName)) {		if (!TextUtils.isEmpty(packageName)) {
	synchronized (mLock) {
	if (mPassedConsumer == null) {		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.");		+ "but no consumer registered.");
	} else {		} else {
			// Call without lock held
	mPassedConsumer.accept(packageName);		mPassedConsumer.accept(packageName);
	}		}
	}
	} else {		} else {
	Slog.w(TAG, "Empty package passed explicit health check?");		Slog.wtf(TAG, "Empty package passed explicit health check?");
	}		}
	}));		}));
	if (mOnConnected == null) {		Slog.i(TAG, "Service initialized, syncing requests");
	Slog.w(TAG, "Health check service connected but no runnable registered.");
	} else {
	mOnConnected.run();
	}
	} catch (RemoteException e) {		} catch (RemoteException e) {
	Slog.wtf(TAG, "Could not setCallback on explicit health check service");		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")		@GuardedBy("mLock")
	private void enforceServiceReadyLocked() {		private boolean prepareServiceLocked(String action) {
	if (mRemoteService == null) {		if (mRemoteService != null && mEnabled) {
	// TODO: Try to bind to service		return true;
	throw new IllegalStateException("Explicit health check service not ready");		}
			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	Original line	Diff line number	Diff line
	@@ -26,7 +26,6 @@ import static org.junit.Assert.assertTrue;
	import android.content.Context;		import android.content.Context;
	import android.content.pm.VersionedPackage;		import android.content.pm.VersionedPackage;
	import android.os.Handler;		import android.os.Handler;
	import android.os.RemoteException;
	import android.os.test.TestLooper;		import android.os.test.TestLooper;
	import android.util.AtomicFile;		import android.util.AtomicFile;

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

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

	@Override		@Override
	public void request(String packageName) throws RemoteException {		public void setEnabled(boolean enabled) {
	if (!mRequestedPackages.contains(packageName)) {		mIsEnabled = enabled;
	mRequestedPackages.add(packageName);		if (!mIsEnabled) {
	}		mSupportedPackages.clear();
	}

	@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		@Override
	public void getRequestedPackages(Consumer<List<String>> consumer) throws RemoteException {		public void setCallbacks(Consumer<String> passedConsumer,
	// Pass copy to prevent ConcurrentModificationException during test		Consumer<List<String>> supportedConsumer, Runnable notifySyncRunnable) {
	consumer.accept(		mPassedConsumer = passedConsumer;
	mIsEnabled ? new ArrayList<>(mRequestedPackages) : Collections.emptyList());		mSupportedConsumer = supportedConsumer;
			mNotifySyncRunnable = notifySyncRunnable;
	}		}

	@Override		@Override
	public void setEnabled(boolean enabled) {		public void syncRequests(Set<String> packages) {
	mIsEnabled = enabled;		mRequestedPackages.clear();
	mStateChangedRunnable.run();		if (mIsEnabled) {
			packages.retainAll(mSupportedPackages);
			mRequestedPackages.addAll(packages);
	}		}
			mSupportedConsumer.accept(mSupportedPackages);
	@Override
	public void setCallbacks(Runnable stateChangedRunnable, Consumer<String> passedConsumer) {
	mStateChangedRunnable = stateChangedRunnable;
	mPassedConsumer = passedConsumer;
	}		}

	public void setSupportedPackages(List<String> packages) {		public void setSupportedPackages(List<String> packages) {