From 9864ca7246e07d90c07717ec82c686d4f7faa02e Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Thu, 1 Dec 2022 11:07:33 -0500 Subject: Fix NPE from dependency initialization order This regression was caused by a botched rebase in ag/20455545 that caused the preview coordinator to be initialized immediately *after* the call up to `super.onCreate()`; it's a dependency for that method and needs to be initialized before (the lines got swapped in the attempted merge). I believe the regression was missed by our tests because of a race where the coordinator effectively gets initialized before any preview-loading tasks get dequeued and start relying on it. It's hard to verify a fix for that kind of flakiness, but fortunately it's obvious how this regression was introduced and why this fixes it. Post-mortem notes: 1. This is a "billion dollar mistake" -- a lot of our dependencies are nullable in the activities because we're unable to initialize them prior to onCreate(). Some options: a. Move these dependencies (and *all* logic that depends on them) into some controller class to decouple from the activity lifecycle. We can instantiate the one controller in onCreate(), but then internally all dependencies are final/non-null. (This would be a big improvement in our design, but the required changes are pretty heavyweight.) b. Formalize our helper for lazy computation so that dependencies never come up "null" simply because we've forgotten to initialize them. This is a more complex design than one where we just initialize everything we need, and it's not necessarily the correct model for all our problems, but it's easier to get right than the current late-init/nullability model. c. Integrate Dagger and generally enjoy an easier time managing dependencies (including a nice formal model for lazy init, if we want). This is probably something we want anyways so that we can migrate away from our inheritance-based test infrastructure, and I expect we'll keep finding more reasons to want it. 2. This would've been less likely to happen if the coordinator was an explicit dependency of `ResolverActivity.onCreate()`; instead, `ChooserActivity.onCreate()` makes a super call partway through that encapsulates an unknown number of additional responsibilities, and before it returns, it makes several calls back down to `ChooserActivity` for other dependencies -- before the activity is finished "creating," and without any explicit inheritance contract to say what's expected of implementors. Activity inheritance causes us any number of other problems, and we're not going to be able to rely on it in the future (e.g. for my embedded share prototype, ag/20519681, which can't be implemented by an Activity). We need to start paying down tech debt around this contract, figuring out where the two activity implementations overlap/differ, and ultimately breaking their inheritance relationship. It might be a good start for us to add explicit parameters to `ResolverActivity.onCreate()` for any of the dependencies it currently delegates back down for. Bug: 260934576 Test: `atest UnbundledChooserActivityTest` -- and tested that this same command failed (flaky) w/o the fix. As noted above, it's hard to verify this as a fix (vs. flake), but the correctness of the change should be obvious. Change-Id: Ie57b551d7d2d013049e86daaba18c5ff4f4c569f --- java/src/com/android/intentresolver/ChooserActivity.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'java/src') diff --git a/java/src/com/android/intentresolver/ChooserActivity.java b/java/src/com/android/intentresolver/ChooserActivity.java index ad106bba..1c52d59f 100644 --- a/java/src/com/android/intentresolver/ChooserActivity.java +++ b/java/src/com/android/intentresolver/ChooserActivity.java @@ -147,7 +147,6 @@ import java.util.Objects; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.function.Consumer; -import java.util.function.Supplier; /** * The Chooser Activity handles intent resolution specifically for sharing intents - @@ -355,6 +354,12 @@ public class ChooserActivity extends ResolverActivity implements mChooserRequest.getTargetIntentFilter()), mChooserRequest.getTargetIntentFilter()); + mPreviewCoordinator = new ChooserContentPreviewCoordinator( + mBackgroundThreadPoolExecutor, + this, + this::hideContentPreview, + this::setupPreDrawForSharedElementTransition); + super.onCreate( savedInstanceState, mChooserRequest.getTargetIntent(), @@ -364,12 +369,6 @@ public class ChooserActivity extends ResolverActivity implements /* rList: List = */ null, /* supportsAlwaysUseOption = */ false); - mPreviewCoordinator = new ChooserContentPreviewCoordinator( - mBackgroundThreadPoolExecutor, - this, - this::hideContentPreview, - this::setupPreDrawForSharedElementTransition); - mChooserShownTime = System.currentTimeMillis(); final long systemCost = mChooserShownTime - intentReceivedTime; -- cgit v1.2.3-59-g8ed1b