Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 53a7cf13 authored by Julia Reynolds's avatar Julia Reynolds
Browse files

Try to avoid duplicate bindings and wasted work 

- onBindingDied is called when a package is upgraded, so we don't
need to rebind during onPackagesChanged
- remove binderDied, which was only partially cleaning up state
- we definitely don't need to mess with the binding of other apps
when one of them updates
- try to track 'we're going to bind to a service' more accurately
- Slog.wtf if we're still managing to bind twice
- Don't unbind just before binding

Flag: com.android.server.notification.use_on_binding_died
Bug: 434679743
Test: ManagedServicesTest
Test: NotificationManagerTest
Test: NotificationAssistantServiceTest
Test: NotificationManagerZenTest
Test: manual - update an app and verify that it's still receiving notifications
after the rebind delay (and that it's only in the list once)
Test: manual - update an app ro remove the BIND_NOTIFICATION_LISTENER_SERVICE
permission - verify that it's not bound after the rebind delay

Change-Id: I92ed7dcb94cc327c162b440afe089f6d8ba5b368
parent 798e5cd0

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