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.

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 "Maximum call stack size exceeded" error #4367

Merged
merged 6 commits into from
Apr 17, 2023
Merged

Conversation

kmagiera
Copy link
Member

@kmagiera kmagiera commented Apr 14, 2023

Summary

This PR addresses a number of issues reported related to exceeding stack when converting objects to shareables (e.g. in #4177).

This issue got introduced in Reanimated 3 after #4060 where we got rid of filtering only attributes accessed in worklets to be captured by worklets.

This PR implements a solution that filters only plain objects before converting things to shareables. In all of the instances of this issue we encountered, the crash was due to the fact we were leaking some react internals (fiber nodes) into the captured variables (e.g. via React's refs or otherwise). These objects are not of a plain object type and hence we wouldn't be able to support transferring these on the UI runtime anyways. With this change we are skipping these type of objects and replace them with a "inaccessible object" which is implemented as a proxy that throws on any attempt of accessing its attributes – this way users will get an error if they do use or access such objects.

In addition we add a cyclic object detection mechanism. Currently cyclic objects cannot be converted to sharables – they weren't supported in Reanimated 2 either. We add it as a part of this change in case the object prototype check wouldn't be sufficient in some cases. In a scenario when cyclic object is captured by a worklet we will detect that and throw a more descriptive error.

Test plan

The below code illustrates a scenario when we are capturing react internals via ref:

function Mleko({ props }: any) {
  const style = useAnimatedStyle(() => {
    console.log(props)
    return {}
  })
  return (
    <View ref={props.ref} />
  );
}

const ref = useRef();
<Mleko props={{ ref }} />

Before this change the above coud would throw stack exceeded error. Now this works ok.

In addition we tested cyclic detection by capturing the following object:

const one = {};
const two = {one};
one.two = two;

The above object would throw stack exceeded error. now it throws an error that says that user is capturing cyclic object.

Lastly, we tested accessing non plain objects:

class Something() {
  constructor() { this.ggg = 7 }
}
const something = new Something();

useAnimatedStyle(() => {
  console.log(something.ggg);
  return {};
});

src/reanimated2/shareables.ts Outdated Show resolved Hide resolved
src/reanimated2/shareables.ts Outdated Show resolved Hide resolved
src/reanimated2/shareables.ts Outdated Show resolved Hide resolved
src/reanimated2/shareables.ts Outdated Show resolved Hide resolved
@tomekzaw tomekzaw changed the title Fix stack exceeded error Fix "Maximum call stack size exceeded" error Apr 17, 2023
kmagiera and others added 2 commits April 17, 2023 10:26
Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>
const DETECT_CYCLIC_OBJECT_DEPTH_THRESHOLD = 30;
// Below variable stores object that we process in makeShareableCloneRecursive at the specified depth.
// We use it to check if later on the function reenters with the same object
let procesedObjectAtThresholdDepth: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let procesedObjectAtThresholdDepth: any;
let processedObjectAtThresholdDepth: any;

@kmagiera kmagiera added this pull request to the merge queue Apr 17, 2023
Merged via the queue into main with commit 8953028 Apr 17, 2023
@kmagiera kmagiera deleted the fix-stack-exceeded-error branch April 17, 2023 12:49
@quant-daddy
Copy link

I'm getting this error after installing the latest version from Github:

Error: Trying to convert a cyclic object to a shareable. This is not supported., js engine: hermes

simulator_screenshot_F9721F0F-771C-4C3B-AEA2-1113E3881C5C

@kmagiera
Copy link
Member Author

Hey @quant-daddy – would you be able to test your app with this patch: #4372 – it won't fix the issue but should give us a more meaningful stack trace that should narrow down the actual object that's causing this exception

@quant-daddy
Copy link

quant-daddy commented Apr 18, 2023

@kmagiera Okay so it seems to be working after install npm git+https://github.com/computerjazz/react-native-draggable-flatlist.git#pull/462

However I'm getting typescript error if I install react-native-draggable-flatlist directly from github. Would you know how to fix that?

@kmagiera
Copy link
Member Author

Hi @quant-daddy can you maybe share more details as to how you use that draggable-flatlist library? I just tried an example from the readme to run it with the most recent version of reanimated and I'm not getting the crash that you mentioned

@quant-daddy
Copy link

Hi @kmagiera,

import DraggableFlatList, {
  ScaleDecorator,
} from 'react-native-draggable-flatlist';

// render this
<DraggableFlatList
    containerStyle={tw`flex-1 px-4 pt-4`}
    showsVerticalScrollIndicator={false}
    data={images}
    onDragEnd={({ data }) => {
      setImages(data);
    }}
    keyExtractor={a => a.id}
    renderItem={({ item: d, drag, isActive }) => {
      return (
        <ScaleDecorator key={d.id}>
          <TouchableOpacity
            onLongPress={drag}
            disabled={isActive}
            onPress={() => {
              setImageModalId(d.id);
            }}>
            <MyImage
              style={[
                tw`mb-2 rounded-lg overflow-hidden object-cover`,
                {
                  width,
                  height,
                },
              ]}
              source={{
                uri: d?.url!,
              }}
            />
            <View style={tw`absolute left-2 top-2 w-4 h-4`}>
              <MoveIcon />
            </View>
          </TouchableOpacity>
        </ScaleDecorator>
      );
    }}
    ListHeaderComponent={() => {
      return (
        <Pressable
          onPress={() => {
            setShowOptions(true);
          }}
          style={[
            {
              width,
              height: height,
            },
            tw`mb-2 flex items-center justify-center bg-dark-50 rounded-lg`,
          ]}>
          <View style={tw`w-6 h-6`}>
            <CameraIcon />
          </View>
          <MyText type="TinyStrong" style={tw`mt-1 text-dark-700`}>
            Add Photos
          </MyText>
        </Pressable>
      );
    }}
  />

I'm getting a crash with the published draggable-flatlist library but github:computerjazz/react-native-draggable-flatlist#pull/462 seems to fix it. However, this one is giving me a typescript error because the typescript is not compiled when installed directly from Github using npm install git+https:// format.

fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 5, 2023
## Summary

This PR addresses a number of issues reported related to exceeding stack
when converting objects to shareables (e.g. in software-mansion#4177).

This issue got introduced in Reanimated 3 after software-mansion#4060 where we got rid
of filtering only attributes accessed in worklets to be captured by
worklets.

This PR implements a solution that filters only plain objects before
converting things to shareables. In all of the instances of this issue
we encountered, the crash was due to the fact we were leaking some react
internals (fiber nodes) into the captured variables (e.g. via React's
refs or otherwise). These objects are not of a plain object type and
hence we wouldn't be able to support transferring these on the UI
runtime anyways. With this change we are skipping these type of objects
and replace them with a "inaccessible object" which is implemented as a
proxy that throws on any attempt of accessing its attributes – this way
users will get an error if they do use or access such objects.

In addition we add a cyclic object detection mechanism. Currently cyclic
objects cannot be converted to sharables – they weren't supported in
Reanimated 2 either. We add it as a part of this change in case the
object prototype check wouldn't be sufficient in some cases. In a
scenario when cyclic object is captured by a worklet we will detect that
and throw a more descriptive error.


## Test plan

The below code illustrates a scenario when we are capturing react
internals via ref:
```js
function Mleko({ props }: any) {
  const style = useAnimatedStyle(() => {
    console.log(props)
    return {}
  })
  return (
    <View ref={props.ref} />
  );
}

const ref = useRef();
<Mleko props={{ ref }} />
```

Before this change the above coud would throw stack exceeded error. Now
this works ok.

In addition we tested cyclic detection by capturing the following
object:
```js
const one = {};
const two = {one};
one.two = two;
```

The above object would throw stack exceeded error. now it throws an
error that says that user is capturing cyclic object.


Lastly, we tested accessing non plain objects:
```js
class Something() {
  constructor() { this.ggg = 7 }
}
const something = new Something();

useAnimatedStyle(() => {
  console.log(something.ggg);
  return {};
});
```

---------

Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants