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

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;
@@ -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

@@ -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() { }
     }
 }