From fb8126b8fc336d681ba2ded6214d90a6f113b92e Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Mon, 25 Sep 2023 17:29:32 +0000 Subject: For consistency, stop throttling "list ready" CBs. This change is best understood through the newly-introduced `testPostListReadyAtEndOfRebuild_` tests: 1. The `synchronous` and `stages` variants pass both before and after this change, to show the expected "normal" behavior. 2. The `queued` variant exercises the hypothetical bug that motivated this change; it only passes once the legacy "throttling" condition is removed in this CL. 3. The `skippedIfStillQueuedOnDestroy` variant covers a side-effect of the fix. The original implementation had logic for this "skipping" condition (and presumably would've passed this new test as-written), but because the fix relaxes the invariant where we can have only a single "active" callback instance at a time, I also had to introduce the new `mDestroyed` condition to effect the same high-level "cancellation" behavior. (This also addresses a minor theoretical bug where the "destroy" could race against our internal two-stage asynchronous flow, such that we'd end up posting the follow-up callback to the handler after we'd supposedly already been destroyed. I didn't think it was important to test for this bug separately from the other coverage of the `mDestroyed` condition.) -- This CL removes a "throttling" condition that was unneeded, but which could cause a hypothetical bug (reproducible in tests). The original condition prevented list-ready messages if we'd already had one posted in the queue (but not yet processed); however, there was no check that the messages had the same parameters to indicate "partial" vs. "complete" progress. Since the legacy mechanism favored the earlier messages, we could end up dropping the one-off "completion" message where our listener actually does work -- i.e., if we had to drop messages, this is exactly the one we would want *not* to drop. I believe this "throttling" mechanism was likely an optimization to support the legacy `ChooserTargetService` design; the simplified newer design requests fewer callbacks and so the throttling should rarely come into play (but presents a bigger risk whenever it might). Even in the older design, I suspect there would've been a risk of the same "dropped completion" bug. And, of course, it's nice to "simplify" by removing this condition, even if it *weren't* strictly harmful. Update: the second snapshot removes the old "callback removal" on destroy, since the new `mDestroyed` condition gives effectively the same behavior. Technically that's the only reason we depended on `Handler` and we could now switch to using an `Executor` or etc -- but I definitely want to keep that update to a separate CL. Bug: as described above Test: IntentResolverUnitTests, CtsSharesheetDeviceTest Change-Id: Ifda9dc9a8ac8512d241e15fe52f24c3dea5bd9e7 --- .../intentresolver/ResolverListAdapter.java | 29 +++++++++++----------- 1 file changed, 14 insertions(+), 15 deletions(-) (limited to 'java/src') diff --git a/java/src/com/android/intentresolver/ResolverListAdapter.java b/java/src/com/android/intentresolver/ResolverListAdapter.java index a243ac2a..4ad8d926 100644 --- a/java/src/com/android/intentresolver/ResolverListAdapter.java +++ b/java/src/com/android/intentresolver/ResolverListAdapter.java @@ -58,6 +58,7 @@ import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicBoolean; public class ResolverListAdapter extends BaseAdapter { private static final String TAG = "ResolverListAdapter"; @@ -82,6 +83,7 @@ public class ResolverListAdapter extends BaseAdapter { private final Set mRequestedLabels = new HashSet<>(); private final Executor mBgExecutor; private final Handler mMainHandler; + private final AtomicBoolean mDestroyed = new AtomicBoolean(); private ResolveInfo mLastChosen; private DisplayResolveInfo mOtherProfile; @@ -93,7 +95,6 @@ public class ResolverListAdapter extends BaseAdapter { private int mLastChosenPosition = -1; private final boolean mFilterLastUsed; - private Runnable mPostListReadyRunnable; private boolean mIsTabLoaded; // Represents the UserSpace in which the Initial Intents should be resolved. private final UserHandle mInitialIntentsUserSpace; @@ -546,17 +547,17 @@ public class ResolverListAdapter extends BaseAdapter { * @param rebuildCompleted Whether the list has been completely rebuilt */ void postListReadyRunnable(boolean doPostProcessing, boolean rebuildCompleted) { - if (mPostListReadyRunnable == null) { - mPostListReadyRunnable = new Runnable() { - @Override - public void run() { - mResolverListCommunicator.onPostListReady(ResolverListAdapter.this, - doPostProcessing, rebuildCompleted); - mPostListReadyRunnable = null; + Runnable listReadyRunnable = new Runnable() { + @Override + public void run() { + if (mDestroyed.get()) { + return; } - }; - mMainHandler.post(mPostListReadyRunnable); - } + mResolverListCommunicator.onPostListReady(ResolverListAdapter.this, + doPostProcessing, rebuildCompleted); + } + }; + mMainHandler.post(listReadyRunnable); } private void addResolveInfoWithAlternates(ResolvedComponentInfo rci) { @@ -772,10 +773,8 @@ public class ResolverListAdapter extends BaseAdapter { } public void onDestroy() { - if (mPostListReadyRunnable != null) { - mMainHandler.removeCallbacks(mPostListReadyRunnable); - mPostListReadyRunnable = null; - } + mDestroyed.set(true); + if (mResolverListController != null) { mResolverListController.destroy(); } -- cgit v1.2.3-59-g8ed1b