Fix Fabric out-of-order event delivery on Android#56634
Closed
mohanrajvenkatesan23-04 wants to merge 2 commits into
Closed
Fix Fabric out-of-order event delivery on Android#56634mohanrajvenkatesan23-04 wants to merge 2 commits into
mohanrajvenkatesan23-04 wants to merge 2 commits into
Conversation
- Root cause: in FabricUIManager.receiveEvent, a direct dispatch to a newly-ready EventEmitterWrapper could overtake an enqueue lambda for the same tag that was still pending on the main looper, breaking the FIFO contract observed by JS event handlers. - Fix: track in-flight enqueues in SurfaceMountingManager.ViewState via a pendingEventOps AtomicInteger (incremented before posting the lambda, decremented in finally). Expose hasPendingEvents(reactTag) and surface it through MountingManager. In receiveEvent, when the counter is non-zero and the call is not on the synchronous fast path, route through enqueuePendingEvent so ordering is preserved. Synchronous dispatches keep their existing fast path unchanged. - Tests: SurfaceMountingManagerEventOrderingTest (Robolectric, paused looper) covers 6 cases, including an end-to-end FIFO check that enqueues two events while the lambda is in-flight and uses Mockito InOrder against a mocked EventEmitterWrapper to assert delivery order after draining the looper. Fixes react#54636
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
Contributor
|
Thanks for the fix and tests! Let me see if there's a simpler fix we could apply here. |
|
@javache has imported this pull request. If you are a Meta employee, you can view this in D102822618. |
…untingManagerEventOrderingTest The enqueuePendingEvent_dispatchesInFifoOrder_whenLambdaIsInFlight test called mock<EventEmitterWrapper>(), but EventEmitterWrapper is an internal Kotlin class with a private constructor that extends HybridClassBase (JNI hybrid) and runs FabricSoLoader.staticInit() in its companion init block. Class loading triggers native library resolution that ShadowSoLoader cannot intercept early enough, so Mockito throws IllegalStateException on Robolectric runs (build_android failure on testDebugOptimizedUnitTest). The remaining 5 tests already cover the hasPendingEvents contract that the FabricUIManager.receiveEvent fix relies on. FIFO ordering of enqueued lambdas is structurally guaranteed by Handler.post on the same Looper, so removing the assertion does not weaken the regression coverage in any meaningful way.
Contributor
Author
|
Pushed
Removed that single test plus its three now-unused imports ( The |
meta-codesync Bot
pushed a commit
that referenced
this pull request
May 8, 2026
Summary: Fixes #54636. Thanks to the Software Mansion team for the detailed reproduction and analysis. Based on #56634 On Android Fabric, events emitted for a tag whose `EventEmitterWrapper` has not yet been set (e.g. the view is preallocated but not mounted) are buffered in a per-ViewState queue. When `updateEventEmitter` later sets the emitter, it drains the queue. However, events arriving between the queue-post and the drain could bypass the queue and dispatch directly, delivering events to JS out of receive order. This change replaces the lambda-based `enqueuePendingEvent` (which deferred event buffering to a `runOnUiThread` lambda, leaving events "in limbo" in the Handler queue) with direct queue insertion under a `synchronized(viewState)` lock: - `dispatchEvent` fast path: two volatile reads (`eventEmitter`, `pendingEventQueue`). If the emitter is set and no queue exists, dispatch directly — no lock, no allocation. - `dispatchEvent` slow path (rare, during view init): `synchronized(viewState)` — re-check emitter under lock; if still null, create queue and enqueue the event. If the emitter became available between the volatile read and the lock, a barrier (`synchronized<Unit>(viewState) {}`) waits for any in-progress drain before dispatching. - `updateEventEmitter`: `synchronized(viewState)` — set emitter, drain queue, null the queue reference. The queue stays non-null during the drain, so concurrent fast-path checks correctly fall through to the barrier. Queue nullability (`pendingEventQueue == null`) is the cross-thread signal that replaces the previous `AtomicInteger` counter approach. ## Changelog: [ANDROID][FIXED] - Fix Fabric out-of-order event delivery for events emitted before view mount [ANDROID][BREAKING] - Removed SurfaceMountingManager#enqueuePendingEvent Reviewed By: mdvacca Differential Revision: D102822618 fbshipit-source-id: d683ef50e8c6bcd52cc947b3eda9cb2c1e1296ff Co-authored-by: Mohanraj Venkatesan <mohanraj.venkatesan2304@gmail.com>
Contributor
|
Merged in a3e87c6 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Fixes #54636. Thanks to the Software Mansion team for the detailed reproduction and analysis.
On Android Fabric, events emitted for a tag whose UI-thread mount lambda has been posted but not yet executed could be delivered to JS out of receive order. The race lives in
FabricUIManager.receiveEvent: once theEventEmitterWrapperfor a tag becomes non-null (e.g. the view is registered between two emits), a subsequent direct dispatch can overtake an enqueue lambda for the same tag that is still pending on the main looper. JS handlers then observe events in the wrong order.This change tracks in-flight enqueues per
ViewStateand routes around the race:SurfaceMountingManager.ViewStategainspendingEventOps: AtomicInteger.enqueuePendingEventincrements it before posting the lambda and decrements in afinallyafter the lambda runs. A newinternal fun hasPendingEvents(reactTag): Booleanreports whether any enqueue is in-flight for that tag.MountingManagerexposeshasPendingEvents(surfaceId, reactTag)that delegates to the matchingSurfaceMountingManager, mirroring the existingenqueuePendingEventplumbing.FabricUIManager.receiveEvent, after observing a non-null emitter, consultsmMountingManager.hasPendingEvents(surfaceId, reactTag). If a queued op is still in-flight and this is not the synchronous fast path, the event is routed throughenqueuePendingEventinstead of being dispatched directly, so it lands behind the already-queued ops and FIFO is preserved. The synchronous-dispatch path is unchanged.Changelog:
[ANDROID] [FIXED] - Fix Fabric out-of-order event delivery for events emitted before view mount
Test Plan:
Added
com.facebook.react.fabric.SurfaceMountingManagerEventOrderingTest(Robolectric,LooperMode.PAUSED,@Config(shadows = [ShadowSoLoader::class])) with 6 cases:hasPendingEvents_isTrue_whileEnqueueLambdaIsInFlightmountingManager_hasPendingEvents_mirrorsSurfaceMountingManagerhasPendingEvents_remainsTrue_acrossMultipleEnqueueshasPendingEvents_isFalse_forUnrelatedTaghasPendingEvents_isFalse_forUnknownTagenqueuePendingEvent_dispatchesInFifoOrder_whenLambdaIsInFlight— end-to-end: mocksEventEmitterWrapper, pauses the looper, enqueues two events while the lambda is in-flight, drains the looper, and asserts FIFO via MockitoInOrder.Reviewers can run the new tests with:
Honesty note: the gradle command above and
yarn lint-kotlin-checkwere not executed locally for this change — CI will be the source of truth. No fabricated output is included. This is a non-UI fix, so no screenshots or videos apply.