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

Some skia APIs deadlock on android app when using react-native-v8 #1705

Open
evelant opened this issue Jul 6, 2023 · 13 comments
Open

Some skia APIs deadlock on android app when using react-native-v8 #1705

evelant opened this issue Jul 6, 2023 · 13 comments
Labels
bug Something isn't working needs-reproduction

Comments

@evelant
Copy link

evelant commented Jul 6, 2023

Description

I'm attempting to switch my app from hermes to v8 on android since v8 is 4-5x faster than hermes for me. I must be tripping over some nasty hermes perf corner case. In doing so I discovered that some skia views cause the app to hang when running on v8.

Unfortunately I haven't been able to pinpoint exactly the conditions that create this yet. It seems that in some cases a component containing even an empty can hang the app. A lot of other uses of Canvas work perfectly fine however.

I'm not quite sure how to debug this. It seems likely to me that there's some difference between how hermes and v8 handle JSI libraries that's causing the issue, but I lack the knowledge to attempt to figure out what that could be.

I'm sorry this issue is vague and doesn't have a good reproduction yet. I'm attempting to narrow it down. Until then, can anybody offer some pointers on where I might start looking?

issue filed on react-native-v8: Kudo/react-native-v8#186

Version

0.1.197

Steps to reproduce

  1. Install skia in an app alongside react-native-v8
  2. Observe that some skia views cause the app to hang. It's completely unresponsive, not even an ANR screen.

Snack, code example, screenshot, or link to a repository

Unfortunately I don't have one yet. I'm working on narrowing down the issue.

Environment:
"@shopify/react-native-skia": "0.1.197",
"expo": "~49.0.0",
"react-native": "0.72.1",
"react-native-v8": "2.2.2"
"v8-android-jit": "11.1000.3"

If it's relevant, I do not have reanimated installed.

@evelant evelant added the bug Something isn't working label Jul 6, 2023
@evelant
Copy link
Author

evelant commented Jul 6, 2023

I've uncovered a little bit more info. I got the android studio debugger attached and when the app is hung it always pauses on

//in SkiaDomView.java:41

protected native void registerView(int nativeId);

Not sure if that's useful but at least it's a clue.

@evelant
Copy link
Author

evelant commented Jul 6, 2023

Further probing -- pausing the app native thread reveals it's frozen on the following stack. It appers there's a deadlock at rnv8::V8PointerValue::invalidate()
image

@evelant
Copy link
Author

evelant commented Jul 6, 2023

mqt_js thread looks interesting too, it appears stuck at RNSkia::RNSkJsiViewApi::getEnsuredViewInfo

image

@evelant evelant changed the title Some skia APIs hang an android app when using react-native-v8 Some skia APIs deadlock on android app when using react-native-v8 Jul 6, 2023
@evelant
Copy link
Author

evelant commented Jul 6, 2023

I'm at the end of my ability to debug this further I think. @chrfalch @wcandillon does anything obvious jump out at you as the cause of this deadlock?

@chrfalch
Copy link
Contributor

Hi @evelant! Thanks for the insights about running RN Skia with RN V8. We've not yet done any explicit tests running react-native-v8 - and we don't have any direct requests for this meaning that we'll probl. not be able to prioritise this for now.

I've seen that @Kudo is already giving great feedback on this - we'll wait to see if he can reproduce and see the error before moving on on our side.

@evelant
Copy link
Author

evelant commented Jul 17, 2023

I figured that was the case 🙂 I'll hopefully have a bit of time tomorrow to reproduce this and will work with @Kudo on it. I filed this bug just for your awareness and for the off chance that the problem might immedidately jump out to you. Thanks for all your hard work on this awesome lib!

@evelant
Copy link
Author

evelant commented Jul 17, 2023

I think I found the root cause. Reproduction here: https://github.com/evelant/rnv8skiacrash

tl;dr -- v8 crashes or deadlocks when provided with invalid props such as NaN, hermes and JSC don't.

I don't really know how JSI works internally but I'm guessing this means it's likely a fix needed in react-native-v8, not skia.

@evelant
Copy link
Author

evelant commented Jul 17, 2023

It seems I may have been mistaken -- the issue actually appears to be something deeper involving useFont. My app on v8 seems to crash as soon as it renders skia text using a font loaded with useFont.

@wcandillon
Copy link
Contributor

@evelant This may not be surprising. Did you switch your own loading to load fonts? useFont is just a convenience method, wrapper for

const data = await Skia.Data.fromURI(require(./font.ttf));
const font Skia.Font(Skia.Typeface.MakeFreeTypeFaceFromData(data), size)`;

I'd be curious if this works for you. If it does, we could document it and even look at improving our data loading hooks.

@CyberCyclone
Copy link

@chrfalch I haven't got anything to contribute to the ticket, but I just wanted to share that v8 is used for the project I'm working on because of the performance increase it gives. So while there might not be much vocal support behind v8, I think it would be something really worth while testing against as people who use Skia will have performance in mind and will probably want to run it in v8.

@evelant
Copy link
Author

evelant commented Aug 16, 2023

@wcandillon Sorry to take so long to get back to you, I was on vacation. I'm not quite understanding what you mean -- I already use useFont so if it's just a convenience wrapper for the snippet above how could that change the behavior?

@wcandillon
Copy link
Contributor

@evelant because we have a bug in the useFont possibily

@evelant
Copy link
Author

evelant commented Aug 16, 2023

@wcandillon OK I tried loading the font manually like you specified above. Unfortunately the behavior is the same. My app still deadlocks on v8 in the same place as the stack traces above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-reproduction
Projects
None yet
Development

No branches or pull requests

4 participants