-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix crash caused by using ShareableHandle
in multiple remote runtimes
#6796
Fix crash caused by using ShareableHandle
in multiple remote runtimes
#6796
Conversation
Please confirm that we don't need any additional synchronization involving cc @tjzel who touched this code some time ago |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…es (#6796) ## Summary Fixes Expensify/react-native-live-markdown#574. See root cause analysis in Expensify/react-native-live-markdown#574 (comment). This PR fixes a crash caused by calling `ShareableHandle::toJSValue` with second remote runtime after initializing `remoteValue_` with a `jsi::Value` belonging to the first remote runtime. I assume that this is a rare scenario so we only memoize the value for the first remote runtime and we recreate the value for all subsequent runtimes. ## Test plan Reproduction: <details> <summary>EmptyExample.tsx</summary> ```tsx import { Text, StyleSheet, View } from 'react-native'; import React from 'react'; import { createWorkletRuntime, runOnRuntime, useAnimatedStyle, } from 'react-native-reanimated'; const regex = /\d/; const workletRuntime = createWorkletRuntime('another'); export default function EmptyExample() { useAnimatedStyle(() => { console.log('useAnimatedStyle', String(regex)); return {}; }); runOnRuntime(workletRuntime, () => { 'worklet'; console.log('runOnRuntime', String(regex)); return {}; })(); return ( <View style={styles.container}> <Text>Hello world!</Text> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, alignItems: 'center', justifyContent: 'center', }, }); ``` </details>
…es (#6796) ## Summary Fixes Expensify/react-native-live-markdown#574. See root cause analysis in Expensify/react-native-live-markdown#574 (comment). This PR fixes a crash caused by calling `ShareableHandle::toJSValue` with second remote runtime after initializing `remoteValue_` with a `jsi::Value` belonging to the first remote runtime. I assume that this is a rare scenario so we only memoize the value for the first remote runtime and we recreate the value for all subsequent runtimes. ## Test plan Reproduction: <details> <summary>EmptyExample.tsx</summary> ```tsx import { Text, StyleSheet, View } from 'react-native'; import React from 'react'; import { createWorkletRuntime, runOnRuntime, useAnimatedStyle, } from 'react-native-reanimated'; const regex = /\d/; const workletRuntime = createWorkletRuntime('another'); export default function EmptyExample() { useAnimatedStyle(() => { console.log('useAnimatedStyle', String(regex)); return {}; }); runOnRuntime(workletRuntime, () => { 'worklet'; console.log('runOnRuntime', String(regex)); return {}; })(); return ( <View style={styles.container}> <Text>Hello world!</Text> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, alignItems: 'center', justifyContent: 'center', }, }); ``` </details>
Changes look ok, and probably we also need to add that logic to RetainingShareable. What do you think? 🤔 |
Looks like Lines 142 to 163 in 2a947a8
|
…es (#6796) ## Summary Fixes Expensify/react-native-live-markdown#574. See root cause analysis in Expensify/react-native-live-markdown#574 (comment). This PR fixes a crash caused by calling `ShareableHandle::toJSValue` with second remote runtime after initializing `remoteValue_` with a `jsi::Value` belonging to the first remote runtime. I assume that this is a rare scenario so we only memoize the value for the first remote runtime and we recreate the value for all subsequent runtimes. ## Test plan Reproduction: <details> <summary>EmptyExample.tsx</summary> ```tsx import { Text, StyleSheet, View } from 'react-native'; import React from 'react'; import { createWorkletRuntime, runOnRuntime, useAnimatedStyle, } from 'react-native-reanimated'; const regex = /\d/; const workletRuntime = createWorkletRuntime('another'); export default function EmptyExample() { useAnimatedStyle(() => { console.log('useAnimatedStyle', String(regex)); return {}; }); runOnRuntime(workletRuntime, () => { 'worklet'; console.log('runOnRuntime', String(regex)); return {}; })(); return ( <View style={styles.container}> <Text>Hello world!</Text> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, alignItems: 'center', justifyContent: 'center', }, }); ``` </details>
…es (#6796) ## Summary Fixes Expensify/react-native-live-markdown#574. See root cause analysis in Expensify/react-native-live-markdown#574 (comment). This PR fixes a crash caused by calling `ShareableHandle::toJSValue` with second remote runtime after initializing `remoteValue_` with a `jsi::Value` belonging to the first remote runtime. I assume that this is a rare scenario so we only memoize the value for the first remote runtime and we recreate the value for all subsequent runtimes. ## Test plan Reproduction: <details> <summary>EmptyExample.tsx</summary> ```tsx import { Text, StyleSheet, View } from 'react-native'; import React from 'react'; import { createWorkletRuntime, runOnRuntime, useAnimatedStyle, } from 'react-native-reanimated'; const regex = /\d/; const workletRuntime = createWorkletRuntime('another'); export default function EmptyExample() { useAnimatedStyle(() => { console.log('useAnimatedStyle', String(regex)); return {}; }); runOnRuntime(workletRuntime, () => { 'worklet'; console.log('runOnRuntime', String(regex)); return {}; })(); return ( <View style={styles.container}> <Text>Hello world!</Text> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, alignItems: 'center', justifyContent: 'center', }, }); ``` </details>
…es (#6796) ## Summary Fixes Expensify/react-native-live-markdown#574. See root cause analysis in Expensify/react-native-live-markdown#574 (comment). This PR fixes a crash caused by calling `ShareableHandle::toJSValue` with second remote runtime after initializing `remoteValue_` with a `jsi::Value` belonging to the first remote runtime. I assume that this is a rare scenario so we only memoize the value for the first remote runtime and we recreate the value for all subsequent runtimes. ## Test plan Reproduction: <details> <summary>EmptyExample.tsx</summary> ```tsx import { Text, StyleSheet, View } from 'react-native'; import React from 'react'; import { createWorkletRuntime, runOnRuntime, useAnimatedStyle, } from 'react-native-reanimated'; const regex = /\d/; const workletRuntime = createWorkletRuntime('another'); export default function EmptyExample() { useAnimatedStyle(() => { console.log('useAnimatedStyle', String(regex)); return {}; }); runOnRuntime(workletRuntime, () => { 'worklet'; console.log('runOnRuntime', String(regex)); return {}; })(); return ( <View style={styles.container}> <Text>Hello world!</Text> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, alignItems: 'center', justifyContent: 'center', }, }); ``` </details>
Summary
Fixes Expensify/react-native-live-markdown#574. See root cause analysis in Expensify/react-native-live-markdown#574 (comment).
This PR fixes a crash caused by calling
ShareableHandle::toJSValue
with second remote runtime after initializingremoteValue_
with ajsi::Value
belonging to the first remote runtime.I assume that this is a rare scenario so we only memoize the value for the first remote runtime and we recreate the value for all subsequent runtimes.
Test plan
Reproduction:
EmptyExample.tsx