Fix/clarify refinement termination conditions

This primarily closes some gaps where our old implementation was
willing to leave refinement flows hanging in assorted edge cases.

As of this CL, every refinement flow ends in exactly one of:
 A) the flow is cancelled by `ChooserRefinementManager.destroy()` (e.g.
    we do this when `ChooserActivity` is closing);
 B) `ChooserRefinementManager` fires its configured success callback;
 C) `ChooserRefinementManager` fires its configured failure callback.

After any of those outcomes, the refinement manager cleans up its own
state (i.e., clients are no longer responsible for calling `destroy()`
during routine handling of their callbacks), and I've noted in an
implementation comment that we can use the nullability of
`ChooserRefinementManager.mRefinementResultReceiver` as an indication
of whether there's an active refinement session. These receivers are
always destroyed at the end of the session, and can never receive more
than one result callback in a single session. I've added a `@UiThread`
annotation to the entire class to clarify the concurrency model, so
it's easy to reason about the synchronized computation in this class.

A subsequent CL will expose this "active session" condition in the
refinement manager API so that `ChooserActivity` can detect the case
when the refinement activity closes with no result. `ChooserActivity`
also needs to stop suppressing its own termination when a refined
launch fails due to the target being suspended. Then we'll be able to
assert that chooser always finishes after *any* attempted refinement.

Bug: 273864843
Test: manually tested with a custom refinement activity to trigger
all the failure cases under `ChooserRefinementManager` responsibility.
As described above, `ChooserActivity` needs to handle some others
external to the refinement manager; those are punted from this CL.

Change-Id: Iee399e34dec27b9a846ea040831e23fafda46fd5

2 files changed, 36 insertions, 29 deletions

diff --git a/java/src/com/android/intentresolver/ChooserActivity.java b/java/src/com/android/intentresolver/ChooserActivity.java
index 2a73c42a..30ec2b91 100644
--- a/java/src/com/android/intentresolver/ChooserActivity.java
+++ b/java/src/com/android/intentresolver/ChooserActivity.java
@@ -279,10 +279,7 @@ public class ChooserActivity extends ResolverActivity implements
                         finish();
                     }
                 },
-                () -> {
-                    mRefinementManager.destroy();
-                    finish();
-                });
+                this::finish);
 
         mChooserContentPreviewUi = new ChooserContentPreviewUi(
                 mChooserRequest.getTargetIntent(),
diff --git a/java/src/com/android/intentresolver/ChooserRefinementManager.java b/java/src/com/android/intentresolver/ChooserRefinementManager.java
index 23cf6ba0..72b5305d 100644
--- a/java/src/com/android/intentresolver/ChooserRefinementManager.java
+++ b/java/src/com/android/intentresolver/ChooserRefinementManager.java
@@ -17,6 +17,7 @@
 package com.android.intentresolver;
 
 import android.annotation.Nullable;
+import android.annotation.UiThread;
 import android.app.Activity;
 import android.content.Context;
 import android.content.Intent;
@@ -25,7 +26,6 @@ import android.content.IntentSender.SendIntentException;
 import android.os.Bundle;
 import android.os.Handler;
 import android.os.Parcel;
-import android.os.Parcelable;
 import android.os.ResultReceiver;
 import android.util.Log;
 
@@ -41,6 +41,7 @@ import java.util.function.Consumer;
  * convert the format of the payload, or lazy-download some data that was deferred in the original
  * call).
  */
+@UiThread
 public final class ChooserRefinementManager {
     private static final String TAG = "ChooserRefinement";
 
@@ -51,7 +52,7 @@ public final class ChooserRefinementManager {
     private final Consumer<TargetInfo> mOnSelectionRefined;
     private final Runnable mOnRefinementCancelled;
 
-    @Nullable
+    @Nullable    // Non-null only during an active refinement session.
     private RefinementResultReceiver mRefinementResultReceiver;
 
     public ChooserRefinementManager(
@@ -98,7 +99,10 @@ public final class ChooserRefinementManager {
                         mOnRefinementCancelled.run();
                     }
                 },
-                mOnRefinementCancelled,
+                () -> {
+                    destroy();
+                    mOnRefinementCancelled.run();
+                },
                 mContext.getMainThreadHandler());
 
         Intent refinementRequest = makeRefinementRequest(mRefinementResultReceiver, selectedTarget);
@@ -114,7 +118,7 @@ public final class ChooserRefinementManager {
     /** Clean up any ongoing refinement session. */
     public void destroy() {
         if (mRefinementResultReceiver != null) {
-            mRefinementResultReceiver.destroy();
+            mRefinementResultReceiver.destroyReceiver();
             mRefinementResultReceiver = null;
         }
     }
@@ -151,7 +155,7 @@ public final class ChooserRefinementManager {
             mOnRefinementCancelled = onRefinementCancelled;
         }
 
-        public void destroy() {
+        public void destroyReceiver() {
             mDestroyed = true;
         }
 
@@ -161,27 +165,14 @@ public final class ChooserRefinementManager {
                 Log.e(TAG, "Destroyed RefinementResultReceiver received a result");
                 return;
             }
-            if (resultData == null) {
-                Log.e(TAG, "RefinementResultReceiver received null resultData");
-                // TODO: treat as cancellation?
-                return;
-            }
 
-            switch (resultCode) {
-                case Activity.RESULT_CANCELED:
-                    mOnRefinementCancelled.run();
-                    break;
-                case Activity.RESULT_OK:
-                    Parcelable intentParcelable = resultData.getParcelable(Intent.EXTRA_INTENT);
-                    if (intentParcelable instanceof Intent) {
-                        mOnSelectionRefined.accept((Intent) intentParcelable);
-                    } else {
-                        Log.e(TAG, "No valid Intent.EXTRA_INTENT in 'OK' refinement result data");
-                    }
-                    break;
-                default:
-                    Log.w(TAG, "Received unknown refinement result " + resultCode);
-                    break;
+            destroyReceiver();  // This is the single callback we'll accept from this session.
+
+            Intent refinedResult = tryToExtractRefinedResult(resultCode, resultData);
+            if (refinedResult == null) {
+                mOnRefinementCancelled.run();
+            } else {
+                mOnSelectionRefined.accept(refinedResult);
             }
         }
 
@@ -197,5 +188,24 @@ public final class ChooserRefinementManager {
             parcel.recycle();
             return receiverForSending;
         }
+
+        /**
+         * Get the refinement from the result data, if possible, or log diagnostics and return null.
+         */
+        @Nullable
+        private static Intent tryToExtractRefinedResult(int resultCode, Bundle resultData) {
+            if (Activity.RESULT_CANCELED == resultCode) {
+                Log.i(TAG, "Refinement canceled by caller");
+            } else if (Activity.RESULT_OK != resultCode) {
+                Log.w(TAG, "Canceling refinement on unrecognized result code " + resultCode);
+            } else if (resultData == null) {
+                Log.e(TAG, "RefinementResultReceiver received null resultData; canceling");
+            } else if (!(resultData.getParcelable(Intent.EXTRA_INTENT) instanceof Intent)) {
+                Log.e(TAG, "No valid Intent.EXTRA_INTENT in 'OK' refinement result data");
+            } else {
+                return resultData.getParcelable(Intent.EXTRA_INTENT, Intent.class);
+            }
+            return null;
+        }
     }
 }