Skip to content
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.

Sign up for GitHub

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

Jump to bottom

App crashes if parser uses a regex that is used in another worklet #574

Closed
s77rt opened this issue Dec 8, 2024 · 11 comments · Fixed by software-mansion/react-native-reanimated#6796
Closed

App crashes if parser uses a regex that is used in another worklet #574

s77rt opened this issue Dec 8, 2024 · 11 comments · Fixed by software-mansion/react-native-reanimated#6796
Assignees
@tomekzaw
Labels
bug Something isn't working

Comments

@s77rt
Copy link
Contributor

s77rt commented Dec 8, 2024

Using same regex (or objects that contain regex) in multiple worklets causes a crash. This is possibly a bug in react-native-reanimated but I'm not sure. cc @tomekzaw You may have an idea on this

Reproducible example:

function Markdown() {
  const regex = /\d/;

  useAnimatedStyle(() => {
    console.log(regex);
    return {};
  }, []);

  return (
    <MarkdownTextInput
      parser={() => {
        'worklet';
        console.log(regex);
        return [];
      }}
    />
  );
}
@s77rt s77rt mentioned this issue Dec 8, 2024
Open
8 tasks
@tomekzaw
Copy link
Collaborator

tomekzaw commented Dec 8, 2024 •
edited
Loading

@s77rt Thanks for reporting this issue. I can see why using the same regex object in useAnimatedStyle and inside parser worklet could lead to crashes but I'll need to confirm why.

However, I'm not sure if this is the same bug as reported in Expensify/App#52475. Note that PR enabling worklets in react-native-live-markdown (#439) was released in 0.1.188 but Expensify App still uses 0.1.187 (https://github.com/Expensify/App/blob/7925c9b555af60156c62947ae63b517d6c31587f/package.json#L71) where parser is still bundled in a separate file, completely unrelated to reanimated.

@s77rt
Copy link
Contributor Author

s77rt commented Dec 8, 2024

The bug in Expensify/App#52475 is different. I faced the crash while trying to update react-native-live-markdown (which includes the fix for that issue).

@tomekzaw
Copy link
Collaborator

tomekzaw commented Dec 8, 2024

So the fix is in expensify-common and you wanted to bump react-native-live-markdown to get newer version of expensify-common, is that correct? What version of expensify-common is needed?

@s77rt
Copy link
Contributor Author

s77rt commented Dec 8, 2024

Correct! The needed version is 2.0.108

@tomekzaw
Copy link
Collaborator

tomekzaw commented Dec 8, 2024

Alright, so we bumped expensify-common to 2.0.108 in react-native-live-markdown 0.1.192 which already uses worklets. Sounds like there's no easy way and we have to either use patch-package on top if 0.1.187 or just wait until this issue #574 is resolved. Let me know if anything else comes to your mind.

Maybe we should have released worklet support as 0.2.0 so we can keep 0.1.x versions with old engine. Sorry for inconvenience, we don't have much control over versioning scheme as the bot bumps the version in package.json and publishes to npm automatically.

@tomekzaw tomekzaw self-assigned this Dec 8, 2024
@s77rt
Copy link
Contributor Author

s77rt commented Dec 8, 2024

I think I will just avoid the problem. I doubt we need regex in worklets in E/App. It's being captured only because we reference a value that comes from an object CONST.ANIMATION_GYROSCOPE_VALUE. But patching may be the best option and probably the safest.

@tomekzaw
Copy link
Collaborator

tomekzaw commented Dec 8, 2024

Okay, in that case I'll leave the fix in E/App up to you. I will investigate the issue as it's probably rooted in reanimated/worklets.

@tomekzaw
Copy link
Collaborator

tomekzaw commented Dec 8, 2024 •
edited
Loading

I found the root cause of the issue. In Reanimated/Worklets, RegExp objects are cloned to worklet runtimes using ShareableHandle mechanism (see here) via __init factory function that creates an instance of RegExp object on the target runtime.

The C++ implementation of ShareableHandle::toJSValue (see here) is first invoked on the live-markdown worklet runtime and stores the instance from live-markdown runtime in remoteValue_ field (as well as the current runtime in remoteRuntime_ pointer). Then, when ShareableHandle::toJSValue is called on the reanimated runtime, it reads remoteValue_ belonging to another runtime (live-markdown worklet runtime). This leads to a crash since we cannot use jsi::Value from another runtime. Currently, there's no mechanism to detect such case but it can be easily detected with &rt == remoteRuntime_. I will submit a PR to reanimated with the fix.

@s77rt
Copy link
Contributor Author

s77rt commented Dec 8, 2024

Thank you!

@tomekzaw tomekzaw mentioned this issue Dec 8, 2024
Merged
@tomekzaw
Copy link
Collaborator

tomekzaw commented Dec 8, 2024

@s77rt Here's the fix in reanimated: software-mansion/react-native-reanimated#6796

@tomekzaw tomekzaw added the bug Something isn't working label Dec 8, 2024
@Kicu
Copy link

Kicu commented Dec 9, 2024

FYI @s77rt I will be bumping (or trying to 😅) live-markdown to the worklet supporting version, and expensify-common to newest this week.

github-merge-queue bot pushed a commit to software-mansion/react-native-reanimated that referenced this issue Dec 9, 2024
@tomekzaw
Fix crash caused by using ShareableHandle in multiple remote runtim…
73ca5ea 
…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>
tomekzaw added a commit to software-mansion/react-native-reanimated that referenced this issue Dec 9, 2024
@tomekzaw
Fix crash caused by using ShareableHandle in multiple remote runtim…
b47caa0 
…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>
tomekzaw added a commit to software-mansion/react-native-reanimated that referenced this issue Dec 9, 2024
@tomekzaw
Fix crash caused by using ShareableHandle in multiple remote runtim…
d94a8a7 
…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>
@s77rt s77rt mentioned this issue Dec 10, 2024
Merged
50 tasks
tjzel pushed a commit to software-mansion/react-native-reanimated that referenced this issue Dec 13, 2024
@tomekzaw @tjzel
Fix crash caused by using ShareableHandle in multiple remote runtim…
f03ba72 
…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>
tjzel pushed a commit to software-mansion/react-native-reanimated that referenced this issue Dec 13, 2024
@tomekzaw @tjzel
Fix crash caused by using ShareableHandle in multiple remote runtim…
5d7305a 
…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>
tjzel pushed a commit to software-mansion/react-native-reanimated that referenced this issue Dec 13, 2024
@tomekzaw @tjzel
Fix crash caused by using ShareableHandle in multiple remote runtim…
8e7c942 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tomekzaw tomekzaw

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
3 participants
@Kicu @s77rt @tomekzaw