Possible bug in FragmentMvpDelegateImpl onDestroyView()

[epetrenko]

Hi there.

I think there is a possible bug in the latest Mosby release, when using custom DialogFragment, which has MVP structure and uses FragmentMvpDelegateImpl. MVP dialog is shown inside activity with it's own MVP. I have created a sample repo to help reproduce this issue.

Mosby Version: 3.0.0 Actual behavior: crash when closing fragment dialog with MVP Sample repository: https://github.com/epetrenko/MosbyFragmentMvpDelegateExample

There are two main parts in my sample repo:

1. Main screen (based on MvpActivity);
2. Login dialog with it's own presenter (extends from AppCompatDialogFragment and implements MvpDelegateCallback). Login dialog is shown from main screen by clicking on a button. It's done only for example.

Presenters and views are dummy in my example. Dagger stuff was added only as example.

LoginDialogFragment has FragmentMvpDelegateImpl:

public class LoginDialogFragment extends AppCompatDialogFragment implements LoginView,
        MvpDelegateCallback<LoginView, LoginPresenter> {

    LoginPresenter presenter;

    private FragmentMvpDelegate<LoginView, LoginPresenter> delegate =
            new FragmentMvpDelegateImpl<>(this, this, false, false);

     [...]
}

I don't need to preserve presenter neither during screen orientation nor on backstack. MainActivity is implemented only in portrait orientation.

Lifecycle methods are overriden in LoginDialogFragment:

    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        delegate.onCreate(savedInstanceState);
    }

    @Override
    public void onDestroy() {
        super.onDestroy();
        delegate.onDestroy();
    }

    @Override
    public void onPause() {
        super.onPause();
        delegate.onPause();
    }

    @Override
    public void onResume() {
        super.onResume();
        delegate.onResume();
    }

    @Override
    public void onDestroyView() {
        super.onDestroyView();
        delegate.onDestroyView();
    }

    @Override
    public void onViewCreated(View view, @Nullable Bundle savedInstanceState) {
        super.onViewCreated(view, savedInstanceState);
        delegate.onViewCreated(view, savedInstanceState);
    }

    @Override
    public void onStart() {
        super.onStart();
        delegate.onStart();
    }

    @Override
    public void onStop() {
        super.onStop();
        delegate.onStop();
    }

    @Override
    public void onAttach(Activity activity) {
        super.onAttach(activity);
        delegate.onAttach(activity);
    }

    @Override
    public void onDetach() {
        super.onDetach();
        delegate.onDetach();
    }

Application is crashed after onDestroyView() call on delegate. I'm not sure should onDestroyView() be overriden or not, because StatisticsDialog have no onDestroyView() overriding in mail sample from Mosby repo.

I checked onDestroyView() method implementation in FragmentMvpDelegateImpl:

  @Override public void onDestroyView() {

    onViewCreatedCalled = false;

    Activity activity = getActivity();
    boolean retainPresenterInstance = retainPresenterInstance();

    P presenter = getPresenter();
    presenter.detachView(retainPresenterInstance);
    if (!retainPresenterInstance) {
      PresenterManager.remove(activity, mosbyViewId);
    }

    [...]
  }

Method retainPresenterInstance() returns false (as I noticed above I don't need to keep presenter in all cases). Therefore PresenterManager.remove(activity, mosbyViewId) method should be fired.

Inside PresenterManager.remove(activity, viewId) method we have the following:

public static void remove(@NonNull Activity activity, @NonNull String viewId) {
    if (activity == null) {
      throw new NullPointerException("Activity is null");
    }

    ActivityScopedCache activityScope = getActivityScope(activity);
    if (activityScope != null) {
      activityScope.remove(viewId);
    }
  }

As activityScope isn't null, we go into activityScope.remove(viewId). And this method throws NullPointerException exception, because viewId is null.

If onDestroyView() overriding should not be used here, then all is working properly. Hope it helps.

[sockeqwe]

Thanks for this super helpful bug report! Indeed it seems like this is a bug in PresenterManager. I think it won't crash if you set keepPresenterInstance = true (constructor parameter of delegate)

Indeed you have to delegate delegate.onViewDestroyed(), so that is another bug of the demo app.

I will work on that right now, fix should be available soon.

[epetrenko]

@sockeqwe Thanks for a quick response.

You're right, there are no crashes if presenter should be kept.

[sockeqwe]

Is fixed, thanks for reporting.

Fix is available in 3.0.2-SNAPSHOT (building right now on CI, available in about 20 Minutes)

A stable 3.0.2 release to come tomorrow.

[sockeqwe]

3.0.2 has been released.

Thanks again for your very detailed bug report and sample repository to reproduce the bug (it was super helpful)!