From 3a0ac7d534fb3dda80de82a889511344850b2c3f Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Mon, 5 Jun 2023 17:38:47 +0000 Subject: Add a null check for a rare error case. This just addresses the immediate NPE and doesn't explain the root cause of the null value. I expect we *shouldn't* be seeing nulls, but in the context of the other legacy code it seems to make sense to take appropriate precautions against them anyways. This could've been a regression from the legacy code, since an older version used an `instanceof` check that would've correctly skipped over nulls. I've chosen the narrowest fix to reproduce that logic, because it's difficult to reason about other more-"elegant" potential improvements (as briefly noted in b/285565219). There's no obvious way to reproduce the null condition, so this isn't tested to "fix"/"work in" the underlying error case overall (although it should be clear from the code that this would prevent the specific NPE we have at hand). Instead the test runs for this CL just validate that the new null-check condition doesn't add regressions to our currently-tested cases (where the result is non-null). Bug: 285565219 Test: UnbundledChooserActivityTest, CtsSharesheetDeviceTest Change-Id: I3b367206b4eefad579dbf613dfd38f485720cf65 --- java/src/com/android/intentresolver/ChooserActivity.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'java') diff --git a/java/src/com/android/intentresolver/ChooserActivity.java b/java/src/com/android/intentresolver/ChooserActivity.java index 63ac6435..eb38ad54 100644 --- a/java/src/com/android/intentresolver/ChooserActivity.java +++ b/java/src/com/android/intentresolver/ChooserActivity.java @@ -883,7 +883,7 @@ public class ChooserActivity extends ResolverActivity implements final long selectionCost = System.currentTimeMillis() - mChooserShownTime; - if (targetInfo.isMultiDisplayResolveInfo()) { + if ((targetInfo != null) && targetInfo.isMultiDisplayResolveInfo()) { MultiDisplayResolveInfo mti = (MultiDisplayResolveInfo) targetInfo; if (!mti.hasSelected()) { // Add userHandle based badge to the stackedAppDialogBox. @@ -900,7 +900,17 @@ public class ChooserActivity extends ResolverActivity implements super.startSelected(which, always, filtered); - if (currentListAdapter.getCount() > 0) { + // TODO: both of the conditions around this switch logic *should* be redundant, and + // can be removed if certain invariants can be guaranteed. In particular, it seems + // like targetInfo (from `ChooserListAdapter.targetInfoForPosition()`) is *probably* + // expected to be null only at out-of-bounds indexes where `getPositionTargetType()` + // returns TARGET_BAD; then the switch falls through to a default no-op, and we don't + // need to null-check targetInfo. We only need the null check if it's possible that + // the ChooserListAdapter contains null elements "in the middle" of its list data, + // such that they're classified as belonging to one of the real target types. That + // should probably never happen. But why would this method ever be invoked with a + // null target at all? Even an out-of-bounds index should never be "selected"... + if ((currentListAdapter.getCount() > 0) && (targetInfo != null)) { switch (currentListAdapter.getPositionTargetType(which)) { case ChooserListAdapter.TARGET_SERVICE: getChooserActivityLogger().logShareTargetSelected( -- cgit v1.2.3-59-g8ed1b