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
1 file changed