From d4a69f7007a1b35511608c3556116ee3ab921d59 Mon Sep 17 00:00:00 2001 From: Ned Burns Date: Wed, 19 Jun 2019 19:49:19 -0400 Subject: [PATCH] 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 --- .../NotificationViewHierarchyManager.java | 33 +++++++++++++++++-- .../stack/NotificationStackScrollLayout.java | 9 +++++ .../NotificationStackScrollLayoutTest.java | 8 +++-- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java index b3da62eef35b1..b70b45b9db842 100644 --- 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 activeNotifications = mEntryManager.getNotificationData() .getActiveNotifications(); ArrayList 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 6e9fe670bc32d..8fe34180203f3 100644 --- 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 9f49e89dc9c3e..ff835871d8222 100644 --- 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(),