Issue/fix lifecycle crash with appcompat 1 1 0

[khaykov]

Related to #9905

CC @oguzkocer @malinajirka @marecar3

This PR fixes a couple of crashes that resulted from a change in fragment lifecycle behavior introduced in appcompat 1.1.0.

I'm targeting this fix towards the feature branch, since develop branch will need other changes related to migration to the new appcompat library.

The crash happens when we try to access a fragment's layout in onSaveInstanceState during configuration change, while it's in a backstack. I have a feeling we were using the lifecycle bug as a feature :)

Luckily, I was only able to find two places where we do this - in Login and Site Creation flows.

After looking at the code that causes all this jazz, I made a conclusion that it might be redundant, and here is why:

With Login Flow, we were trying to preserve the state of Gravatar loading progress. I don't think we need to do it since the progress indicator is visible when the view is created and is hidden when Glide loads cashed image or request fails. All this happens in onCreateView and we don't need to toggle it manually.

With Site Creation flow, I removed the explicit saving/restoring of the LinearLayoutManager. I don't have enough context to assume what it was used for, but I assume to preserve the scrolling state? If so it's already preserved during a normal configuration change. If it's something else, maybe @malinajirka can correct me.

If you think you might know other places where this issue might happen, please let me know!

To test (Login):

• Go to an email login flow, at the Magic Link request screen, choose to enter the password.
• At the Password input screen, rotate your device twice.

To test (Site Creation):

• Go through site creation flow, while rotating your device multiple times at each screen.

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

Messages

📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPress-Login-Flow-Android. Follow these handy steps! WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history! cd WordPress-Android git checkout issue/fix-lifecycle-crash-with-appcompat-1-1-0 git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/11292 Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/11292 and open a new PR.

Generated by 🚫 dangerJS

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[malinajirka]

The crash happens when we try to access a fragment's layout in onSaveInstanceState during configuration change, while it's in a backstack. I have a feeling we were using the lifecycle bug as a feature :)

AFAIU the changes in the support library, we basically can't rely on any of "onCreateView, onStart, onResume, onPause, onStop," being invoked before onSaveInstanceState, right? So technically if we for example initialize a field in onResume and we want to save that field in onSaveInstanceState, we need to check it's not null, correct?

I think this was never a bug in the lifecycle. Even the documentation states that onStop is invoked before onSaveInstanceState

If called, this method will occur after onStop() for applications targeting platforms starting with Build.VERSION_CODES.P. For applications targeting earlier platform versions this method will occur before onStop() and there are no guarantees about whether it will occur before or after onPause().

They just decided to change this behavior now, since they can't manage ViewModelStores without it. (btw another great decision - making the lifecycle even more complicated is the way to go :D)

But I guess it doesn't matter. The important part is that they are changing this behavior intentionally -> they plan to keep it.

I'm targeting this fix towards the feature branch, since develop branch will need other changes related to migration to the new appcompat library.

Can you please elaborate on the "other changes" part to make sure we are on the same page, thanks!

With Login Flow, we were trying to preserve the state of Gravatar loading progress. I don't think we need to do it since the progress indicator is visible when the view is created and is hidden when Glide loads cashed image or request fails. All this happens in onCreateView and we don't need to toggle it manually.

👍 I believe you are right;)

With Site Creation flow, I removed the explicit saving/restoring of the LinearLayoutManager. I don't have enough context to assume what it was used for, but I assume to preserve the scrolling state? If so it's already preserved during a normal configuration change. If it's something else, maybe @malinajirka can correct me.

Yeah, I believe that was the reason for this logic. If I recall correctly LayourManager didn't save it's state some time ago. It apparently does now, so I agree we can just remove the saving logic.

I have reviewed the code and it LGTM. I tested the site creation flow and the "magic link request" screen and they work as intended. Thanks @khaykov !

[oguzkocer]

Since I didn't look into appcompat changes when I worked on the inflate exception issue, I am going to leave this to people who have experience with it. However, please feel free to ping me if you need another set of eyes.

[khaykov]

@malinajirka thanks for confirming the behavior!

AFAIU the changes in the support library, we basically can't rely on any of "onCreateView, onStart, onResume, onPause, onStop," being invoked before onSaveInstanceState, right? So technically if we for example initialize a field in onResume and we want to save that field in onSaveInstanceState, we need to check it's not null, correct?

I think this was never a bug in the lifecycle. Even the documentation states that onStop is invoked before onSaveInstanceState

Yes, we can only rely on onCreate. And by us using a bug, I meant that we exploited the fact that the fragment's view was never destroyed when it entered back stack and went through lifecycle changes.

Can you please elaborate on the "other changes" part to make sure we are on the same page, thanks!

Sure, there are some semantical changes in method signatures and nullability of variables. I believe this closed PR has all of them.

[malinajirka]

Yes, we can only rely on onCreate. And by us using a bug, I meant that we exploited the fact that the fragment's view was never destroyed when it entered back stack and went through lifecycle changes.

My understanding based on @marecar3 bug report was that the difference between 1.0.0 and 1.1.0 is that

• in 1.0.0 - fragment on backstack is destroyed but not recreated during a configuration change
• in 1.1.0 .- fragment on backstack is destroyed and recreated during a config change even though it's not visible at all -> this results in "onSaveInstanceState being called during a second configuration, which we don't expect (we were following the docs) since this instance of the fragment was never visible -> in other words it's onCreateView was never called.

Either case, I don't think it changes anything. I just want to understand the new behavior.

I'll double check other onSaveInstanceState methods to increase the chance that we don't miss anything.

[malinajirka]

I've tried to go over all the places where we add a fragment to the fragments back stack. It's almost impossible to review all the places. For example the app can crash on double rotation wherever we show FullScreenDialogFragment - if the fragment over which is the dialog shown touches views/adapters in the onSaveInstanceState.
I still hope I don't understand this change correctly, because if I do, this is the worst decision Google has made in a long time. Everyone would need to go over all the places where onSaveInstanceState is used and double check it doesn't touch anything related to the view (adapters, views etc). That's basically impossible to do. Moreover, this crash occurs only if the following conditions are met

• a configuration change happens twice
• there is a fragment on fragments backstack (if the whole activity is on activity backstack, it doesn't crash)
• onSaveInstanceState assumes a field is initialized and the initialisation was supposed to happen in onCreateView/onStart/onPause/onStop

:head-explode:

[khaykov]

FullScreenDialogFragment should not cause the crash since we don't add underlying fragment into the backstack.

I'm also trying to wrap my head around this problem, and there is one thing that bothers me - when you add a fragment to back stack, it goes through the lifecycle until onDestoryView is called. In theory, this means, that we should not be able to access the view in onSaveInstanceState since it would be destroyed, but we can still do it until second rotation. onDestoryView after entering backstack also points to the fact that we should not have touched views in onSaveInstanceState :)

[malinajirka]

FullScreenDialogFragment should not cause the crash since we don't add underlying fragment into the backstack.

Tbh I'm not sure how the OS behaves when you add a fragment into android.R.id.content, but I believe we add it to the back stack -

WordPress-Android/WordPress/src/main/java/org/wordpress/android/ui/FullScreenDialogFragment.java

Line 211 in 97fe60b

return transaction.add(android.R.id.content, this, tag).addToBackStack(null).commit();

. Wdyt?

I'm also trying to wrap my head around this problem, and there is one thing that bothers me - when you add a fragment to back stack, it goes through the lifecycle until onDestoryView is called. In theory, this means, that we should not be able to access the view in onSaveInstanceState since it would be destroyed, but we can still do it until second rotation. onDestoryView after entering backstack also points to the fact that we should not have touched views in onSaveInstanceState :)

I'm not sure I follow, but onSaveInstanceState is called before onDestroyView. Therefore it's fine if we touch the views there during the first rotation. It becomes an issue during the second rotation, since the views weren't even created.

[khaykov]

Tbh I'm not sure how the OS behaves when you add a fragment into android.R.id.content, but I believe we add it to the back stack. Wdyt?

I tested the behavior of FullScreenDialogFragment a bit and looks like it does not cause the issue because of two things:

• We are using different fragment containers.
• We are using add() instead of replace().

So adding the transaction to backstack does not affect the underlying fragment.

So even if we add the FullScreenDialogFragment and underlying fragment to the same container (which is a pretty weird thing to do by itself), as long as we use add() instead of replace() we in a clear.

I'm not sure I follow, but onSaveInstanceState is called before onDestroyView. Therefore it's fine if we touch the views there during the first rotation. It becomes an issue during the second rotation, since the views weren't even created.

When you add a fragment to backstack onSaveInstanceState is not called, but onDestroyView is called. Since onDestroyView is called the moment fragment goes into backstack, it's view should stop existing, and should not be accessible even on first orientation change (when onSaveInstanceState will be called).

[malinajirka]

Ohh, thanks for the clarification!

LGTM! ;)