Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ node_modules/
npm-debug.log
yarn-debug.log
yarn-error.log
package-lock.json

# Expo
.expo/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -79,7 +79,9 @@ class PagerViewViewManager : ViewGroupManager<NestedScrollableHost>(), RNCViewPa
PageScrollStateChangedEvent(host.id, pageScrollState)
)
}
})
}
host.pageChangeCallback = callback
vp.registerOnPageChangeCallback(callback)
Copy link

Choose a reason for hiding this comment

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

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.

Fix in Cursor Fix in Web

UIManagerHelper.getEventDispatcherForReactTag(reactContext, host.id)?.dispatchEvent(
PageSelectedEvent(host.id, vp.currentItem)
)
Expand Down Expand Up @@ -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"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
37 changes: 37 additions & 0 deletions ios/RNCPagerViewComponentView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,32 @@ - (instancetype)initWithFrame:(CGRect)frame
return self;
}

- (void)dealloc {
// Clean up delegates to prevent memory leaks
if (_nativePageViewController) {
_nativePageViewController.dataSource = nil;
_nativePageViewController.delegate = nil;
}

if (scrollView) {
scrollView.delegate = nil;
}
}

- (void)willMoveToSuperview:(UIView *)newSuperview {
if (newSuperview != nil) {
[self initializeNativePageViewController];
[self goTo:_currentIndex animated:NO];
} else {
// Component is being removed from view hierarchy, clean up delegates
if (_nativePageViewController) {
_nativePageViewController.dataSource = nil;
_nativePageViewController.delegate = nil;
}

if (scrollView) {
scrollView.delegate = nil;
}
}
}

Expand Down Expand Up @@ -124,6 +146,21 @@ -(void)updateLayoutMetrics:(const facebook::react::LayoutMetrics &)layoutMetrics

-(void)prepareForRecycle {
[super prepareForRecycle];

// Clear delegates to prevent memory leaks
if (_nativePageViewController) {
_nativePageViewController.dataSource = nil;
_nativePageViewController.delegate = nil;
}

if (scrollView) {
scrollView.delegate = nil;
scrollView = nil;
}

// Clear view controllers array
[_nativeChildrenViewControllers removeAllObjects];

Copy link

Choose a reason for hiding this comment

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

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.

Fix in Cursor Fix in Web

_nativePageViewController = nil;
_currentIndex = -1;
}
Expand Down
Loading