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

1 files changed, 14 insertions, 15 deletions

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<DisplayResolveInfo> 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();
         }