Clean up feature flag `allowRecursiveCommitsWithSynchronousMountOnAnd…

…roid` Summary: Changelog: [Android][Fixed] Fixes some deadlocks when doing commits and state updates synchronously from the UI thread (e.g.: from reanimated). This removes the gating for the fix to allow recursive commits with synchronous mount on Android. See facebook#44725 (comment) and software-mansion/react-native-reanimated#6418 (comment). Differential Revision: D66095539
rubennorte · Nov 18, 2024 · 6cbe54d · 6cbe54d
1 parent 089c87e
commit 6cbe54d
Show file tree

Hide file tree

Showing 20 changed files with 80 additions and 189 deletions.
diff --git a/...Android/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlags.kt b/...Android/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlags.kt
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<1adfbb1f0b9791d93ea00f39fbce5520>>
+ * @generated SignedSource<<124e8e4892062fa63a9a70ae0c53653d>>
  */
 
 /**
@@ -34,12 +34,6 @@ public object ReactNativeFeatureFlags {
   @JvmStatic
   public fun commonTestFlag(): Boolean = accessor.commonTestFlag()
 
-  /**
-   * Adds support for recursively processing commits that mount synchronously (Android only).
-   */
-  @JvmStatic
-  public fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean = accessor.allowRecursiveCommitsWithSynchronousMountOnAndroid()
-
   /**
    * Do not wait for a main-thread dispatch to complete init to start executing work on the JS thread on Android
    */

diff --git a/.../main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsCxxAccessor.kt b/.../main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsCxxAccessor.kt
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<fb943b9c4c43ac8214a8f368076cbd62>>
+ * @generated SignedSource<<28da40176c64703dcab4bc0d1af078a8>>
  */
 
 /**
@@ -21,7 +21,6 @@ package com.facebook.react.internal.featureflags
 
 public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccessor {
   private var commonTestFlagCache: Boolean? = null
-  private var allowRecursiveCommitsWithSynchronousMountOnAndroidCache: Boolean? = null
   private var completeReactInstanceCreationOnBgThreadOnAndroidCache: Boolean? = null
   private var disableEventLoopOnBridgelessCache: Boolean? = null
   private var disableMountItemReorderingAndroidCache: Boolean? = null
@@ -77,15 +76,6 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
     return cached
   }
 
-  override fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean {
-    var cached = allowRecursiveCommitsWithSynchronousMountOnAndroidCache
-    if (cached == null) {
-      cached = ReactNativeFeatureFlagsCxxInterop.allowRecursiveCommitsWithSynchronousMountOnAndroid()
-      allowRecursiveCommitsWithSynchronousMountOnAndroidCache = cached
-    }
-    return cached
-  }
-
   override fun completeReactInstanceCreationOnBgThreadOnAndroid(): Boolean {
     var cached = completeReactInstanceCreationOnBgThreadOnAndroidCache
     if (cached == null) {

diff --git a/...c/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsCxxInterop.kt b/...c/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsCxxInterop.kt
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<3980c51f97745c20e0fd510f595477dc>>
+ * @generated SignedSource<<e53c4f66b6261f983ad1502adc31f1dc>>
  */
 
 /**
@@ -30,8 +30,6 @@ public object ReactNativeFeatureFlagsCxxInterop {
 
   @DoNotStrip @JvmStatic public external fun commonTestFlag(): Boolean
 
-  @DoNotStrip @JvmStatic public external fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean
-
   @DoNotStrip @JvmStatic public external fun completeReactInstanceCreationOnBgThreadOnAndroid(): Boolean
 
   @DoNotStrip @JvmStatic public external fun disableEventLoopOnBridgeless(): Boolean

diff --git a/...src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsDefaults.kt b/...src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsDefaults.kt
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<f4f263578308798dcf7561918f58e0cc>>
+ * @generated SignedSource<<3d25d7b8ee194721c39fb5eb5eda87d4>>
  */
 
 /**
@@ -25,8 +25,6 @@ public open class ReactNativeFeatureFlagsDefaults : ReactNativeFeatureFlagsProvi
 
   override fun commonTestFlag(): Boolean = false
 
-  override fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean = false
-
   override fun completeReactInstanceCreationOnBgThreadOnAndroid(): Boolean = true
 
   override fun disableEventLoopOnBridgeless(): Boolean = false

diff --git a/...ain/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsLocalAccessor.kt b/...ain/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsLocalAccessor.kt
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<bd3529c1455508333a45a5852c3213c6>>
+ * @generated SignedSource<<fb8747b040cd9c29e9ca0a7c1647ad91>>
  */
 
 /**
@@ -25,7 +25,6 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
   private val accessedFeatureFlags = mutableSetOf<String>()
 
   private var commonTestFlagCache: Boolean? = null
-  private var allowRecursiveCommitsWithSynchronousMountOnAndroidCache: Boolean? = null
   private var completeReactInstanceCreationOnBgThreadOnAndroidCache: Boolean? = null
   private var disableEventLoopOnBridgelessCache: Boolean? = null
   private var disableMountItemReorderingAndroidCache: Boolean? = null
@@ -82,16 +81,6 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
     return cached
   }
 
-  override fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean {
-    var cached = allowRecursiveCommitsWithSynchronousMountOnAndroidCache
-    if (cached == null) {
-      cached = currentProvider.allowRecursiveCommitsWithSynchronousMountOnAndroid()
-      accessedFeatureFlags.add("allowRecursiveCommitsWithSynchronousMountOnAndroid")
-      allowRecursiveCommitsWithSynchronousMountOnAndroidCache = cached
-    }
-    return cached
-  }
-
   override fun completeReactInstanceCreationOnBgThreadOnAndroid(): Boolean {
     var cached = completeReactInstanceCreationOnBgThreadOnAndroidCache
     if (cached == null) {

diff --git a/...src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsProvider.kt b/...src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsProvider.kt
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<a74e9cdce26b156a74e397fd065618c2>>
+ * @generated SignedSource<<24bae6a173c941923c84f418a114ec2f>>
  */
 
 /**
@@ -25,8 +25,6 @@ import com.facebook.proguard.annotations.DoNotStrip
 public interface ReactNativeFeatureFlagsProvider {
   @DoNotStrip public fun commonTestFlag(): Boolean
 
-  @DoNotStrip public fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean
-
   @DoNotStrip public fun completeReactInstanceCreationOnBgThreadOnAndroid(): Boolean
 
   @DoNotStrip public fun disableEventLoopOnBridgeless(): Boolean

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp
@@ -559,27 +559,24 @@ void FabricUIManagerBinding::schedulerShouldRenderTransactions(
     return;
   }
 
-  if (ReactNativeFeatureFlags::
-          allowRecursiveCommitsWithSynchronousMountOnAndroid()) {
-    std::vector<MountingTransaction> pendingTransactions;
-
-    {
-      // Retain the lock to access the pending transactions but not to execute
-      // the mount operations because that method can call into this method
-      // again.
-      std::unique_lock<std::mutex> lock(pendingTransactionsMutex_);
-      pendingTransactions_.swap(pendingTransactions);
-    }
+  std::vector<MountingTransaction> pendingTransactions;
 
-    for (auto& transaction : pendingTransactions) {
-      mountingManager->executeMount(transaction);
-    }
-  } else {
+  {
+    // Retain the lock to access the pending transactions but not to execute
+    // the mount operations because that method can call into this method
+    // again.
+    //
+    // This can be re-entrant when mounting manager triggers state updates
+    // synchronously (this can happen when committing from the UI thread).
+    // This is safe because we're already combining all the transactions for the
+    // same surface ID in a single transaction in the pending transactions list,
+    // so operations won't run out of order.
     std::unique_lock<std::mutex> lock(pendingTransactionsMutex_);
-    for (auto& transaction : pendingTransactions_) {
-      mountingManager->executeMount(transaction);
-    }
-    pendingTransactions_.clear();
+    pendingTransactions_.swap(pendingTransactions);
+  }
+
+  for (auto& transaction : pendingTransactions) {
+    mountingManager->executeMount(transaction);
   }
 }
 

diff --git a/...ative/ReactAndroid/src/main/jni/react/featureflags/JReactNativeFeatureFlagsCxxInterop.cpp b/...ative/ReactAndroid/src/main/jni/react/featureflags/JReactNativeFeatureFlagsCxxInterop.cpp
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<9c1572172189f9f9ded86a9fdb1cb021>>
+ * @generated SignedSource<<17da000ed077668a52a4dacba88162b0>>
  */
 
 /**
@@ -45,12 +45,6 @@ class ReactNativeFeatureFlagsProviderHolder
     return method(javaProvider_);
   }
 
-  bool allowRecursiveCommitsWithSynchronousMountOnAndroid() override {
-    static const auto method =
-        getReactNativeFeatureFlagsProviderJavaClass()->getMethod<jboolean()>("allowRecursiveCommitsWithSynchronousMountOnAndroid");
-    return method(javaProvider_);
-  }
-
   bool completeReactInstanceCreationOnBgThreadOnAndroid() override {
     static const auto method =
         getReactNativeFeatureFlagsProviderJavaClass()->getMethod<jboolean()>("completeReactInstanceCreationOnBgThreadOnAndroid");
@@ -330,11 +324,6 @@ bool JReactNativeFeatureFlagsCxxInterop::commonTestFlag(
   return ReactNativeFeatureFlags::commonTestFlag();
 }
 
-bool JReactNativeFeatureFlagsCxxInterop::allowRecursiveCommitsWithSynchronousMountOnAndroid(
-    facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop> /*unused*/) {
-  return ReactNativeFeatureFlags::allowRecursiveCommitsWithSynchronousMountOnAndroid();
-}
-
 bool JReactNativeFeatureFlagsCxxInterop::completeReactInstanceCreationOnBgThreadOnAndroid(
     facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop> /*unused*/) {
   return ReactNativeFeatureFlags::completeReactInstanceCreationOnBgThreadOnAndroid();
@@ -594,9 +583,6 @@ void JReactNativeFeatureFlagsCxxInterop::registerNatives() {
       makeNativeMethod(
         "commonTestFlag",
         JReactNativeFeatureFlagsCxxInterop::commonTestFlag),
-      makeNativeMethod(
-        "allowRecursiveCommitsWithSynchronousMountOnAndroid",
-        JReactNativeFeatureFlagsCxxInterop::allowRecursiveCommitsWithSynchronousMountOnAndroid),
       makeNativeMethod(
         "completeReactInstanceCreationOnBgThreadOnAndroid",
         JReactNativeFeatureFlagsCxxInterop::completeReactInstanceCreationOnBgThreadOnAndroid),

diff --git a/...-native/ReactAndroid/src/main/jni/react/featureflags/JReactNativeFeatureFlagsCxxInterop.h b/...-native/ReactAndroid/src/main/jni/react/featureflags/JReactNativeFeatureFlagsCxxInterop.h
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<92fcd7e8c814a6a76ec15c4cd60e00e4>>
+ * @generated SignedSource<<20fa39ef7b31222a0a34e131d60e6bec>>
  */
 
 /**
@@ -33,9 +33,6 @@ class JReactNativeFeatureFlagsCxxInterop
   static bool commonTestFlag(
     facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop>);
 
-  static bool allowRecursiveCommitsWithSynchronousMountOnAndroid(
-    facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop>);
-
   static bool completeReactInstanceCreationOnBgThreadOnAndroid(
     facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop>);
 

diff --git a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.cpp b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.cpp
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<93b93588adcd6b45ec748dff7ee18f07>>
+ * @generated SignedSource<<cfa6f544c0bcb190836329d8f597efc6>>
  */
 
 /**
@@ -30,10 +30,6 @@ bool ReactNativeFeatureFlags::commonTestFlag() {
   return getAccessor().commonTestFlag();
 }
 
-bool ReactNativeFeatureFlags::allowRecursiveCommitsWithSynchronousMountOnAndroid() {
-  return getAccessor().allowRecursiveCommitsWithSynchronousMountOnAndroid();
-}
-
 bool ReactNativeFeatureFlags::completeReactInstanceCreationOnBgThreadOnAndroid() {
   return getAccessor().completeReactInstanceCreationOnBgThreadOnAndroid();
 }

diff --git a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h
@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<b24144040cb5bd51f9910b81675ba5b2>>
+ * @generated SignedSource<<0c0217362f582ac635d5f46347e65252>>
  */
 
 /**
@@ -44,11 +44,6 @@ class ReactNativeFeatureFlags {
    */
   RN_EXPORT static bool commonTestFlag();
 
-  /**
-   * Adds support for recursively processing commits that mount synchronously (Android only).
-   */
-  RN_EXPORT static bool allowRecursiveCommitsWithSynchronousMountOnAndroid();
-
   /**
    * Do not wait for a main-thread dispatch to complete init to start executing work on the JS thread on Android
    */