From 61b692c7ff3c66518b3a13fe2b1e36a8853d0717 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Thu, 29 Oct 2026 09:17:35 -0700 Subject: [PATCH] Support C++ construction of WritableNativeMap (#59618) Summary: Pull Request resolved: https://github.com/react/react-native/pull/59618 Avoid initialising the native object twice when constructing `WritableNativeMap` from C++. Give `newObjectCxxArgs` a no-init Java constructor path while preserving the normal empty-map Java constructor. Build touch-event payloads as complete `folly::dynamic` maps and pass them to that factory, instead of populating writable maps and arrays field by field. The payload shape, value types and writable event APIs remain unchanged. Changelog: [Android][Fixed] - Allow `WritableNativeMap` construction from C++ without double initialisation. [Android][Changed] - Construct writable touch-event payloads directly in C++. Differential Revision: D128312388 --- .../react/bridge/WritableNativeMap.kt | 10 +++- .../FabricMountingManagerTestHelper.cpp | 14 +++++ .../src/main/jni/react/jni/TouchesHelper.cpp | 26 ++------ .../main/jni/react/jni/WritableNativeMap.cpp | 9 +++ .../main/jni/react/jni/WritableNativeMap.h | 3 + ...abricMountingManagerInstrumentationTest.kt | 60 ++++++++++++++++++- .../fabric/FabricMountingManagerTestHelper.kt | 6 ++ .../api-snapshots/ReactAndroidDebugCxx.api | 1 + .../ReactAndroidFrameworksCxx.api | 1 + .../api-snapshots/ReactAndroidNewarchCxx.api | 1 + .../api-snapshots/ReactAndroidReleaseCxx.api | 1 + 11 files changed, 107 insertions(+), 25 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/WritableNativeMap.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/WritableNativeMap.kt index bc30404a4c78..5767d60117d1 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/WritableNativeMap.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/WritableNativeMap.kt @@ -15,9 +15,15 @@ import com.facebook.proguard.annotations.DoNotStrip * to stub out creating this class in a test. TODO(5815532): Check if consumed on read */ @DoNotStrip -public class WritableNativeMap : ReadableNativeMap(), WritableMap { +public class WritableNativeMap @DoNotStrip private constructor(initializeNative: Boolean) : + ReadableNativeMap(), WritableMap { + /** Creates an empty writable map backed by native memory. */ + public constructor() : this(true) + init { - initHybrid() + if (initializeNative) { + initHybrid() + } } external override fun putBoolean(key: String, value: Boolean) diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp index 0486e4529769..83c84cf78cec 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -62,6 +63,17 @@ class FabricMountingManagerTestHelper::DeleteRecorder namespace { +jni::local_ref createWritableNativeMapFromCxx( + jni::alias_ref /*unused*/, + jboolean withContents) { + if (withContents == JNI_FALSE) { + return WritableNativeMap::newObjectCxxArgs(); + } + folly::dynamic payload = folly::dynamic::object("value", 42)( + "nested", folly::dynamic::object("enabled", true)); + return WritableNativeMap::newObjectCxxArgs(std::move(payload)); +} + ShadowView makeShadowView(jint surfaceId, jint tag) { ShadowView sv{}; sv.componentName = "View"; @@ -402,6 +414,8 @@ FabricMountingManagerTestHelper::pulledDeleteTags() { void FabricMountingManagerTestHelper::registerNatives() { registerHybrid({ + makeNativeMethod( + "createWritableNativeMapFromCxx", createWritableNativeMapFromCxx), makeNativeMethod( "initHybrid", FabricMountingManagerTestHelper::initHybrid), makeNativeMethod( diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/TouchesHelper.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/TouchesHelper.cpp index 6696c93d0bb8..cee03fe6832a 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/TouchesHelper.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/TouchesHelper.cpp @@ -8,8 +8,8 @@ #include "TouchesHelper.h" #include +#include -#include "WritableNativeArray.h" #include "WritableNativeMap.h" using namespace facebook::jni; @@ -72,25 +72,11 @@ local_ref createTouchPayload( } } - auto jPayload = WritableNativeMap::newObjectJavaArgs(); - auto* payload = jPayload->cthis(); - // NOLINTBEGIN(cppcoreguidelines-pro-bounds-pointer-arithmetic) - const double* fields = values.get() + eventPointer * kPointerFields; - payload->putDouble("pageX", fields[kPageXIndex]); - payload->putDouble("pageY", fields[kPageYIndex]); - payload->putDouble("locationX", fields[kLocationXIndex]); - payload->putDouble("locationY", fields[kLocationYIndex]); - payload->putInt( - "targetSurface", static_cast(fields[kTargetSurfaceIndex])); - payload->putInt("target", static_cast(fields[kTargetIndex])); - payload->putDouble("timestamp", fields[kTimestampIndex]); - payload->putDouble("identifier", fields[kIdentifierIndex]); - // NOLINTEND(cppcoreguidelines-pro-bounds-pointer-arithmetic) - WritableNativeArray changedTouchesArray(std::move(changedTouches)); - payload->putNativeArray("changedTouches", &changedTouchesArray); - WritableNativeArray touchesArray(std::move(touches)); - payload->putNativeArray("touches", &touchesArray); - return jPayload; + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + auto payload = createTouch(values.get() + eventPointer * kPointerFields); + payload.insert("changedTouches", std::move(changedTouches)); + payload.insert("touches", std::move(touches)); + return WritableNativeMap::newObjectCxxArgs(std::move(payload)); } } // namespace diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/WritableNativeMap.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/WritableNativeMap.cpp index 086bab13a34b..ecbce35b49a7 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/WritableNativeMap.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/WritableNativeMap.cpp @@ -7,6 +7,8 @@ #include "WritableNativeMap.h" +#include + using namespace facebook::jni; namespace facebook::react { @@ -20,6 +22,13 @@ WritableNativeMap::WritableNativeMap(folly::dynamic&& val) } } +local_ref WritableNativeMap::newObjectCxxArgs( + folly::dynamic&& val) { + auto map = newObjectJavaArgs(false); + setCxxInstance(map, std::move(val)); + return map; +} + void WritableNativeMap::initHybrid(alias_ref jobj) { setCxxInstance(jobj); } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/WritableNativeMap.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/WritableNativeMap.h index d11799824146..848a219c6174 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/WritableNativeMap.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/WritableNativeMap.h @@ -26,6 +26,9 @@ struct WritableNativeMap : jni::HybridClass newObjectCxxArgs(folly::dynamic &&val = folly::dynamic::object()); + static void initHybrid(jni::alias_ref jobj); void putNull(std::string key); 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 f88622d5e48d..bac20710a0d4 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 @@ -8,6 +8,10 @@ package com.facebook.react.fabric import android.graphics.Rect +import android.os.ParcelFileDescriptor +import android.os.Process +import android.os.SystemClock +import android.util.Log import android.view.View import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry @@ -35,6 +39,7 @@ import com.facebook.react.uimanager.events.BatchEventDispatchedListener import com.facebook.react.views.view.ReactViewGroup import com.facebook.react.views.view.ReactViewManager import com.facebook.soloader.SoLoader +import java.util.UUID import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.within import org.junit.After @@ -134,6 +139,55 @@ class FabricMountingManagerInstrumentationTest { assertThat(smm.getView(42)).isNotNull() } + @Test + fun writableNativeMap_createdInCxx_preservesContentsAndRemainsWritable() { + val map = createWritableNativeMapFromCxx(true) + + map.putInt("added", 7) + map.putString("label", "writable") + assertThat(map.getInt("value")).isEqualTo(42) + assertThat(map.getMap("nested")?.getBoolean("enabled")).isTrue() + assertThat(map.getInt("added")).isEqualTo(7) + assertThat(map.getString("label")).isEqualTo("writable") + } + + @Test + fun writableNativeMap_createdInCxxWithoutContents_isEmptyAndWritable() { + val map = createWritableNativeMapFromCxx(false) + + assertThat(map.toString()).isEqualTo("{}") + map.putInt("value", 42) + assertThat(map.getInt("value")).isEqualTo(42) + } + + private fun createWritableNativeMapFromCxx(withContents: Boolean): WritableNativeMap { + val token = UUID.randomUUID().toString() + val startMarker = "WritableNativeMap start $token" + val endMarker = "WritableNativeMap end $token" + Log.i("WritableNativeMapTest", startMarker) + val map = FabricMountingManagerTestHelper.createWritableNativeMapFromCxx(withContents) + Log.i("WritableNativeMapTest", endMarker) + + val deadline = SystemClock.elapsedRealtime() + 2_000 + do { + val descriptor = + InstrumentationRegistry.getInstrumentation() + .uiAutomation + .executeShellCommand("logcat -d -b main --pid=${Process.myPid()}") + val logs = + ParcelFileDescriptor.AutoCloseInputStream(descriptor).bufferedReader().use { + it.readText() + } + if (logs.contains(endMarker)) { + assertThat(logs).contains(startMarker) + assertThat(logs.substringAfter(startMarker).substringBefore(endMarker)) + .doesNotContain("Attempt to set C++ native pointer twice") + return map + } + } while (SystemClock.elapsedRealtime() < deadline) + throw AssertionError("WritableNativeMap log markers did not arrive") + } + @Test fun writableNativeMap_mutationAfterImportingValues_preservesCachedPointers() { val map = WritableNativeMap() @@ -252,7 +306,7 @@ class FabricMountingManagerInstrumentationTest { } private fun mountPaddedView( - fixTextLayoutOnPixelDensityChange: Boolean + fixTextLayoutOnPixelDensityChange: Boolean, ): Pair { val fabricUIManager = createFabricUIManager(listOf(PaddingViewManager())) val helper = FabricMountingManagerTestHelper.create(fabricUIManager) @@ -281,7 +335,7 @@ class FabricMountingManagerInstrumentationTest { ReactNativeFeatureFlagsCxxInterop.dangerouslyForceOverride( object : ReactNativeFeatureFlagsDefaults() { override fun enableMountingCoordinatorPullModelAndroid(): Boolean = true - } + }, ) } @@ -501,7 +555,7 @@ class FabricMountingManagerInstrumentationTest { override fun enablePropsUpdateReconciliationAndroid(): Boolean = pullModel override fun enablePreallocatedPropsDiffOnInsertAndroid(): Boolean = pullModel - } + }, ) } diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerTestHelper.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerTestHelper.kt index 5edd4f0c00cb..f4cd1a414fba 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerTestHelper.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerTestHelper.kt @@ -8,9 +8,11 @@ package com.facebook.react.fabric import com.facebook.jni.HybridClassBase +import com.facebook.proguard.annotations.DoNotStrip import com.facebook.react.bridge.NativeMap import com.facebook.react.bridge.RuntimeExecutor import com.facebook.react.bridge.RuntimeScheduler +import com.facebook.react.bridge.WritableNativeMap import com.facebook.soloader.SoLoader /** @@ -90,6 +92,10 @@ private constructor(fabricUIManager: FabricUIManager) : HybridClassBase() { SoLoader.loadLibrary("fabricjni_test_helper") } + @DoNotStrip + @JvmStatic + external fun createWritableNativeMapFromCxx(withContents: Boolean): WritableNativeMap + @JvmStatic fun create(fabricUIManager: FabricUIManager): FabricMountingManagerTestHelper { return FabricMountingManagerTestHelper(fabricUIManager) diff --git a/scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api b/scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api index 97a6a1f652cc..8ef92b0a272b 100644 --- a/scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api +++ b/scripts/cxx-api/api-snapshots/ReactAndroidDebugCxx.api @@ -3756,6 +3756,7 @@ struct facebook::react::WritableNativeMap : public jni::HybridClass newObjectCxxArgs(folly::dynamic&& val = folly::dynamic::object()); public static void initHybrid(jni::alias_ref jobj); public static void registerNatives(); public void mergeNativeMap(facebook::react::ReadableNativeMap* other); diff --git a/scripts/cxx-api/api-snapshots/ReactAndroidFrameworksCxx.api b/scripts/cxx-api/api-snapshots/ReactAndroidFrameworksCxx.api index e793d6f7de2c..36556303a6a3 100644 --- a/scripts/cxx-api/api-snapshots/ReactAndroidFrameworksCxx.api +++ b/scripts/cxx-api/api-snapshots/ReactAndroidFrameworksCxx.api @@ -6806,6 +6806,7 @@ struct facebook::react::WritableNativeMap : public jni::HybridClass newObjectCxxArgs(folly::dynamic&& val = folly::dynamic::object()); public static void initHybrid(jni::alias_ref jobj); public static void registerNatives(); public void mergeNativeMap(facebook::react::ReadableNativeMap* other); diff --git a/scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api b/scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api index 05dc03891252..a141d7e06bf5 100644 --- a/scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api +++ b/scripts/cxx-api/api-snapshots/ReactAndroidNewarchCxx.api @@ -3749,6 +3749,7 @@ struct facebook::react::WritableNativeMap : public jni::HybridClass newObjectCxxArgs(folly::dynamic&& val = folly::dynamic::object()); public static void initHybrid(jni::alias_ref jobj); public static void registerNatives(); public void mergeNativeMap(facebook::react::ReadableNativeMap* other); diff --git a/scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api b/scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api index 05dc03891252..a141d7e06bf5 100644 --- a/scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api +++ b/scripts/cxx-api/api-snapshots/ReactAndroidReleaseCxx.api @@ -3749,6 +3749,7 @@ struct facebook::react::WritableNativeMap : public jni::HybridClass newObjectCxxArgs(folly::dynamic&& val = folly::dynamic::object()); public static void initHybrid(jni::alias_ref jobj); public static void registerNatives(); public void mergeNativeMap(facebook::react::ReadableNativeMap* other);