Fix passing remote functions as arguments to runOnJS

[tomekzaw]

Summary

Fixes #4613.

Work in progress, need to fix types.

Test plan

App.tsx

import {Button, StyleSheet, View} from 'react-native';
import {runOnJS, runOnUI} from 'react-native-reanimated';

import React from 'react';

export default function App() {
  const resolve = () => {
    console.log('OK');
  };

  const finishTransition = resolve => {
    resolve();
  };

  const handlePress = () => {
    runOnUI(() => {
      runOnJS(finishTransition)(resolve);
    })();
  };

  return (
    <View style={styles.container}>
      <Button onPress={handlePress} title="Click me" />
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    alignItems: 'center',
    justifyContent: 'center',
  },
});

[tomekzaw]

not a huge fan of this case here

[tjzel]

[AlexanderEggers]

@tomekzaw @tjzel I patched these locally in my project and I am running into quite a bit of Tried to synchronously call a non-worklet function on the UI thread. errors. That wasn't the case with the previous changes that were part of this PR.

Error:

08-15 12:55:09.392  4144  4144 E unknown:ReactNative: Exception in native call
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: com.facebook.jni.CppException: Tried to synchronously call a non-worklet function on the UI thread.
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: Possible solutions are:
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:   a) If you want to synchronously execute this method, mark it as a worklet
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:   b) If you want to execute this function on the JS thread, wrap it using `runOnJS`
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: Error: Tried to synchronously call a non-worklet function on the UI thread.
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: Possible solutions are:
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:   a) If you want to synchronously execute this method, mark it as a worklet
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:   b) If you want to execute this function on the JS thread, wrap it using `runOnJS`
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:     at fun (worklet_13358486790341:1:1149)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:     at cloneRecursive (/Users/alex.eggers/Documents/Projects/demo-project/node_modules/react-native-reanimated/src/reanimated2/shareables.ts:276:31)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:     at makeShareableCloneOnUIRecursive (/Users/alex.eggers/Documents/Projects/demo-project/node_modules/react-native-reanimated/src/reanimated2/shareables.ts:295:25)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:     at anonymous (/Users/alex.eggers/Documents/Projects/demo-project/node_modules/react-native-reanimated/src/reanimated2/threads.ts:185:44)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:     at reportFatalError (/Users/alex.eggers/Documents/Projects/demo-project/node_modules/react-native-reanimated/src/reanimated2/initializers.ts:166:39)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative:     at callGuardDEV (worklet_7535208113410:1:122)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at com.swmansion.reanimated.Scheduler.triggerUI(Native Method)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at com.swmansion.reanimated.Scheduler$1.run(Scheduler.java:24)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at com.swmansion.reanimated.Scheduler$2.runGuarded(Scheduler.java:43)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at com.facebook.react.bridge.GuardedRunnable.run(GuardedRunnable.java:30)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at android.os.Handler.handleCallback(Handler.java:942)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at android.os.Handler.dispatchMessage(Handler.java:99)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at android.os.Looper.loopOnce(Looper.java:201)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at android.os.Looper.loop(Looper.java:288)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at android.app.ActivityThread.main(ActivityThread.java:7872)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at java.lang.reflect.Method.invoke(Native Method)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
08-15 12:55:09.392  4144  4144 E unknown:ReactNative: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)

This is my patch based on 3.4.2:

diff --git a/node_modules/react-native-reanimated/src/reanimated2/commonTypes.ts b/node_modules/react-native-reanimated/src/reanimated2/commonTypes.ts
index 5ab6085..07770a5 100644
--- a/node_modules/react-native-reanimated/src/reanimated2/commonTypes.ts
+++ b/node_modules/react-native-reanimated/src/reanimated2/commonTypes.ts
@@ -55,6 +55,12 @@ export type ShareableRef<T> = {
   __hostObjectShareableJSRef: T;
 };
 
+// In case of objects with depth or arrays of objects or arrays of arrays etc.
+// we add this utility type that makes it a SharaebleRef of the outermost type.
+export type FlatShareableRef<T> = T extends ShareableRef<infer U>
+  ? ShareableRef<U>
+  : ShareableRef<T>;
+
 export type ShareableSyncDataHolderRef<T> = {
   __hostObjectShareableJSRefSyncDataHolder: T;
 };
diff --git a/node_modules/react-native-reanimated/src/reanimated2/globals.d.ts b/node_modules/react-native-reanimated/src/reanimated2/globals.d.ts
index f3259e4..343695a 100644
--- a/node_modules/react-native-reanimated/src/reanimated2/globals.d.ts
+++ b/node_modules/react-native-reanimated/src/reanimated2/globals.d.ts
@@ -8,6 +8,7 @@ import type {
   ShareableSyncDataHolderRef,
   ShadowNodeWrapper,
   ComplexWorkletFunction,
+  FlatShareableRef,
 } from './commonTypes';
 import type { AnimatedStyle } from './helperTypes';
 import type { FrameCallbackRegistryUI } from './frameCallback/FrameCallbackRegistryUI';
@@ -35,7 +36,7 @@ declare global {
   ) => void;
   var _notifyAboutEnd: (tag: number, removeView: boolean) => void;
   var _setGestureState: (handlerTag: number, newState: number) => void;
-  var _makeShareableClone: <T>(value: T) => ShareableRef<T>;
+  var _makeShareableClone: <T>(value: T) => FlatShareableRef<T>;
   var _updateDataSynchronously: (
     dataHolder: ShareableSyncDataHolderRef<any>,
     data: ShareableRef<any>
diff --git a/node_modules/react-native-reanimated/src/reanimated2/shareables.ts b/node_modules/react-native-reanimated/src/reanimated2/shareables.ts
index 03575ab..e03f727 100644
--- a/node_modules/react-native-reanimated/src/reanimated2/shareables.ts
+++ b/node_modules/react-native-reanimated/src/reanimated2/shareables.ts
@@ -1,5 +1,9 @@
 import NativeReanimatedModule from './NativeReanimated';
-import type { ShareableRef, WorkletFunction } from './commonTypes';
+import type {
+  FlatShareableRef,
+  ShareableRef,
+  WorkletFunction,
+} from './commonTypes';
 import { shouldBeUseWeb } from './PlatformChecker';
 import { registerWorkletStackDetails } from './errors';
 import { jsVersion } from './platform-specific/jsVersion';
@@ -20,7 +24,7 @@ const _shareableFlag = Symbol('shareable flag');
 
 const MAGIC_KEY = 'REANIMATED_MAGIC_KEY';
 
-function isHostObject(value: NonNullable<object>): boolean {
+function isHostObject(value: NonNullable<object>) {
   // We could use JSI to determine whether an object is a host object, however
   // the below workaround works well and is way faster than an additional JSI call.
   // We use the fact that host objects have broken implementation of `hasOwnProperty`
@@ -247,28 +251,44 @@ function getWorkletCode(value: WorkletFunction) {
   return code;
 }
 
-export function makeShareableCloneOnUIRecursive<T>(value: T): ShareableRef<T> {
+type RemoteFunction<T> = {
+  __remoteFunction: FlatShareableRef<T>;
+};
+
+function isRemoteFunction<T>(value: object): value is RemoteFunction<T> {
+  return '__remoteFunction' in value;
+}
+
+export function makeShareableCloneOnUIRecursive<T>(
+  value: T
+): FlatShareableRef<T> {
   'worklet';
   if (USE_STUB_IMPLEMENTATION) {
     // @ts-ignore web is an interesting place where we don't run a secondary VM on the UI thread
     // see more details in the comment where USE_STUB_IMPLEMENTATION is defined.
     return value;
   }
-  function cloneRecursive<T>(value: T): ShareableRef<T> {
-    const type = typeof value;
-    if ((type === 'object' || type === 'function') && value !== null) {
-      let toAdapt: any;
+  function cloneRecursive<T>(value: T): FlatShareableRef<T> {
+    if (
+      (typeof value === 'object' && value !== null) ||
+      typeof value === 'function'
+    ) {
+      if (isRemoteFunction<T>(value)) {
+        return value.__remoteFunction;
+      }
+      if (isHostObject(value)) {
+        return value as FlatShareableRef<T>;
+      }
       if (Array.isArray(value)) {
-        toAdapt = value.map((element) => cloneRecursive(element));
-      } else if (value !== undefined) {
-        toAdapt = {};
-        for (const [key, element] of Object.entries(
-          value as Record<string, unknown>
-        )) {
-          toAdapt[key] = cloneRecursive(element);
-        }
+        return _makeShareableClone(
+          value.map(cloneRecursive)
+        ) as FlatShareableRef<T>;
+      }
+      const toAdapt: Record<string, FlatShareableRef<T>> = {};
+      for (const [key, element] of Object.entries(value)) {
+        toAdapt[key] = cloneRecursive<T>(element);
       }
-      return _makeShareableClone(toAdapt);
+      return _makeShareableClone(toAdapt) as FlatShareableRef<T>;
     }
     return _makeShareableClone(value);
   }

[tomekzaw]

@AlexanderEggers I think I know what's the issue, can you please mark isHostObject with 'worklet'; directive?

@tjzel Would be nice to include function name in the error message, for instance "Tried to synchronously call a non-worklet function `isHostObject` on the UI thread" (if possible, in a separate PR). Also, what do you think about describing this error on the brand new Troubleshooting page?

[AlexanderEggers]

@tomekzaw I tried that already but made no difference.

[tomekzaw]

@AlexanderEggers Have you also tried marking isRemoteFunction as worklet?

[AlexanderEggers]

@tomekzaw Adding 'worklet'; to isRemoteFunction and isHostObject solved the error for me.

[tjzel]

@AlexanderEggers I think I know what's the issue, can you please mark isHostObject with 'worklet'; directive?

@tjzel Would be nice to include function name in the error message, for instance "Tried to synchronously call a non-worklet function isHostObject on the UI thread" (if possible, in a separate PR). Also, what do you think about describing this error on the brand new Troubleshooting page?

🤔

good idea

[AlexanderEggers]

@tomekzaw Do you have an ETA when these would be available in a release?

[tomekzaw]

This PR will make it to 3.5.0 which we plan to release next week.

[AlexanderEggers]

@tomekzaw I did a bit more testing using this implementation and unfortunately these changes are crashing in release builds (on Android and iOS) for some reasons.

RCTFatalException: Unhandled JS Exception: undefined is not a function

[tjzel]

@AlexanderEggers I didn't get any crash in the reproduction you provided here but I still have a clue what might be the offender. Could you check it with the changes that I now pushed to this branch? If this doesn't help, could you provide your test suite so I could replicate the crash?

[AlexanderEggers]

@tjzel Thanks, I had a quick test and that seemed to have fixed the crash for me. I am just curious, do you have an idea why my case would crash because of that incorrect order?

[tjzel]

Yes, it's because of how host objects works, or rather the in operator for them - it always returns true.

In debug we have this special object wrapper called RemoteFunction (terrible name imho) that has the actual host object in its __remoteFunction property. Now, if we get a host object and we check first, if it has this property, we will always get true - but this property doesn't exist and accessing it and calling it will result in a crash.

Therefore we must firstly check if it's a host object and then if it's a RemoteFunction - we overlooked this in the first place.

[AlexanderEggers]

oh, that makes sense. Thanks for taking the time to explain that

[tomekzaw]

Thanks @tjzel for landing this PR.