Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added fix for app freeze issue when modal closes #32450

Closed
wants to merge 4 commits into from

Conversation

hurali97
Copy link
Collaborator

@hurali97 hurali97 commented Oct 21, 2021

Summary

As per the current version of react-native i.e, 0.66.0 , there is an issue when we close the modal, the whole app freezes. I looked at the code in RCTModalHostView, and the method named dismissModalViewController, seemed to be at fault, because it wasn't dismissing the view controller from the main thread, if I am not wrong. So I called the dismissal method from the main queue and voila, the app don't freeze anymore.

Other members are also facing this issue, so I tried to create a PR to get this fix in the main repo.
Issue: #32329

Changelog

[iOS] [Fixed] - There's one change in the file React/Views/RCTModalHostView.m/dismissModalViewController

Test Plan

Tested on react-native 0.66.0 , app don't freeze anymore after closing the modal

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 21, 2021
@facebook-github-bot
Copy link
Contributor

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

@pull-bot
Copy link

PR build artifact for 7808b8e is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@analysis-bot
Copy link

analysis-bot commented Oct 21, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,350,969 +0
android hermes armeabi-v7a 8,333,034 +0
android hermes x86 9,983,357 +0
android hermes x86_64 9,768,730 +0
android jsc arm64-v8a 10,665,547 +0
android jsc armeabi-v7a 9,311,086 +0
android jsc x86 10,777,605 +0
android jsc x86_64 11,216,905 +0

Base commit: 88ebec9
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 21, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 88ebec9
Branch: main

@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Oct 21, 2021
@hurali97
Copy link
Collaborator Author

Hi, Please let me know what's causing the test to fail, so I can resolve it.

Screenshot 2021-10-21 at 6 49 55 PM

React/Views/RCTModalHostView.m Outdated Show resolved Hide resolved
@pull-bot
Copy link

PR build artifact for b9b1718 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@patrickkempff
Copy link
Contributor

I have applied this patch and can confirm it solved our problem with freezing modals on react-native v0.66.1

@patrickkempff
Copy link
Contributor

patrickkempff commented Nov 5, 2021

I have applied this patch and can confirm it solved our problem with freezing modals on react-native v0.66.1

We have discovered that the issue is not fully fixed with the changes requested in this PR.

When we present a react-native modal on top of a view controller that is also modally presented, when dismissing the modal it dismisses the underlaying modal also. It seems that RCTModalHostView:dismissModalViewController is being called twice. We tried to add the if (_isPresented) { check also to the main queue call and that seems to work.

The following patch seems to solve the issue:

diff --git a/node_modules/react-native/React/Views/RCTModalHostView.m b/node_modules/react-native/React/Views/RCTModalHostView.m
index bea9e08..b3bdeb7 100644
--- a/node_modules/react-native/React/Views/RCTModalHostView.m
+++ b/node_modules/react-native/React/Views/RCTModalHostView.m
@@ -116,10 +116,12 @@ - (void)didUpdateReactSubviews

 - (void)dismissModalViewController
 {
-  if (_isPresented) {
-    [_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
-    _isPresented = NO;
-  }
+    dispatch_async(dispatch_get_main_queue(), ^{
+      if (_isPresented) {
+        [_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
+        _isPresented = NO;
+      }
+    });
 }

 - (void)didMoveToWindow

@hurali97
Copy link
Collaborator Author

hurali97 commented Nov 5, 2021

I have applied this patch and can confirm it solved our problem with freezing modals on react-native v0.66.1

We have discovered that the issue is not fully fixed with the changes requested in this PR.

When we present a react-native modal on top of a view controller that is also modally presented, when dismissing the modal it dismisses the underlaying modal also. It seems that RCTModalHostView:dismissModalViewController is being called twice. We tried to add the if (_isPresented) { check also to the main queue call and that seems to work.

The following patch seems to solve the issue:

diff --git a/node_modules/react-native/React/Views/RCTModalHostView.m b/node_modules/react-native/React/Views/RCTModalHostView.m
index bea9e08..b3bdeb7 100644
--- a/node_modules/react-native/React/Views/RCTModalHostView.m
+++ b/node_modules/react-native/React/Views/RCTModalHostView.m
@@ -116,10 +116,12 @@ - (void)didUpdateReactSubviews

 - (void)dismissModalViewController
 {
-  if (_isPresented) {
-    [_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
-    _isPresented = NO;
-  }
+    dispatch_async(dispatch_get_main_queue(), ^{
+      if (_isPresented) {
+        [_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
+        _isPresented = NO;
+      }
+    });
 }

 - (void)didMoveToWindow

@patrickkempff So you're trying to say, that we should move the if condition inside the callback for the queue, and it kind of makes sense as well. I will get this fixed, thanks for highlighting this mate.

@huextrat
Copy link

huextrat commented Nov 5, 2021

I have also applied this patch and it works well on react-native v0.66.0 ! Thanks for this PR @hurali97 !

This must be merged @feedthejim it's quite annoying to have modal that freezes the app 😏

@patrickkempff
Copy link
Contributor

I have applied this patch and can confirm it solved our problem with freezing modals on react-native v0.66.1

We have discovered that the issue is not fully fixed with the changes requested in this PR.
When we present a react-native modal on top of a view controller that is also modally presented, when dismissing the modal it dismisses the underlaying modal also. It seems that RCTModalHostView:dismissModalViewController is being called twice. We tried to add the if (_isPresented) { check also to the main queue call and that seems to work.
The following patch seems to solve the issue:

diff --git a/node_modules/react-native/React/Views/RCTModalHostView.m b/node_modules/react-native/React/Views/RCTModalHostView.m
index bea9e08..b3bdeb7 100644
--- a/node_modules/react-native/React/Views/RCTModalHostView.m
+++ b/node_modules/react-native/React/Views/RCTModalHostView.m
@@ -116,10 +116,12 @@ - (void)didUpdateReactSubviews

 - (void)dismissModalViewController
 {
-  if (_isPresented) {
-    [_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
-    _isPresented = NO;
-  }
+    dispatch_async(dispatch_get_main_queue(), ^{
+      if (_isPresented) {
+        [_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
+        _isPresented = NO;
+      }
+    });
 }

 - (void)didMoveToWindow

@patrickkempff So you're trying to say, that we should move the if condition inside the callback for the queue, and it kind of makes sense as well. I will get this fixed, thanks for highlighting this mate.

Awesome!

@katsimoto
Copy link

@hurali97, thanks for PR!

@feedthejim, please, merge it, it's very annoying 🙏🏻

@msaqlain
Copy link

Working fine now. Thanks for the PR @hurali97

Please merge this into main repo!!

@katsimoto
Copy link

katsimoto commented Nov 12, 2021

It looks like there is another problem with the modal window.

Scenario:

  1. Call the modal window and then close it.
  2. UI does not freeze, everything fine (with this fix).
  3. Use navigation.navigate and move to another screen.
  4. Use navigation.goBack and move back.
  5. UI freeze now.

Ugly workaround: use StackActions.replace on the same route after closing the modal.

@katsimoto
Copy link

And one more issue with this fix.

Scenario:

  1. Call the modal window and then close it (change visible props).
  2. Call the modal window again.
  3. Modal not show and UI freeze now.

@hurali97
Copy link
Collaborator Author

And one more issue with this fix.

Scenario:

  1. Call the modal window and then close it (change visible props).
  2. Call the modal window again.
  3. Modal not show and UI freeze now.

Hi, This is strange. Because I have tried your both use cases, but doesn't seem to repro them. Can you try to show the code.

@katsimoto
Copy link

katsimoto commented Nov 14, 2021

Hi, This is strange. Because I have tried your both use cases, but doesn't seem to repro them. Can you try to show the code.

Hi. Ok, I'll try to create a minimally reproducible example, but later, I'm busy right now, sorry. I will write about the result here or create a new issue.

I have reproduced this on a real device, iPhone 13 Pro, if that helps.

@reaperdtme
Copy link

+1. This fixes Modal dismiss issues for iOS on react-native 0.67.0-rc3

@lucis
Copy link

lucis commented Jan 9, 2022

I can confirm this fixes the most basic use case of closing a modal, but somehow I can still experience the problem of app freezing (or at least modals not working anymore, even after killing the app). Still haven't found the root cause, but I'm certain it has something to do with a ghost RCTModalHostView lying around.

The flow that exposes the problem in the app I'm developing is doing an app restart (we use this to allow devs to change the backend environment).

@marcshilling
Copy link

I had an issue where after my modal dismissed it would re-appear right away and screen interaction would be broken. The patch above seems to work to fix that issue!

Just wanted to call out some warnings that Xcode is showing with the new code:
image

Simply following the auto-fixes clears the warnings and the fix still works. Might want to consider making this change @hurali97!
image

@hurali97 hurali97 closed this Jul 5, 2022
@hurali97
Copy link
Collaborator Author

hurali97 commented Jul 5, 2022

Closing this as it's now already fixed and working as expected :) So it's needless to have it open.

@dcalhoun
Copy link

dcalhoun commented Jul 5, 2022

@hurali97 are you able to share which PR fixed this issue? Thanks!

@marcshilling
Copy link

marcshilling commented Jul 26, 2022

Closing this as it's now already fixed and working as expected :) So it's needless to have it open.

@hurali97 I just confirmed that this issue is not fixed in v0.69.3 of React Native. I'm not sure this should have been closed. The file touched by this pull request has not been updated on main: https://github.com/facebook/react-native/commits/main/React/Views/RCTModalHostView.m

mahi8813 pushed a commit to myntra/react-native that referenced this pull request Jan 25, 2023
nitish24p added a commit to myntra/react-native that referenced this pull request Jan 25, 2023
 Fix for Modal Freeze Issue on iOS. Ref thread: facebook#32450
@KarthikBaleneni
Copy link

Patch is working fine in React-native. 0.70.10

@krmao
Copy link

krmao commented Feb 23, 2024

Patch is not working on React-native. 0.72.4

image image image

dismissModalHostView not work

image

viewController.presentingViewController is nil

--------AAAAAAAAAAAAA presentModalHostView ---<RCTModalHostViewManager: 0x28182d530>  <RCTModalHostViewController: 0x167cdb170> , <RCTModalHostView: 0x162c19ad0; frame = (0 0; 428 926); layer = <CALayer: 0x28136b4e0>>
--------AAAAAAAAAAAAA presentModalHostView ---<RCTModalHostViewManager: 0x28182d530>  <RCTModalHostViewController: 0x16dae3f90> , <RCTModalHostView: 0x15f22fb60; frame = (0 0; 428 926); layer = <CALayer: 0x281003e80>>
--------AAAAAAAAAAAAA dismissModalHostView <RCTModalHostViewManager: 0x28182d530> ,<RCTModalHostViewController: 0x167cdb170> , presentingViewController=<UIViewController: 0x15df178b0>, self=<RCTModalHostView: 0x162c19ad0; frame = (0 0; 428 926); layer = <CALayer: 0x28136b4e0>>
AAAAAAAAAAAAA---->self->_dismissalBlock--->(null)
AAAAAAAAAAAAA---->modalHostView.identifier--->16
AAAAAAAAAAAAA---->[modalHostView reactViewController]--->(null)
AAAAAAAAAAAAA---->viewController---><RCTModalHostViewController: 0x167cdb170>
AAAAAAAAAAAAA---->viewController.presentingViewController---><UIViewController: 0x15df178b0>
--------AAAAAAAAAAAAA presentModalHostView ---<RCTModalHostViewManager: 0x28182d530>  <RCTModalHostViewController: 0x16cf15bf0> , <RCTModalHostView: 0x16cf32270; frame = (0 0; 428 926); layer = <CALayer: 0x28131e480>>
--------AAAAAAAAAAAAA dismissModalHostView <RCTModalHostViewManager: 0x28182d530> ,<RCTModalHostViewController: 0x16dae3f90> , presentingViewController=(null), self=<RCTModalHostView: 0x15f22fb60; frame = (0 0; 428 926); layer = <CALayer: 0x281003e80>>
AAAAAAAAAAAAA---->self->_dismissalBlock--->(null)
AAAAAAAAAAAAA---->modalHostView.identifier--->17
AAAAAAAAAAAAA---->[modalHostView reactViewController]--->(null)
AAAAAAAAAAAAA---->viewController---><RCTModalHostViewController: 0x16dae3f90>
AAAAAAAAAAAAA---->viewController.presentingViewController--->(null)
--------AAAAAAAAAAAAA dismissModalHostView <RCTModalHostViewManager: 0x28182d530> ,<RCTModalHostViewController: 0x16cf15bf0> , presentingViewController=(null), self=<RCTModalHostView: 0x16cf32270; frame = (0 0; 428 926); layer = <CALayer: 0x28131e480>>
AAAAAAAAAAAAA---->self->_dismissalBlock--->(null)
AAAAAAAAAAAAA---->modalHostView.identifier--->18
AAAAAAAAAAAAA---->[modalHostView reactViewController]--->(null)
AAAAAAAAAAAAA---->viewController---><RCTModalHostViewController: 0x16cf15bf0>
AAAAAAAAAAAAA---->viewController.presentingViewController--->(null)

Finally I found the key

the key is when A modal is showing, and then show B modal, and then dimiss A modal, the bug is comming! such as showLoading -> showDialogIfAccountNotRegister -> hideLoading should be change to showLoading -> hideLoading -> showDialogIfAccountNotRegister

and set the modal animation duration is 1 for react-native-modal and while coverScreen=true

      coverScreen={true}
      animationInTiming={1}
      animationOutTiming={1}
      backdropTransitionInTiming={1}
      backdropTransitionOutTiming={1}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.