Add main thread and reentrant asserts to chase down crashes

We're seeing crashes due to view hierarchy violations that shouldn't be
possible. Adding some guards to make sure we aren't running into
off-thread hierarchy manipulation or re-entrant calls to the update
code.

Test: manual
Bug: 135018709
Change-Id: I4b1f2bd7e3a6f80384486d59b9f56fc3713537cf

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java
index b3da62e..b70b45b 100644 (file)
--- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java
@@ -35,7 +35,7 @@ import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow
 import com.android.systemui.statusbar.notification.stack.NotificationListContainer;
 import com.android.systemui.statusbar.phone.NotificationGroupManager;
 import com.android.systemui.statusbar.phone.ShadeController;
-import com.android.systemui.statusbar.phone.UnlockMethodCache;
+import com.android.systemui.util.Assert;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -84,6 +84,9 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle
     private NotificationPresenter mPresenter;
     private NotificationListContainer mListContainer;
 
+    // Used to help track down re-entrant calls to our update methods, which will cause bugs.
+    private boolean mPerformingUpdate;
+
     @Inject
     public NotificationViewHierarchyManager(Context context,
             NotificationLockscreenUserManager notificationLockscreenUserManager,
@@ -119,6 +122,9 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle
      */
     //TODO: Rewrite this to focus on Entries, or some other data object instead of views
     public void updateNotificationViews() {
+        Assert.isMainThread();
+        beginUpdate();
+
         ArrayList<NotificationEntry> activeNotifications = mEntryManager.getNotificationData()
                 .getActiveNotifications();
         ArrayList<ExpandableNotificationRow> toShow = new ArrayList<>(activeNotifications.size());
@@ -244,9 +250,11 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle
         // clear the map again for the next usage
         mTmpChildOrderMap.clear();
 
-        updateRowStates();
+        updateRowStatesInternal();
 
         mListContainer.onNotificationViewUpdateFinished();
+
+        endUpdate();
     }
 
     private void addNotificationChildrenAndSort() {
@@ -330,6 +338,13 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle
      * Updates expanded, dimmed and locked states of notification rows.
      */
     public void updateRowStates() {
+        Assert.isMainThread();
+        beginUpdate();
+        updateRowStatesInternal();
+        endUpdate();
+    }
+
+    private void updateRowStatesInternal() {
         Trace.beginSection("NotificationViewHierarchyManager#updateRowStates");
         final int N = mListContainer.getContainerChildCount();
 
@@ -422,4 +437,18 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle
     public void onDynamicPrivacyChanged() {
         updateNotificationViews();
     }
+
+    private void beginUpdate() {
+        if (mPerformingUpdate) {
+            throw new IllegalStateException("Re-entrant code during update.");
+        }
+        mPerformingUpdate = true;
+    }
+
+    private void endUpdate() {
+        if (!mPerformingUpdate) {
+            throw new IllegalStateException("Manager state has become desynced.");
+        }
+        mPerformingUpdate = false;
+    }
 }

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java
index 6e9fe67..8fe3418 100644 (file)
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java
@@ -142,6 +142,7 @@ import com.android.systemui.statusbar.policy.ConfigurationController.Configurati
 import com.android.systemui.statusbar.policy.HeadsUpUtil;
 import com.android.systemui.statusbar.policy.ScrollAdapter;
 import com.android.systemui.tuner.TunerService;
+import com.android.systemui.util.Assert;
 
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
@@ -2850,6 +2851,7 @@ public class NotificationStackScrollLayout extends ViewGroup implements ScrollAd
 
     @ShadeViewRefactor(RefactorComponent.SHADE_VIEW)
     public void setChildTransferInProgress(boolean childTransferInProgress) {
+        Assert.isMainThread();
         mChildTransferInProgress = childTransferInProgress;
     }
 
@@ -3293,6 +3295,11 @@ public class NotificationStackScrollLayout extends ViewGroup implements ScrollAd
     @Override
     @ShadeViewRefactor(RefactorComponent.STATE_RESOLVER)
     public void changeViewPosition(ExpandableView child, int newIndex) {
+        Assert.isMainThread();
+        if (mChangePositionInProgress) {
+            throw new IllegalStateException("Reentrant call to changeViewPosition");
+        }
+
         int currentIndex = indexOfChild(child);
 
         if (currentIndex == -1) {
@@ -5053,12 +5060,14 @@ public class NotificationStackScrollLayout extends ViewGroup implements ScrollAd
     @Override
     @ShadeViewRefactor(RefactorComponent.SHADE_VIEW)
     public void removeContainerView(View v) {
+        Assert.isMainThread();
         removeView(v);
     }
 
     @Override
     @ShadeViewRefactor(RefactorComponent.SHADE_VIEW)
     public void addContainerView(View v) {
+        Assert.isMainThread();
         addView(v);
     }
 

diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutTest.java
index 9f49e89..ff83587 100644 (file)
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutTest.java
@@ -38,11 +38,12 @@ import static org.mockito.Mockito.when;
 
 import android.metrics.LogMaker;
 import android.provider.Settings;
+import android.testing.AndroidTestingRunner;
+import android.testing.TestableLooper;
 import android.view.View;
 
 import androidx.test.annotation.UiThreadTest;
 import androidx.test.filters.SmallTest;
-import androidx.test.runner.AndroidJUnit4;
 
 import com.android.internal.logging.MetricsLogger;
 import com.android.internal.logging.nano.MetricsProto;
@@ -95,7 +96,8 @@ import java.util.ArrayList;
  * Tests for {@link NotificationStackScrollLayout}.
  */
 @SmallTest
-@RunWith(AndroidJUnit4.class)
+@RunWith(AndroidTestingRunner.class)
+@TestableLooper.RunWithLooper
 public class NotificationStackScrollLayoutTest extends SysuiTestCase {
 
     private NotificationStackScrollLayout mStackScroller;  // Normally test this
@@ -122,6 +124,8 @@ public class NotificationStackScrollLayoutTest extends SysuiTestCase {
     @Before
     @UiThreadTest
     public void setUp() throws Exception {
+        com.android.systemui.util.Assert.sMainLooper = TestableLooper.get(this).getLooper();
+
         mOriginalInterruptionModelSetting = Settings.Secure.getInt(mContext.getContentResolver(),
                 NOTIFICATION_NEW_INTERRUPTION_MODEL, 0);
         Settings.Secure.putInt(mContext.getContentResolver(),