fix: iOS crash occurring when navigating to a new app screen with a displaying modal

[zhouzh1]

Summary:

Our app is using the react-native v0.74.2 with the react-navigation lib for screen navigation, we're facing an issue in the built iOS app that when we try to navigate to a new app screen with the react-navigation's reset or replace method and meanwhile there's a react native modal displaying, then the iOS app always crashes.

I saw there is already a relevant PR and discussion targeting this issue, but I still think it would be better if this kind of crash can be suppressed in the framework level, currently I guess it's common in the iOS apps based on react native.

Changelog:

[IOS] [FIXED] - app crash happening when navigate to a new app screen with a displaying modal

Test Plan:

More issue details and the reproduction steps can be found in this PR :)

[facebook-github-bot]

Hi @zhouzh1!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[zhouzh1]

@mojodna @jsierles @augustl could you help take a review? Thanks.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[zhouzh1]

Anyone can help take a look or provide some insights about if this change is safe or not?

[mrand15]

This issue is causing multiple crashes for us in production every day. Would appreciate if a fix like this could be merged. We use reset/replace actions when following deep links. No good way to know if the user has a modal open when we do so.

Tested applying the suggested fix manually and it seems to work without issue.

[zhouzh1]

@mrand15 Glad to hear that it's working for you, I already applied this fix in our production app for a long while, seems that this issue has been resolved and this change didn't introduce other new issues.
@cortinico Could you help take a look please?

[cipolleschi]

I can see that this is a fix for the old architecture only. Can I ask you to work also on the New Architecture?

The twin file should be this one: RCTModalHostViewComponentView.

[cipolleschi]

Sorry, I misclicked on 'Approve'.
We try not to accept fixes for the Old Architecture only. We would like at least to have the same fix in the New Arch or have a check that the New Arch does not suffer from the same issue.

[zhouzh1]

@cipolleschi Sorry that our app hasn't started to apply the new React native architecture, so I am not sure if it's suffering from this issue as well, but I took a look at the above RCTModalHostViewComponentView file that you posted, I guess it's no problem, because in this file, the _shouldPresent variable replaces the _visible variable in the old architecture, and there is already such a process similar to what this fix PR does.

[zhouzh1]

@cipolleschi Could you help take a look again? We're going to upgrade the react native in our project, if this fix makes sense, then we may don't need the extra patch file for react native.

[cipolleschi]

thanks for fixing! Which version of React Native are you using?

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@cipolleschi merged this pull request in 52888c0.

[react-native-bot]

This pull request was successfully merged by Zhi Zhou in 52888c0

^{When will my fix make it into a release? | How to file a pick request?}

[cipolleschi]

@zhouzh1 which version are you using? can you create a pick request for the versions that needs the fix?

[gkasireddy202]

@cipolleschi - My project has upgraded to react-native:0.73.7 3 months back.I raised the issue #44612 .
Can I add this line [self setVisible: NO]; below the code for a temporary fix or Can I pick a request to react-native:0.73.7 to fix issue #44612 ?
node_modules>react-native>React>Views>RCTModalHostView.m
We will upgrade my project to the latest react-native version once the client approves the newest version.

(void)dismissModalViewController
{
if (_isPresented) {
[_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
_isPresented = NO;
[self setVisible: NO] // Add this line //
}
}

[cipolleschi]

yeah, 0.73 is still in the support window.
I created the cherry pick request: reactwg/react-native-releases#531
We need the same pick for other versions too, let me create them.

[gkasireddy202]

@cipolleschi - Thanks for your update.
reactwg/react-native-releases#531 release fix will resolve my issue #44612 ?

[cipolleschi]

@gkasireddy202 I don't know... I understood you were asking to pick the fix because you tested it and it fixed it... 😅

Can you try to:

1. add that line in your project
2. try to trigger the crash
3. report back whether it worked or not

[gkasireddy202]

@cipolleschi - I added that line to my project and tested it. I triggered the crash.
This is the issue that I faced in react-native:0.73.7.

#44612 (comment)

[github-actions]

This pull request was successfully merged by Zhi Zhou in 8ec6722.

^{When will my fix make it into a release? | How to file a pick request?}