Skip to content

fix(iOS): unregister tabs accessory observer from observed wrapper#3948

Open
safaiyeh wants to merge 2 commits intosoftware-mansion:mainfrom
safaiyeh:fix/tabs-bottom-accessory-kvo-invalidation
Open

fix(iOS): unregister tabs accessory observer from observed wrapper#3948
safaiyeh wants to merge 2 commits intosoftware-mansion:mainfrom
safaiyeh:fix/tabs-bottom-accessory-kvo-invalidation

Conversation

@safaiyeh
Copy link
Copy Markdown

Description

Fixes a possible iOS 26 bottom accessory crash during invalidation.

The helper registers KVO for center on self.nativeWrapperView, but invalidation removed the observer from _bottomAccessoryView.superview.superview at teardown time. UIKit can detach or swap the bottom accessory wrapper before invalidation runs, so the teardown path may attempt to remove the observer from a different wrapper view and raise an NSRangeException because that object was never observed.

I do not see an existing issue for this exact stack, but we observed it in production on react-native-screens 4.23.0, and the same registration/removal pattern is present on current main.

Changes

  • Stores the exact native wrapper view that was registered for KVO.
  • Makes registerForAccessoryFrameChanges idempotent for the same wrapper and unregisters the previous wrapper if the wrapper changes.
  • Removes the observer from the stored observed wrapper during invalidation instead of resolving the current superview chain again.
  • Uses a dedicated KVO context and forwards unrelated observations to super.

Test plan

  • yarn install --immutable
  • yarn prepare
  • yarn check-types
  • xcrun clang-format -i ios/tabs/bottom-accessory/RNSTabsBottomAccessoryHelper.mm
  • git diff --check

Existing examples that exercise bottom accessory behavior:

  • apps/src/tests/issue-tests/Test3288.tsx
  • apps/src/tests/single-feature-tests/tabs/bottom-accessory-layout.tsx

I did not add a JS unit test for this because the failure is caused by UIKit/KVO object identity during native view invalidation. A meaningful automated regression test would need a native iOS XCTest harness or an iOS integration/e2e test that drives bottom accessory mount, detach, and invalidation lifecycle. This repository does not appear to have an iOS native unit-test target for library internals today.

I attempted yarn test:unit after initializing the react-navigation submodule, but the root Jest command also collects the submodule's own test suites and fails on unrelated submodule Jest setup / React version issues before exercising this native change.

Checklist

  • Included code example that can be used to test this change. Existing bottom accessory examples are listed above; no new JS example was added for this native KVO lifecycle fix.
  • For visual changes, included screenshots / GIFs / recordings documenting the change. N/A, no visual changes.
  • For API changes, updated relevant public types. N/A, no API changes.
  • Ensured that CI passes

@safaiyeh safaiyeh marked this pull request as ready for review April 28, 2026 16:10
@kkafar kkafar requested review from Copilot and kligarski and removed request for kligarski April 29, 2026 06:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a potential iOS 26 crash during bottom accessory invalidation by ensuring KVO is unregistered from the exact wrapper view instance that was originally observed (rather than re-deriving the wrapper via the current superview chain at teardown time).

Changes:

  • Track and store the specific native wrapper view used for KVO registration, and unregister from that stored instance during invalidation.
  • Make frame-change KVO registration idempotent for the same wrapper and safely re-register if the wrapper view changes.
  • Use a dedicated KVO context and forward unrelated observations to super.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@kligarski kligarski left a comment

Choose a reason for hiding this comment

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

Hi, @safaiyeh. Thank you for the PR, it looks good.

I'll try to push a change with removal of the redundant boolean (see comment below) and merge the changes after CI passes.

@implementation RNSTabsBottomAccessoryHelper {
RNSTabsBottomAccessoryComponentView *__weak _bottomAccessoryView;
UIView *__weak _observedNativeWrapperView;
BOOL _isObservingNativeWrapperView;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems to me that this boolean is redundant. If _observedNativeWrapperView is nil, we know that we don't observe anything. It also introduces possibility of inconsistent state (e.g. _isObservingNativeWrapperView = YES and _observedNativeWrapperView = nil) so I don't think we should keep it.

@kligarski kligarski self-assigned this Apr 30, 2026
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.

3 participants