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

Memory Leak. iOS. Strong reference cycle between RNSScreen and RNSScreenView #1754

Closed
mkondakov opened this issue Apr 12, 2023 · 19 comments
Closed
Assignees
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided

Comments

@mkondakov
Copy link

Description

Instance of RNSScreen class creates strong reference cycle with instance of RNSScreenView class. This issue is critical in our project where user can open and close react-native app inside native iOS app many times.

Steps to reproduce

  1. clone project
  2. open TestRNSScreens folder in terminal
  3. run "pod install"
  4. open "TestRNSScreens.xcworkspace" in Xcode
  5. run
  6. click on "Open RN App" button
  7. click on "close" button
  8. repeat 6 and 7 steps few times
  9. check memory usage in Xcode

Snack or a link to a repository

https://github.com/mkondakov/RNSScreensMemoryLeak

Screens version

3.20.0

React Native version

0.71.6

Platforms

iOS

JavaScript runtime

None

Workflow

React Native (without Expo)

Architecture

None

Build type

Release mode

Device

Real device

Device model

all models

Acknowledgements

Yes

@github-actions github-actions bot added Repro provided A reproduction with a snack or repo is provided Platform: iOS This issue is specific to iOS labels Apr 12, 2023
@kkafar kkafar self-assigned this Apr 12, 2023
@ajstokar
Copy link

I believe we are seeing the same issue. Navigating back and forth between 2 screens causes memory to consistently go up.

@mkondakov83
Copy link

@kkafar do you have any progress on this issue? We have found that this memory leak is the main reason why application crashes because of low memory conditions. In our app we open and close react-native instances a lot.

@kkafar
Copy link
Member

kkafar commented Jun 23, 2023

Hey, small update from my side.
I'm investigating the issue but I've not determined root cause of the leak yet unfortunately.

  1. It seems that in standard environment (fully React Native application) there is no leak

(simple two screen application)

image

You can see in Xcode log console that I've pushed & popped few screens, but in the end there are only two RNSScreen controllers allocated -- the first one (for the first screen) and most recent one for the second screen.

However, at the same time memory usage has risen a bit:

image

But as I can detect no apparent leak it nudges me towards conclusion that GC simply was not triggered yet.

  1. I can confirm the leak being present in your reproduction, however it is not clear for me, what change in application setup introduces the key difference here.

Just letting you know that I'm looking, will be back once I have solution.

@mkondakov
Copy link
Author

mkondakov commented Jun 23, 2023

@kkafar

I can confirm the leak being present in your reproduction, however it is not clear for me, what change in application setup introduces the key difference here.

This memory leak is critical when native iOS app creates multiple instances of react-native applications.
Maybe it is not relevant if you have only 1 instance and navigate inside single react-native app.
But it becomes critical if you close react-native instance.

We have created a temporary workaround. When our ReactAppViewController is closed (it owns RCTRootView) we call invalidate() for all RNSScreenView instances. In other words we destroy all strong-reference cycles between RNSScreenView and RNSScreen

image

@kkafar
Copy link
Member

kkafar commented Jun 27, 2023

Can I get confirmation that this:

Fixes the issue in your case?

You can try it out by installing screens from the last commit of that PR:

package.json:

"react-native-screens": "software-mansion/react-native-screens#751aab52a043d33e8d13a53de5688e3436a2ef95"

@hudson-s
Copy link

alike

@hudson-s
Copy link

This problem still exists in version 3.22.1.
demo:https://github.com/OrrHsiao/ReactNativeDemo

@hudson-s
Copy link

My app is a native app that has infiltrated the RN module. When closing RN and returning to the native module, RTCxxxView cannot trigger the dealloc function . (The RTCxxxViewManage dealloc function will trigger)

@hudson-s
Copy link

这个问题在3.22.1版本中仍然存在。
演示:https://github.com/OrrHsiao/ReactNativeDemo

@mkondakov
Copy link
Author

Can I get confirmation that this:

Fixes the issue in your case?

You can try it out by installing screens from the last commit of that PR:

package.json:

"react-native-screens": "software-mansion/react-native-screens#751aab52a043d33e8d13a53de5688e3436a2ef95"

@kkafar, looks like your fix works for our case. I have checked your branch:

"react-native-screens": "software-mansion/react-native-screens#@kkafar/retain-cycle-ios"

@hudson-s
Copy link

我可以得到确认吗:

解决了您的问题吗?

您可以通过安装该 PR 上次提交的屏幕来尝试一下:

package.json:

"react-native-screens": "software-mansion/react-native-screens#751aab52a043d33e8d13a53de5688e3436a2ef95"

Has the official version been released

@kkafar
Copy link
Member

kkafar commented Jul 21, 2023

Hey,

Has the official version been released

No, I have not merged it into main branch yet, as the solution I've provided solves this issue, but causes few other ones (e. g. tab navigation crashes).

The issue here is that while dismissing react native view controller from native side react-native-screens components react life-cycle methods do not get triggered - so from library perspective we do not have enough information to determine whether the screen is definitively removed or only temporarily detached from view hierarchy. Thus it looks that the issue arises from buggy interaction between react-native and iOS. I'll try to see what can be done there.

@mkondakov
Copy link
Author

@sunhongda, while you are waiting for a fix you may use our workaround: #1754 (comment)
We just braking strong-reference cycle manually on UIViewController deinit. UIViewController is a host for RCTRootView. You just need to have access to RCTBridge.

@hudson-s
Copy link

@sunhongda, while you are waiting for a fix you may use our workaround: #1754 (comment) We just braking strong-reference cycle manually on UIViewController deinit. UIViewController is a host for RCTRootView. You just need to have access to RCTBridge.

thks

@hudson-s
Copy link

Android also has similar problems, is there any temporary solution to solve it

@hudson-s
Copy link

@sunhongda, while you are waiting for a fix you may use our workaround: #1754 (comment)
We just braking strong-reference cycle manually on UIViewController deinit. UIViewController is a host for RCTRootView. You just need to have access to RCTBridge.

Android also has similar problems, is there any temporary solution to solve it

@hudson-s
Copy link

Hey,

Has the official version been released

img_v2_46e387c4-1621-448a-bde0-c0ce98107f2g

img_v2_a8f569c0-f78e-41a2-93ba-fc7d75ef359g

No, I have not merged it into main branch yet, as the solution I've provided solves this issue, but causes few other ones (e. g. tab navigation crashes).

The issue here is that while dismissing react native view controller from native side react-native-screens components react life-cycle methods do not get triggered - so from library perspective we do not have enough information to determine whether the screen is definitively removed or only temporarily detached from view hierarchy. Thus it looks that the issue arises from buggy interaction between react-native and iOS. I'll try to see what can be done there.

Android leak

facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Oct 11, 2023
Summary:
Talking about Paper & iOS here.

In standard RN applications when a native component is removed permanently from view hierarchy [it is invalidated (if it implements `RCTInvalidating`)](https://github.com/facebook/react-native/blob/e64756ae5bb5c0607a4d97a134620fafcb132b3b/packages/react-native/React/Modules/RCTUIManager.m#L483-L495). Components that implement `RCTInvalidating` such as [`RNSScreenView`](https://github.com/software-mansion/react-native-screens/blob/9fb3bd00850bcdf29b46daa57e56eabda3ae30ea/ios/RNSScreen.mm#L35) of [`react-native-screens`](https://github.com/software-mansion/react-native-screens) library rely on `RCTInvalidating#invalidate` method being called in adequate moment to release retained resources (in my case the `RNSScreenView` holds a strong reference to it's view controller preventing it from being garbage collected).

However in case of brownfield applications (React Native is used only for a particular view & loaded on demand, see: software-mansion/react-native-screens#1754 for discussion & app example) when view controller holding `RCTRootView` is dismissed and whole `React Native` managed view / controller tree gets deallocated, `RCTInvalidating#invalidate` method is not called on the dismissed components, thus in my particular use case, leading to memory leak.

Right now I've added call to `RCTUIManager#_purgeChildren:fromRegistry:` (which internally invalidates all components which implement `RCTInvalidating`) in `RCTUIManager#invalidate`.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[IOS][FIXED] - Purge children from view registry on `RCTUIManager` invalidation.

Pull Request resolved: #38617

Test Plan:
You can run the [demo](https://github.com/mkondakov/RNSScreensMemoryLeak) provided in the [issue](software-mansion/react-native-screens#1754).

Following screenshots show that memory leak in brownfield application is resolved.

Without the change (`invalidate` method is not being called on native components)

![image](https://github.com/facebook/react-native/assets/50801299/dac331c2-1e7c-4e66-a8c1-b88f7a007d9b)

With the change:

![image](https://github.com/facebook/react-native/assets/50801299/7a8afbe9-446c-47a2-a972-d7589b921677)

Reviewed By: NickGerleman

Differential Revision: D49952215

Pulled By: javache

fbshipit-source-id: 6336b86774615acc40279c97e6ae0bb777bda8ad
@kkafar
Copy link
Member

kkafar commented Oct 11, 2023

Hey, as my PR to React Native is merged I'm closing the issue. I do not know in which RN version it will be first included, but will keep you updated.

facebook/react-native#38617

@kkafar kkafar closed this as completed Oct 11, 2023
lunaleaps pushed a commit to facebook/react-native that referenced this issue Nov 17, 2023
Summary:
Talking about Paper & iOS here.

In standard RN applications when a native component is removed permanently from view hierarchy [it is invalidated (if it implements `RCTInvalidating`)](https://github.com/facebook/react-native/blob/e64756ae5bb5c0607a4d97a134620fafcb132b3b/packages/react-native/React/Modules/RCTUIManager.m#L483-L495). Components that implement `RCTInvalidating` such as [`RNSScreenView`](https://github.com/software-mansion/react-native-screens/blob/9fb3bd00850bcdf29b46daa57e56eabda3ae30ea/ios/RNSScreen.mm#L35) of [`react-native-screens`](https://github.com/software-mansion/react-native-screens) library rely on `RCTInvalidating#invalidate` method being called in adequate moment to release retained resources (in my case the `RNSScreenView` holds a strong reference to it's view controller preventing it from being garbage collected).

However in case of brownfield applications (React Native is used only for a particular view & loaded on demand, see: software-mansion/react-native-screens#1754 for discussion & app example) when view controller holding `RCTRootView` is dismissed and whole `React Native` managed view / controller tree gets deallocated, `RCTInvalidating#invalidate` method is not called on the dismissed components, thus in my particular use case, leading to memory leak.

Right now I've added call to `RCTUIManager#_purgeChildren:fromRegistry:` (which internally invalidates all components which implement `RCTInvalidating`) in `RCTUIManager#invalidate`.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[IOS][FIXED] - Purge children from view registry on `RCTUIManager` invalidation.

Pull Request resolved: #38617

Test Plan:
You can run the [demo](https://github.com/mkondakov/RNSScreensMemoryLeak) provided in the [issue](software-mansion/react-native-screens#1754).

Following screenshots show that memory leak in brownfield application is resolved.

Without the change (`invalidate` method is not being called on native components)

![image](https://github.com/facebook/react-native/assets/50801299/dac331c2-1e7c-4e66-a8c1-b88f7a007d9b)

With the change:

![image](https://github.com/facebook/react-native/assets/50801299/7a8afbe9-446c-47a2-a972-d7589b921677)

Reviewed By: NickGerleman

Differential Revision: D49952215

Pulled By: javache

fbshipit-source-id: 6336b86774615acc40279c97e6ae0bb777bda8ad
@kkafar
Copy link
Member

kkafar commented Nov 20, 2023

My patch was just recently picked into RN, and AFAIK it will be available with react-native
0.73.0-rc.5. However I do not recommend testing it yet, as react-native-screens are not fully compliant with 0.73 yet, and there can be even some build issues. (There is PR in progress to fix that)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants