Pass a top-level JS null TurboModule arg to ObjC as nil, not NSNull (#58190)

Summary:

Changelog: [iOS][Fixed] - Pass a top-level JS `null` TurboModule argument to Objective-C as `nil` instead of `NSNull` when `enableModuleArgumentNSNullConversionIOS` is enabled

When `enableModuleArgumentNSNullConversionIOS` is on, `convertJSIValueToObjCObject`
maps a JS `null` to `(id)kCFNull`. That is the intended behaviour for nulls *nested*
inside arrays and dictionaries, but a `null` in **argument position** must still reach
Objective-C as `nil` — `NSNull` is truthy and does not respond to the selectors the
receiver expects, so leaking it crashes the callee.

The guard that enforced this lived three branches deep in
`ObjCTurboModule::setInvocationArg`, reachable only when all of the following held:

- `objCArgType == encode(id)`, and
- `getArgumentTypeName(...)` returned non-nil, and
- `RCTConvert` responded to a selector named after that type.

`getArgumentTypeName` resolves the argument type by scanning for `__rct_export__`-prefixed
selectors, which the compiler only emits for methods declared with `RCT_EXPORT_METHOD`.
Any TurboModule method without that macro — or with an `id`-typed argument, since
`[RCTConvert respondsToSelector:selector(id:)]` is `NO` — silently skipped the guard and
received `NSNull`.

This diff hoists the check to immediately after the conversion, so it applies to every
argument regardless of the method's `__rct_export__` metadata, its ObjC type encoding, or
whether an `RCTConvert` converter exists. Returning without calling `setArgument:` leaves
the `NSInvocation` slot zeroed, i.e. `nil` — identical to what the old guard did.

Behaviour is unchanged when the flag is off: the check short-circuits on the flag. Nested
`NSNull` inside arrays and dictionaries is untouched, as asserted by the new
`testInvokeTurboModuleKeepsNestedNullAsNSNullWhenFlagEnabled` case. (The flag-enabled
branch of the pre-existing `testInvokeTurboModuleWithNull` case never executes while the
flag defaults to `false`, so it did not cover this.)

The pre-existing check inside the `RCTConvert` branch is left in place. It is now
effectively unreachable — `objCArg == kCFNull` is the only way `convertedObjCArg` can be
`kCFNull`, because every `RCTConvert` converter either returns a non-`kCFNull` input
unchanged or builds a new object — but it costs nothing and keeps the diff narrow.

Differential Revision: D117960577

Author: christophpurrer

packages/react-native/ReactCommon/react/nativemodule/core/iostests/RCTTurboModuleTests.mm

@@ -12,7 +12,9 @@
 #import <hermes/hermes.h>
 #import <jsi/decorator.h>
 #import <react/featureflags/ReactNativeFeatureFlags.h>
+#import <react/featureflags/ReactNativeFeatureFlagsDefaults.h>
 
+#import <array>
 #import <memory>
 #import <vector>
 
@@ -22,6 +24,15 @@
 
 @interface RCTTestTurboModule : NSObject <RCTBridgeModule>
 
+// Deliberately not exported with RCT_EXPORT_METHOD: TurboModules dispatch through the codegen'd
+// `...SpecJSI`, so many of them carry no `__rct_export__` metadata and `getArgumentTypeName` returns
+// nil for their arguments.
+- (void)testMethodWhichTakesStringWithoutExportMacro:(NSString *)string;
+
+// Mirrors -[RCTAnalytics logEvent:data:analyticsModule:] in its post-codemod, unexported form: a
+// trailing nullable object argument that JS always passes as null. See T286617647.
+- (void)logEvent:(NSString *)eventName data:(NSDictionary *)data analyticsModule:(nullable NSString *)analyticsModule;
+
 @end
 
 @implementation RCTTestTurboModule
@@ -30,8 +41,24 @@ @implementation RCTTestTurboModule
 
 RCT_EXPORT_METHOD(testMethodWhichTakesObject : (id)object) {}
 
+- (void)testMethodWhichTakesStringWithoutExportMacro:(NSString *)string
+{
+}
+
+- (void)logEvent:(NSString *)eventName data:(NSDictionary *)data analyticsModule:(nullable NSString *)analyticsModule
+{
+}
+
 @end
 
+class ReactNativeFeatureFlagsNSNullConversionEnabled : public ReactNativeFeatureFlagsDefaults {
+ public:
+  bool enableModuleArgumentNSNullConversionIOS() override
+  {
+    return true;
+  }
+};
+
 // Minimal concrete MutableBuffer that owns its bytes, used to observe lifetime.
 class TestMutableBuffer : public facebook::jsi::MutableBuffer {
  public:
@@ -122,6 +149,8 @@ - (void)tearDown
   module_ = nullptr;
   instance_ = nil;
 
+  ReactNativeFeatureFlags::dangerouslyReset();
+
   [super tearDown];
 }
 
@@ -159,6 +188,84 @@ - (void)testInvokeTurboModuleWithNull
   OCMVerify(OCMTimes(1), [instance_ testMethodWhichTakesObject:nil]);
 }
 
+// A JS `null` argument must arrive as `nil` even when the method carries no `__rct_export__`
+// metadata, so that nullability checks in the receiver behave. `NSNull` is truthy and does not
+// respond to most NSString/NSDictionary selectors, so leaking it crashes the callee.
+- (void)testInvokeUnexportedTurboModuleMethodWithNullPassesNil
+{
+  ReactNativeFeatureFlags::dangerouslyForceOverride(std::make_unique<ReactNativeFeatureFlagsNSNullConversionEnabled>());
+
+  auto hermesRuntime = facebook::hermes::makeHermesRuntime();
+  facebook::jsi::Runtime *rt = hermesRuntime.get();
+
+  std::array<facebook::jsi::Value, 1> args = {facebook::jsi::Value::null()};
+  module_->invokeObjCMethod(
+      *rt,
+      VoidKind,
+      "testMethodWhichTakesStringWithoutExportMacro",
+      @selector(testMethodWhichTakesStringWithoutExportMacro:),
+      args.data(),
+      args.size());
+
+  OCMVerify(OCMTimes(1), [instance_ testMethodWhichTakesStringWithoutExportMacro:nil]);
+  OCMVerify(OCMNever(), [instance_ testMethodWhichTakesStringWithoutExportMacro:(id)kCFNull]);
+}
+
+// The shape that actually crashed in T286617647: JS calls `Analytics.logEvent(name, data, null)`, so
+// the null lands on a trailing nullable object argument of an unexported method. `NSNull` is truthy,
+// so -[RCTAnalytics logEvent:data:analyticsModule:]'s `analyticsModule ? analyticsModule : @""`
+// fallback forwarded it to FBAnalyticsMergeStructuredLogEventMetadata, which sent it -mutableCopy.
+- (void)testInvokeUnexportedTurboModuleMethodWithNullTrailingArgumentPassesNil
+{
+  ReactNativeFeatureFlags::dangerouslyForceOverride(std::make_unique<ReactNativeFeatureFlagsNSNullConversionEnabled>());
+
+  auto hermesRuntime = facebook::hermes::makeHermesRuntime();
+  facebook::jsi::Runtime *rt = hermesRuntime.get();
+
+  __block id capturedAnalyticsModule = (id)kCFNull;
+  OCMStub([instance_ logEvent:OCMOCK_ANY
+                         data:OCMOCK_ANY
+              analyticsModule:[OCMArg checkWithBlock:^BOOL(id value) {
+                capturedAnalyticsModule = value;
+                return YES;
+              }]]);
+
+  std::array<facebook::jsi::Value, 3> args = {
+      facebook::jsi::String::createFromAscii(*rt, "some_event"),
+      facebook::jsi::Object(*rt),
+      facebook::jsi::Value::null()};
+  args[1].asObject(*rt).setProperty(*rt, "key", "value");
+
+  module_->invokeObjCMethod(
+      *rt, VoidKind, "logEvent", @selector(logEvent:data:analyticsModule:), args.data(), args.size());
+
+  OCMVerify(OCMTimes(1), [instance_ logEvent:@"some_event" data:@{@"key" : @"value"} analyticsModule:nil]);
+  XCTAssertNil(capturedAnalyticsModule);
+
+  // The operation that threw once NSNull reached the module.
+  NSString *analyticsModule = capturedAnalyticsModule ? capturedAnalyticsModule : @"";
+  XCTAssertNoThrow([analyticsModule mutableCopy]);
+}
+
+// The counterpart to the two cases above: scrubbing a null in argument position must not scrub nulls
+// *nested* inside a collection argument, which is the behaviour the flag exists to introduce. The
+// flag-enabled branch of testInvokeTurboModuleWithNull never runs while the flag defaults to false,
+// so this is what actually pins the nested behaviour the fix has to leave alone.
+- (void)testInvokeTurboModuleKeepsNestedNullAsNSNullWhenFlagEnabled
+{
+  ReactNativeFeatureFlags::dangerouslyForceOverride(std::make_unique<ReactNativeFeatureFlagsNSNullConversionEnabled>());
+
+  auto hermesRuntime = facebook::hermes::makeHermesRuntime();
+  facebook::jsi::Runtime *rt = hermesRuntime.get();
+
+  std::array<facebook::jsi::Value, 1> args = {facebook::jsi::Object(*rt)};
+  args[0].asObject(*rt).setProperty(*rt, "foo", facebook::jsi::Value::null());
+  module_->invokeObjCMethod(
+      *rt, VoidKind, "testMethodWhichTakesObject", @selector(testMethodWhichTakesObject:), args.data(), args.size());
+
+  OCMVerify(OCMTimes(1), [instance_ testMethodWhichTakesObject:@{@"foo" : (id)kCFNull}]);
+}
+
 // A native-backed ArrayBuffer is aliased rather than copied, and the RCTArrayBuffer retains
 // the backing MutableBuffer, so the alias outlives the JS object.
 - (void)testNativeBackedArrayBufferIsAliasedAndKeepsBackingStoreAlive

packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm

@@ -727,6 +727,16 @@ TraceSection s(
   BOOL enableModuleArgumentNSNullConversionIOS = ReactNativeFeatureFlags::enableModuleArgumentNSNullConversionIOS();
   id objCArg =
       convertJSIValueToObjCObject(runtime, arg, jsInvoker_, enableModuleArgumentNSNullConversionIOS, mustCopyBytes);
+
+  /**
+   * A JS `null` in argument position must reach ObjC as `nil`; only nulls *nested* inside arrays and
+   * dictionaries are preserved as `kCFNull`. Returning without calling `setArgument:` leaves the
+   * NSInvocation slot zeroed, i.e. `nil`.
+   */
+  if (enableModuleArgumentNSNullConversionIOS && objCArg == (id)kCFNull) {
+    return;
+  }
+
   if (objCArg != nullptr) {
     NSString *methodNameNSString = @(methodName);