-
|
Hello RN community, I work on a complex RN app and we migrated around 50 NativeModules to TurboModules(platform and cxx mixed) during our new architecture migration effort. Once we released our app to market with new architecture after many layers of QA validations, we encountered very high crash rate for iOS/tvOS in production and the crashes were untraceable. Attaching some crashes and some random crashes in HermesEngine such as (just posting individual crash lines)
Obviously, we stumbled upon this very difficult issue while our customers were facing higher crashes. Apparently, we are not the only ones and found several tickets people reported(posting a few)
This is a quite concern for people who seeks to migrate to NewArchitecture. Eventually, we were able to mitigate the issue and it was coming at this line . Basically, On our side, we had to patch RCTTurboModule.mm by passing a custom crash handler and disabling the line that throws the JSError in order to fix this reporting issue. I was hesitant to create a PR because the changes we did was too specific for our needs. Therefore, I wanted to create a ticket and discuss how we can resolve this issue as I am sure many others would suffer for this issue. Thus, please let me know your thoughts on this annoying issue. |
Beta Was this translation helpful? Give feedback.
Replies: 3 comments 15 replies
-
|
Hi @SemihGk, thanks for opening the issue. Can you post the full crash report? This is indeed a problem and we want to solve it. If you patched that line, it should be interesting to gather some more data. That lines has all the data we need:
Alternatively, you can modify the To clarify, patching that line is curing the symptom, that's not the cause of the crash. Answering the above questions might help us narrowing down where the actual problem lies. |
Beta Was this translation helpful? Give feedback.
-
|
Thank you for your reply @cipolleschi . I am afraid I might not be allowed to share our crash files publicly, but will get back to you. Alternatively, is it possible that I can send the crash files privately if there is any email address I can use? Just to clarify, I only pinpoint the reporting issue that On our side, we did something similar to this . Basically, we override the exception handler of RCTTurboModule and passed On the other hand, With that being said, it is fairly easy to replicate this issue on our side. We throw a random NSException within the try block and observe the crash in In this case, do you consider a similar approach of what we did? If we can override the exception handler of RCTTurboModule, we can control this behaviour. Maybe we might utilize RCTFatalHandler. However, people would still face the issue on their side if they would not be aware of this override behaviour. Ideally, |
Beta Was this translation helpful? Give feedback.
-
|
Apologies for the inconvenience @cipolleschi . Unfortunately, I am still waiting confirmation to share the crash logs publicly. I will share once I receive the confirmation, but there is no crash message or any meaningful info. All those On the other hand, we rolled out our release with the patch to 5% of our customer base so far and all those As a next step, I see 2 options we have:
|
Beta Was this translation helpful? Give feedback.
-
|
Of course we would like to fix the issue. For you and for all our other customers. We are aware of some concurrenc issue in turbomodules and we are issuing some fixes for them. Do you have a sense on whether those crashes happens at startup or if they happens later in the user sessions? |
Beta Was this translation helpful? Give feedback.
-
|
Once again, those crashes occur whenever an NSException is thrown in an ObjTurboModule. As I stated earlier, it is very easy to replicate on our side. If it would be helpful, I can try to create a sample project for you? However, I am not 100% sure if this issue may occur on a small project. |
Beta Was this translation helpful? Give feedback.
-
|
Yes, if you can, that would be great. You can start from this template. |
Beta Was this translation helpful? Give feedback.
-
|
I will try to replicate from the template and put something on the weekend. Thank you. |
Beta Was this translation helpful? Give feedback.
-
|
@SemihGk, thank you for filing the report! I think there's a mistake in this pr: facebook/react-native#36925 This code... @try {
[inv invokeWithTarget:strongModule];
} @catch (NSException *exception) {
throw convertNSExceptionToJSError(runtime, exception);
} @finally {
[retainedObjectsForInvocation removeAllObjects];
}Should look maybe look like this... if (isSync) {
@try {
[inv invokeWithTarget:strongModule];
} @catch (NSException *exception) {
throw convertNSExceptionToJSError(runtime, exception);
} @finally {
[retainedObjectsForInvocation removeAllObjects];
}
} else {
@try {
[inv invokeWithTarget:strongModule];
} @finally {
[retainedObjectsForInvocation removeAllObjects];
}
}If the native module method is async, it doesn't make sense to convert the objc exception to a js error, and re-throw it. Nothing in the stack will be capable of understanding that js error. What happens if you patch your react native fork with my suggestion? |
Beta Was this translation helpful? Give feedback.
-
|
Can I ask you to quickly try with this variant? It should be sematically equivalent. @try {
[inv invokeWithTarget:strongModule];
} @catch (NSException *exception) {
if (isSync) {
throw convertNSExceptionToJSError(runtime, exception);
} else {
@thorw exception;
}
} @finally {
[retainedObjectsForInvocation removeAllObjects];
} |
Beta Was this translation helpful? Give feedback.
-
|
Yes, it seems same, but will give a try and inform you. Also, yes will fix that minor throw typo |
Beta Was this translation helpful? Give feedback.
-
|
yes it works fine for both sync and async function. I have a minor suggestion: when JS error is captured for sync function, I do not necessarily get the stack trace. This is unrelated issue, but if we could add moduleName and methodName to the errorMessage. That would be super helpful at this line |
Beta Was this translation helpful? Give feedback.
-
|
@SemihGk this is the PR: facebook/react-native#50193 |
Beta Was this translation helpful? Give feedback.
-
|
Thank you very much for putting PR this quickly. I really appreciate your time on this. |
Beta Was this translation helpful? Give feedback.
@SemihGk, thank you for filing the report!
I think there's a mistake in this pr: facebook/react-native#36925
This code...
Should look maybe look like this...