Skip to content
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

0.73.rc iOS oldarch - infinite recursion with -[RCTUIManager setNeedsLayout] #41545

Closed
mfazekas opened this issue Nov 18, 2023 · 3 comments
Closed
Labels
Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. Needs: Triage 🔍 Platform: iOS iOS applications. Type: Upgrade Issue Issues reported from upgrade issue form Version: unspecified No version information could be extracted

Comments

@mfazekas
Copy link
Contributor

mfazekas commented Nov 18, 2023

New Version

0.73.rc.4

Old Version

0.72.6

Build Target(s)

iOS simulator

Output of react-native info

info Fetching system and libraries information...
System:
OS: macOS 13.6
CPU: (12) arm64 Apple M2 Max
Memory: 104.39 MB / 32.00 GB
Shell:
version: "5.9"
path: /bin/zsh
Binaries:
Node:
version: 18.18.0
path: ~/.nvm/versions/node/v18.18.0/bin/node
Yarn:
version: 1.22.21
path: ~/.nvm/versions/node/v18.18.0/bin/yarn
npm:
version: 9.8.1
path: ~/.nvm/versions/node/v18.18.0/bin/npm
Watchman:
version: 2023.09.04.00
path: /opt/homebrew/bin/watchman
Managers:
CocoaPods:
version: 1.14.2
path: /Users/boga/.rbenv/shims/pod
SDKs:
iOS SDK:
Platforms:
- DriverKit 22.4
- iOS 16.4
- macOS 13.3
- tvOS 16.4
- watchOS 9.4
Android SDK: Not Found
IDEs:
Android Studio: 2022.3 AI-223.8836.35.2231.10811636
Xcode:
version: 14.3.1/14E300c
path: /usr/bin/xcodebuild
Languages:
Java:
version: 15.0.10
path: /usr/bin/javac
Ruby:
version: 2.7.8
path: /Users/boga/.rbenv/shims/ruby
npmPackages:
"@react-native-community/cli": Not Found
react:
installed: 18.2.0
wanted: 18.2.0
react-native:
installed: 0.73.0-rc.4
wanted: 0.73.0-rc.4
react-native-macos: Not Found
npmGlobalPackages:
"react-native": Not Found
Android:
hermesEnabled: true
newArchEnabled: false
iOS:
hermesEnabled: true
newArchEnabled: false

Issue and Reproduction Steps

npx react-native@0.73.0-rc.4 init RN073RC4 --version 0.73.0-rc.4

Add the following to App.tsx, just after the last import

import {TurboModuleRegistry} from 'react-native';
const foo = TurboModuleRegistry.getEnforcing('no_such_module');
console.log('foo', foo);

I get infinite recursion with RCTUIManager setNeedsLayout

#26824	0x000000010091c4b4 in -[RCTView reactSetFrame:] at /tmp/RN073RC4/node_modules/react-native/React/Views/RCTView.m:785
#26825	0x00000001009042d8 in __51-[RCTUIManager uiBlockWithLayoutUpdateForRootView:]_block_invoke.135 at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:687
#26826	0x0000000100909a04 in __44-[RCTUIManager flushUIBlocksWithCompletion:]_block_invoke at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1199
#26827	0x0000000100909cc8 in __44-[RCTUIManager flushUIBlocksWithCompletion:]_block_invoke.207 at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1219
#26828	0x0000000100911328 in RCTExecuteOnMainQueue at /tmp/RN073RC4/node_modules/react-native/React/Base/RCTUtils.m:262
#26829	0x000000010090984c in -[RCTUIManager flushUIBlocksWithCompletion:] at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1213
#26830	0x00000001009094ac in -[RCTUIManager _layoutAndMount] at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1172
#26831	0x0000000100909e6c in -[RCTUIManager setNeedsLayout] at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1232
#26832	0x0000000100901814 in __37-[RCTUIManager setLocalData:forView:]_block_invoke at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:419
#26833	0x00000001009011d4 in __51-[RCTUIManager _executeBlockWithShadowView:forTag:]_block_invoke at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:389
#26834	0x000000010090f194 in RCTExecuteOnUIManagerQueue at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManagerUtils.m:57
#26835	0x00000001009010e8 in -[RCTUIManager _executeBlockWithShadowView:forTag:] at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:379
#26836	0x0000000100901774 in -[RCTUIManager setLocalData:forView:] at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:416
#26837	0x00000001008e0ba0 in -[RCTSafeAreaView setSafeAreaInsets:] at /tmp/RN073RC4/node_modules/react-native/React/Views/SafeAreaView/RCTSafeAreaView.m:67
#26838	0x000000011e7d2d44 in -[UIView _updateSafeAreaInsets] ()
#26839	0x000000011e7da27c in -[UIView _updateCombinedInsetsIfNecessary] ()
#26840	0x000000011e7da908 in -[UIView setCenter:] ()
#26841	0x000000010092ccb8 in -[UIView(React) reactSetFrame:] at /tmp/RN073RC4/node_modules/react-native/React/Views/UIView+React.m:204
#26842	0x000000010091c4b4 in -[RCTView reactSetFrame:] at /tmp/RN073RC4/node_modules/react-native/React/Views/RCTView.m:785
#26843	0x00000001009042d8 in __51-[RCTUIManager uiBlockWithLayoutUpdateForRootView:]_block_invoke.135 at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:687
#26844	0x0000000100909a04 in __44-[RCTUIManager flushUIBlocksWithCompletion:]_block_invoke at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1199
#26845	0x0000000100909cc8 in __44-[RCTUIManager flushUIBlocksWithCompletion:]_block_invoke.207 at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1219
#26846	0x0000000100911328 in RCTExecuteOnMainQueue at /tmp/RN073RC4/node_modules/react-native/React/Base/RCTUtils.m:262
#26847	0x000000010090984c in -[RCTUIManager flushUIBlocksWithCompletion:] at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1213
#26848	0x00000001009094ac in -[RCTUIManager _layoutAndMount] at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1172
#26849	0x0000000100909e6c in -[RCTUIManager setNeedsLayout] at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManager.m:1232
#26850	0x00000001008f6298 in __56-[RCTSurface setMinimumSize:maximumSize:viewportOffset:]_block_invoke at /tmp/RN073RC4/node_modules/react-native/React/Base/Surface/RCTSurface.mm:404
#26851	0x000000010090f39c in RCTUnsafeExecuteOnUIManagerQueueSync at /tmp/RN073RC4/node_modules/react-native/React/Modules/RCTUIManagerUtils.m:86
#26852	0x00000001008f5fdc in -[RCTSurface setMinimumSize:maximumSize:viewportOffset:] at /tmp/RN073RC4/node_modules/react-native/React/Base/Surface/RCTSurface.mm:396
#26853	0x00000001008f5e1c in -[RCTSurface setSize:] at /tmp/RN073RC4/node_modules/react-native/React/Base/Surface/RCTSurface.mm:379
#26854	0x00000001009607a4 in -[RCTLogBoxView layoutSubviews] at /tmp/RN073RC4/node_modules/react-native/React/CoreModules/RCTLogBoxView.mm:73
#26855	0x000000011e7f6c00 in -[UIView(CALayerDelegate) layoutSublayersOfLayer:] ()
@mfazekas mfazekas added Needs: Triage 🔍 Type: Upgrade Issue Issues reported from upgrade issue form labels Nov 18, 2023
@github-actions github-actions bot added Version: unspecified No version information could be extracted Platform: iOS iOS applications. Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. labels Nov 18, 2023
Copy link

⚠️ Missing Reproducible Example
ℹ️ We could not detect a reproducible example in your issue report. Please provide either:
  • If your bug is UI related: a Snack
  • If your bug is build/update related: use our Reproducer Template. A reproducer needs to be in a GitHub repository under your username.

@cipolleschi
Copy link
Contributor

Hi! thanks for reporting the issue.
I investigated it and the porblem started manifesting in RC3.
RC1 and RC2 are fine.

I isolated the change to this commit: 3168055. Reverting the changes in the react-native/React/Modules and react-native/React/Views is enough to fix the problem.

I'm working with the Release Crew (@huntie @lunaleaps @Titozzz @fortmarek) to figure out how to resolve this in the best way.

NickGerleman added a commit to NickGerleman/react-native that referenced this issue Nov 22, 2023
Summary:
Fixes facebook#41545

RN lays out the descendants of a root view using Yoga, then recurses to make a list of ShadowNodes which have new layout updates.

iOS Paper uses that list directly for ordering of calling `reactSetFrame` which informs UIViews of their dimensions.

As part of aligning Paper to Fabric's model of top-down onLayout events, facebook@3168055 changed the ordering of the list from being pseudo-random, to always top-down.

After the change, we can deterministically see a case of infinite recursion where:
1. We set the frame of the rootview
2. This triggers `safeAreaInsetsDidChange` of `RCTSafeAreaView`, which doesn't have a frame yet
3. `RCTSafeAreaView` triggers sync relayout of root view

Assigning frames bottom up (and is also what Android Paper does) ensures we lay out the RootView last, which avoids the issue, while keeping a deterministic order.

Changelog:
[iOS][Fixed] - Call `reactSetFrame` on affected layouts bottom-up

Differential Revision: D51534209
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Nov 22, 2023
Summary:
Fixes facebook#41545

SafeAreaView works by adding padding in order to shift content out of the safe area. This may change the layout dimensions of the SafeAreaView, in turn effecting its safe area insets.

This can cause layout results to change, which in turn changes the inset value. Because of this, there is a tolerance, where safe area inset changes do not trigger a new update.

Yoga is instructed to round layout dimensions to the closest physical pixel, so a very small difference in layout may result being off by about a pixel. Right now the tolerance is exactly one physical pixel, and if there is FP error here, we may not pass the test, and start oscillating with different layout values.

After changing affected ShadowNode order to always be root-first, the first call to set the frame of the `SafeAreaView` happens when a non-zero-sized RootView is present, which I think may lead to a safe area inset update communicated that wasn't before? Or other cosmic butterflies. Layout rounds to one physical pixel in difference, and our tolerance is `0.00001` dips off (not helped that 1/3 screen scale cannot be represented as decimal, even without FP error).

This adds a small tolerance beyond just the pixel boundary, matching the logic in Fabric, which seems to resolve the issue.

Changelog:
[iOS][Fixed] - FP Tolerance in iOS Paper SafeAreaView debouncing

Reviewed By: philIip

Differential Revision: D51539091
NickGerleman added a commit to NickGerleman/react-native that referenced this issue Nov 22, 2023
Summary:

Fixes facebook#41545

SafeAreaView works by adding padding in order to shift content out of the safe area. This may change the layout dimensions of the SafeAreaView, in turn effecting its safe area insets.

This can cause layout results to change, which in turn changes the inset value. Because of this, there is a tolerance, where safe area inset changes do not trigger a new update.

Yoga is instructed to round layout dimensions to the closest physical pixel, so a very small difference in layout may result being off by about a pixel. Right now the tolerance is exactly one physical pixel, and if there is FP error here, we may not pass the test, and start oscillating with different layout values.

After changing affected ShadowNode order to always be root-first, the first call to set the frame of the `SafeAreaView` happens when a non-zero-sized RootView is present, which I think may lead to a safe area inset update communicated that wasn't before? Or other cosmic butterflies. Layout rounds to one physical pixel in difference, and our tolerance is `0.00001` dips off (not helped that 1/3 screen scale cannot be represented as decimal, even without FP error).

This adds a small tolerance beyond just the pixel boundary, matching the logic in Fabric, which seems to resolve the issue.

Changelog:
[iOS][Fixed] - FP Tolerance in iOS Paper SafeAreaView debouncing

Reviewed By: philIip

Differential Revision: D51539091
@NickGerleman
Copy link
Contributor

Pick request: reactwg/react-native-releases#64 (comment)

huntie pushed a commit that referenced this issue Nov 24, 2023
Summary:
Pull Request resolved: #41614

Fixes #41545

SafeAreaView works by adding padding in order to shift content out of the safe area. This may change the layout dimensions of the SafeAreaView, in turn effecting its safe area insets.

This can cause layout results to change, which in turn changes the inset value. Because of this, there is a tolerance, where safe area inset changes do not trigger a new update.

Yoga is instructed to round layout dimensions to the closest physical pixel, so a very small difference in layout may result being off by about a pixel. Right now the tolerance is exactly one physical pixel, and if there is FP error here, we may not pass the test, and start oscillating with different layout values.

After changing affected ShadowNode order to always be root-first, the first call to set the frame of the `SafeAreaView` happens when a non-zero-sized RootView is present, which I think may lead to a safe area inset update communicated that wasn't before? Or other cosmic butterflies. Layout rounds to one physical pixel in difference, and our tolerance is `0.00001` dips off (not helped that 1/3 screen scale cannot be represented as decimal, even without FP error).

This adds a small tolerance beyond just the pixel boundary, matching the logic in Fabric, which seems to resolve the issue.

Changelog:
[iOS][Fixed] - FP Tolerance in iOS Paper SafeAreaView debouncing

Reviewed By: philIip

Differential Revision: D51539091

fbshipit-source-id: 88bddc38c7cd8d93feef5f12da64b124af22f46d
Othinn pushed a commit to Othinn/react-native that referenced this issue Jan 9, 2024
Summary:
Pull Request resolved: facebook#41614

Fixes facebook#41545

SafeAreaView works by adding padding in order to shift content out of the safe area. This may change the layout dimensions of the SafeAreaView, in turn effecting its safe area insets.

This can cause layout results to change, which in turn changes the inset value. Because of this, there is a tolerance, where safe area inset changes do not trigger a new update.

Yoga is instructed to round layout dimensions to the closest physical pixel, so a very small difference in layout may result being off by about a pixel. Right now the tolerance is exactly one physical pixel, and if there is FP error here, we may not pass the test, and start oscillating with different layout values.

After changing affected ShadowNode order to always be root-first, the first call to set the frame of the `SafeAreaView` happens when a non-zero-sized RootView is present, which I think may lead to a safe area inset update communicated that wasn't before? Or other cosmic butterflies. Layout rounds to one physical pixel in difference, and our tolerance is `0.00001` dips off (not helped that 1/3 screen scale cannot be represented as decimal, even without FP error).

This adds a small tolerance beyond just the pixel boundary, matching the logic in Fabric, which seems to resolve the issue.

Changelog:
[iOS][Fixed] - FP Tolerance in iOS Paper SafeAreaView debouncing

Reviewed By: philIip

Differential Revision: D51539091

fbshipit-source-id: 88bddc38c7cd8d93feef5f12da64b124af22f46d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. Needs: Triage 🔍 Platform: iOS iOS applications. Type: Upgrade Issue Issues reported from upgrade issue form Version: unspecified No version information could be extracted
Projects
None yet
3 participants