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

Commit b3b4bd39 authored by Ned Burns's avatar Ned Burns
Browse files

Delay possible re-entrant call to updateNotificationViews() 

NVHM.onDynamicPrivacyChanged() both calls updateNotificationViews() and
could be called by updateNotificationViews(), resulting in very-bad,
no-good re-entrant behavior. It's too late to try to rearchitect the
entire shade to avoid this relationship, so instead we delay a frame
before applying the update.

Bug: 135018709
Test: atest, manual
Change-Id: I0b17d5ba16e272015b10be182a07cae6b29270e6
parent 949a22d7

packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java

+25 −0
Original line number Diff line number Diff line
@@ -16,8 +16,11 @@ 


package com.android.systemui.statusbar;



import static com.android.systemui.Dependency.MAIN_HANDLER_NAME;



import android.content.Context;

import android.content.res.Resources;

import android.os.Handler;

import android.os.Trace;

import android.os.UserHandle;

import android.util.Log;
@@ -43,6 +46,7 @@ import java.util.List; 
import java.util.Stack;



import javax.inject.Inject;

import javax.inject.Named;

import javax.inject.Singleton;



import dagger.Lazy;
@@ -58,6 +62,8 @@ import dagger.Lazy; 
public class NotificationViewHierarchyManager implements DynamicPrivacyController.Listener {

    private static final String TAG = "NotificationViewHierarchyManager";



    private final Handler mHandler;



    //TODO: change this top <Entry, List<Entry>>?

    private final HashMap<ExpandableNotificationRow, List<ExpandableNotificationRow>>

            mTmpChildOrderMap = new HashMap<>();
@@ -86,9 +92,13 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle 


    // Used to help track down re-entrant calls to our update methods, which will cause bugs.

    private boolean mPerformingUpdate;

    // Hack to get around re-entrant call in onDynamicPrivacyChanged() until we can track down

    // the problem.

    private boolean mIsHandleDynamicPrivacyChangeScheduled;



    @Inject

    public NotificationViewHierarchyManager(Context context,

            @Named(MAIN_HANDLER_NAME) Handler mainHandler,

            NotificationLockscreenUserManager notificationLockscreenUserManager,

            NotificationGroupManager groupManager,

            VisualStabilityManager visualStabilityManager,
@@ -97,6 +107,7 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle 
            Lazy<ShadeController> shadeController,

            BubbleData bubbleData,

            DynamicPrivacyController privacyController) {

        mHandler = mainHandler;

        mLockscreenUserManager = notificationLockscreenUserManager;

        mGroupManager = groupManager;

        mVisualStabilityManager = visualStabilityManager;
@@ -435,6 +446,20 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle 


    @Override

    public void onDynamicPrivacyChanged() {

        if (mPerformingUpdate) {

            Log.w(TAG, "onDynamicPrivacyChanged made a re-entrant call");

        }

        // This listener can be called from updateNotificationViews() via a convoluted listener

        // chain, so we post here to prevent a re-entrant call. See b/136186188

        // TODO: Refactor away the need for this

        if (!mIsHandleDynamicPrivacyChangeScheduled) {

            mIsHandleDynamicPrivacyChangeScheduled = true;

            mHandler.post(this::onHandleDynamicPrivacyChanged);

        }

    }



    private void onHandleDynamicPrivacyChanged() {

        mIsHandleDynamicPrivacyChangeScheduled = false;

        updateNotificationViews();

    }

packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationViewHierarchyManagerTest.java

+70 −3
Original line number Diff line number Diff line
@@ -20,12 +20,16 @@ import static junit.framework.Assert.assertTrue; 


import static org.junit.Assert.assertEquals;

import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyInt;

import static org.mockito.Mockito.clearInvocations;

import static org.mockito.Mockito.doAnswer;

import static org.mockito.Mockito.mock;

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.os.Handler;

import android.testing.AndroidTestingRunner;

import android.testing.TestableLooper;

import android.view.View;
@@ -78,13 +82,19 @@ public class NotificationViewHierarchyManagerTest extends SysuiTestCase { 
    @Mock private VisualStabilityManager mVisualStabilityManager;

    @Mock private ShadeController mShadeController;



    private TestableLooper mTestableLooper;

    private Handler mHandler;

    private NotificationViewHierarchyManager mViewHierarchyManager;

    private NotificationTestHelper mHelper;

    private boolean mMadeReentrantCall = false;



    @Before

    public void setUp() {

        MockitoAnnotations.initMocks(this);

        Assert.sMainLooper = TestableLooper.get(this).getLooper();

        mTestableLooper = TestableLooper.get(this);

        Assert.sMainLooper = mTestableLooper.getLooper();

        mHandler = Handler.createAsync(mTestableLooper.getLooper());



        mDependency.injectTestDependency(NotificationEntryManager.class, mEntryManager);

        mDependency.injectTestDependency(NotificationLockscreenUserManager.class,

                mLockscreenUserManager);
@@ -97,7 +107,7 @@ public class NotificationViewHierarchyManagerTest extends SysuiTestCase { 
        when(mEntryManager.getNotificationData()).thenReturn(mNotificationData);



        mViewHierarchyManager = new NotificationViewHierarchyManager(mContext,

                mLockscreenUserManager, mGroupManager, mVisualStabilityManager,

                mHandler, mLockscreenUserManager, mGroupManager, mVisualStabilityManager,

                mock(StatusBarStateControllerImpl.class), mEntryManager,

                () -> mShadeController, new BubbleData(mContext), mock(DynamicPrivacyController.class));

        Dependency.get(InitController.class).executePostInitTasks();
@@ -212,9 +222,60 @@ public class NotificationViewHierarchyManagerTest extends SysuiTestCase { 
        verify(entry0.getRow(), times(1)).showAppOpsIcons(any());

    }



    @Test

    public void testReentrantCallsToOnDynamicPrivacyChangedPostForLater() {

        // GIVEN a ListContainer that will make a re-entrant call to updateNotificationViews()

        mMadeReentrantCall = false;

        doAnswer((invocation) -> {

            if (!mMadeReentrantCall) {

                mMadeReentrantCall = true;

                mViewHierarchyManager.onDynamicPrivacyChanged();

            }

            return null;

        }).when(mListContainer).setMaxDisplayedNotifications(anyInt());



        // WHEN we call updateNotificationViews()

        mViewHierarchyManager.updateNotificationViews();



        // THEN onNotificationViewUpdateFinished() is only called once

        verify(mListContainer).onNotificationViewUpdateFinished();



        // WHEN we drain the looper

        mTestableLooper.processAllMessages();



        // THEN updateNotificationViews() is called a second time (for the reentrant call)

        verify(mListContainer, times(2)).onNotificationViewUpdateFinished();

    }



    @Test

    public void testMultipleReentrantCallsToOnDynamicPrivacyChangedOnlyPostOnce() {

        // GIVEN a ListContainer that will make many re-entrant calls to updateNotificationViews()

        mMadeReentrantCall = false;

        doAnswer((invocation) -> {

            if (!mMadeReentrantCall) {

                mMadeReentrantCall = true;

                mViewHierarchyManager.onDynamicPrivacyChanged();

                mViewHierarchyManager.onDynamicPrivacyChanged();

                mViewHierarchyManager.onDynamicPrivacyChanged();

                mViewHierarchyManager.onDynamicPrivacyChanged();

            }

            return null;

        }).when(mListContainer).setMaxDisplayedNotifications(anyInt());



        // WHEN we call updateNotificationViews() and drain the looper

        mViewHierarchyManager.updateNotificationViews();

        verify(mListContainer).onNotificationViewUpdateFinished();

        clearInvocations(mListContainer);

        mTestableLooper.processAllMessages();



        // THEN updateNotificationViews() is called only one more time

        verify(mListContainer).onNotificationViewUpdateFinished();

    }



    private class FakeListContainer implements NotificationListContainer {

        final LinearLayout mLayout = new LinearLayout(mContext);

        final List<View> mRows = Lists.newArrayList();

        private boolean mMakeReentrantCallDuringSetMaxDisplayedNotifications;



        @Override

        public void setChildTransferInProgress(boolean childTransferInProgress) {}
@@ -263,7 +324,11 @@ public class NotificationViewHierarchyManagerTest extends SysuiTestCase { 
        }



        @Override

        public void setMaxDisplayedNotifications(int maxNotifications) {}

        public void setMaxDisplayedNotifications(int maxNotifications) {

            if (mMakeReentrantCallDuringSetMaxDisplayedNotifications) {

                mViewHierarchyManager.onDynamicPrivacyChanged();

            }

        }



        @Override

        public ViewGroup getViewParentForNotification(NotificationEntry entry) {
@@ -298,5 +363,7 @@ public class NotificationViewHierarchyManagerTest extends SysuiTestCase { 
            return false;

        }



        @Override

        public void onNotificationViewUpdateFinished() { }

    }

}