Fix memory leaks when PagerView is unmounted #1040
Conversation
Co-authored-by: troZee <12766071+troZee@users.noreply.github.com>
Co-authored-by: troZee <12766071+troZee@users.noreply.github.com>
|
this addresses only the Fabric, it should also address the Paper. |
|
Hello @efstathiosntonas https://github.com/callstack/react-native-pager-view?tab=readme-ov-file#migration |
|
@troZee understood but this patch would suffice: diff --git a/node_modules/react-native-pager-view/ios/ReactNativePageView.m b/node_modules/react-native-pager-view/ios/ReactNativePageView.m
index eacfbe8..c61745f 100644
--- a/node_modules/react-native-pager-view/ios/ReactNativePageView.m
+++ b/node_modules/react-native-pager-view/ios/ReactNativePageView.m
@@ -44,7 +44,7 @@ - (instancetype)initWithEventDispatcher:(RCTEventDispatcher *)eventDispatcher {
_dismissKeyboard = UIScrollViewKeyboardDismissModeNone;
_coalescingKey = 0;
_eventDispatcher = eventDispatcher;
- _cachedControllers = [NSHashTable hashTableWithOptions:NSHashTableStrongMemory];
+ _cachedControllers = [NSHashTable hashTableWithOptions:NSHashTableWeakMemory];
_overdrag = NO;
_layoutDirection = @"ltr";
}It would be nice if it could be squeezed 😅 |
|
@copilot |
... The file The equivalent memory leak fix for the new architecture has already been applied in commit 4bb3c51 to If you need support for the old Paper architecture (versions < 7.x), this would need to be applied to a different branch that still contains |
|
thanks @troZee ! I just runned it on ios and on a Pixel7a A16 and no issues so far. I use this as a photo carousel on posts so usage is extremely heavy per render on the list. So far so good. using Paper. leaving the full patch here just in case another poor soul is still using Paper:
Click me to reveal patchdiff --git a/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/NestedScrollableHost.kt b/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/NestedScrollableHost.kt
index 87b58d0f..e9d0ace1 100644
--- a/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/NestedScrollableHost.kt
+++ b/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/NestedScrollableHost.kt
@@ -25,6 +25,7 @@ class NestedScrollableHost : FrameLayout {
constructor(context: Context, attrs: AttributeSet?) : super(context, attrs)
public var initialIndex: Int? = null
public var didSetInitialIndex = false
+ public var pageChangeCallback: ViewPager2.OnPageChangeCallback? = null
private var touchSlop = 0
private var initialX = 0f
private var initialY = 0f
diff --git a/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/PagerViewViewManager.kt b/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/PagerViewViewManager.kt
index 8ec286a7..19f46363 100644
--- a/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/PagerViewViewManager.kt
+++ b/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/PagerViewViewManager.kt
@@ -52,7 +52,7 @@ class PagerViewViewManager : ViewGroupManager<NestedScrollableHost>(), RNCViewPa
vp.isSaveEnabled = false
vp.post {
- vp.registerOnPageChangeCallback(object : OnPageChangeCallback() {
+ val callback = object : OnPageChangeCallback() {
override fun onPageScrolled(position: Int, positionOffset: Float, positionOffsetPixels: Int) {
super.onPageScrolled(position, positionOffset, positionOffsetPixels)
UIManagerHelper.getEventDispatcherForReactTag(reactContext, host.id)?.dispatchEvent(
@@ -79,7 +79,9 @@ class PagerViewViewManager : ViewGroupManager<NestedScrollableHost>(), RNCViewPa
PageScrollStateChangedEvent(host.id, pageScrollState)
)
}
- })
+ }
+ host.pageChangeCallback = callback
+ vp.registerOnPageChangeCallback(callback)
UIManagerHelper.getEventDispatcherForReactTag(reactContext, host.id)?.dispatchEvent(
PageSelectedEvent(host.id, vp.currentItem)
)
@@ -200,6 +202,20 @@ class PagerViewViewManager : ViewGroupManager<NestedScrollableHost>(), RNCViewPa
}
}
+ override fun onDropViewInstance(view: NestedScrollableHost) {
+ // Unregister the page change callback to prevent memory leaks
+ val viewPager = PagerViewViewManagerImpl.getViewPager(view)
+ view.pageChangeCallback?.let { callback ->
+ viewPager.unregisterOnPageChangeCallback(callback)
+ view.pageChangeCallback = null
+ }
+
+ // Clear the adapter to release references to child views
+ viewPager.adapter = null
+
+ super.onDropViewInstance(view)
+ }
+
override fun getExportedCustomDirectEventTypeConstants(): MutableMap<String, Map<String, String>> {
return MapBuilder.of(
PageScrollEvent.EVENT_NAME, MapBuilder.of("registrationName", "onPageScroll"),
diff --git a/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/ViewPagerAdapter.kt b/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/ViewPagerAdapter.kt
index fd3530e1..8aab5f66 100644
--- a/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/ViewPagerAdapter.kt
+++ b/node_modules/react-native-pager-view/android/src/main/java/com/reactnativepagerview/ViewPagerAdapter.kt
@@ -30,6 +30,12 @@ class ViewPagerAdapter() : Adapter<ViewPagerViewHolder>() {
container.addView(child)
}
+ override fun onViewRecycled(holder: ViewPagerViewHolder) {
+ super.onViewRecycled(holder)
+ // Clean up the holder's container to prevent memory leaks
+ holder.container.removeAllViews()
+ }
+
override fun getItemCount(): Int {
return childrenViews.size
}
diff --git a/node_modules/react-native-pager-view/ios/ReactNativePageView.m b/node_modules/react-native-pager-view/ios/ReactNativePageView.m
index eacfbe8..c61745f 100644
--- a/node_modules/react-native-pager-view/ios/ReactNativePageView.m
+++ b/node_modules/react-native-pager-view/ios/ReactNativePageView.m
@@ -44,7 +44,7 @@ - (instancetype)initWithEventDispatcher:(RCTEventDispatcher *)eventDispatcher {
_dismissKeyboard = UIScrollViewKeyboardDismissModeNone;
_coalescingKey = 0;
_eventDispatcher = eventDispatcher;
- _cachedControllers = [NSHashTable hashTableWithOptions:NSHashTableStrongMemory];
+ _cachedControllers = [NSHashTable hashTableWithOptions:NSHashTableWeakMemory];
_overdrag = NO;
_layoutDirection = @"ltr";
} |
package-lock.json
Outdated
Co-authored-by: troZee <12766071+troZee@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bug: Nil object access during component recycling.
After prepareForRecycle sets scrollView = nil on line 158, updateProps accesses scrollView.scrollEnabled without a nil check. If the component is recycled and updateProps is called before willMoveToSuperview reinitializes the view, this causes a crash or undefined behavior when accessing properties on a nil object.
ios/RNCPagerViewComponentView.mm#L204-L206
react-native-pager-view/ios/RNCPagerViewComponentView.mm
Lines 204 to 206 in 71650e3
Bug: Imperative Commands Crash on Nil State Access
The setScrollEnabledImperatively method accesses scrollView without checking if it's nil. After prepareForRecycle sets scrollView = nil on line 158, calling this imperative command from JavaScript will crash or produce undefined behavior. Since this is an imperative API callable at any time, it needs a nil check.
ios/RNCPagerViewComponentView.mm#L425-L428
react-native-pager-view/ios/RNCPagerViewComponentView.mm
Lines 425 to 428 in 71650e3
| }) | ||
| } | ||
| host.pageChangeCallback = callback | ||
| vp.registerOnPageChangeCallback(callback) |
There was a problem hiding this comment.
Bug: Race Condition Causes Memory Leak
The callback is registered inside vp.post {}, deferring execution to the next frame, but onDropViewInstance may be called before this posted runnable executes. This creates a race condition where the callback gets registered after the view is dropped, causing a memory leak since it's never unregistered. The callback should be stored and registered synchronously, or the cleanup should account for pending posted runnables.
|
|
||
| // Clear view controllers array | ||
| [_nativeChildrenViewControllers removeAllObjects]; | ||
|
|
There was a problem hiding this comment.
Bug: Lifecycle issue crashes keyboard dismissal.
The shouldDismissKeyboard method accesses scrollView.keyboardDismissMode without checking if scrollView is nil. After prepareForRecycle sets scrollView = nil, calling this method (e.g., from updateProps on line 202) will crash or produce undefined behavior.
Summary
Fixes #702 - Memory leaks when component is disposed
When PagerView is unmounted, delegates and callbacks retain references to the component, preventing deallocation. This causes memory to accumulate proportionally to the resources held by active pages.
iOS (RNCPagerViewComponentView.mm)
dealloc,willMoveToSuperview, andprepareForRecycleprepareForRecycleAndroid
onDropViewInstance(PagerViewViewManager)onViewRecycled(ViewPagerAdapter)Project Configuration
package-lock.jsonfile (project uses Bun, not npm)package-lock.jsonto.gitignoreto prevent future additionsTest Plan
What's required for testing (prerequisites)?
What are the steps to reproduce (after prerequisites)?
Compatibility
Checklist
README.md💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Note
Cleans up PagerView callbacks, delegates, and adapters on unmount (iOS/Android) and recycles holder views; adds package-lock.json to .gitignore.
OnPageChangeCallbackinNestedScrollableHostand unregister it inonDropViewInstance.ViewPager2adapter inonDropViewInstanceto release child references.ViewPagerAdapter.onViewRecycled.UIPageViewControllerandUIScrollViewdelegates indealloc,willMoveToSuperview, andprepareForRecycle.scrollViewand empty_nativeChildrenViewControllersduring recycle.package-lock.jsonto.gitignore.Written by Cursor Bugbot for commit 71650e3. This will update automatically on new commits. Configure here.