Move inject before onCreate for Activity

[bubenheimer]

DaggerAppCompatActivity.onCreate() currently looks like this:

    super.onCreate(var1);
    AndroidInjection.inject(this);

This causes a NullPointerException when a Fragment is added via the support FragmentManager and a configuration change occurs (e.g. screen rotation), and the Fragment is restored by the framework. The problem is that Fragment.onAttach() is then called within the Activity's onCreate(), where fields are not yet injected, but needed to inject the Fragment.

Instead, the order of the two statements needs to be flipped - injection needs to happen before super.onCreate().

I've created a sample app to demonstrate the issue: https://github.com/bubenheimer/daggerinitorderbug
The sample app uses my own builds of the Dagger artifacts, to work around other issues with RC1.

I don't know the behavior with framework Activities, as opposed to AppCompatActivities. I'd think it should be treated the same, at least to be consistent, but have not tested it.

I don't have a strong opinion on the order for DaggerFragment - the equivalent problem does not occur for support child Fragments (children of a parent Fragment) - also shown in my sample app. Injecting after 'super.onAttach()' works fine in all of my production code, too, but perhaps it would be better to flip the order to be consistent.

Seeing this issue in the code was what led me to spend my time on evaluating the RC, to get it changed before reaching release. Thanks for providing an RC!

[stanmots]

Thank you, @bubenheimer!

I've tried to integrate the compiled artifacts from your repo and it works like a charm even with Kotlin. In fact, this is the only workable solution I've found so far which makes it possible to play with the new Android Injection feature.

[bubenheimer]

In the meantime I've looked at the behavior when using framework Activity & Fragment classes instead of support classes, and the problem & fix are the same, on API level 24. I've added the code changes for this test to the "framework" branch of my Github example.

Also, to whoever decided to use my custom-built Dagger artifacts, consider grabbing the latest - I've refreshed the Android-related artifacts, and I noticed that I previously may have accidentally incorporated my suggested code change in the dagger-android artifact already (in DaggerActivity.class).

[ronshapiro]

Hm, I thought I responded to this but maybe not. I'm going to investigate this, and I hope to have an answer soon, but I have a few things on my plate right now. I regretfully should have tested configuration changes :(

[digitalbuddha]

Any issue with doing the fragment injection within onActivityCreated?

[bubenheimer]

I'd commonly want to access Dagger-injected values before onActivityCreated. You'd be skipping three major lifecycle methods.

[gk5885]

Yes, ideally we want to trigger members injection as close to object construction as possible. We don't suspect that there should be any issue with doing it before super.onCreate rather than after, but @ronshapiro is doing the due diligence to make sure that it's generally safe.

[bubenheimer]

I suppose the general concern should be one of minimizing problems for the default approach. There can be issues whichever order & timing we choose. I ran into problems in the past with doing injection before onCreate(), I guess because I was using aspects of the Activity class during injection that were waiting to be initialized via onCreate(). However, that's an issue tied to my own code structure that is more readily identified and resolved than standard framework problems.

That said I am more hesitant now to advocate placing Fragment injection before super.onAttach(), unless it solves a specific problem apart from consistency/confusion. In my own code I have generally had Fragments inject themselves after super.onAttach(). More recently I avoided self-injection for Fragments and Views entirely, by using Activity.onCreateView(), Activity.onAttachFragment, and Fragment.onAttachFragment for child fragments.

[stanmots]

Since you added AndroidInjectionBeforeSuper errorprone bug pattern the projects cannot be compiled now if the AndroidInjection.inject(this); is invoked after super.onCreate(savedInstanceState); activity lifecycle method. However, sometimes this is required behavior.

For example, consider RxPermissions injection. It uses the fragment transactions under the hood. Therefore, the exception Java.lang.IllegalStateException Activity has been destroyed will be thrown if we try to inject it before calling activity's onCreate method.

I know that it is possible to suppress the errorprone by @SuppressWarnings("AndroidInjectionBeforeSuper"). Just wanted to inform you about such use cases.

[ronshapiro]

@storix I'm not following what is happening in RxPermissions. Can you explain a bit more?

[tbroyer]

AFAICT, RxPermissions' “constructor does real work” and specifically uses the Activity passed as argument, therefore depending on its state.

@storix How about injecting a Lazy<RxPermissions> then? (but ideally RxPermissions class should move its initialization to an explicit lifecycle method; possibly providing a static factory for those that would always call that init/start method just after instantiating the class, or lazily instantiating the internal RxPermissionsFragment)

[stanmots]

@ronshapiro It uses the retained fragment to save/restore some important properties on configuration changes. When the activity is recreated it retrieves the fragment upon its object creation to restore the required fields so we can resubscribe to the permissions notifications right away.

Actually, you don't even need to analyze all the logic behind RxPermissions. You can just try to use the fragment manager in any activity like this:

public class MainActivity extends AppCompatActivity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        // Crashes app with `Activity has been destroyed` exception
        getSupportFragmentManager().beginTransaction().commit();

        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
    }
}

@tbroyer Yes, injecting a Lazy<RxPermissions> helped. However, I also encountered a similar problem with another object which calls getSupportLoaderManager(); under the hood. What's interesting about the loader manager case is that it doesn't crash the app immediately - only after several device rotations. It seems I should carefully analyze all third party libraries I use now to wrap the objects as lazy only when necessary.

Idk, maybe some warning should be added to the docs regarding such situations.

[ronshapiro]

Why are you injecting the RxPermissions object?

[stanmots]

I'm injecting it into the constructor of the presenter which manages all permissions related logic.