Merge "Delay possible re-entrant call to updateNotificationViews()" into qt-dev

am: bce828f157

Change-Id: I1510f3e1d6a7c1a1149e917dfd55b3a634e91aec

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

@@ -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;
@@ -44,6 +47,7 @@ import java.util.List;
 import java.util.Stack;
 
 import javax.inject.Inject;
+import javax.inject.Named;
 import javax.inject.Singleton;
 
 import dagger.Lazy;
@@ -59,6 +63,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<>();
@@ -88,9 +94,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,
@@ -100,6 +110,7 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle
             BubbleData bubbleData,
             KeyguardBypassController bypassController,
             DynamicPrivacyController privacyController) {
+        mHandler = mainHandler;
         mLockscreenUserManager = notificationLockscreenUserManager;
         mBypassController = bypassController;
         mGroupManager = groupManager;
@@ -438,6 +449,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

@@ -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;
@@ -79,13 +83,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);
@@ -98,7 +108,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(KeyguardBypassController.class),
@@ -215,9 +225,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) {}
@@ -266,7 +327,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) {
@@ -301,5 +366,7 @@ public class NotificationViewHierarchyManagerTest extends SysuiTestCase {
             return false;
         }
 
+        @Override
+        public void onNotificationViewUpdateFinished() { }
     }
 }