From 006f5afe120c290a37cf6ff896748fbc062bf7ed Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Sat, 19 Jun 2021 22:55:36 -0700 Subject: [PATCH] Guard against unsafe EventEmitter setup and teardown Summary: Because of T92179998, T93607943, and T93394807, we are still seeking resolution to tricky crashes wrt the use of EventEmitters. I believe the recent spike is because of two recent changes: we pass in EventEmitters earlier, during PreAllocation; and we clean them up earlier, during stopSurface, to avoid jsi::~Pointer crashes. Additionally, the gating previously added around the PreAllocation path was incorrect and led to more nullptrs being passed around as EventEmitters. To mitigate these issues: 1) I am adding/fixing gating to preallocation and early cleanup paths 2) I am making EventEmitterWrapper more resilient by ensuring EventEmitter is non-null before invoking it. 3) I am making sure that in more cases, we pass a non-null EventEmitter pointer to Java. 4) I am backing out the synchronization in EventEmitterWrapper (java side) as that did not resolve the issue and is a pessimisation There are older, unchanged paths that could still be passing in nullptr as the EventEmitter (Update and Create). As those have not changed recently, I'm not going to fix those cases and instead, we can now rely on the caller to ensure that the EventEmitter is non-null before calling. Changelog: [internal] Differential Revision: D29252806 fbshipit-source-id: 5c68d95fa2465afe45e0083a0685c8c1abf31619 --- .../react/config/ReactFeatureFlags.java | 2 ++ .../fabric/events/EventEmitterWrapper.java | 26 +++++++------------ .../com/facebook/react/fabric/jni/Binding.cpp | 11 +++++--- .../react/fabric/jni/EventEmitterWrapper.cpp | 16 +++++++++--- .../mounting/SurfaceMountingManager.java | 9 ++++--- 5 files changed, 38 insertions(+), 26 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index f5b43205099..006398ac7d9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -66,6 +66,8 @@ public class ReactFeatureFlags { public static boolean enableLockFreeEventDispatcher = false; + public static boolean enableAggressiveEventEmitterCleanup = false; + // // ScrollView C++ UpdateState vs onScroll race fixes // diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/events/EventEmitterWrapper.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/events/EventEmitterWrapper.java index 7938b793c5d..69d0e1c8a8a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/events/EventEmitterWrapper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/events/EventEmitterWrapper.java @@ -48,13 +48,11 @@ public class EventEmitterWrapper { * @param params {@link WritableMap} payload of the event */ public void invoke(@NonNull String eventName, @Nullable WritableMap params) { - synchronized (mHybridData) { - if (!isValid()) { - return; - } - NativeMap payload = params == null ? new WritableNativeMap() : (NativeMap) params; - invokeEvent(eventName, payload); + if (!isValid()) { + return; } + NativeMap payload = params == null ? new WritableNativeMap() : (NativeMap) params; + invokeEvent(eventName, payload); } /** @@ -66,20 +64,16 @@ public class EventEmitterWrapper { */ public void invokeUnique( @NonNull String eventName, @Nullable WritableMap params, int customCoalesceKey) { - synchronized (mHybridData) { - if (!isValid()) { - return; - } - NativeMap payload = params == null ? new WritableNativeMap() : (NativeMap) params; - invokeUniqueEvent(eventName, payload, customCoalesceKey); + if (!isValid()) { + return; } + NativeMap payload = params == null ? new WritableNativeMap() : (NativeMap) params; + invokeUniqueEvent(eventName, payload, customCoalesceKey); } public void destroy() { - synchronized (mHybridData) { - if (mHybridData != null) { - mHybridData.resetNative(); - } + if (mHybridData != null) { + mHybridData.resetNative(); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index cb2179bc6fb..9aa47320d35 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -1151,11 +1151,14 @@ void Binding::preallocateShadowView( } // Do not hold a reference to javaEventEmitter from the C++ side. - auto javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs(); + local_ref javaEventEmitter = nullptr; if (enableEarlyEventEmitterUpdate_) { SharedEventEmitter eventEmitter = shadowView.eventEmitter; - EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter); - cEventEmitter->eventEmitter = eventEmitter; + if (eventEmitter != nullptr) { + javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs(); + EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter); + cEventEmitter->eventEmitter = eventEmitter; + } } local_ref props = castReadableMap( @@ -1169,7 +1172,7 @@ void Binding::preallocateShadowView( component.get(), props.get(), (javaStateWrapper != nullptr ? javaStateWrapper.get() : nullptr), - javaEventEmitter.get(), + (javaEventEmitter != nullptr ? javaEventEmitter.get() : nullptr), isLayoutableShadowNode); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp index afb73f9927a..b4e98990575 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.cpp @@ -21,8 +21,13 @@ EventEmitterWrapper::initHybrid(jni::alias_ref) { void EventEmitterWrapper::invokeEvent( std::string eventName, NativeMap *payload) { - eventEmitter->dispatchEvent( - eventName, payload->consume(), EventPriority::AsynchronousBatched); + // It is marginal, but possible for this to be constructed without a valid + // EventEmitter. In those cases, make sure we noop/blackhole events instead of + // crashing. + if (eventEmitter != nullptr) { + eventEmitter->dispatchEvent( + eventName, payload->consume(), EventPriority::AsynchronousBatched); + } } void EventEmitterWrapper::invokeUniqueEvent( @@ -30,7 +35,12 @@ void EventEmitterWrapper::invokeUniqueEvent( NativeMap *payload, int customCoalesceKey) { // TODO: customCoalesceKey currently unused - eventEmitter->dispatchUniqueEvent(eventName, payload->consume()); + // It is marginal, but possible for this to be constructed without a valid + // EventEmitter. In those cases, make sure we noop/blackhole events instead of + // crashing. + if (eventEmitter != nullptr) { + eventEmitter->dispatchUniqueEvent(eventName, payload->consume()); + } } void EventEmitterWrapper::registerNatives() { diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index ba39ab4a613..f394b073c1a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -27,6 +27,7 @@ import com.facebook.react.bridge.RetryableMountingLayerException; import com.facebook.react.bridge.SoftAssertions; import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.common.build.ReactBuildConfig; +import com.facebook.react.config.ReactFeatureFlags; import com.facebook.react.fabric.events.EventEmitterWrapper; import com.facebook.react.fabric.mounting.MountingManager.MountItemExecutor; import com.facebook.react.fabric.mounting.mountitems.MountItem; @@ -250,9 +251,11 @@ public class SurfaceMountingManager { viewState.mStateWrapper.destroyState(); viewState.mStateWrapper = null; } - if (viewState.mEventEmitter != null) { - viewState.mEventEmitter.destroy(); - viewState.mEventEmitter = null; + if (ReactFeatureFlags.enableAggressiveEventEmitterCleanup) { + if (viewState.mEventEmitter != null) { + viewState.mEventEmitter.destroy(); + viewState.mEventEmitter = null; + } } }