From 36f3bf2f583596a08a59e1f65016aaf4652fc341 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Sat, 2 Oct 2021 09:02:20 -0700 Subject: [PATCH] Add option to use raw pointer instead of shared_ptr in EventEmitterWrapper Summary: changelog: [internal] Retaining `EventEmitter` beyond runtime triggers a crash. Let's try to use raw pointer in `EventEmitterWrapper` to see if it fixes some crashes. Reviewed By: philIip Differential Revision: D31307332 fbshipit-source-id: cd059b6c56f8dffe985b3ecb62cdafe823ba1462 --- .../com/facebook/react/fabric/jni/Binding.cpp | 27 ++++++++++++++----- .../com/facebook/react/fabric/jni/Binding.h | 1 + .../react/fabric/jni/EventEmitterWrapper.cpp | 14 ++++++++++ .../react/fabric/jni/EventEmitterWrapper.h | 1 + 4 files changed, 37 insertions(+), 6 deletions(-) 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 edbe5613e66ad4..bdabaaf8821abe 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 @@ -243,8 +243,9 @@ Binding::getInspectorDataForInstance( } EventEmitterWrapper *cEventEmitter = cthis(eventEmitterWrapper); - InspectorData data = - scheduler->getInspectorDataForInstance(*cEventEmitter->eventEmitter); + InspectorData data = scheduler->getInspectorDataForInstance( + enableEventEmitterRawPointer_ ? *cEventEmitter->eventEmitterPointer + : *cEventEmitter->eventEmitter); folly::dynamic result = folly::dynamic::object; result["fileName"] = data.fileName; @@ -511,6 +512,9 @@ void Binding::installFabricUIManager( disableRevisionCheckForPreallocation_ = config->getBool("react_fabric:disable_revision_check_for_preallocation"); + enableEventEmitterRawPointer_ = + config->getBool("react_fabric:enable_event_emitter_wrapper_raw_pointer"); + if (enableFabricLogs_) { LOG(WARNING) << "Binding::installFabricUIManager() was called (address: " << this << ")."; @@ -920,8 +924,11 @@ void Binding::schedulerDidFinishTransaction( mountItem.newChildShadowView.eventEmitter; auto javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs(); EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter); - cEventEmitter->eventEmitter = eventEmitter; - + if (enableEventEmitterRawPointer_) { + cEventEmitter->eventEmitterPointer = eventEmitter.get(); + } else { + cEventEmitter->eventEmitter = eventEmitter; + } temp[0] = mountItem.newChildShadowView.tag; temp[1] = isLayoutable; env->SetIntArrayRegion(intBufferArray, intBufferPosition, 2, temp); @@ -1071,7 +1078,11 @@ void Binding::schedulerDidFinishTransaction( // Do not hold a reference to javaEventEmitter from the C++ side. auto javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs(); EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter); - cEventEmitter->eventEmitter = eventEmitter; + if (enableEventEmitterRawPointer_) { + cEventEmitter->eventEmitterPointer = eventEmitter.get(); + } else { + cEventEmitter->eventEmitter = eventEmitter; + } (*objBufferArray)[objBufferPosition++] = javaEventEmitter.get(); } @@ -1198,7 +1209,11 @@ void Binding::preallocateShadowView( if (eventEmitter != nullptr) { javaEventEmitter = EventEmitterWrapper::newObjectJavaArgs(); EventEmitterWrapper *cEventEmitter = cthis(javaEventEmitter); - cEventEmitter->eventEmitter = eventEmitter; + if (enableEventEmitterRawPointer_) { + cEventEmitter->eventEmitterPointer = eventEmitter.get(); + } else { + cEventEmitter->eventEmitter = eventEmitter; + } } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h index 9750a51bf2376d..717d1be076888e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h @@ -202,6 +202,7 @@ class Binding : public jni::HybridClass, bool enableFabricLogs_{false}; bool enableEarlyEventEmitterUpdate_{false}; bool disableRevisionCheckForPreallocation_{false}; + bool enableEventEmitterRawPointer_{false}; }; } // namespace react 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 af207a64b7c1b1..b535c2ca7500a9 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 @@ -22,6 +22,15 @@ void EventEmitterWrapper::invokeEvent( std::string eventName, NativeMap *payload, int category) { + if (eventEmitterPointer) { + eventEmitterPointer->dispatchEvent( + std::move(eventName), + payload->consume(), + EventPriority::AsynchronousBatched, + static_cast(category)); + return; + } + // 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. @@ -38,6 +47,11 @@ void EventEmitterWrapper::invokeUniqueEvent( std::string eventName, NativeMap *payload, int customCoalesceKey) { + if (eventEmitterPointer) { + eventEmitterPointer->dispatchUniqueEvent( + std::move(eventName), payload->consume()); + return; + } // TODO: customCoalesceKey currently unused // 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 diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h index bc2d3cac90a48a..b36911da928cae 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/EventEmitterWrapper.h @@ -24,6 +24,7 @@ class EventEmitterWrapper : public jni::HybridClass { static void registerNatives(); SharedEventEmitter eventEmitter; + EventEmitter const *eventEmitterPointer; void invokeEvent(std::string eventName, NativeMap *params, int category); void invokeUniqueEvent(