From 89faf0c9a87f6de68ca416d10566dbcbe80d9450 Mon Sep 17 00:00:00 2001 From: Marshall Mann-Wood Date: Wed, 15 Dec 2021 13:33:56 -0800 Subject: [PATCH 1/4] Added saftey for ensuring callback is called Summary: Adds a return value to `MessageQueueThread#runOnQueue`, it's implementors and one caller. It is currently possible for a callback to not be called (because the HandlerThread has been shutdown). If the callback is mission-critical (frees resources, releases a lock, etc) the callee has no indication that it needs to do something. This surfaces that information to the callee. Changelog: [Android][Changed] - Made `MessageQueueThread#runOnQueue` return a boolean. Made `MessageQueueThreadImpl#runOnQueue` return false when the runnable is not submitted. Reviewed By: RSNara Differential Revision: D33075516 fbshipit-source-id: bd214a39ae066c0e7d413f2eaaaa05bd072ead2a --- .../src/main/java/com/facebook/react/bridge/ReactContext.java | 4 ++-- .../com/facebook/react/bridge/queue/MessageQueueThread.java | 2 +- .../facebook/react/bridge/queue/MessageQueueThreadImpl.java | 4 +++- ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java index dbc51207af..24b4396e8a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java @@ -396,8 +396,8 @@ public class ReactContext extends ContextWrapper { return Assertions.assertNotNull(mJSMessageQueueThread).isOnThread(); } - public void runOnJSQueueThread(Runnable runnable) { - Assertions.assertNotNull(mJSMessageQueueThread).runOnQueue(runnable); + public boolean runOnJSQueueThread(Runnable runnable) { + return Assertions.assertNotNull(mJSMessageQueueThread).runOnQueue(runnable); } /** diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/queue/MessageQueueThread.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/queue/MessageQueueThread.java index f40885e8ac..563df0304c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/queue/MessageQueueThread.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/queue/MessageQueueThread.java @@ -19,7 +19,7 @@ public interface MessageQueueThread { * if it is being submitted from the same queue Thread. */ @DoNotStrip - void runOnQueue(Runnable runnable); + boolean runOnQueue(Runnable runnable); /** * Runs the given Callable on this Thread. It will be submitted to the end of the event queue even diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/queue/MessageQueueThreadImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/queue/MessageQueueThreadImpl.java index 00beffd3a5..5ad0839277 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/queue/MessageQueueThreadImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/queue/MessageQueueThreadImpl.java @@ -55,15 +55,17 @@ public class MessageQueueThreadImpl implements MessageQueueThread { */ @DoNotStrip @Override - public void runOnQueue(Runnable runnable) { + public boolean runOnQueue(Runnable runnable) { if (mIsFinished) { FLog.w( ReactConstants.TAG, "Tried to enqueue runnable on already finished thread: '" + getName() + "... dropping Runnable."); + return false; } mHandler.post(runnable); + return true; } @DoNotStrip diff --git a/ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp b/ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp index badb136f4c..9b889aa315 100644 --- a/ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp +++ b/ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp @@ -69,7 +69,7 @@ void JMessageQueueThread::runOnQueue(std::function &&runnable) { jni::ThreadScope guard; static auto method = JavaMessageQueueThread::javaClassStatic() - ->getMethod("runOnQueue"); + ->getMethod("runOnQueue"); method( m_jobj, JNativeRunnable::newObjectCxxArgs(wrapRunnable(std::move(runnable))) From 1d45b20b6c6ba66df0485cdb9be36463d96cf182 Mon Sep 17 00:00:00 2001 From: "Julio C. Rocha" Date: Wed, 15 Dec 2021 14:36:22 -0800 Subject: [PATCH 2/4] Set CxxModule instance before retrieving Method vector (#32719) Summary: Allows `CxxModule` objects to set their `instance_` member before `getMethods()` is invoked, allowing for the `Method` callbacks to capture a weak reference to `getInstance()` if needed. ## Changelog [General] [Fixed] - Set CxxModules' Instance before retrieving their Method vector. Pull Request resolved: https://github.com/facebook/react-native/pull/32719 Test Plan: Ensure this statement swap does not break any scenarios with existing CI validation. Reviewed By: JoshuaGross Differential Revision: D32955616 Pulled By: genkikondo fbshipit-source-id: fd7a23ca2c12c01882ff1600f5aef86b1fe4a984 --- ReactCommon/cxxreact/CxxNativeModule.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ReactCommon/cxxreact/CxxNativeModule.cpp b/ReactCommon/cxxreact/CxxNativeModule.cpp index 0bcf13657e..bab53966dc 100644 --- a/ReactCommon/cxxreact/CxxNativeModule.cpp +++ b/ReactCommon/cxxreact/CxxNativeModule.cpp @@ -244,8 +244,8 @@ void CxxNativeModule::lazyInit() { module_ = provider_(); provider_ = nullptr; if (module_) { - methods_ = module_->getMethods(); module_->setInstance(instance_); + methods_ = module_->getMethods(); } } From acda0fec48b6973c9d11badae0d5db76c6d2db13 Mon Sep 17 00:00:00 2001 From: Andrei Shikov Date: Wed, 15 Dec 2021 15:00:58 -0800 Subject: [PATCH 3/4] Use the same lock for fields cleared on Fabric native binding uninstall Summary: Scheduler and JavaUIManager mutexes share the lifetime (between install-uninstall), so we can use singular shared lock to ensure that both sections are locked exclusively for install/uninstall and in shared mode for access. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D33130686 fbshipit-source-id: bf283fd5410e386d2635645e17680a9f4cb7f288 --- .../com/facebook/react/fabric/jni/Binding.cpp | 16 ++++------------ .../java/com/facebook/react/fabric/jni/Binding.h | 6 ++---- 2 files changed, 6 insertions(+), 16 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 d9f1cb088b..9fde6bc9fe 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 @@ -223,13 +223,13 @@ jni::local_ref Binding::initHybrid( // Thread-safe getter jni::global_ref Binding::getJavaUIManager() { - std::lock_guard uiManagerLock(javaUIManagerMutex_); + std::shared_lock lock(installMutex_); return javaUIManager_; } // Thread-safe getter std::shared_ptr Binding::getScheduler() { - std::lock_guard lock(schedulerMutex_); + std::shared_lock lock(installMutex_); return scheduler_; } @@ -521,10 +521,7 @@ void Binding::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); + std::unique_lock lock(installMutex_); javaUIManager_ = make_global(javaUIManager); @@ -625,13 +622,8 @@ void Binding::uninstallFabricUIManager() { LOG(WARNING) << "Binding::uninstallFabricUIManager() was called (address: " << this << ")."; } - // 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); + std::unique_lock lock(installMutex_); animationDriver_ = nullptr; scheduler_ = nullptr; javaUIManager_ = nullptr; 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 3d8ef22fd8..598974f702 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 @@ -178,8 +178,9 @@ class Binding : public jni::HybridClass, void uninstallFabricUIManager(); // Private member variables + better::shared_mutex installMutex_; jni::global_ref javaUIManager_; - std::mutex javaUIManagerMutex_; + std::shared_ptr scheduler_; // LayoutAnimations void onAnimationStarted() override; @@ -187,9 +188,6 @@ class Binding : public jni::HybridClass, std::shared_ptr animationDriver_; std::unique_ptr backgroundExecutor_; - std::shared_ptr scheduler_; - std::mutex schedulerMutex_; - better::map surfaceHandlerRegistry_{}; better::shared_mutex surfaceHandlerRegistryMutex_; // Protects `surfaceHandlerRegistry_`. From b1ecac9d143cdce8bcd2d6071c0b8ff869daac7b Mon Sep 17 00:00:00 2001 From: Adam Cmiel Date: Wed, 15 Dec 2021 15:11:53 -0800 Subject: [PATCH 4/4] replace __nullable with _Nullable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Modernize our null annotations From the swift blog: This feature was first released in Xcode 6.3 with the keywords nullable and nonnull. Due to potential conflicts with third-party libraries, we’ve changed them in Xcode 7 to the _Nullable and _Nonnull you see here. drop-conflicts Reviewed By: cuva Differential Revision: D33121042 fbshipit-source-id: 821f5ec858d9afd5bfb1d6081c669f4ca18a36ed --- .../ComponentViews/ScrollView/RCTScrollViewComponentView.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.h b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.h index 96b07f9272..2167ba6ebc 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.h +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.h @@ -28,7 +28,7 @@ NS_ASSUME_NONNULL_BEGIN /* * Finds and returns the closet RCTScrollViewComponentView component to the given view */ -+ (RCTScrollViewComponentView *_Nullable)findScrollViewComponentViewForView:(UIView *)view; ++ (nullable RCTScrollViewComponentView *)findScrollViewComponentViewForView:(UIView *)view; /* * Returns an actual UIScrollView that this component uses under the hood.