-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[No QA] Add patch that fixes memory leaks in react-native-performance
on Android
#72341
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
[No QA] Add patch that fixes memory leaks in react-native-performance
on Android
#72341
Conversation
|
react-native-performance
react-native-performance
on Android
Given that this is |
Ah yes, I added test steps for C+. It's not the easiest piece of the app to test so probably extra logs will be needed in |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-app-2025-10-14_11.50.35.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from my tests - I do see a massively reduced number of duplicate listeners vs. main (only loadReactNativeSoFileStart/End
repeats once).
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.2.33-0 🚀
|
Explanation of Change
This PR fixes three leaks of listeners:
CONTENT_APPEARED
andRELOAD
to a constant to prevent leak when module is invalidated. Now we remove this listener ininvalidate
.RNPerformance.removeListener
had an incorrect condition that prevented listeners from being removed.getPackages
can be called multiple times in React Native, thePerformancePackage
constructor may also be invoked multiple times, resulting in several identical listeners being added as different instances.ReactMarker
prevents duplicates only when the same instance is added, so extracting the listener to a static constant resolves this issue.Additionally I changed
onCatalystInstanceDestroyed
toinvalidate
, because previous method is deprecated.Upstream PR: oblador/react-native-performance#118
Fixed Issues
$ #63979
PROPOSAL:
Tests
Android only:
This PR fixes memory leaks by preventing duplication of listeners and ensuring they are properly removed on invalidate.
The main thing to verify is that
nativeMarksObserver
still behaves the same way as it does on main.Note: On main, listeners on the native side could be duplicated, which might have caused some native events to fire multiple times. Keep this in mind when comparing behaviors.
Record Troubleshoot Data
(it might be worth to add logs tonativeMarksObserver
inPerformance.tsx
to verify it, otherwise you won't see if anything is processed by observer)Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectioncanBeMissing
param foruseOnyx
toggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)npm run compress-svg
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop