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

Commit 7deb904f authored by Ioana Alexandru's avatar Ioana Alexandru
Browse files

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
parent 0aa176c0

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

+27 −20
Original line number Diff line number Diff line
@@ -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,7 +1384,13 @@ public final class NotificationRecord { 
     * Collect all {@link Uri} that should have permission granted to whoever

     * will be rendering it.

     */

    protected void calculateGrantableUris() {

    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;



            final Notification notification = getNotification();

            notification.visitUris((uri) -> {

                visitGrantableUri(uri, false, false);
@@ -1400,6 +1403,9 @@ public final class NotificationRecord { 
                            & 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,

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

+21 −27
Original line number Diff line number Diff line
@@ -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