Fix crash on double rotation with modal

[mkevins]

Fixes #10227, #10920

WIP - Do Not Merge Yet

Related PRs

previous gutenberg-mobile: wordpress-mobile/gutenberg-mobile#1288
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#2011
gutenberg: WordPress/gutenberg#20860

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: #10227 (comment) and here: #10227 (comment)

Update release notes:

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

[mkevins]

I've updated the PR to target develop on WordPress-Android (and likewise for gutenberg-mobile submodule), rather than the last release tag.

The approach used here prevents the crash, and also prevents the leaked context. That one was a bit elusive at first, but I believe I've narrowed it down to the following chain of references:

GutenbergContainerFragment -> mWPAndroidGlueCode -> mReactInstanceManager -> mCurrentActivity

So, the leak occurs when the activity is destroyed, but the fragment still holds its reference. I believe this reference would be lost after a new activity is launched, and the fragment's onResume method is called, which calls through the onResumes on the same chain above. At that point, the ReactInstanceManager will have a new mCurrentActivity, and the previous reference is lost, and the underlying object can be garbage collected. However, the mDefaultBackButtonImpl is never set to null, and the synchronized moveToBeforeLifecycleState is never called, so some signalling to other threads is lost.

Calling onHostDestroy when the fragment detaches solves this, but I've encountered another issue as a result. The blockList's isBlockInsertionPointVisible flag is never falsified, thought he modal is effectively dismissed on rotation. This results in the "----ADD--BLOCK--HERE----" being rendered from renderAddBlockSeparator, and it doesn't go away until the add block button is tapped twice.

Add block separator still showing:

[cameronvoell]

Found another existing issue after the fix. If you open a BottomSheet menu from Image Options, or from new image or new video upload, and then do a double rotate, there is no longer a crash, but that same BottomSheet will no longer open. Inserter is the exception here, it will open. Verified for BottomSheet opened from image options (gear button), upload a new image, or upload a new video. See video from image options BottomSheet modal below:
Tested adding this PR onto WordPress develop 78f11d9 and gutenberg-mobile release/1.11.0 branch

[peril-wordpress-mobile]

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

[mkevins]

Hey @cameronvoell 👋

I'm circling back to this fix for the double-rotation crash. I've updated this PR (and the related gutenberg-mobile PR) to target the latest develop branch. It seems this still fixes the issue, but the described "side effects" are also still present.

there is no longer a crash, but that same BottomSheet will no longer open. Inserter is the exception here

One thing I noticed is that the ADD IMAGE button stops working as well, but starts working again after tapping elsewhere. I'm not sure why that's the case, but maybe could provide a clue. On the contrary, when the rotation occurs for the gear icon BottomSheet, it seems the gear button won't work (even after tapping elsewhere). But, if I tap the block inserter ("+" button) twice to bring up that BottomSheet, then dismiss it, the gear starts working again. 🤷‍♂️

Also, there is another issue that I have confirmed is related to the double rotation crash: #10920, and this PR seems to fix that as well.

I think the approach in this PR is worth pursuing, but I think we need more context (e.g. the intent / direction introduced in this PR: #9030), and whether there are other alternative solutions that do not have these side effects.

@hypest 👋 do you have any thoughts / guidance on the approach here? I'm not sure I have the complete context on the earlier PR to retain gutenberg on orientation changes.

One other thing I've tried is to add supportedOrientations={ [ 'portrait', 'landscape' ] } to packages/components/src/mobile/bottom-sheet/index.native.js, but this did not seem to have any effect.

[cameronvoell]

I think the approach in this PR is worth pursuing, but I think we need more context

@mkevins I agree this PR is worth pursuing some more.

ADD IMAGE button stops working as well, but starts working again after tapping elsewhere.

I agree this is a good clue. Other interesting clues could be traced from determining why we're getting incorrect focus on rotate gb-mobile 1343(without modals open), and the fact that the rotation crash does not happen on pre API 28 devices.

From my previous investigation into gutenberg rotation issues on Android, it seems to me that this will either be fixed with A) a small strategic incremental fix similar to the changes in this PR so far, or if some max number of dev hours searching for a smaller strategic fix are not fruitful, then B) rotation issues should be approached collaboratively with input from @hypest or others who worked on it in the past, perhaps starting with a post summarizing current issues with rotation on Android (gb-mobile 1343, #10700, gb-mobile 1333), paths attempted for fixing in the past (#8800, #8874), and rough time estimates for testing potential solutions.

As far as potential solutions go, I think #8874 looks promising, but that direction is such a rabbit hole that I think input from @hypest or others on those past PR's would be essential for not duplicating a bunch of learnings from a year ago.

I'm happy to take another pass at this. Perhaps one more solid pass on this from each of us will yield a path for fixing this incrementally, or enough evidence to make us feel justified in proposing plan B) above with options for larger changes on the table.

[malinajirka]

@mkevins It seems there hasn't been any activity for more than two months. We might want to consider assigning it to Groundskeeping project board if it's still worth looking into. Wdyt?

[mkevins]

Hi @malinajirka 👋

@mkevins It seems there hasn't been any activity for more than two months. We might want to consider assigning it to Groundskeeping project board if it's still worth looking into. Wdyt?

Is it typical to have a PR on the board as well as the issue(s)? In any case, feel free to add. I'm unsure whether the approach used here has any blockers, or whether it has the potential to introduce other issues (or performance considerations?). Perhaps @hypest has some insight about that. Thanks for following up Jirka 👍 .

[cameronvoell]

@mkevins In my last commit while updating this branch with develop, I updated the gutenberg-mobile reference to my branch seen in this gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2011.

That branch has identical changes to your branch from wordpress-mobile/gutenberg-mobile#1288, except I added some additional code for fixing the side effects that we were seeing related to modals not opening after rotation. I figured I'd leave your old PR intact in case there were concerns with my approach in wordpress-mobile/gutenberg-mobile#2011, but still reuse this PR since it has good context and my updates require the exact same code here.

[cameronvoell]

@mkevins I also tried to address some of @hypest questions regarding these changes here. Feel free to add anything you think I missed.

I feel more confident now that this PR has moved past WIP into ready to review, so will mark the PR as such. Let me know if you have any concerns.

[mkevins]

I figured I'd leave your old PR intact in case there were concerns with my approach in wordpress-mobile/gutenberg-mobile#2011, but still reuse this PR since it has good context and my updates require the exact same code here.

Sounds good to me! Also, for future note, for the situation on the gutenberg-mobile PR, another way could have been to target the original draft PR with the additional changes, and merge them there too. No need for that at this point though, and I'm mainly making this comment as a mental note to myself to close the draft PR without merging once the superceding gutenberg-mobile PR is merged 😄

Feel free to add anything you think I missed.

AFAICT, you covered everything very nicely. Thanks for the great work in investigating and finding a solution to the other remaining issues we observed!

[cameronvoell]

Tested via wordpress-mobile/gutenberg-mobile#2011. Looks good!

[cameronvoell]

Thanks @mkevins for kicking off this fix!

[mkevins]

Thanks @mkevins for kicking off this fix!

Thanks @cameronvoell for seeing it through!