From 6d4d8ed181953617b7b19880dc303dd3fd1991fc Mon Sep 17 00:00:00 2001 From: Winson Chung Date: Sun, 7 Feb 2021 15:56:45 -0800 Subject: [PATCH] Remove some blocking calls in BubbleManager - When shell thread is enabled, there's a deadlock that can happen from having blocking calls from both Shell -> SysUI and vice versa. Specifically if you have a scenario like: Shell SysUI A ----------------> f() (shell blocking call, posts onto thread & awaits) | g() <-------------- B (sysui blocking call while f() is posted) | | | V V f() runs g() runs The call f() will never actually run since B came in which is blocked on g() which can't finish because f() has not returned. - Instead, replace some blocking calls with preloaded info (ie. can send bubble entry and whether the the entry should bubble up to onRankingUpdated since it just calls back to query it), and make getShouldRestoredEntries() and getPendingOrActiveEntry() async with a callback. - Can remove isNotificationShadeExpand() since it's not actually used anymore in bubble code Bug: 161979899 Test: atest WMShellUnitTests Test: atest SystemUITests Change-Id: Iba834172c17c3f4b03c23448fef346c71c16c1bb Signed-off-by: Winson Chung --- .../wm/shell/bubbles/BubbleController.java | 90 +++++++++---------- .../com/android/wm/shell/bubbles/Bubbles.java | 17 ++-- .../startingsurface/TaskSnapshotWindow.java | 3 +- .../systemui/wmshell/BubblesManager.java | 74 ++++++--------- 4 files changed, 81 insertions(+), 103 deletions(-) diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java index d73fc6dca4c66..f61233d99c81a 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java @@ -86,6 +86,7 @@ import com.android.wm.shell.pip.PinnedStackListenerForwarder; import java.io.FileDescriptor; import java.io.PrintWriter; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Objects; import java.util.concurrent.Executor; @@ -586,11 +587,15 @@ public class BubbleController { // There were no bubbles saved for this used. return; } - for (BubbleEntry e : mSysuiProxy.getShouldRestoredEntries(savedBubbleKeys)) { - if (canLaunchInActivityView(mContext, e)) { - updateBubble(e, true /* suppressFlyout */, false /* showInShade */); - } - } + mSysuiProxy.getShouldRestoredEntries(savedBubbleKeys, (entries) -> { + mMainExecutor.execute(() -> { + for (BubbleEntry e : entries) { + if (canLaunchInActivityView(mContext, e)) { + updateBubble(e, true /* suppressFlyout */, false /* showInShade */); + } + } + }); + }); // Finally, remove the entries for this user now that bubbles are restored. mSavedBubbleKeysPerUser.remove(mCurrentUserId); } @@ -856,21 +861,24 @@ public class BubbleController { } } - private void onRankingUpdated(RankingMap rankingMap) { + private void onRankingUpdated(RankingMap rankingMap, + HashMap> entryDataByKey) { if (mTmpRanking == null) { mTmpRanking = new NotificationListenerService.Ranking(); } String[] orderedKeys = rankingMap.getOrderedKeys(); for (int i = 0; i < orderedKeys.length; i++) { String key = orderedKeys[i]; - BubbleEntry entry = mSysuiProxy.getPendingOrActiveEntry(key); + Pair entryData = entryDataByKey.get(key); + BubbleEntry entry = entryData.first; + boolean shouldBubbleUp = entryData.second; rankingMap.getRanking(key, mTmpRanking); boolean isActiveBubble = mBubbleData.hasAnyBubbleWithKey(key); if (isActiveBubble && !mTmpRanking.canBubble()) { // If this entry is no longer allowed to bubble, dismiss with the BLOCKED reason. // This means that the app or channel's ability to bubble has been revoked. mBubbleData.dismissBubbleWithKey(key, DISMISS_BLOCKED); - } else if (isActiveBubble && !mSysuiProxy.shouldBubbleUp(key)) { + } else if (isActiveBubble && !shouldBubbleUp) { // If this entry is allowed to bubble, but cannot currently bubble up, dismiss it. // This happens when DND is enabled and configured to hide bubbles. Dismissing with // the reason DISMISS_NO_BUBBLE_UP will retain the underlying notification, so that @@ -919,17 +927,20 @@ public class BubbleController { private void setIsBubble(@NonNull final Bubble b, final boolean isBubble) { Objects.requireNonNull(b); b.setIsBubble(isBubble); - final BubbleEntry entry = mSysuiProxy.getPendingOrActiveEntry(b.getKey()); - if (entry != null) { - // Updating the entry to be a bubble will trigger our normal update flow - setIsBubble(entry, isBubble, b.shouldAutoExpand()); - } else if (isBubble) { - // If bubble doesn't exist, it's a persisted bubble so we need to add it to the - // stack ourselves - Bubble bubble = mBubbleData.getOrCreateBubble(null, b /* persistedBubble */); - inflateAndAdd(bubble, bubble.shouldAutoExpand() /* suppressFlyout */, - !bubble.shouldAutoExpand() /* showInShade */); - } + mSysuiProxy.getPendingOrActiveEntry(b.getKey(), (entry) -> { + mMainExecutor.execute(() -> { + if (entry != null) { + // Updating the entry to be a bubble will trigger our normal update flow + setIsBubble(entry, isBubble, b.shouldAutoExpand()); + } else if (isBubble) { + // If bubble doesn't exist, it's a persisted bubble so we need to add it to the + // stack ourselves + Bubble bubble = mBubbleData.getOrCreateBubble(null, b /* persistedBubble */); + inflateAndAdd(bubble, bubble.shouldAutoExpand() /* suppressFlyout */, + !bubble.shouldAutoExpand() /* showInShade */); + } + }); + }); } @SuppressWarnings("FieldCanBeLocal") @@ -992,14 +1003,17 @@ public class BubbleController { } } - final BubbleEntry entry = mSysuiProxy.getPendingOrActiveEntry(bubble.getKey()); - if (entry != null) { - final String groupKey = entry.getStatusBarNotification().getGroupKey(); - if (getBubblesInGroup(groupKey).isEmpty()) { - // Time to potentially remove the summary - mSysuiProxy.notifyMaybeCancelSummary(bubble.getKey()); - } - } + mSysuiProxy.getPendingOrActiveEntry(bubble.getKey(), (entry) -> { + mMainExecutor.execute(() -> { + if (entry != null) { + final String groupKey = entry.getStatusBarNotification().getGroupKey(); + if (getBubblesInGroup(groupKey).isEmpty()) { + // Time to potentially remove the summary + mSysuiProxy.notifyMaybeCancelSummary(bubble.getKey()); + } + } + }); + }); } mDataRepository.removeBubbles(mCurrentUserId, bubblesToBeRemovedFromRepository); @@ -1121,23 +1135,6 @@ public class BubbleController { mStackView.updateContentDescription(); } - /** - * The task id of the expanded view, if the stack is expanded and not occluded by the - * status bar, otherwise returns {@link ActivityTaskManager#INVALID_TASK_ID}. - */ - private int getExpandedTaskId() { - if (mStackView == null) { - return INVALID_TASK_ID; - } - final BubbleViewProvider expandedViewProvider = mStackView.getExpandedBubble(); - if (expandedViewProvider != null && isStackExpanded() - && !mStackView.isExpansionAnimating() - && !mSysuiProxy.isNotificationShadeExpand()) { - return expandedViewProvider.getTaskId(); - } - return INVALID_TASK_ID; - } - @VisibleForTesting public BubbleStackView getStackView() { return mStackView; @@ -1343,9 +1340,10 @@ public class BubbleController { } @Override - public void onRankingUpdated(RankingMap rankingMap) { + public void onRankingUpdated(RankingMap rankingMap, + HashMap> entryDataByKey) { mMainExecutor.execute(() -> { - BubbleController.this.onRankingUpdated(rankingMap); + BubbleController.this.onRankingUpdated(rankingMap, entryDataByKey); }); } diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubbles.java b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubbles.java index 6a1026bb24fea..8e061e9c98741 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubbles.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubbles.java @@ -26,6 +26,7 @@ import android.os.Bundle; import android.os.Looper; import android.service.notification.NotificationListenerService.RankingMap; import android.util.ArraySet; +import android.util.Pair; import android.view.View; import androidx.annotation.IntDef; @@ -37,6 +38,7 @@ import java.io.FileDescriptor; import java.io.PrintWriter; import java.lang.annotation.Retention; import java.lang.annotation.Target; +import java.util.HashMap; import java.util.List; import java.util.concurrent.Executor; import java.util.function.BiConsumer; @@ -182,8 +184,11 @@ public interface Bubbles { * permissions on the notification channel or the global setting. * * @param rankingMap the updated ranking map from NotificationListenerService + * @param entryDataByKey a map of ranking key to bubble entry and whether the entry should + * bubble up */ - void onRankingUpdated(RankingMap rankingMap); + void onRankingUpdated(RankingMap rankingMap, + HashMap> entryDataByKey); /** * Called when the status bar has become visible or invisible (either permanently or @@ -243,14 +248,10 @@ public interface Bubbles { /** Callback to tell SysUi components execute some methods. */ interface SysuiProxy { - @Nullable - BubbleEntry getPendingOrActiveEntry(String key); + void getPendingOrActiveEntry(String key, Consumer callback); - List getShouldRestoredEntries(ArraySet savedBubbleKeys); - - boolean isNotificationShadeExpand(); - - boolean shouldBubbleUp(String key); + void getShouldRestoredEntries(ArraySet savedBubbleKeys, + Consumer> callback); void setNotificationInterruption(String key); diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/startingsurface/TaskSnapshotWindow.java b/libs/WindowManager/Shell/src/com/android/wm/shell/startingsurface/TaskSnapshotWindow.java index a6f44efd76458..b7fd3cb67b2b3 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/startingsurface/TaskSnapshotWindow.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/startingsurface/TaskSnapshotWindow.java @@ -43,6 +43,7 @@ import static com.android.internal.policy.DecorView.NAVIGATION_BAR_COLOR_VIEW_AT import static com.android.internal.policy.DecorView.STATUS_BAR_COLOR_VIEW_ATTRIBUTES; import static com.android.internal.policy.DecorView.getNavigationBarRect; +import android.annotation.BinderThread; import android.annotation.Nullable; import android.app.ActivityManager; import android.app.ActivityManager.TaskDescription; @@ -498,7 +499,7 @@ public class TaskSnapshotWindow { } } - @ExternalThread + @BinderThread static class Window extends BaseIWindow { private TaskSnapshotWindow mOuter; diff --git a/packages/SystemUI/src/com/android/systemui/wmshell/BubblesManager.java b/packages/SystemUI/src/com/android/systemui/wmshell/BubblesManager.java index 6e7aed0641591..afeda967c8c15 100644 --- a/packages/SystemUI/src/com/android/systemui/wmshell/BubblesManager.java +++ b/packages/SystemUI/src/com/android/systemui/wmshell/BubblesManager.java @@ -45,6 +45,7 @@ import android.service.notification.NotificationListenerService.RankingMap; import android.service.notification.ZenModeConfig; import android.util.ArraySet; import android.util.Log; +import android.util.Pair; import android.view.View; import androidx.annotation.NonNull; @@ -86,10 +87,12 @@ import java.io.FileDescriptor; import java.io.PrintWriter; import java.lang.reflect.Array; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Optional; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; +import java.util.function.Consumer; import java.util.function.IntConsumer; import java.util.function.Supplier; @@ -248,38 +251,19 @@ public class BubblesManager implements Dumpable { }); mSysuiProxy = new Bubbles.SysuiProxy() { - private T executeBlockingForResult(Supplier runnable, Executor executor, - Class clazz) { - if (Looper.myLooper() == Looper.getMainLooper()) { - return runnable.get(); - } - final T[] result = (T[]) Array.newInstance(clazz, 1); - final CountDownLatch latch = new CountDownLatch(1); - executor.execute(() -> { - result[0] = runnable.get(); - latch.countDown(); - }); - try { - latch.await(); - return result[0]; - } catch (InterruptedException e) { - return null; - } - } - @Override - @Nullable - public BubbleEntry getPendingOrActiveEntry(String key) { - return executeBlockingForResult(() -> { + public void getPendingOrActiveEntry(String key, Consumer callback) { + sysuiMainExecutor.execute(() -> { NotificationEntry entry = mNotificationEntryManager.getPendingOrActiveNotif(key); - return entry == null ? null : notifToBubbleEntry(entry); - }, sysuiMainExecutor, BubbleEntry.class); + callback.accept(entry == null ? null : notifToBubbleEntry(entry)); + }); } @Override - public List getShouldRestoredEntries(ArraySet savedBubbleKeys) { - return executeBlockingForResult(() -> { + public void getShouldRestoredEntries(ArraySet savedBubbleKeys, + Consumer> callback) { + sysuiMainExecutor.execute(() -> { List result = new ArrayList<>(); List activeEntries = mNotificationEntryManager.getActiveNotificationsForCurrentUser(); @@ -291,27 +275,8 @@ public class BubblesManager implements Dumpable { result.add(notifToBubbleEntry(entry)); } } - return result; - }, sysuiMainExecutor, List.class); - } - - @Override - public boolean isNotificationShadeExpand() { - return executeBlockingForResult(() -> { - return mNotificationShadeWindowController.getPanelExpanded(); - }, sysuiMainExecutor, Boolean.class); - } - - @Override - public boolean shouldBubbleUp(String key) { - return executeBlockingForResult(() -> { - final NotificationEntry entry = - mNotificationEntryManager.getPendingOrActiveNotif(key); - if (entry != null) { - return mNotificationInterruptStateProvider.shouldBubbleUp(entry); - } - return false; - }, sysuiMainExecutor, Boolean.class); + callback.accept(result); + }); } @Override @@ -587,7 +552,20 @@ public class BubblesManager implements Dumpable { } void onRankingUpdate(RankingMap rankingMap) { - mBubbles.onRankingUpdated(rankingMap); + String[] orderedKeys = rankingMap.getOrderedKeys(); + HashMap> pendingOrActiveNotif = new HashMap<>(); + for (int i = 0; i < orderedKeys.length; i++) { + String key = orderedKeys[i]; + NotificationEntry entry = mNotificationEntryManager.getPendingOrActiveNotif(key); + BubbleEntry bubbleEntry = entry != null + ? notifToBubbleEntry(entry) + : null; + boolean shouldBubbleUp = entry != null + ? mNotificationInterruptStateProvider.shouldBubbleUp(entry) + : false; + pendingOrActiveNotif.put(key, new Pair<>(bubbleEntry, shouldBubbleUp)); + } + mBubbles.onRankingUpdated(rankingMap, pendingOrActiveNotif); } /**