Skip to content

EventQueueProcessor/EventTarget: SCOPE_EXIT-guard release on throwing dispatch + fix unsigned retainCount_ underflow (#56490)#56537

Closed
meta-yaohway wants to merge 1 commit into
react:mainfrom
meta-yaohway:export-D100280132
Closed

EventQueueProcessor/EventTarget: SCOPE_EXIT-guard release on throwing dispatch + fix unsigned retainCount_ underflow (#56490)#56537
meta-yaohway wants to merge 1 commit into
react:mainfrom
meta-yaohway:export-D100280132

Conversation

@meta-yaohway

Copy link
Copy Markdown
Contributor

Summary:
1) EventQueueProcessor::flushEvents — release on every exit path. flushEvents retains every EventTarget in the batch, then for each event calls eventPipe_ (which dispatches into JS via UIManagerBinding::dispatchEventToJS and can throw jsi::JSError), then eventPipeConclusion_, and finally runs a trailing loop that calls event.eventTarget->release(runtime). If either pipe call throws, stack unwinding skips the release loop, leaving strongInstanceHandle_ (a jsi::Value strong reference to the JS event-target object) pinned and retainCount_ desynchronised.

This change replaces the trailing release loop with a folly SCOPE_EXIT block immediately after the retain pass, so every retained target is released on every exit path — normal return, eventPipe_ throw, or eventPipeConclusion_ throw. (Earlier revisions used a hand-rolled RAII class; switched to SCOPE_EXIT per reviewer feedback. folly/ScopeGuard.h is already in the OSS RN folly subset — RCT-Folly.podspec, ReactAndroid/.../folly/CMakeLists.txt, react-native-fantom/.../folly/CMakeLists.txt — and is a transitive dep of //xplat/folly:dynamic, so no new build edges.)

Behaviour-preserving on the happy path: same release ordering (after eventPipeConclusion_), same no-DispatchMutex policy for release (the events vector still holds the strong shared_ptrs), zero added allocation.

2) EventTarget::release — guard the unsigned decrement. retainCount_ is size_t. retain() early-returns when the target is disabled, but the matching release() always runs, so release() can be entered with retainCount_ == 0; the unconditional --retainCount_ then wraps to SIZE_MAX, the == 0 check never fires again, and strongInstanceHandle_ is never cleared. The trailing react_native_assert(retainCount_ >= 0) is a tautology on an unsigned type and is removed. This change guards the decrement (if (retainCount_ > 0) --retainCount_;) and drops the dead assert and the now-unused react_native_assert.h include.

Adds EventQueueProcessorTest.releasesEventTargetsWhenDispatchThrows (proves the strong handle is released during unwind when eventPipe_ throws; fails on the pre-fix trailing-loop code) and EventTargetTests.releaseWhileDisabledDoesNotCorruptRetainCount (retain/release while disabled, then enable+retain must still acquire the handle; fails on the pre-fix unconditional decrement).

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D100280132

… dispatch + fix unsigned retainCount_ underflow (react#56490)

Summary:
**1) `EventQueueProcessor::flushEvents` — release on every exit path.** `flushEvents` retains every `EventTarget` in the batch, then for each event calls `eventPipe_` (which dispatches into JS via `UIManagerBinding::dispatchEventToJS` and can throw `jsi::JSError`), then `eventPipeConclusion_`, and finally runs a trailing loop that calls `event.eventTarget->release(runtime)`. If either pipe call throws, stack unwinding skips the release loop, leaving `strongInstanceHandle_` (a `jsi::Value` strong reference to the JS event-target object) pinned and `retainCount_` desynchronised.

This change replaces the trailing release loop with a folly `SCOPE_EXIT` block immediately after the retain pass, so every retained target is released on every exit path — normal return, `eventPipe_` throw, or `eventPipeConclusion_` throw. (Earlier revisions used a hand-rolled RAII class; switched to `SCOPE_EXIT` per reviewer feedback. `folly/ScopeGuard.h` is already in the OSS RN folly subset — `RCT-Folly.podspec`, `ReactAndroid/.../folly/CMakeLists.txt`, `react-native-fantom/.../folly/CMakeLists.txt` — and is a transitive dep of `//xplat/folly:dynamic`, so no new build edges.)

Behaviour-preserving on the happy path: same release ordering (after `eventPipeConclusion_`), same no-`DispatchMutex` policy for release (the `events` vector still holds the strong `shared_ptr`s), zero added allocation.

**2) `EventTarget::release` — guard the unsigned decrement.** `retainCount_` is `size_t`. `retain()` early-returns when the target is disabled, but the matching `release()` always runs, so `release()` can be entered with `retainCount_ == 0`; the unconditional `--retainCount_` then wraps to `SIZE_MAX`, the `== 0` check never fires again, and `strongInstanceHandle_` is never cleared. The trailing `react_native_assert(retainCount_ >= 0)` is a tautology on an unsigned type and is removed. This change guards the decrement (`if (retainCount_ > 0) --retainCount_;`) and drops the dead assert and the now-unused `react_native_assert.h` include.

Adds `EventQueueProcessorTest.releasesEventTargetsWhenDispatchThrows` (proves the strong handle is released during unwind when `eventPipe_` throws; fails on the pre-fix trailing-loop code) and `EventTargetTests.releaseWhileDisabledDoesNotCorruptRetainCount` (retain/release while disabled, then enable+retain must still acquire the handle; fails on the pre-fix unconditional decrement).

Changelog: [Internal]


Reviewed By: javache

Differential Revision: D100280132
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 21, 2026
@meta-codesync

meta-codesync Bot commented Apr 21, 2026

Copy link
Copy Markdown

@meta-yaohway has exported this pull request. If you are a Meta employee, you can view the originating Diff in D100280132.

@meta-codesync meta-codesync Bot closed this in ccff70b Apr 25, 2026
@facebook-github-tools facebook-github-tools Bot added the Merged This PR has been merged. label Apr 25, 2026
@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @meta-yaohway in ccff70b

When will my fix make it into a release? | How to file a pick request?

@meta-codesync

meta-codesync Bot commented Apr 25, 2026

Copy link
Copy Markdown

This pull request has been merged in ccff70b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. meta-exported p: Facebook Partner: Facebook Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants