From 4ce06695d699beed2b7d283a2efb797511621fca Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Fri, 2 Sep 2022 15:53:47 +0000 Subject: [PATCH] Fix binder error when an app has many channels By using a ParceledListSlice when sending the data between processes. Test: PreferencesHelperTest Test: NotificationListenersTest (new since last attempt to land) Test: NotificationManagerServiceTest Test: view the notification settings page for an app with hundreds of notification channels Bug: 215072888 Change-Id: Iac1ba92b6636e86cf506f1ab3c12dcf803101322 --- .../android/app/NotificationChannelGroup.java | 17 ++++-- .../server/notification/ManagedServices.java | 8 +++ .../NotificationManagerService.java | 9 ++-- .../NotificationListenersTest.java | 54 ++++++++++++++++++- .../notification/PreferencesHelperTest.java | 30 +++++++++++ 5 files changed, 109 insertions(+), 9 deletions(-) diff --git a/core/java/android/app/NotificationChannelGroup.java b/core/java/android/app/NotificationChannelGroup.java index 3bd86c1eb94b4..92db8b3290456 100644 --- a/core/java/android/app/NotificationChannelGroup.java +++ b/core/java/android/app/NotificationChannelGroup.java @@ -20,6 +20,7 @@ import android.annotation.SystemApi; import android.annotation.TestApi; import android.compat.annotation.UnsupportedAppUsage; import android.content.Intent; +import android.content.pm.ParceledListSlice; import android.os.Parcel; import android.os.Parcelable; import android.text.TextUtils; @@ -68,7 +69,7 @@ public final class NotificationChannelGroup implements Parcelable { private CharSequence mName; private String mDescription; private boolean mBlocked; - private List mChannels = new ArrayList<>(); + private List mChannels = new ArrayList(); // Bitwise representation of fields that have been changed by the user private int mUserLockedFields; @@ -106,7 +107,12 @@ public final class NotificationChannelGroup implements Parcelable { } else { mDescription = null; } - in.readParcelableList(mChannels, NotificationChannel.class.getClassLoader(), android.app.NotificationChannel.class); + if (in.readByte() != 0) { + mChannels = in.readParcelable(NotificationChannelGroup.class.getClassLoader(), + ParceledListSlice.class).getList(); + } else { + mChannels = new ArrayList<>(); + } mBlocked = in.readBoolean(); mUserLockedFields = in.readInt(); } @@ -138,7 +144,12 @@ public final class NotificationChannelGroup implements Parcelable { } else { dest.writeByte((byte) 0); } - dest.writeParcelableList(mChannels, flags); + if (mChannels != null) { + dest.writeByte((byte) 1); + dest.writeParcelable(new ParceledListSlice<>(mChannels), flags); + } else { + dest.writeByte((byte) 0); + } dest.writeBoolean(mBlocked); dest.writeInt(mUserLockedFields); } diff --git a/services/core/java/com/android/server/notification/ManagedServices.java b/services/core/java/com/android/server/notification/ManagedServices.java index 004caf3da1165..f2327482ae319 100644 --- a/services/core/java/com/android/server/notification/ManagedServices.java +++ b/services/core/java/com/android/server/notification/ManagedServices.java @@ -1718,6 +1718,14 @@ abstract public class ManagedServices { return ManagedServices.this; } + public IInterface getService() { + return service; + } + + public boolean isSystem() { + return isSystem; + } + @Override public String toString() { return new StringBuilder("ManagedServiceInfo[") diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index b06c4118f9b84..57ae9343fc189 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -9962,8 +9962,9 @@ public class NotificationManagerService extends SystemService { * instance per user, we want to filter out interactions that are not for the user that the * given NAS is bound in. */ - private boolean isInteractionVisibleToListener(ManagedServiceInfo info, int userId) { - boolean isAssistantService = isServiceTokenValid(info.service); + @VisibleForTesting + boolean isInteractionVisibleToListener(ManagedServiceInfo info, int userId) { + boolean isAssistantService = isServiceTokenValid(info.getService()); return !isAssistantService || info.isSameUser(userId); } @@ -11265,7 +11266,7 @@ public class NotificationManagerService extends SystemService { } BackgroundThread.getHandler().post(() -> { - if (info.isSystem || hasCompanionDevice(info)) { + if (info.isSystem() || hasCompanionDevice(info)) { notifyNotificationChannelGroupChanged( info, pkg, user, group, modificationType); } @@ -11348,7 +11349,7 @@ public class NotificationManagerService extends SystemService { private void notifyNotificationChannelGroupChanged(ManagedServiceInfo info, final String pkg, final UserHandle user, final NotificationChannelGroup group, final int modificationType) { - final INotificationListener listener = (INotificationListener) info.service; + final INotificationListener listener = (INotificationListener) info.getService(); try { listener.onNotificationChannelGroupModification(pkg, user, group, modificationType); } catch (RemoteException ex) { diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java index 581cf430d359f..90d148856fffd 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationListenersTest.java @@ -28,24 +28,29 @@ import static com.google.common.truth.Truth.assertThat; import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.intThat; import static org.mockito.ArgumentMatchers.nullable; import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import android.app.INotificationManager; import android.app.Notification; import android.app.NotificationChannel; +import android.app.NotificationChannelGroup; import android.app.NotificationManager; import android.content.ComponentName; import android.content.pm.IPackageManager; @@ -54,7 +59,10 @@ import android.content.pm.ServiceInfo; import android.content.pm.VersionedPackage; import android.content.res.Resources; import android.os.Bundle; +import android.os.Parcel; +import android.os.RemoteException; import android.os.UserHandle; +import android.service.notification.INotificationListener; import android.service.notification.NotificationListenerFilter; import android.service.notification.NotificationListenerService; import android.service.notification.NotificationRankingUpdate; @@ -63,10 +71,10 @@ import android.service.notification.StatusBarNotification; import android.testing.TestableContext; import android.util.ArraySet; import android.util.Pair; -import android.util.Xml; - import com.android.modules.utils.TypedXmlPullParser; import com.android.modules.utils.TypedXmlSerializer; +import android.util.Xml; + import com.android.server.UiServiceTestCase; import com.google.common.collect.ImmutableList; @@ -113,6 +121,8 @@ public class NotificationListenersTest extends UiServiceTestCase { getContext().setMockPackageManager(mPm); doNothing().when(mContext).sendBroadcastAsUser(any(), any(), any()); + when(mNm.isInteractionVisibleToListener(any(), anyInt())).thenReturn(true); + mListeners = spy(mNm.new NotificationListeners( mContext, new Object(), mock(ManagedServices.UserProfiles.class), miPm)); when(mNm.getBinderService()).thenReturn(mINm); @@ -596,4 +606,44 @@ public class NotificationListenersTest extends UiServiceTestCase { verify(mPmi).grantImplicitAccess(sbn.getUserId(), null, UserHandle.getAppId(33), sbn.getUid(), false, false); } + + @Test + public void testUpdateGroup_notifyTwoListeners() throws Exception { + final NotificationChannelGroup updated = new NotificationChannelGroup("id", "name"); + updated.setChannels(ImmutableList.of( + new NotificationChannel("a", "a", 1), new NotificationChannel("b", "b", 2))); + updated.setBlocked(true); + + ManagedServices.ManagedServiceInfo i1 = getParcelingListener(updated); + ManagedServices.ManagedServiceInfo i2= getParcelingListener(updated); + when(mListeners.getServices()).thenReturn(ImmutableList.of(i1, i2)); + NotificationChannelGroup existing = new NotificationChannelGroup("id", "name"); + + mListeners.notifyNotificationChannelGroupChanged("pkg", UserHandle.of(0), updated, 0); + Thread.sleep(500); + + verify(((INotificationListener) i1.getService()), times(1)) + .onNotificationChannelGroupModification(anyString(), any(), any(), anyInt()); + } + + private ManagedServices.ManagedServiceInfo getParcelingListener( + final NotificationChannelGroup toParcel) + throws RemoteException { + ManagedServices.ManagedServiceInfo i1 = mock(ManagedServices.ManagedServiceInfo.class); + when(i1.isSystem()).thenReturn(true); + INotificationListener l1 = mock(INotificationListener.class); + when(i1.enabledAndUserMatches(anyInt())).thenReturn(true); + doAnswer(invocationOnMock -> { + try { + toParcel.writeToParcel(Parcel.obtain(), 0); + } catch (Exception e) { + fail("Failed to parcel group to listener"); + return e; + + } + return null; + }).when(l1).onNotificationChannelGroupModification(anyString(), any(), any(), anyInt()); + when(i1.getService()).thenReturn(l1); + return i1; + } } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/PreferencesHelperTest.java b/services/tests/uiservicestests/src/com/android/server/notification/PreferencesHelperTest.java index b64b28137f808..d5be31c5fece9 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/PreferencesHelperTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/PreferencesHelperTest.java @@ -93,6 +93,7 @@ import android.net.Uri; import android.os.AsyncTask; import android.os.Build; import android.os.Bundle; +import android.os.Parcel; import android.os.RemoteCallback; import android.os.RemoteException; import android.os.UserHandle; @@ -2446,6 +2447,35 @@ public class PreferencesHelperTest extends UiServiceTestCase { mLogger.get(6).event); // Final log is the deletion of the channel. } + @Test + public void testGetNotificationChannelGroup() throws Exception { + NotificationChannelGroup notDeleted = new NotificationChannelGroup("not", "deleted"); + NotificationChannel base = + new NotificationChannel("not deleted", "belongs to notDeleted", IMPORTANCE_DEFAULT); + base.setGroup("not"); + NotificationChannel convo = + new NotificationChannel("convo", "belongs to notDeleted", IMPORTANCE_DEFAULT); + convo.setGroup("not"); + convo.setConversationId("not deleted", "banana"); + + mHelper.createNotificationChannelGroup(PKG_N_MR1, UID_N_MR1, notDeleted, true); + mHelper.createNotificationChannel(PKG_N_MR1, UID_N_MR1, base, true, false); + mHelper.createNotificationChannel(PKG_N_MR1, UID_N_MR1, convo, true, false); + mHelper.createNotificationChannelGroup(PKG_N_MR1, UID_N_MR1, notDeleted, true); + + NotificationChannelGroup g + = mHelper.getNotificationChannelGroup(notDeleted.getId(), PKG_N_MR1, UID_N_MR1); + Parcel parcel = Parcel.obtain(); + g.writeToParcel(parcel, 0); + parcel.setDataPosition(0); + + NotificationChannelGroup g2 + = mHelper.getNotificationChannelGroup(notDeleted.getId(), PKG_N_MR1, UID_N_MR1); + Parcel parcel2 = Parcel.obtain(); + g2.writeToParcel(parcel2, 0); + parcel2.setDataPosition(0); + } + @Test public void testOnUserRemoved() throws Exception { int[] user0Uids = {98, 235, 16, 3782};