software-mansion / react-native-screens

Native navigation primitives for your React Native app.
https://docs.swmansion.com/react-native-screens/
MIT License
3.05k stars 516 forks source link

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

Closed mkondakov closed 1 year ago

mkondakov commented 1 year ago

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

ajstokar commented 1 year ago

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

mkondakov83 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

alike

hudson-s commented 1 year ago

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

hudson-s commented 1 year ago

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 commented 1 year ago

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

mkondakov commented 1 year ago

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 commented 1 year ago

我可以得到确认吗:

解决了您的问题吗?

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

package.json:

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

Has the official version been released

kkafar commented 1 year ago

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 commented 1 year ago

@sunhongda, while you are waiting for a fix you may use our workaround: https://github.com/software-mansion/react-native-screens/issues/1754#issuecomment-1604307926 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 commented 1 year ago

@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 commented 1 year ago

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

hudson-s commented 1 year ago

@sunhongda, while you are waiting for a fix you may use our workaround: https://github.com/software-mansion/react-native-screens/issues/1754#issuecomment-1604307926 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 commented 1 year ago

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

kkafar commented 1 year ago

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.

https://github.com/facebook/react-native/pull/38617

kkafar commented 11 months ago

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)