Fix crash on double rotation with modal

[mkevins]

Fixes wordpress-mobile/WordPress-Android#10227

WIP - Do Not Merge Yet

Related PRs

WordPress-Android: wordpress-mobile/WordPress-Android#10383

Description

Since GutenbergContainerFragment is retained, it lives outside of the activity lifecycle. This results in a change in the way the fragment lifecycle methods are called with respect to the activity lifecylcle. Specifically, onDestroy is not called when an activity is re-created.

The GutenbergContainerFragment instantiates and keeps a reference to a WPAndroidGlueCode, which, in turn, instantiates and keeps a reference to a ReactInstanceManager, all of which live outside the activity lifecycle. When the fragment is destroyed, the ReactInstanceManager's onHostDestroy method is called with the current activity, to perform cleanup. However, this does not occur when the fragment is retained, as it is only called through from the fragment's onDestroy method.

This PR overrides the fragment's onDetach method, calling through to a new onDetach method on the WPAndroidGlueCode. That method performs the cleanup with the activity being destroyed (e.g. during an orientation change).

This approach works, but I think the current state could be cleaned up, thus, this PR is only a draft, with the intent to gather more context. It may be that we can simplify this to changing the fragment to directly call WPAndroidGlueCode's onDestroy method, rather than creating the method of the same name there.

To test:

Follow steps here: wordpress-mobile/WordPress-Android#10227 (comment) and here: wordpress-mobile/WordPress-Android#10227 (comment)

Update release notes:

• If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

[hypest]

👋 @mkevins, gave this solution a go and seems to work OK. Thing is, I am not sure what's the full set of assumptions ReactInstanceManager.onHostDestroy() assumes, and it seems that we need to dive into the implementation to understand what's going on. In particular, the question to answer would be: does the onHostDestroy() lead to destroying the React context that holds the app and the data? If yes, under which circumstances?

Obviously, what we want to avoid here is the case where the user rotates the device and data is lost. Trivial testing (tried undo redo history too) shows that data is preserved, but it'd be better if we get to the bottom of this. Can you drill in when you get the chance?

[mkevins]

@mkevins, gave this solution a go and seems to work OK. Thing is, I am not sure what's the full set of assumptions ReactInstanceManager.onHostDestroy() assumes, and it seems that we need to dive into the implementation to understand what's going on. In particular, the question to answer would be: does the onHostDestroy() lead to destroying the React context that holds the app and the data? If yes, under which circumstances?

Obviously, what we want to avoid here is the case where the user rotates the device and data is lost. Trivial testing (tried undo redo history too) shows that data is preserved, but it'd be better if we get to the bottom of this. Can you drill in when you get the chance?

@hypest 👋 I somehow missed this ping. Cameron's comment here sums up the lifecycle methods nicely. In short, the ReactInstanceManager.onHostDestroy method won't destroy the React context, but instead nullifies the instance's reference to the (soon to be destroyed) Activity.

[SergioEstevao]

@mkevins do we still need this PR?

[mkevins]

@mkevins do we still need this PR?

Looks like this was superseded by #2011, so I'll close this. Thanks for the reminder!