[android][release] makeShareableCloneRecursive Call Stack Size Exceeded

[thespacemanatee]

Description

I'll start by saying that I've spent many days on this and already looked at #4140, #4177, #4367 and #4475, and this is a separate issue. I also created an issue at expo/expo#23297 but I don't think it's expo's fault because I could not reproduce this on their bare template. It's worth noting that this does not occur on react-native@0.71 or expo@48.

The bug, for me, only occurs on the Android release build or the debug build with the JS Dev Mode disabled.

This gave me the idea to search for all __DEV__ flags within the installed version in my node_modules and flip all of them so that I can see the error in dev mode and get a nicer stacktrace. Here's what I got:

This narrows down the problem quite well, and I found that the following patch solves my issue:

diff --git a/node_modules/react-native-reanimated/src/reanimated2/shareables.ts b/node_modules/react-native-reanimated/src/reanimated2/shareables.ts
index 4f358f4..8be3e2b 100644
--- a/node_modules/react-native-reanimated/src/reanimated2/shareables.ts
+++ b/node_modules/react-native-reanimated/src/reanimated2/shareables.ts
@@ -1,6 +1,6 @@
 import NativeReanimatedModule from './NativeReanimated';
-import { ShareableRef } from './commonTypes';
 import { shouldBeUseWeb } from './PlatformChecker';
+import { ShareableRef } from './commonTypes';
 import { registerWorkletStackDetails } from './errors';
 import { jsVersion } from './platform-specific/jsVersion';
 
@@ -146,8 +146,9 @@ export function makeShareableCloneRecursive<T>(
               value.__workletHash,
               value.__stackDetails
             );
-            delete value.__stackDetails;
           }
+          // TODO: This is the fixes Maximum call stack size exceeded error
+          delete value.__stackDetails;
           // to save on transferring static __initData field of worklet structure
           // we request shareable value to persist its UI counterpart. This means
           // that the __initData field that contains long strings represeting the

Obviously, this is not ideal and doesn't seem like the official solution (why would stackDetails have anything to do with the fix?), so I'm hoping the team will have a better idea. Thanks for the great work!

Steps to reproduce

I could not reproduce this on the bare expo template, but I'm linking it anyway. I hope the patch I provided above will give some clue as to why it only happens on some projects.

1. Install react-native-reanimated@3.3.0
2. npx pod-install
3. Importing the library using import 'react-native-reanimated' in App.tsx is enough to trigger the bug

Snack or a link to a repository

https://github.com/thespacemanatee/expo-dev-menu-repro/tree/repro/reanimated-makeShareableCloneRecursive-crash

Reanimated version

3.3.0

React Native version

0.72.1

Platforms

Android

JavaScript runtime

Hermes

Workflow

Expo bare workflow

Architecture

Paper (Old Architecture)

Build type

Release mode

Device

Android emulator

Device model

Pixel 7 Pro (Android 13) and Pixel 3a (Android 10)

Acknowledgements

Yes

[thespacemanatee]

@tomekzaw this is still an issue for my team. Could we get some advisory on whether the workaround above is viable, and why it might be happening?

[tomekzaw]

Hey @thespacemanatee, thanks for reporting this issue, investigating the root cause as well as providing the workaround.

Could we get some advisory on whether the workaround above is viable, and why it might be happening?

Sure, we will investigate this issue once we finish investigating other issues that we're looking into right now.

[thespacemanatee]

Just documenting here that something about the react native setup on Android isn't idempotent, the issue has been spuriously occuring in the past week since updating to Expo 49 and RN 0.72. One thing that helped was clearing .gradle folder and rebuilding the entire project, although I'm not sure if it will crop up again.

[devoren]

same issue +1, I hope it will be resolved soon

[a-eid]

I'm getting a similar issue on android in release mode, in development everything works fine.

[hirbod]

This issue is also blocking our release right now. The crash only occurs in release mode (--variant Release) and works fine in dev mode.

[tomekzaw]

Hey @hirbod, thanks for reporting this. We suspect that the root cause of the issue is using dev bundle with release build.

Can you please confirm that worklets in your release bundle don't contain __version property?

In the meantime, @piaskowyk is working on a PR that will throw a more descriptive error in such case.

[hirbod]

@tomekzaw can you give me some directions how/where I should look for __version ?

[tomekzaw]

@hirbod Sure! Are you able to extract the JS bundle file from your app? If not, I'll think of some different way on how to check this.

[hirbod]

@tomekzaw, the only thing that might support your guess is that I can see the Expo CLI outputs env: load .env.development, even though I did pass the --variant Release parameter. There are no errors in SDK 48. Maybe something happened between these versions.

I need to do some quick research on how to dump the release bundle, never did that before.

[hirbod]

@tomekzaw, I cd'd into android (after prebuild), ran STAGE=production ./gradlew clean (we're using process environment variables; that's what the STAGE is for), and then ran STAGE=production npx expo run:android --variant Release again. The app is now running.

So it appears your guess was correct. Could it be some kind of strange caching issue?

[tomekzaw]

@hirbod You can also add something like if (!isRelease()) { throw new Error('expecting release mode'); } somewhere in the Reanimated Babel plugin (don't forget to run cd plugin && yarn generate to compile TS to JS).

[tomekzaw]

So it appears your guess was correct. Could it be some kind of strange caching issue?

Yep, might be. That's why we'll add an additional runtime check on Reanimated side.

[hirbod]

Maybe the recent changes to Metro and Expos ability to load .env files are the reason here. Seems like --variant Release is still loading .env.development instead of .env.production.

So this means, between every local change from development -> production, we need to run ./gradlew clean or it will break.

[thespacemanatee]

@tomekzaw the issue occurs again now and then and doesn't seem to have a deterministic solution, @hirbod could you confirm if it works every time and on different machines?

I also think it's weird that this issue only happens to Android. If it is what the team says it is, that the Android build cache needs to be cleared, then why does the solution in #4737 suggest fixes in the JS-land (solution 2)? And why does this affect Android only and not iOS as well? More investigations need to be done to find the root cause.

[hirbod]

@thespacemanatee I did not test on a different machine but it was working for my mate and on EAS. I also had the suspicion that --variant Release vs --variant release (lowercase) might have been the root cause to the issue. After using EXPO_NO_DOTENV=1 with lowercase "release" I did not run into this issue anymore so far.

Something is not cleared up correctly while switching between dev and release on the same machine.

[thespacemanatee]

@hirbod the variant casing should depend on the build flavours that you defined in app/build.gradle. Mine is productionRelease. I can also confirm (for now) that the builds on EAS work, but not local builds, even after deleting and cloning the repository again. Maybe it's something on expo's side.

[hirbod]

I haven't defined any build flavours, and have been using the defaults provided by Expo. I figured out that using an uppercase 'R' was causing some issues for me. For example, it generated a production app, but loaded a development bundle. I'm not certain if this was merely a coincidence or not, but something seems off since the introduction of SDK 49 (and it's ability to read our local DOTENV / NODE_ENV). Maybe we need to be more verbose now passing --no-dev --minify as well.

[tomekzaw]

@hirbod @thespacemanatee I think I found something. This is how we detect dev/release bundle in Reanimated Babel plugin:

react-native-reanimated/plugin/src/utils.ts

Lines 1 to 6 in 5d4e02e

	export function isRelease() {
	return (
	process.env.BABEL_ENV &&
	['production', 'release'].includes(process.env.BABEL_ENV)
	);
	}

If you pass Release as BABEL_ENV it will probably still generate dev bundle.

[hirbod]

@tomekzaw this matches exactly my issue. So the uppercase R was the main problem for me. I retried couple of times and could reproduce this reliable.

[thespacemanatee]

@tomekzaw @hirbod If my variant is productionRelease, how would I go about this? The funny thing is that I tried npx expo run:android productionrelease and it works, and thereafter npx expo run:android productionRelease also works as well, until it stops working randomly. I think it's related to expo but I'm not sure how to phrase it as an issue.

[hirbod]

@thespacemanatee I've never seen your syntax. I would assume that both of your commands would generate a dev bundle.

I might be wrong, but I am only aware of

npx expo run:android --variant release and npx expo run:ios --configuration Release (with uppercase R)
Regarding to Evan Bacon, only these two inputs are officially supported (and based on that, NODE_ENV and other environment variables are set). Using a different "keyword" would most likely break with the Reanimated Babel plugin and Expo fails to set NODE_ENV for you.

[thespacemanatee]

@hirbod please read about multiple build variants here: https://docs.expo.dev/build-reference/variants/

The syntax I provided is correct. Internally it transformed the variant to assembleProductionRelease etc.

I might also add that this has been working since SDK 45 and probably before that as well. Are you sure it's not 'officially' supported? Any source?

[hirbod]

I don't see any indication that your syntax is correct. Only EAS commands should include --profile. I'm not saying you're incorrect, but this is new to me and I can't recall any documents using your syntax.

Here is a link for reference: https://docs.expo.dev/build-reference/troubleshooting/#verify-that-your-project-builds-and-runs-locally

The only "official" guidance I have is a direct message from @EvanBacon, who figured I passed an uppercase 'R' for android (instead iOS). Perhaps he can provide further clarification. For me, it fixed the issues.

[thespacemanatee]

@hirbod I've seen that documentation. If you try creating multiple build variants you'll realise that the npx expo run:android --variant release command no longer works because there's no such Gradle task installRelease, which is expected because it should be installProductionRelease. We figured by trial and error that we have to pass in --variant productionRelease, and I've also seen many projects using such syntax. If NODE_ENV is somehow being set by this then it should be considered a bug.

[hirbod]

PS: the reason why I said your syntax looks wrong to me is because you didn't include --variant.

[thespacemanatee]

@hirbod sorry I was typing on my phone. This is very helpful, thanks. There isn't really any documentation about configuring NODE_ENV so I guess we'll have to pass that env variable with every build command?

[hirbod]

Yes you can try NODE_ENV=production npx expo run:android....

[thespacemanatee]

@hirbod Just tried it and the issue still occurs 🤔 @tomekzaw I patched plugin.js with #4747 but the issue still occurs. Even just returning true doesn't work, what gives?