From 4a5060f4cbcb54b261dfa86b387ecccacb876039 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 18 Jun 2019 10:34:13 -0700 Subject: [PATCH] Binding.cpp will use locks to guard access to scheduler and UIManager Summary: It is possible that race conditions exist in access of scheduler and UIManager in Binding.cpp, such that deallocated or null schedulers/UIManagers could be passed as arguments into various functions, causing strange and exotic crashes down the line once garbage memory (or null pointers) are dereferenced. I think it's possible that navigating away from a Fabric screen while there's high memory pressure could cause things to get deallocated more aggressively and cause scheduler and/or UIManager to disappear while mounting instructions are being created, for instance. Reviewed By: mdvacca Differential Revision: D15863193 fbshipit-source-id: a85cb71d88ea54826b40f44e788931dfc422f045 --- .../com/facebook/react/fabric/jni/Binding.cpp | 183 ++++++++++++------ .../com/facebook/react/fabric/jni/Binding.h | 5 + 2 files changed, 130 insertions(+), 58 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 78b4305470a..e51d2cc1d73 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 @@ -23,6 +23,8 @@ #include #include +#include + using namespace facebook::jni; using namespace facebook::jsi; @@ -46,15 +48,33 @@ jni::local_ref Binding::initHybrid( return makeCxxInstance(); } +// Thread-safe getter +jni::global_ref Binding::getJavaUIManager() { + std::lock_guard uiManagerLock(javaUIManagerMutex_); + return javaUIManager_; +} + +// Thread-safe getter +std::shared_ptr Binding::getScheduler() { + std::lock_guard lock(schedulerMutex_); + return scheduler_; +} + + void Binding::startSurface( jint surfaceId, jni::alias_ref moduleName, NativeMap *initialProps) { SystraceSection s("FabricUIManagerBinding::startSurface"); - if (scheduler_) { - scheduler_->startSurface( - surfaceId, moduleName->toStdString(), initialProps->consume()); + + std::shared_ptr scheduler = getScheduler(); + if (!scheduler) { + LOG(ERROR) << "Binding::startSurface: scheduler disappeared"; + return; } + + scheduler->startSurface( + surfaceId, moduleName->toStdString(), initialProps->consume()); } void Binding::startSurfaceWithConstraints( @@ -65,41 +85,58 @@ void Binding::startSurfaceWithConstraints( jfloat maxWidth, jfloat minHeight, jfloat maxHeight) { - if (scheduler_) { - auto minimumSize = - Size{minWidth / pointScaleFactor_, minHeight / pointScaleFactor_}; - auto maximumSize = - Size{maxWidth / pointScaleFactor_, maxHeight / pointScaleFactor_}; + SystraceSection s("FabricUIManagerBinding::startSurfaceWithConstraints"); - LayoutContext context; - context.pointScaleFactor = {pointScaleFactor_}; - LayoutConstraints constraints = {}; - constraints.minimumSize = minimumSize; - constraints.maximumSize = maximumSize; - - scheduler_->startSurface( - surfaceId, - moduleName->toStdString(), - initialProps->consume(), - constraints, - context); + std::shared_ptr scheduler = getScheduler(); + if (!scheduler) { + LOG(ERROR) << "Binding::startSurfaceWithConstraints: scheduler disappeared"; + return; } + + auto minimumSize = + Size{minWidth / pointScaleFactor_, minHeight / pointScaleFactor_}; + auto maximumSize = + Size{maxWidth / pointScaleFactor_, maxHeight / pointScaleFactor_}; + + LayoutContext context; + context.pointScaleFactor = {pointScaleFactor_}; + LayoutConstraints constraints = {}; + constraints.minimumSize = minimumSize; + constraints.maximumSize = maximumSize; + + scheduler->startSurface( + surfaceId, + moduleName->toStdString(), + initialProps->consume(), + constraints, + context); } void Binding::renderTemplateToSurface(jint surfaceId, jstring uiTemplate) { SystraceSection s("FabricUIManagerBinding::renderTemplateToSurface"); - if (scheduler_) { - auto env = Environment::current(); - const char *nativeString = env->GetStringUTFChars(uiTemplate, JNI_FALSE); - scheduler_->renderTemplateToSurface(surfaceId, nativeString); - env->ReleaseStringUTFChars(uiTemplate, nativeString); + + std::shared_ptr scheduler = getScheduler(); + if (!scheduler) { + LOG(ERROR) << "Binding::renderTemplateToSurface: scheduler disappeared"; + return; } + + auto env = Environment::current(); + const char *nativeString = env->GetStringUTFChars(uiTemplate, JNI_FALSE); + scheduler->renderTemplateToSurface(surfaceId, nativeString); + env->ReleaseStringUTFChars(uiTemplate, nativeString); } void Binding::stopSurface(jint surfaceId) { - if (scheduler_) { - scheduler_->stopSurface(surfaceId); + SystraceSection s("FabricUIManagerBinding::stopSurface"); + + std::shared_ptr scheduler = getScheduler(); + if (!scheduler) { + LOG(ERROR) << "Binding::stopSurface: scheduler disappeared"; + return; } + + scheduler->stopSurface(surfaceId); } void Binding::setConstraints( @@ -108,20 +145,26 @@ void Binding::setConstraints( jfloat maxWidth, jfloat minHeight, jfloat maxHeight) { - if (scheduler_) { - auto minimumSize = - Size{minWidth / pointScaleFactor_, minHeight / pointScaleFactor_}; - auto maximumSize = - Size{maxWidth / pointScaleFactor_, maxHeight / pointScaleFactor_}; + SystraceSection s("FabricUIManagerBinding::setConstraints"); - LayoutContext context; - context.pointScaleFactor = {pointScaleFactor_}; - LayoutConstraints constraints = {}; - constraints.minimumSize = minimumSize; - constraints.maximumSize = maximumSize; - - scheduler_->constraintSurfaceLayout(surfaceId, constraints, context); + std::shared_ptr scheduler = getScheduler(); + if (!scheduler) { + LOG(ERROR) << "Binding::setConstraints: scheduler disappeared"; + return; } + + auto minimumSize = + Size{minWidth / pointScaleFactor_, minHeight / pointScaleFactor_}; + auto maximumSize = + Size{maxWidth / pointScaleFactor_, maxHeight / pointScaleFactor_}; + + LayoutContext context; + context.pointScaleFactor = {pointScaleFactor_}; + LayoutConstraints constraints = {}; + constraints.minimumSize = minimumSize; + constraints.maximumSize = maximumSize; + + scheduler->constraintSurfaceLayout(surfaceId, constraints, context); } void Binding::installFabricUIManager( @@ -132,6 +175,12 @@ void Binding::installFabricUIManager( ComponentFactoryDelegate *componentsRegistry, jni::alias_ref reactNativeConfig) { SystraceSection s("FabricUIManagerBinding::installFabricUIManager"); + + // Use std::lock and std::adopt_lock to prevent deadlocks by locking mutexes at the same time + std::lock(schedulerMutex_, javaUIManagerMutex_); + std::lock_guard schedulerLock(schedulerMutex_, std::adopt_lock); + std::lock_guard uiManagerLock(javaUIManagerMutex_, std::adopt_lock); + javaUIManager_ = make_global(javaUIManager); ContextContainer::Shared contextContainer = @@ -182,6 +231,11 @@ void Binding::installFabricUIManager( } void Binding::uninstallFabricUIManager() { + // Use std::lock and std::adopt_lock to prevent deadlocks by locking mutexes at the same time + std::lock(schedulerMutex_, javaUIManagerMutex_); + std::lock_guard schedulerLock(schedulerMutex_, std::adopt_lock); + std::lock_guard uiManagerLock(javaUIManagerMutex_, std::adopt_lock); + scheduler_ = nullptr; javaUIManager_ = nullptr; } @@ -393,6 +447,13 @@ local_ref createCreateMountItem( void Binding::schedulerDidFinishTransaction( MountingCoordinator::Shared const &mountingCoordinator) { SystraceSection s("FabricUIManagerBinding::schedulerDidFinishTransaction"); + long finishTransactionStartTime = getTime(); + + jni::global_ref localJavaUIManager = getJavaUIManager(); + if (!localJavaUIManager) { + LOG(ERROR) << "Binding::schedulerDidFinishTransaction: JavaUIManager disappeared"; + return; + } auto mountingTransaction = mountingCoordinator->pullTransaction(); @@ -408,8 +469,6 @@ void Binding::schedulerDidFinishTransaction( // Upper bound estimation of mount items to be delivered to Java side. int size = mutations.size() * 3 + 42; - long finishTransactionStartTime = getTime(); - local_ref> mountItemsArray = JArrayClass::newArray(size); @@ -430,20 +489,20 @@ void Binding::schedulerDidFinishTransaction( deletedViewTags.find(mutation.newChildShadowView.tag) != deletedViewTags.end()) { mountItems[position++] = - createCreateMountItem(javaUIManager_, mutation, surfaceId); + createCreateMountItem(localJavaUIManager, mutation, surfaceId); } break; } case ShadowViewMutation::Remove: { if (!isVirtual) { mountItems[position++] = - createRemoveMountItem(javaUIManager_, mutation); + createRemoveMountItem(localJavaUIManager, mutation); } break; } case ShadowViewMutation::Delete: { mountItems[position++] = - createDeleteMountItem(javaUIManager_, mutation); + createDeleteMountItem(localJavaUIManager, mutation); deletedViewTags.insert(mutation.oldChildShadowView.tag); break; @@ -453,21 +512,21 @@ void Binding::schedulerDidFinishTransaction( if (mutation.oldChildShadowView.props != mutation.newChildShadowView.props) { mountItems[position++] = - createUpdatePropsMountItem(javaUIManager_, mutation); + createUpdatePropsMountItem(localJavaUIManager, mutation); } if (mutation.oldChildShadowView.localData != mutation.newChildShadowView.localData) { mountItems[position++] = - createUpdateLocalData(javaUIManager_, mutation); + createUpdateLocalData(localJavaUIManager, mutation); } if (mutation.oldChildShadowView.state != mutation.newChildShadowView.state) { mountItems[position++] = - createUpdateStateMountItem(javaUIManager_, mutation); + createUpdateStateMountItem(localJavaUIManager, mutation); } auto updateLayoutMountItem = - createUpdateLayoutMountItem(javaUIManager_, mutation); + createUpdateLayoutMountItem(localJavaUIManager, mutation); if (updateLayoutMountItem) { mountItems[position++] = updateLayoutMountItem; } @@ -476,7 +535,7 @@ void Binding::schedulerDidFinishTransaction( if (mutation.oldChildShadowView.eventEmitter != mutation.newChildShadowView.eventEmitter) { auto updateEventEmitterMountItem = - createUpdateEventEmitterMountItem(javaUIManager_, mutation); + createUpdateEventEmitterMountItem(localJavaUIManager, mutation); if (updateEventEmitterMountItem) { mountItems[position++] = updateEventEmitterMountItem; } @@ -487,30 +546,30 @@ void Binding::schedulerDidFinishTransaction( if (!isVirtual) { // Insert item mountItems[position++] = - createInsertMountItem(javaUIManager_, mutation); + createInsertMountItem(localJavaUIManager, mutation); if (mutation.newChildShadowView.props->revision > 1 || deletedViewTags.find(mutation.newChildShadowView.tag) != deletedViewTags.end()) { mountItems[position++] = - createUpdatePropsMountItem(javaUIManager_, mutation); + createUpdatePropsMountItem(localJavaUIManager, mutation); // State if (mutation.newChildShadowView.state) { mountItems[position++] = - createUpdateStateMountItem(javaUIManager_, mutation); + createUpdateStateMountItem(localJavaUIManager, mutation); } } // LocalData if (mutation.newChildShadowView.localData) { mountItems[position++] = - createUpdateLocalData(javaUIManager_, mutation); + createUpdateLocalData(localJavaUIManager, mutation); } // Layout auto updateLayoutMountItem = - createUpdateLayoutMountItem(javaUIManager_, mutation); + createUpdateLayoutMountItem(localJavaUIManager, mutation); if (updateLayoutMountItem) { mountItems[position++] = updateLayoutMountItem; } @@ -518,7 +577,7 @@ void Binding::schedulerDidFinishTransaction( // EventEmitter auto updateEventEmitterMountItem = - createUpdateEventEmitterMountItem(javaUIManager_, mutation); + createUpdateEventEmitterMountItem(localJavaUIManager, mutation); if (updateEventEmitterMountItem) { mountItems[position++] = updateEventEmitterMountItem; } @@ -538,7 +597,7 @@ void Binding::schedulerDidFinishTransaction( "createBatchMountItem"); auto batch = createMountItemsBatchContainer( - javaUIManager_, mountItemsArray.get(), position); + localJavaUIManager, mountItemsArray.get(), position); static auto scheduleMountItems = jni::findClassStatic(UIManagerJavaDescriptor) @@ -548,7 +607,7 @@ void Binding::schedulerDidFinishTransaction( long finishTransactionEndTime = getTime(); scheduleMountItems( - javaUIManager_, + localJavaUIManager, batch.get(), telemetry.getCommitStartTime(), telemetry.getLayoutTime(), @@ -563,6 +622,13 @@ void Binding::setPixelDensity(float pointScaleFactor) { void Binding::schedulerDidRequestPreliminaryViewAllocation( const SurfaceId surfaceId, const ShadowView &shadowView) { + + jni::global_ref localJavaUIManager = getJavaUIManager(); + if (!localJavaUIManager) { + LOG(ERROR) << "Binding::schedulerDidRequestPreliminaryViewAllocation: JavaUIManager disappeared"; + return; + } + bool isLayoutableShadowNode = shadowView.layoutMetrics != EmptyLayoutMetrics; static auto preallocateView = @@ -584,8 +650,9 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation( local_ref props = castReadableMap( ReadableNativeMap::newObjectCxxArgs(shadowView.props->rawProps)); auto component = getPlatformComponentName(shadowView); + preallocateView( - javaUIManager_, + localJavaUIManager, surfaceId, shadowView.tag, component.get(), 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 7c674a962b8..afa7849c0bb 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 @@ -27,12 +27,17 @@ class Binding : public jni::HybridClass, public SchedulerDelegate { static void registerNatives(); jni::global_ref javaUIManager_; + std::mutex javaUIManagerMutex_; std::shared_ptr scheduler_; + std::mutex schedulerMutex_; float pointScaleFactor_ = 1; private: + jni::global_ref getJavaUIManager(); + std::shared_ptr getScheduler(); + void setConstraints( jint surfaceId, jfloat minWidth,