From 751eae94d4e5fa5973d36702cb5770e0ee72ffd7 Mon Sep 17 00:00:00 2001 From: Wojciech Lewicki Date: Fri, 26 Jan 2024 12:41:50 -0800 Subject: [PATCH] fix: restore proper event dispatch order on Fabric (#42664) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This PR fixes the cpp crash coming from events being consumed in `dispatchModern` before forwarding it to listeners: https://github.com/facebook/react-native/blob/ccff2bb8d19b2db244f30293b4e8d68a524c2059/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/FabricEventDispatcher.java#L42-L45 Because of this, all listeners that require reading from the native map (like RNReanimated using event.toString()) cause a crash since the data is no longer available - [https://github.com/facebook/react-native/blob/49f6ffc92f56953b68f340783e326145a377[…]/react-native/ReactAndroid/src/main/jni/react/jni/NativeMap.cpp](https://github.com/facebook/react-native/blob/49f6ffc92f56953b68f340783e326145a377c3b0/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeMap.cpp#L16-L19) Restoring the previous behavior, where the event was forwarded to the listeners first, and only then dispatched further solves the problem: https://github.com/facebook/react-native/blob/ccff2bb8d19b2db244f30293b4e8d68a524c2059/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/EventDispatcherImpl.java#L116-L126 Was there a reason for changing the order here? Or maybe there was another reason why the order was changed? ## Changelog: [ANDROID] [FIXED] - Proper event dispatch order on Fabric Pull Request resolved: https://github.com/facebook/react-native/pull/42664 Test Plan: Run: https://github.com/WoLewicki/NewArchStylingBug/tree/%40wolewicki/show-fabric-event-crash and try to move the square. It will cause a crash in here: https://github.com/facebook/react-native/blob/ccff2bb8d19b2db244f30293b4e8d68a524c2059/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/EventAnimationDriver.java#L110 when trying to read from a consumed map. Reviewed By: sammy-SC, jessebwr Differential Revision: D53089137 Pulled By: mdvacca fbshipit-source-id: 8c282a417d1889732792070a404cf87acad98523 --- .../facebook/react/uimanager/events/FabricEventDispatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/FabricEventDispatcher.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/FabricEventDispatcher.java index a1515559cd1..4a3650f2deb 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/FabricEventDispatcher.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/FabricEventDispatcher.java @@ -39,10 +39,10 @@ public class FabricEventDispatcher implements EventDispatcher, LifecycleEventLis @Override public void dispatchEvent(Event event) { - event.dispatchModern(mReactEventEmitter); for (EventDispatcherListener listener : mListeners) { listener.onEventDispatch(event); } + event.dispatchModern(mReactEventEmitter); event.dispose(); maybePostFrameCallbackFromNonUI();