Skip to content

Fix crash on double rotation with modal#1288

Closed
mkevins wants to merge 2 commits intodevelopfrom
try/fix-double-rotation-modal-crash
Closed

Fix crash on double rotation with modal#1288
mkevins wants to merge 2 commits intodevelopfrom
try/fix-double-rotation-modal-crash

Conversation

@mkevins
Copy link
Copy Markdown
Contributor

@mkevins mkevins commented Aug 13, 2019 •
edited
Loading

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.

@mkevins mkevins mentioned this pull request Aug 13, 2019
Merged
1 task
@mkevins mkevins changed the base branch from develop to try/fix-double-rotation-modal-crash-temp-base August 13, 2019 08:46
@mkevins mkevins force-pushed the try/fix-double-rotation-modal-crash branch from 86e1b7f to 3d7c59f Compare August 14, 2019 04:46
@mkevins mkevins changed the base branch from try/fix-double-rotation-modal-crash-temp-base to develop August 14, 2019 04:46
@hypest
Copy link
Copy Markdown
Contributor

hypest commented Feb 21, 2020

👋 @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?

@cameronvoell cameronvoell mentioned this pull request Mar 14, 2020
Merged
2 tasks
@mkevins
Copy link
Copy Markdown
Contributor Author

mkevins commented Mar 17, 2020

@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
Copy link
Copy Markdown
Contributor

SergioEstevao commented May 25, 2020

@mkevins do we still need this PR?

@mkevins
Copy link
Copy Markdown
Contributor Author

mkevins commented May 26, 2020

@mkevins do we still need this PR?

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

@mkevins mkevins closed this May 26, 2020
@mkevins mkevins deleted the try/fix-double-rotation-modal-crash branch May 26, 2020 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bugfix [OS] Android

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Crash in block editor on double rotation when inserter is open

3 participants

@mkevins @hypest @SergioEstevao