Skip to content

Conversation

@WoLewicki
Copy link
Member

@WoLewicki WoLewicki commented May 21, 2025

Description

PR analogous to software-mansion/react-native-reanimated#7515 regarding the javaPart_.
Right now when you do a reload in a simple app with screens installed, you will see in AS profiler that instances of all NativeProxys are still in the memory. It is caused by javaPart_ never being released thus keeping java part after reload.

I also removed empty destructor of NativeProxy.

Test code and steps to reproduce

Make simple app with screens and do a couple of reloads and check in profiler if the NativeProxy leaks.

@WoLewicki WoLewicki requested review from kkafar and tjzel May 21, 2025 14:11
Copy link
Contributor

@tjzel tjzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but make sure that RNScreens doesn't actually try to use anything from javaPart_ once it was nulled.

In the case of Reanimated and Worklets some (concurrent) code always expected that it's not nulled.

@WoLewicki
Copy link
Member Author

But isn't it prohibited behavior for the code to run anything after invalidate was called on Java side?

@tjzel
Copy link
Contributor

tjzel commented May 22, 2025

It is prohibited - but the question is, if it's actually respected. For instance, in Worklets and Reanimated we have to make sure that other threads do gracefully abort when invalidate is called on the JS thread.

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

Regarding what @tjzel has said: we should be safe on that front, because:

  1. the lifecycle of a native proxy is tied & bound by lifecycle of RNScreensModule, which in turn is managed by React,
  2. If React calls our RNSScreenRemovalListener after the screen module has been invalidated it would mean that something went seriously wrong - am I right here @WoLewicki? Or is it possible for native modules to be invalidated && before the view hierarchy is torn down?


external fun nativeAddMutationsListener(fabricUIManager: FabricUIManager)

external fun invalidateCpp()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have native proxy, native methods in Java, we use Java Native interface => let's call it invalidateNativePart or invalidateNative.

@WoLewicki
Copy link
Member Author

Or is it possible for native modules to be invalidated && before the view hierarchy is torn down?

Haven't seen it be a case in the in the apps I worked on so hopefully there is no way. I think it would lead to other problems then anyway 🤷‍♂️

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can proceed then

@WoLewicki WoLewicki merged commit 5fcb545 into main Jun 3, 2025
5 of 7 checks passed
@WoLewicki WoLewicki deleted the @wolewicki/fix-nativeproxy-memleak branch June 3, 2025 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants