Merge "Try to avoid duplicate bindings and wasted work" into main (b94dec98) · Commits · e / os / android_frameworks_base

services/core/java/com/android/server/notification/ManagedServices.java

+85 −25

Original line number	Diff line number	Diff line
		@@ -32,6 +32,7 @@ import static com.android.server.notification.NotificationManagerService.private

		import android.annotation.FlaggedApi;
		import android.annotation.NonNull;
		import android.annotation.UserIdInt;
		import android.app.ActivityManager;
		import android.app.ActivityOptions;
		import android.app.IBinderSession;
		@@ -113,12 +114,12 @@ abstract public class ManagedServices {
		protected final boolean DEBUG = Log.isLoggable(TAG, Log.DEBUG);

		private static final int ON_BINDING_DIED_REBIND_DELAY_MS = 10000;
		private boolean mRebindAsyncDelay = true;
		protected static final String ENABLED_SERVICES_SEPARATOR = ":";
		private static final String DB_VERSION_1 = "1";
		private static final String DB_VERSION_2 = "2";
		private static final String DB_VERSION_3 = "3";


		/**
		* List of components and apps that can have running {@link ManagedServices}.
		*/
		@@ -1205,13 +1206,14 @@ abstract public class ManagedServices {
		}
		}
		}

		if (!Flags.useOnBindingDied()) {
		if (anyServicesInvolved) {
		// make sure we're still bound to any of our services who may have just upgraded
		rebindServices(false, USER_ALL);
		}
		}
		}
		}

		public void onUserRemoved(int user) {
		Slog.i(TAG, "Removing approved services for removed user " + user);
		@@ -1817,6 +1819,10 @@ abstract public class ManagedServices {
		if (isPackageOrComponentAllowedWithPermission(cn, userId)) {
		registerService(cn, userId);
		} else {
		if (Flags.useOnBindingDied()) {
		// clean up enabled component list
		mEnabledServicesByUser.get(resolveUserId(userId)).remove(cn);
		}
		if (DEBUG) Slog.v(TAG, "skipped reregisterService cn=" + cn + " u=" + userId
		+ " because of isPackageOrComponentAllowedWithPermission check");
		}
		@@ -1831,11 +1837,33 @@ abstract public class ManagedServices {
		}
		}

		@GuardedBy("mMutex")
		private void disconnectServiceLocked(ServiceConnection sc, IInterface service,
		@UserIdInt int userId, ComponentName cn) {
		if (service != null) {
		Slog.w(TAG, userId + " " + getCaption() + " unregister: " + cn);
		unregisterService(service, userId);
		} else {
		Slog.w(TAG, userId + " " + getCaption() + " unbind: " + cn);
		unbindService(sc, cn, userId);
		}
		}

		@GuardedBy("mMutex")
		private void registerServiceLocked(final ComponentName name, final int userid) {
		registerServiceLocked(name, userid, false /* isSystem */);
		}

		@GuardedBy("mMutex")
		private boolean isServiceAlreadyBoundLocked(ManagedServiceInfo info) {
		for (ManagedServiceInfo info2 : mServices) {
		if (Objects.equals(info.component, info2.component) && info.userid == info2.userid) {
		return true;
		}
		}
		return false;
		}

		@GuardedBy("mMutex")
		private void registerServiceLocked(final ComponentName name, final int userid,
		final boolean isSystem) {
		@@ -1843,12 +1871,13 @@ abstract public class ManagedServices {

		final Pair<ComponentName, Integer> servicesBindingTag = Pair.create(name, userid);
		if (mServicesBound.contains(servicesBindingTag)) {
		Slog.v(TAG, "Not registering " + name + " is already bound");
		Slog.v(TAG, "Not registering " + name + " is already binding");
		// stop registering this thing already! we're working on it
		return;
		}
		mServicesBound.add(servicesBindingTag);

		if (!Flags.useOnBindingDied()) {
		final int N = mServices.size();
		for (int i = N - 1; i >= 0; i--) {
		final ManagedServiceInfo info = mServices.get(i);
		@@ -1862,6 +1891,7 @@ abstract public class ManagedServices {
		}
		}
		}
		}

		Intent intent = new Intent(mConfig.serviceInterface);
		intent.setComponent(name);
		@@ -1909,9 +1939,20 @@ abstract public class ManagedServices {
		info = new ManagedServiceInfo(mService, name, userid, isSystem,
		this, targetSdkVersion, uid);
		}
		if (!Flags.useOnBindingDied()) {
		binder.linkToDeath(info, 0);
		}
		if (Flags.useOnBindingDied() && isServiceAlreadyBoundLocked(info)) {
		Slog.wtfStack(TAG, "Duplicate binding " + info);
		}
		added = mServices.add(info);
		} catch (RemoteException e) {
		if (Flags.useOnBindingDied()) {
		// This block is likely unreachable because RemoteException is
		// typically thrown by binder.linkToDeath, which is skipped
		// when Flags.useOnBindingDied() is true.
		mServicesBound.remove(servicesBindingTag);
		}
		Slog.e(TAG, "Failed to linkToDeath, already dead", e);
		}
		}
		@@ -1937,12 +1978,20 @@ abstract public class ManagedServices {
		public void onBindingDied(ComponentName name) {
		Slog.w(TAG, userid + " " + getCaption() + " binding died: " + name);
		synchronized (mMutex) {
		if (Flags.useOnBindingDied()) {
		disconnectServiceLocked(this, mService, userid, name);
		} else {
		unbindService(this, name, userid);
		}
		if (!mServicesRebinding.contains(servicesBindingTag)) {
		mServicesRebinding.add(servicesBindingTag);
		if (mRebindAsyncDelay) {
		mHandler.postDelayed(() ->
		reregisterService(name, userid),
		ON_BINDING_DIED_REBIND_DELAY_MS);
		} else {
		reregisterService(name, userid);
		}
		} else {
		Slog.v(TAG, getCaption() + " not rebinding in user " + userid
		+ " as a previous rebind attempt was made: " + name);
		@@ -1953,8 +2002,12 @@ abstract public class ManagedServices {
		@Override
		public void onNullBinding(ComponentName name) {
		Slog.v(TAG, "onNullBinding() called with: name = [" + name + "]");
		if (Flags.useOnBindingDied()) {
		disconnectServiceLocked(this, mService, userid, name);
		} else {
		mContext.unbindService(this);
		}
		}
		};
		if (!mContext.bindServiceAsUser(intent,
		serviceConnection,
		@@ -1963,7 +2016,6 @@ abstract public class ManagedServices {
		mServicesBound.remove(servicesBindingTag);
		Slog.w(TAG, "Unable to bind " + getCaption() + " service: " + intent
		+ " in user " + userid);
		return;
		}
		} catch (SecurityException ex) {
		mServicesBound.remove(servicesBindingTag);
		@@ -2134,6 +2186,11 @@ abstract public class ManagedServices {
		*/
		protected abstract boolean allowRebindForParentUser();

		@VisibleForTesting
		void setRebindAsyncDelay(boolean rebindAsyncDelay) {
		mRebindAsyncDelay = rebindAsyncDelay;
		}

		public class ManagedServiceInfo implements IBinder.DeathRecipient {
		public IInterface service;
		public ComponentName component;
		@@ -2251,6 +2308,9 @@ abstract public class ManagedServices {

		@Override
		public void binderDied() {
		if (Flags.useOnBindingDied()) {
		return;
		}
		if (DEBUG) Slog.d(TAG, "binderDied " + this);
		// Remove the service, but don't unbind from the service. The system will bring the
		// service back up, and the onServiceConnected handler will read the service with the

services/core/java/com/android/server/notification/flags.aconfig

+11 −1

Original line number	Diff line number	Diff line
		@@ -241,6 +241,16 @@ flag {
		}
		}

		flag {
		name: "use_on_binding_died"
		namespace: "notifications"
		description: "Prefer onBindingDied for package updates to avoid duplicate service bindings"
		bug: "434679743"
		metadata {
		purpose: PURPOSE_BUGFIX
		}
		}

		flag {
		name: "limit_managed_services_count"
		namespace: "systemui"

services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java

+36 −57

Original line number	Diff line number	Diff line
		@@ -15,8 +15,8 @@
		*/
		package com.android.server.notification;

		import static android.content.Context.DEVICE_POLICY_SERVICE;
		import static android.app.Flags.FLAG_LIFETIME_EXTENSION_REFACTOR;
		import static android.content.Context.DEVICE_POLICY_SERVICE;
		import static android.os.UserHandle.USER_ALL;
		import static android.os.UserHandle.USER_CURRENT;
		import static android.os.UserManager.USER_TYPE_FULL_SECONDARY;
		@@ -40,11 +40,12 @@ import static junit.framework.Assert.assertFalse;
		import static junit.framework.Assert.assertNotNull;
		import static junit.framework.Assert.assertTrue;

		import static org.mockito.ArgumentMatchers.anyString;
		import static org.mockito.ArgumentMatchers.any;
		import static org.mockito.ArgumentMatchers.anyInt;
		import static org.mockito.ArgumentMatchers.anyLong;
		import static org.mockito.ArgumentMatchers.anyString;
		import static org.mockito.ArgumentMatchers.eq;
		import static org.mockito.Mockito.doNothing;
		import static org.mockito.Mockito.doReturn;
		import static org.mockito.Mockito.mock;
		import static org.mockito.Mockito.never;
		@@ -76,6 +77,7 @@ import android.os.UserHandle;
		import android.os.UserManager;
		import android.platform.test.annotations.DisableFlags;
		import android.platform.test.annotations.EnableFlags;
		import android.platform.test.flag.junit.FlagsParameterization;
		import android.platform.test.flag.junit.SetFlagsRule;
		import android.provider.Settings;
		import android.text.TextUtils;
		@@ -92,9 +94,13 @@ import com.android.server.UiServiceTestCase;

		import com.google.android.collect.Lists;

		import platform.test.runner.parameterized.ParameterizedAndroidJunit4;
		import platform.test.runner.parameterized.Parameters;

		import org.junit.Before;
		import org.junit.Rule;
		import org.junit.Test;
		import org.junit.runner.RunWith;
		import org.mockito.ArgumentCaptor;
		import org.mockito.Mock;
		import org.mockito.MockitoAnnotations;
		@@ -114,6 +120,7 @@ import java.util.List;
		import java.util.Set;
		import java.util.concurrent.CountDownLatch;

		@RunWith(ParameterizedAndroidJunit4.class)
		public class ManagedServicesTest extends UiServiceTestCase {
		private static final IBinderSession NULL_BINDER_SESSION = null;

		@@ -160,6 +167,16 @@ public class ManagedServicesTest extends UiServiceTestCase {
		private static final int PKG2_UID = 10002;
		private static final int PKG3_UID = 10003;

		@Parameters(name = "{0}")
		public static List<FlagsParameterization> getParams() {
		return FlagsParameterization.allCombinationsOf(
		Flags.FLAG_MANAGED_SERVICES_CONCURRENT_MULTIUSER, Flags.FLAG_USE_ON_BINDING_DIED);
		}

		public ManagedServicesTest(FlagsParameterization flags) {
		mSetFlagsRule.setFlagsParameterization(flags);
		}

		@Before
		public void setUp() throws Exception {
		MockitoAnnotations.initMocks(this);
		@@ -1292,14 +1309,16 @@ public class ManagedServicesTest extends UiServiceTestCase {
		}

		@Test
		@DisableFlags(FLAG_MANAGED_SERVICES_CONCURRENT_MULTIUSER)
		public void testUpgradeAppNoPermissionNoRebind() throws Exception {
		Context context = spy(getContext());
		doReturn(true).when(context).bindServiceAsUser(any(), any(), any(), any());
		ArgumentCaptor<ServiceConnection> captor = ArgumentCaptor.forClass(ServiceConnection.class);
		doReturn(true).when(context).bindServiceAsUser(any(), captor.capture(), any(), any());
		doNothing().when(context).unbindService(any());

		ManagedServices service = new TestManagedServices(context, mLock, mUserProfiles,
		mIpm,
		APPROVAL_BY_COMPONENT);
		service.setRebindAsyncDelay(false);

		List<String> packages = new ArrayList<>();
		packages.add("package");
		@@ -1317,58 +1336,10 @@ public class ManagedServicesTest extends UiServiceTestCase {

		loadXml(service);

		//Component package/C1 loses bind permission
		when(mIpm.getServiceInfo(any(), anyLong(), anyInt())).thenAnswer(
		(Answer<ServiceInfo>) invocation -> {
		ComponentName invocationCn = invocation.getArgument(0);
		if (invocationCn != null) {
		ServiceInfo serviceInfo = new ServiceInfo();
		serviceInfo.packageName = invocationCn.getPackageName();
		serviceInfo.name = invocationCn.getClassName();
		if (invocationCn.equals(unapprovedComponent)) {
		serviceInfo.permission = "none";
		} else {
		serviceInfo.permission = service.getConfig().bindPermission;
		}
		serviceInfo.metaData = null;
		return serviceInfo;
		}
		return null;
		}
		);

		// Trigger package update
		service.onPackagesChanged(false, new String[]{"package"}, new int[]{0});

		assertFalse(service.isComponentEnabledForCurrentProfiles(unapprovedComponent));
		assertTrue(service.isComponentEnabledForCurrentProfiles(approvedComponent));
		}

		@Test
		@EnableFlags(FLAG_MANAGED_SERVICES_CONCURRENT_MULTIUSER)
		public void testUpgradeAppNoPermissionNoRebind_concurrent_multiUser() throws Exception {
		Context context = spy(getContext());
		doReturn(true).when(context).bindServiceAsUser(any(), any(), any(), any());

		ManagedServices service = new TestManagedServices(context, mLock, mUserProfiles,
		mIpm,
		APPROVAL_BY_COMPONENT);

		List<String> packages = new ArrayList<>();
		packages.add("package");
		addExpectedServices(service, packages, 0);

		final ComponentName unapprovedComponent = ComponentName.unflattenFromString("package/C1");
		final ComponentName approvedComponent = ComponentName.unflattenFromString("package/C2");

		// Both components are approved initially
		mExpectedPrimaryComponentNames.clear();
		mExpectedPrimaryPackages.clear();
		mExpectedPrimaryComponentNames.put(0, "package/C1:package/C2");
		mExpectedSecondaryComponentNames.clear();
		mExpectedSecondaryPackages.clear();

		loadXml(service);
		captor.getAllValues().get(0).onServiceConnected(
		unapprovedComponent, mock(IBinder.class), mock(IBinderSession.class));
		captor.getAllValues().get(1).onServiceConnected(
		approvedComponent, mock(IBinder.class), mock(IBinderSession.class));

		//Component package/C1 loses bind permission
		when(mIpm.getServiceInfo(any(), anyLong(), anyInt())).thenAnswer(
		@@ -1391,10 +1362,18 @@ public class ManagedServicesTest extends UiServiceTestCase {
		);

		// Trigger package update
		if (Flags.useOnBindingDied()) {
		captor.getAllValues().get(0).onBindingDied(unapprovedComponent);
		captor.getAllValues().get(1).onBindingDied(approvedComponent);
		} else {
		service.onPackagesChanged(false, new String[]{"package"}, new int[]{0});
		}

		assertFalse(service.isComponentEnabledForUser(unapprovedComponent, 0));
		assertTrue(service.isComponentEnabledForUser(approvedComponent, 0));

		assertThat(service.isBound(unapprovedComponent, 0)).isFalse();
		assertThat(service.isBound(approvedComponent, 0)).isTrue();
		}

		@Test