diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp index 0926ffa7ded3..0ed7581b4c38 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JDynamicNative.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JDynamicNative.cpp index a6c1b52a0777..e8fbf6f384f2 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JDynamicNative.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JDynamicNative.cpp @@ -17,7 +17,7 @@ jboolean JDynamicNative::isNullNative() { return static_cast(payload_.isNull()); } -jni::local_ref JDynamicNative::getTypeNative() { +jni::alias_ref JDynamicNative::getTypeNative() { return ReadableType::getType(payload_.type()); } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JDynamicNative.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JDynamicNative.h index 1ba0d485c0b1..9030889b74f7 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JDynamicNative.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JDynamicNative.h @@ -42,7 +42,7 @@ class JDynamicNative : public jni::HybridClass { private: friend HybridBase; - jni::local_ref getTypeNative(); + jni::alias_ref getTypeNative(); jni::local_ref asString(); jboolean asBoolean(); jdouble asDouble(); diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JavaModuleWrapper.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JavaModuleWrapper.cpp index fd3d05082217..ca047917efda 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/JavaModuleWrapper.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/JavaModuleWrapper.cpp @@ -20,6 +20,7 @@ #include #endif +#include "NativeMap.h" #include "ReadableNativeArray.h" #ifndef RCT_REMOVE_LEGACY_ARCH diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeCommon.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeCommon.cpp index 04078f143d16..eb8f7839f707 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeCommon.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeCommon.cpp @@ -27,32 +27,32 @@ alias_ref getTypeField(const char* fieldName) { } // namespace -local_ref ReadableType::getType(folly::dynamic::Type type) { +alias_ref ReadableType::getType(folly::dynamic::Type type) { switch (type) { case folly::dynamic::Type::NULLT: { - static alias_ref val = getTypeField("Null"); - return make_local(val); + static auto val = getTypeField("Null"); + return val; } case folly::dynamic::Type::BOOL: { - static alias_ref val = getTypeField("Boolean"); - return make_local(val); + static auto val = getTypeField("Boolean"); + return val; } case folly::dynamic::Type::DOUBLE: case folly::dynamic::Type::INT64: { - static alias_ref val = getTypeField("Number"); - return make_local(val); + static auto val = getTypeField("Number"); + return val; } case folly::dynamic::Type::STRING: { - static alias_ref val = getTypeField("String"); - return make_local(val); + static auto val = getTypeField("String"); + return val; } case folly::dynamic::Type::OBJECT: { - static alias_ref val = getTypeField("Map"); - return make_local(val); + static auto val = getTypeField("Map"); + return val; } case folly::dynamic::Type::ARRAY: { - static alias_ref val = getTypeField("Array"); - return make_local(val); + static auto val = getTypeField("Array"); + return val; } default: throwNewJavaException( diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeCommon.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeCommon.h index 8f79c5db6795..444fbd1a0aec 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeCommon.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/NativeCommon.h @@ -19,7 +19,7 @@ namespace facebook::react { struct ReadableType : public jni::JavaClass { static auto constexpr kJavaDescriptor = "Lcom/facebook/react/bridge/ReadableType;"; - static jni::local_ref getType(folly::dynamic::Type type); + static jni::alias_ref getType(folly::dynamic::Type type); }; namespace exceptions { @@ -29,7 +29,7 @@ extern const char *gUnexpectedNativeTypeExceptionClass; template void throwIfObjectAlreadyConsumed(const T &t, const char *msg) { - if (t->isConsumed) { + if (t->isConsumed) [[unlikely]] { jni::throwNewJavaException("com/facebook/react/bridge/ObjectAlreadyConsumedException", msg); } } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp index bdc4d6bbbbb4..9eea2b738fc5 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp @@ -23,7 +23,7 @@ void ReadableNativeArray::mapException(std::exception_ptr ex) { } local_ref> ReadableNativeArray::importArray() { - auto size = static_cast(array_.size()); + auto size = static_cast(array_.size()); auto jarray = JArrayClass::newArray(size); for (jint ii = 0; ii < size; ii++) { addDynamicToJArray(jarray, ii, array_.at(ii)); @@ -32,10 +32,10 @@ local_ref> ReadableNativeArray::importArray() { } local_ref> ReadableNativeArray::importTypeArray() { - auto size = static_cast(array_.size()); + auto size = static_cast(array_.size()); auto jarray = JArrayClass::newArray(size); for (jint ii = 0; ii < size; ii++) { - (*jarray)[ii] = ReadableType::getType(array_.at(ii).type()); + jarray->setElement(ii, ReadableType::getType(array_.at(ii).type()).get()); } return jarray; } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.h index 0129c066165d..6c54224b2e7a 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.h @@ -9,9 +9,6 @@ #include "NativeArray.h" -#include "NativeCommon.h" -#include "NativeMap.h" - namespace facebook::react { struct ReadableArray : jni::JavaClass { diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp index 8f89b03f9150..bb21dc150ada 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp @@ -7,6 +7,8 @@ #include "ReadableNativeMap.h" +#include "ReadableNativeArray.h" + using namespace facebook::jni; namespace facebook::react { @@ -20,84 +22,86 @@ void ReadableNativeMap::mapException(std::exception_ptr ex) { } } +void ReadableNativeMap::throwIfKeysNotImported() const { + if (!values_.has_value()) [[unlikely]] { + throwNewJavaException( + "java/lang/IllegalStateException", + "importKeys must be called before importing values or types"); + } +} + void addDynamicToJArray( - local_ref> jarray, + alias_ref> jarray, jint index, const folly::dynamic& dyn) { + local_ref value; switch (dyn.type()) { - case folly::dynamic::Type::NULLT: { - jarray->setElement(index, nullptr); - break; - } - case folly::dynamic::Type::BOOL: { - (*jarray)[index] = - JBoolean::valueOf(static_cast(dyn.getBool())); + case folly::dynamic::Type::BOOL: + value = JBoolean::valueOf(static_cast(dyn.getBool())); break; - } - case folly::dynamic::Type::INT64: { - (*jarray)[index] = JDouble::valueOf(dyn.getInt()); + case folly::dynamic::Type::INT64: + value = JDouble::valueOf(static_cast(dyn.getInt())); break; - } - case folly::dynamic::Type::DOUBLE: { - (*jarray)[index] = JDouble::valueOf(dyn.getDouble()); + case folly::dynamic::Type::DOUBLE: + value = JDouble::valueOf(dyn.getDouble()); break; - } - case folly::dynamic::Type::STRING: { - (*jarray)[index] = make_jstring(dyn.getString()); + case folly::dynamic::Type::STRING: + value = make_jstring(dyn.getString()); break; - } - case folly::dynamic::Type::OBJECT: { - (*jarray)[index] = ReadableNativeMap::newObjectCxxArgs(dyn); + case folly::dynamic::Type::OBJECT: + value = ReadableNativeMap::newObjectCxxArgs(dyn); break; - } - case folly::dynamic::Type::ARRAY: { - (*jarray)[index] = ReadableNativeArray::newObjectCxxArgs(dyn); + case folly::dynamic::Type::ARRAY: + value = ReadableNativeArray::newObjectCxxArgs(dyn); break; - } + case folly::dynamic::Type::NULLT: default: - jarray->setElement(index, nullptr); break; } + jarray->setElement(index, value.get()); } local_ref> ReadableNativeMap::importKeys() { throwIfConsumed(); - keys_ = folly::dynamic::array(); - if (map_ == nullptr) { - return JArrayClass::newArray(0); - } - auto jarray = JArrayClass::newArray(map_.size()); + auto size = map_ == nullptr ? 0 : static_cast(map_.size()); + std::vector values(size); + + auto jarray = JArrayClass::newArray(size); jint i = 0; - for (auto& pair : map_.items()) { - auto value = pair.first.asString(); - (*keys_).push_back(value); - (*jarray)[i++] = make_jstring(value); + if (map_ != nullptr) { + for (auto& pair : map_.items()) { + values[i] = &pair.second; + jarray->setElement(i++, make_jstring(pair.first.getString()).get()); + } } + values_ = std::move(values); return jarray; } local_ref> ReadableNativeMap::importValues() { throwIfConsumed(); + throwIfKeysNotImported(); - auto size = static_cast(keys_.value().size()); + const auto& values = values_.value(); + auto size = static_cast(values.size()); auto jarray = JArrayClass::newArray(size); for (jint ii = 0; ii < size; ii++) { - const std::string& key = (*keys_)[ii].getString(); - addDynamicToJArray(jarray, ii, map_.at(key)); + addDynamicToJArray(jarray, ii, *values[ii]); } return jarray; } local_ref> ReadableNativeMap::importTypes() { throwIfConsumed(); + throwIfKeysNotImported(); - auto size = static_cast(keys_.value().size()); + const auto& values = values_.value(); + auto size = static_cast(values.size()); auto jarray = JArrayClass::newArray(size); for (jint ii = 0; ii < size; ii++) { - const std::string& key = (*keys_)[ii].getString(); - (*jarray)[ii] = ReadableType::getType(map_.at(key).type()); + jarray->setElement(ii, ReadableType::getType(values[ii]->type()).get()); } return jarray; } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h index b2818cb48ab4..baed5d3f8dae 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h @@ -11,10 +11,9 @@ #include #include #include +#include -#include "NativeCommon.h" #include "NativeMap.h" -#include "ReadableNativeArray.h" namespace facebook::react { @@ -24,7 +23,7 @@ struct ReadableMap : jni::JavaClass { static auto constexpr kJavaDescriptor = "Lcom/facebook/react/bridge/ReadableMap;"; }; -void addDynamicToJArray(jni::local_ref> jarray, jint index, const folly::dynamic &dyn); +void addDynamicToJArray(jni::alias_ref> jarray, jint index, const folly::dynamic &dyn); struct ReadableNativeMap : jni::HybridClass { static auto constexpr kJavaDescriptor = "Lcom/facebook/react/bridge/ReadableNativeMap;"; @@ -32,7 +31,6 @@ struct ReadableNativeMap : jni::HybridClass { jni::local_ref> importKeys(); jni::local_ref> importValues(); jni::local_ref> importTypes(); - std::optional keys_; static jni::local_ref createWithContents(folly::dynamic &&map); static void mapException(std::exception_ptr ex); @@ -41,6 +39,14 @@ struct ReadableNativeMap : jni::HybridClass { using HybridBase::HybridBase; friend HybridBase; friend struct WritableNativeMap; + + private: + void throwIfKeysNotImported() const; + + // folly::dynamic stores object entries in an F14NodeMap, so these pointers + // remain valid across the insertions and replacements exposed by + // WritableNativeMap. + std::optional> values_; }; } // namespace facebook::react diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt index 0c0e4333f195..ea33b5cb289a 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt @@ -13,6 +13,8 @@ import com.facebook.react.ReactRootView import com.facebook.react.bridge.JSExceptionHandler import com.facebook.react.bridge.JavaOnlyMap import com.facebook.react.bridge.ReactApplicationContext +import com.facebook.react.bridge.ReadableType +import com.facebook.react.bridge.WritableNativeMap import com.facebook.react.fabric.mounting.MountingManager import com.facebook.react.fabric.mounting.MountingManager.MountItemExecutor import com.facebook.react.fabric.mounting.mountitems.IntBufferBatchMountItem @@ -110,6 +112,22 @@ class FabricMountingManagerInstrumentationTest { assertThat(smm.getView(42)).isNotNull() } + @Test + fun writableNativeMap_mutationAfterImportingValues_preservesCachedPointers() { + val map = WritableNativeMap() + map.putDouble("opacity", 1.0) + + assertThat(map.hasKey("opacity")).isTrue() + assertThat(map.getType("opacity")).isEqualTo(ReadableType.Number) + + map.putDouble("opacity", 0.3) + repeat(64) { map.putInt("newKey$it", it) } + + val entry = map.entryIterator.next() + assertThat(entry.key).isEqualTo("opacity") + assertThat(entry.value).isEqualTo(0.3) + } + /** * Simulates the scenario fixed by D98729251 via IntBufferBatchMountItem: * 1. Preallocate a view (simulates C++ preallocateShadowView calling Java preallocateView) diff --git a/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp b/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp index 99886999293d..23640f6dc641 100644 --- a/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp +++ b/packages/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/android/react/renderer/textlayoutmanager/TextLayoutManager.cpp @@ -11,7 +11,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api b/scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api index 2e6dc7e0b4e2..e0f7b206a665 100644 --- a/scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api +++ b/scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api @@ -1064,7 +1064,7 @@ uint8_t facebook::react::blueFromColor(facebook::react::SharedColor color) noexc uint8_t facebook::react::greenFromColor(facebook::react::SharedColor color) noexcept; uint8_t facebook::react::redFromColor(facebook::react::SharedColor color) noexcept; void facebook::react::FBReactNativeSpec_registerComponentDescriptorsFromCodegen(std::shared_ptr registry); -void facebook::react::addDynamicToJArray(jni::local_ref> jarray, jint index, const folly::dynamic& dyn); +void facebook::react::addDynamicToJArray(jni::alias_ref> jarray, jint index, const folly::dynamic& dyn); void facebook::react::bindHasComponentProvider(facebook::jsi::Runtime& runtime, facebook::react::HasComponentProviderFunctionType&& provider); void facebook::react::bindNativeLogger(facebook::jsi::Runtime& runtime, facebook::react::Logger logger); void facebook::react::bindNativePerformanceNow(facebook::jsi::Runtime& runtime); @@ -7877,12 +7877,11 @@ struct facebook::react::ReadableNativeMap : public jni::HybridClass createWithContents(folly::dynamic&& map); public static void mapException(std::exception_ptr ex); public static void registerNatives(); - public std::optional keys_; } struct facebook::react::ReadableType : public facebook::jni::JavaClass { public static constexpr auto kJavaDescriptor; - public static jni::local_ref getType(folly::dynamic::Type type); + public static jni::alias_ref getType(folly::dynamic::Type type); } struct facebook::react::RecoverableError : public std::exception { diff --git a/scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api b/scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api index 7ec351405ee3..2f5f619dcc99 100644 --- a/scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api +++ b/scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api @@ -1060,7 +1060,7 @@ uint8_t facebook::react::blueFromColor(facebook::react::SharedColor color) noexc uint8_t facebook::react::greenFromColor(facebook::react::SharedColor color) noexcept; uint8_t facebook::react::redFromColor(facebook::react::SharedColor color) noexcept; void facebook::react::FBReactNativeSpec_registerComponentDescriptorsFromCodegen(std::shared_ptr registry); -void facebook::react::addDynamicToJArray(jni::local_ref> jarray, jint index, const folly::dynamic& dyn); +void facebook::react::addDynamicToJArray(jni::alias_ref> jarray, jint index, const folly::dynamic& dyn); void facebook::react::bindHasComponentProvider(facebook::jsi::Runtime& runtime, facebook::react::HasComponentProviderFunctionType&& provider); void facebook::react::bindNativeLogger(facebook::jsi::Runtime& runtime, facebook::react::Logger logger); void facebook::react::bindNativePerformanceNow(facebook::jsi::Runtime& runtime); @@ -7637,12 +7637,11 @@ struct facebook::react::ReadableNativeMap : public jni::HybridClass createWithContents(folly::dynamic&& map); public static void mapException(std::exception_ptr ex); public static void registerNatives(); - public std::optional keys_; } struct facebook::react::ReadableType : public facebook::jni::JavaClass { public static constexpr auto kJavaDescriptor; - public static jni::local_ref getType(folly::dynamic::Type type); + public static jni::alias_ref getType(folly::dynamic::Type type); } struct facebook::react::RecoverableError : public std::exception { diff --git a/scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api b/scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api index 6843410835c3..47881292af27 100644 --- a/scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api +++ b/scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api @@ -1064,7 +1064,7 @@ uint8_t facebook::react::blueFromColor(facebook::react::SharedColor color) noexc uint8_t facebook::react::greenFromColor(facebook::react::SharedColor color) noexcept; uint8_t facebook::react::redFromColor(facebook::react::SharedColor color) noexcept; void facebook::react::FBReactNativeSpec_registerComponentDescriptorsFromCodegen(std::shared_ptr registry); -void facebook::react::addDynamicToJArray(jni::local_ref> jarray, jint index, const folly::dynamic& dyn); +void facebook::react::addDynamicToJArray(jni::alias_ref> jarray, jint index, const folly::dynamic& dyn); void facebook::react::bindHasComponentProvider(facebook::jsi::Runtime& runtime, facebook::react::HasComponentProviderFunctionType&& provider); void facebook::react::bindNativeLogger(facebook::jsi::Runtime& runtime, facebook::react::Logger logger); void facebook::react::bindNativePerformanceNow(facebook::jsi::Runtime& runtime); @@ -7868,12 +7868,11 @@ struct facebook::react::ReadableNativeMap : public jni::HybridClass createWithContents(folly::dynamic&& map); public static void mapException(std::exception_ptr ex); public static void registerNatives(); - public std::optional keys_; } struct facebook::react::ReadableType : public facebook::jni::JavaClass { public static constexpr auto kJavaDescriptor; - public static jni::local_ref getType(folly::dynamic::Type type); + public static jni::alias_ref getType(folly::dynamic::Type type); } struct facebook::react::RecoverableError : public std::exception {