fix: backport RN scheduler delegate UAF guard#93878
Conversation
|
|
e403fd3 to
28138a4
Compare
|
@mkhutornyi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@mkhutornyi Kind ping here. Thanks! |
|
@mkhutornyi Kind ping here, Thanks! |
mkhutornyi
left a comment
There was a problem hiding this comment.
- Run
python3 /Users/kushjariwla/.codex/skills/expensify-app-targeted-checks/scripts/touched_files.py --base upstream/main- Verify the only touched files are
patches/react-native/details.mdandpatches/react-native/react-native+0.83.1+040+fix-runtime-scheduler-delegate-uaf-APP-25V.patch- Run
npx prettier --write patches/react-native/details.md- Run
git diff --check- Run
git diff --check upstream/main- Apply
patches/react-native/react-native+0.83.1+040+fix-runtime-scheduler-delegate-uaf-APP-25V.patchwithpatch --dry-runagainst fresh RNv0.83.1Scheduler.cppandScheduler.hsources- Verify the patch applies cleanly to both RN scheduler files
Please avoid this kind of tests steps in future PRs. It should be real test scenario users can follow.
Screenshots/Videos
iOS: Native
N/A. This PR does not change the UI.
iOS test is needed.
Understood. Actually, up until now, I was putting the test scenarios in the QA steps and the literal tests that were run under 'Tests'. Now I understand how it should be done. Updated PR body for same. Thanks!
@mkhutornyi Actually i do not have access to hybrid app, i can test from an iOS HybridApp ad-hoc test build for this PR with |
yes, standalone app test is fine for you |
|
Done, Thanks! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safari |
|
@codex review |
|
@MelvinBot review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@mountiny Kind ping here. Thanks! |
mountiny
left a comment
There was a problem hiding this comment.
@jakubstec for visibility we will need to address this in future rn updates
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.22-1 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.24-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.24-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.24-0 🚀
|
| - Reason: Fixes a fatal iOS HybridApp crash (APP-25V) where `RuntimeScheduler_Modern::runEventLoopTick` can drain deferred Fabric rendering updates after the captured `SchedulerDelegate` has been torn down. RN 0.83.1 queues `Scheduler::uiManagerDidFinishTransaction` and `Scheduler::uiManagerDidDispatchCommand` callbacks with lambdas that capture the raw `delegate_` pointer by value. During OldDot↔NewDot lifecycle churn, the delegate can be replaced or destroyed before the scheduled rendering update runs, causing `EXC_BAD_ACCESS` when the lambda dereferences stale native memory. This patch backports the upstream RN scheduler-delegate invalidation guard by adding a per-delegate `shared_ptr<atomic<bool>>` token, invalidating the old token on delegate changes and Scheduler destruction, and making already-queued lambdas no-op before touching a stale delegate. | ||
| - Upstream PR/issue: https://github.com/facebook/react-native/pull/56680 / https://github.com/facebook/react-native/commit/aadbe965792bd900ca70412d6704b76e339d1aca | ||
| - E/App issue: https://github.com/Expensify/App/issues/92412 | ||
| - PR introducing patch: 🛑 |
There was a problem hiding this comment.
Please follow the PATCHES.md guide when adding a new patch 🙏
Ideally, every patch should have a PR introducing patch field filled out, so it's easier to find a PR that introduced it. It's way easier to verify if such patch is still necessary or not after some time. Thanks!
There was a problem hiding this comment.
@KJ21-ENG Can you please create a follow up pr for this? thanks!
Sorry for missing that
There was a problem hiding this comment.
Np, I'm just making sure the new patches are at least linked to the PRs. Also, this is not a big deal because the patch will be removed in the next RN bump, but let's update it for clarity
There was a problem hiding this comment.
@KJ21-ENG Can you please create a follow up pr for this? thanks!
Sorry for missing that
On it.
There was a problem hiding this comment.
Follow-up PR created: #96445
cc: @mkhutornyi @mountiny
Explanation of Change
Backports the upstream React Native scheduler delegate invalidation guard from RN 0.86 into the RN 0.83.1 patch layer.
RN 0.83.1 schedules deferred Fabric rendering-update lambdas that capture the raw
SchedulerDelegate*. During HybridApp lifecycle churn, the delegate can be torn down beforeRuntimeScheduler_Moderndrains the queued rendering update, so this patch adds a per-delegate invalidation token and makes already-queued lambdas no-op before dereferencing a stale delegate.The guard is intentionally unconditional in this backport. Upstream RN gates it behind
enableSchedulerDelegateInvalidation(), but this branch does not backport RN's feature-flag plumbing and follows the approved proposal's unconditional option because calling a destroyed delegate is never valid.Fixed Issues
$ #92412
PROPOSAL: #92412 (comment)
Tests
RuntimeScheduler_Modern::runEventLoopTick.Offline tests
N/A. This is a native React Native scheduler lifetime patch and does not change app network/offline behavior.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A. This PR does not change the UI.
Android: mWeb Chrome
N/A. This PR does not change the UI.
iOS: Native
ezyZip.58.mp4
iOS: mWeb Safari
N/A. This PR does not change the UI.
MacOS: Chrome / Safari
N/A. This PR does not change the UI.