From 7deb904fdfd7cbd098c47ae0e22f40bfd3e5053e Mon Sep 17 00:00:00 2001 From: Ioana Alexandru Date: Wed, 12 Jul 2023 10:01:17 +0000 Subject: [PATCH] Add early exits to visitGrantableUri/calculateGrantableUris. This is to improve the performance of the URI checks by avoiding unnecessary calls. Also added a trace so that we can track this moving forward, and figure out if it needs to be improved further. Note that the tests had to be updated because calculateGrantableUris initially got called in the constructor before we mocked the UriGrantsManager (which should, by the way, only be visible for testing), which populates mGrantableUris, so the early exit kicked in. Normally, calculateGrantableUris should only be called once in the constructor, so that's what we should be testing. Test: atest NotificationRecordTest NotificationManagerServiceTest Fix: 284193006 Bug: 281044385 Change-Id: I44005e4a3d083c9ed9e8dda6918f5c9a2b932d93 --- .../notification/NotificationRecord.java | 45 ++++++++++-------- .../notification/NotificationRecordTest.java | 46 ++++++++----------- 2 files changed, 46 insertions(+), 45 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationRecord.java b/services/core/java/com/android/server/notification/NotificationRecord.java index b2d3fca161126..100c63863f7fb 100644 --- a/services/core/java/com/android/server/notification/NotificationRecord.java +++ b/services/core/java/com/android/server/notification/NotificationRecord.java @@ -25,8 +25,6 @@ import static android.service.notification.NotificationListenerService.Ranking.U import static android.service.notification.NotificationListenerService.Ranking.USER_SENTIMENT_POSITIVE; import android.annotation.Nullable; -import android.app.ActivityManager; -import android.app.IActivityManager; import android.app.KeyguardManager; import android.app.Notification; import android.app.NotificationChannel; @@ -48,6 +46,7 @@ import android.os.Build; import android.os.Bundle; import android.os.IBinder; import android.os.PowerManager; +import android.os.Trace; import android.os.UserHandle; import android.os.VibrationEffect; import android.provider.Settings; @@ -98,8 +97,7 @@ public final class NotificationRecord { // the period after which a notification is updated where it can make sound private static final int MAX_SOUND_DELAY_MS = 2000; private final StatusBarNotification sbn; - IActivityManager mAm; - UriGrantsManagerInternal mUgmInternal; + private final UriGrantsManagerInternal mUgmInternal; final int mTargetSdkVersion; final int mOriginalFlags; private final Context mContext; @@ -223,7 +221,6 @@ public final class NotificationRecord { this.sbn = sbn; mTargetSdkVersion = LocalServices.getService(PackageManagerInternal.class) .getPackageTargetSdkVersion(sbn.getPackageName()); - mAm = ActivityManager.getService(); mUgmInternal = LocalServices.getService(UriGrantsManagerInternal.class); mOriginalFlags = sbn.getNotification().flags; mRankingTimeMs = calculateRankingTimeMs(0L); @@ -1387,18 +1384,27 @@ public final class NotificationRecord { * Collect all {@link Uri} that should have permission granted to whoever * will be rendering it. */ - protected void calculateGrantableUris() { - final Notification notification = getNotification(); - notification.visitUris((uri) -> { - visitGrantableUri(uri, false, false); - }); + private void calculateGrantableUris() { + Trace.beginSection("NotificationRecord.calculateGrantableUris"); + try { + // We can't grant URI permissions from system. + final int sourceUid = getSbn().getUid(); + if (sourceUid == android.os.Process.SYSTEM_UID) return; - if (notification.getChannelId() != null) { - NotificationChannel channel = getChannel(); - if (channel != null) { - visitGrantableUri(channel.getSound(), (channel.getUserLockedFields() - & NotificationChannel.USER_LOCKED_SOUND) != 0, true); + final Notification notification = getNotification(); + notification.visitUris((uri) -> { + visitGrantableUri(uri, false, false); + }); + + if (notification.getChannelId() != null) { + NotificationChannel channel = getChannel(); + if (channel != null) { + visitGrantableUri(channel.getSound(), (channel.getUserLockedFields() + & NotificationChannel.USER_LOCKED_SOUND) != 0, true); + } } + } finally { + Trace.endSection(); } } @@ -1413,13 +1419,14 @@ public final class NotificationRecord { private void visitGrantableUri(Uri uri, boolean userOverriddenUri, boolean isSound) { if (uri == null || !ContentResolver.SCHEME_CONTENT.equals(uri.getScheme())) return; - // We can't grant Uri permissions from system - final int sourceUid = getSbn().getUid(); - if (sourceUid == android.os.Process.SYSTEM_UID) return; + if (mGrantableUris != null && mGrantableUris.contains(uri)) { + return; // already verified this URI + } + final int sourceUid = getSbn().getUid(); final long ident = Binder.clearCallingIdentity(); try { - // This will throw SecurityException if caller can't grant + // This will throw a SecurityException if the caller can't grant. mUgmInternal.checkGrantUriPermission(sourceUid, null, ContentProvider.getUriWithoutUserId(uri), Intent.FLAG_GRANT_READ_URI_PERMISSION, diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java index fae92d9ac738f..f83a1df358bdc 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java @@ -36,7 +36,7 @@ import static junit.framework.Assert.assertNull; import static junit.framework.Assert.assertTrue; import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.fail; +import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; @@ -44,7 +44,6 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import android.app.ActivityManager; -import android.app.IActivityManager; import android.app.Notification; import android.app.Notification.Builder; import android.app.NotificationChannel; @@ -76,6 +75,7 @@ import androidx.test.runner.AndroidJUnit4; import com.android.internal.R; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; +import com.android.server.LocalServices; import com.android.server.UiServiceTestCase; import com.android.server.uri.UriGrantsManagerInternal; @@ -850,84 +850,78 @@ public class NotificationRecordTest extends UiServiceTestCase { @Test public void testCalculateGrantableUris_PappProvided() { - IActivityManager am = mock(IActivityManager.class); UriGrantsManagerInternal ugm = mock(UriGrantsManagerInternal.class); when(ugm.checkGrantUriPermission(anyInt(), eq(null), any(Uri.class), anyInt(), anyInt())).thenThrow(new SecurityException()); + LocalServices.removeServiceForTest(UriGrantsManagerInternal.class); + LocalServices.addService(UriGrantsManagerInternal.class, ugm); + channel.setSound(null, null); Notification n = new Notification.Builder(mContext, channel.getId()) .setSmallIcon(Icon.createWithContentUri(Uri.parse("content://something"))) .build(); StatusBarNotification sbn = new StatusBarNotification(PKG_P, PKG_P, id1, tag1, uid, uid, n, mUser, null, uid); - NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); - record.mAm = am; - record.mUgmInternal = ugm; - try { - record.calculateGrantableUris(); - fail("App provided uri for p targeting app should throw exception"); - } catch (SecurityException e) { - // expected - } + assertThrows("App provided uri for p targeting app should throw exception", + SecurityException.class, + () -> new NotificationRecord(mMockContext, sbn, channel)); } @Test public void testCalculateGrantableUris_PappProvided_invalidSound() { - IActivityManager am = mock(IActivityManager.class); UriGrantsManagerInternal ugm = mock(UriGrantsManagerInternal.class); when(ugm.checkGrantUriPermission(anyInt(), eq(null), any(Uri.class), anyInt(), anyInt())).thenThrow(new SecurityException()); + LocalServices.removeServiceForTest(UriGrantsManagerInternal.class); + LocalServices.addService(UriGrantsManagerInternal.class, ugm); + channel.setSound(Uri.parse("content://something"), mock(AudioAttributes.class)); Notification n = mock(Notification.class); when(n.getChannelId()).thenReturn(channel.getId()); StatusBarNotification sbn = new StatusBarNotification(PKG_P, PKG_P, id1, tag1, uid, uid, n, mUser, null, uid); - NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); - record.mAm = am; - record.mUgmInternal = ugm; - record.calculateGrantableUris(); + NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); assertEquals(Settings.System.DEFAULT_NOTIFICATION_URI, record.getSound()); } @Test public void testCalculateGrantableUris_PuserOverridden() { - IActivityManager am = mock(IActivityManager.class); UriGrantsManagerInternal ugm = mock(UriGrantsManagerInternal.class); when(ugm.checkGrantUriPermission(anyInt(), eq(null), any(Uri.class), anyInt(), anyInt())).thenThrow(new SecurityException()); + LocalServices.removeServiceForTest(UriGrantsManagerInternal.class); + LocalServices.addService(UriGrantsManagerInternal.class, ugm); + channel.lockFields(NotificationChannel.USER_LOCKED_SOUND); Notification n = mock(Notification.class); when(n.getChannelId()).thenReturn(channel.getId()); StatusBarNotification sbn = new StatusBarNotification(PKG_P, PKG_P, id1, tag1, uid, uid, n, mUser, null, uid); - NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); - record.mAm = am; - record.calculateGrantableUris(); + new NotificationRecord(mMockContext, sbn, channel); // should not throw } @Test public void testCalculateGrantableUris_prePappProvided() { - IActivityManager am = mock(IActivityManager.class); UriGrantsManagerInternal ugm = mock(UriGrantsManagerInternal.class); when(ugm.checkGrantUriPermission(anyInt(), eq(null), any(Uri.class), anyInt(), anyInt())).thenThrow(new SecurityException()); + LocalServices.removeServiceForTest(UriGrantsManagerInternal.class); + LocalServices.addService(UriGrantsManagerInternal.class, ugm); + Notification n = mock(Notification.class); when(n.getChannelId()).thenReturn(channel.getId()); StatusBarNotification sbn = new StatusBarNotification(PKG_O, PKG_O, id1, tag1, uid, uid, n, mUser, null, uid); - NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); - record.mAm = am; - record.calculateGrantableUris(); - // should not throw + new NotificationRecord(mMockContext, sbn, channel); // should not throw } @Test