From 7e145c5e1c4be62c47bee591d6e96eb361c9dfdc Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Thu, 23 Mar 2023 15:02:23 +0000 Subject: Skip refinement for suspended targets. It's just rude to send users into the refinement flow when we expect the launch to fail (with a message dialog indicating that the entire target app was never supposed to work; i.e., we make it clear to users that this failure had nothing to do with their refinement choices). OTOH if we skip refinement, we have some handling to bounce back into a usable Sharesheet session after that dialog appears, so we'd like to take advantage of that. A subsequent CL will "plug all the holes" in our handling of various refinement exit conditions. There's currently an analogous post-refinement "bounce back" for suspended targets, but we won't be able to recover a Sharesheet session at that time, so (TODO) we'll turn that into a hard failure (as with any other refinement problems). Bug: 273864843 Test: manually tested `adb shell cmd package suspend ` confirm we no longer invoke refinement (and instead trigger the system dialog immediately) when the suspended target is chosen. Change-Id: Iff1cf67f67015c754437726b8f52431deca797e0 --- java/src/com/android/intentresolver/ChooserRefinementManager.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/java/src/com/android/intentresolver/ChooserRefinementManager.java b/java/src/com/android/intentresolver/ChooserRefinementManager.java index 3ddc1c7c..23cf6ba0 100644 --- a/java/src/com/android/intentresolver/ChooserRefinementManager.java +++ b/java/src/com/android/intentresolver/ChooserRefinementManager.java @@ -77,6 +77,13 @@ public final class ChooserRefinementManager { if (selectedTarget.getAllSourceIntents().isEmpty()) { return false; } + if (selectedTarget.isSuspended()) { + // We expect all launches to fail for this target, so don't make the user go through the + // refinement flow first. Besides, the default (non-refinement) handling displays a + // warning in this case and recovers the session; we won't be equipped to recover if + // problems only come up after refinement. + return false; + } destroy(); // Terminate any prior sessions. mRefinementResultReceiver = new RefinementResultReceiver( -- cgit v1.2.3-59-g8ed1b From 64f9435ffbd82cf2f025aebc05795691aa5e1e08 Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Thu, 23 Mar 2023 15:54:59 +0000 Subject: 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 --- .../android/intentresolver/ChooserActivity.java | 5 +- .../intentresolver/ChooserRefinementManager.java | 60 +++++++++++++--------- 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 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; + } } } -- cgit v1.2.3-59-g8ed1b From 90aab48da1198be6a154412b3cd9a44cf97b7764 Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Thu, 23 Mar 2023 16:26:28 +0000 Subject: Always terminate Chooser after a refinement flow. In normal (non-refinement) target selection, if the user selects a suspended target, we attempt to launch it anyways in order to prompt a system message dialog, but we suppress our normal post-launch termination so that the user can try to select a different target. We had similar "bounce-back" logic if a post-refinement launch failed as a result of a suspended target, but the UX doesn't really make sense in the refinement case (and I'm also not 100% confident that our implementation would be technically equipped to handle the re-entry). I've removed the "bounce-back" suppression in the post-refinement case, so if we show the dialog about the package being suspended, that's the end of the session. (Although note, unless the package was suspended *during* refinement, we would've already shown the dialog instead of going to refinement in the first place.) Bug: 273864843 Test: manually tested behavior around suspended targets, and ran CTS to verify no regressions in normal refinement usage. Change-Id: I4e57add285224b7f311c4d0532168dc984ee331b --- java/src/com/android/intentresolver/ChooserActivity.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/java/src/com/android/intentresolver/ChooserActivity.java b/java/src/com/android/intentresolver/ChooserActivity.java index 30ec2b91..71a94e11 100644 --- a/java/src/com/android/intentresolver/ChooserActivity.java +++ b/java/src/com/android/intentresolver/ChooserActivity.java @@ -275,9 +275,13 @@ public class ChooserActivity extends ResolverActivity implements mChooserRequest.getRefinementIntentSender(), (validatedRefinedTarget) -> { maybeRemoveSharedText(validatedRefinedTarget); - if (super.onTargetSelected(validatedRefinedTarget, false)) { - finish(); - } + + // We already block suspended targets from going to refinement, and we probably + // can't recover a Chooser session if that's the reason the refined target fails + // to launch now. Fire-and-forget the refined launch; ignore the return value + // and just make sure the Sharesheet session gets cleaned up regardless. + super.onTargetSelected(validatedRefinedTarget, false); + finish(); }, this::finish); -- cgit v1.2.3-59-g8ed1b From aec171251864334d43c3fd14fe10f8bc9f45b535 Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Thu, 23 Mar 2023 17:05:01 +0000 Subject: Terminate refinement on back-out w/ no result Bug: 273864843 Test: manually tested back-out behavior with a custom refinement activity that calls `finish()` on launch. Ran CTS & `IntentResolverUnitTests` to confirm no regressions in the normal refinement flow. (Also noted that this condition *isn't* triggered when we finish after sending a valid RESULT_OK refinement. That's not really required for correctness, but it's still simpler/preferable that way, so it was a relief to confirm.) Change-Id: I165c958e3633430006b7977faa7617a3f87b1092 --- java/src/com/android/intentresolver/ChooserActivity.java | 9 +++++++++ .../com/android/intentresolver/ChooserRefinementManager.java | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/java/src/com/android/intentresolver/ChooserActivity.java b/java/src/com/android/intentresolver/ChooserActivity.java index 71a94e11..34278065 100644 --- a/java/src/com/android/intentresolver/ChooserActivity.java +++ b/java/src/com/android/intentresolver/ChooserActivity.java @@ -620,6 +620,15 @@ public class ChooserActivity extends ResolverActivity implements super.onResume(); Log.d(TAG, "onResume: " + getComponentName().flattenToShortString()); maybeCancelFinishAnimation(); + + if (mRefinementManager.isAwaitingRefinementResult()) { + // This can happen if the refinement activity terminates without ever sending a response + // to our `ResultReceiver`. We're probably not prepared to return the user into a valid + // Chooser session, so we'll treat it as a cancellation instead. + Log.w(TAG, "Chooser resumed while awaiting refinement result; aborting"); + mRefinementManager.destroy(); + finish(); + } } @Override diff --git a/java/src/com/android/intentresolver/ChooserRefinementManager.java b/java/src/com/android/intentresolver/ChooserRefinementManager.java index 72b5305d..8d7b1aac 100644 --- a/java/src/com/android/intentresolver/ChooserRefinementManager.java +++ b/java/src/com/android/intentresolver/ChooserRefinementManager.java @@ -66,6 +66,16 @@ public final class ChooserRefinementManager { mOnRefinementCancelled = onRefinementCancelled; } + /** + * @return whether a refinement session has been initiated (i.e., an earlier call to + * {@link #maybeHandleSelection(TargetInfo)} returned true), and isn't yet complete. The session + * is complete if the refinement activity calls {@link ResultReceiver#onResultReceived()} (with + * any result), or if it's cancelled on our side by {@link ChooserRefinementManager#destroy()}. + */ + public boolean isAwaitingRefinementResult() { + return (mRefinementResultReceiver != null); + } + /** * Delegate the user's {@code selectedTarget} to the refinement flow, if possible. * @return true if the selection should wait for a now-started refinement flow, or false if it -- cgit v1.2.3-59-g8ed1b