-
Notifications
You must be signed in to change notification settings - Fork 97
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
Crash in arm x64 #50
Comments
I reproduced it in the measure app. When running the benchmarks over and over again, once in about 20 runs I'm getting:
|
This branch has the custom RN and configured app: |
Confirmed does not reproduce in arm x32 devices. |
Interesting to note: lowering the size of the bundle (by commenting out imports) makes the crash happen more rarely. Greatly lowering the bundle size (like in a clean new RN project) makes the crash onLoad about 1-5% of app starts. |
Another possible factor could be differences between Android 7 and 8. Unfortunately I only have arm32 devices on 7 and arm64 on 8, so no cross validation. IIRC Android introduced different garbage collection algorithms and I wonder if this causes race conditions between Java and Javascript. |
@mrueger42 Correct me if I'm wrong but I don't see how it relates to the Java GC. They don't share address space. 2 things I want to try:
|
@DanielZlotin |
@DanielZlotin But, having said that, there is a lot of low level memory stuff going on in the context of strings. Java GC deciding to throw away something, WebKit code thinking it is still there, voila, null pointer exception. |
Finding a phone with armv7a and android 8 is harder than I thought 😆 |
Hi, I've built JSC with debug symbols, and that's what I've got (Android 8.0):
|
After some digging, it looks like some kind of race condition: some iterators get invalidated while they're being used. Looking further, we have ENABLE_DFG_JIT=0, which (according to Platform.h) makes ENABLE_CONCURRENT_JS=0, which in turn makes everything use dymmy NoLock implementation for locking. We still have no idea why JSC without DFG would require locking, but after patching Platform.h to set ENABLE_CONCURRENT_JS=1 for all 64bit builds apparently fixes the crash. We also are not sure why 32bit build is fine and 64bit has this crash, however there are some JIT-specific files that have significant amount of code depend on architecture. I'll submit a pull request after I do some more tests. So far after ~100 runs no crashes. |
After adding debug symbols to release build, I get this stacktrace for the crash:
|
Great work @pmlocek thank you. |
Another update: I've disabled concurrent GC and it seems to be stable again. I'll also try disabling concurrent JIT, as it might have less impact than disabling concurrent GC. |
@DanielZlotin Note that the options we now are trying to disable (concurrentGC and concurrentJIT) are OFF on 32bit builds and only configured to be ON by default on 64bit platforms. |
OK so after playing around with the source I discovered a few interesting things:
|
What is the impact of Concurrent GC on performance? |
Not related to the crash (yet) but I found that the scripts should pass the compiler as an ENV argument in order for the webkit scripts to accept the build as a cross compile build, and also the |
Using the above approach greatly lessens the crash rate but it still happens. I'll add symbols to see if it's the same crash. |
Adding debug symbols ( |
Today we got following crash on a build with no concurrent GC:
|
Another possible lead: we've noticed a lot of the code around the place where the crash happens is inlined, so it's difficult to debug. So we've decided to disable inlining in Clang, to make debugging easier. So far, the build with no inlining didn't crash after running 3200 tests. |
@pmlocek wow that's an amazing catch! What is controlling the inlining? I want to check it out myself and see if we can live with the perf/size hit |
@DanielZlotin |
@pmlocek did you find an easy way to reproduce the crash? I'm not able to reproduce it in the measure app no matter what I do now, just in the actual Wix app :\ |
We're running measure app without 'Profile Javascript' and it usually crashes within 200 runs. |
Damn was hoping you found something quicker 😕 |
Great success! (well.. a small step) |
Thanks, that'll be a great help. That manages to produce the crash every time the app bundle loads? |
About 50% of the app launches |
Update: managed to build with symbols and got this stacktrace:
Doesn't look like the same stacktrace you guys got.. I'll try with |
Hi @DanielZlotin , is this crash specific to RN 0.51? We were hoping to use the new JSC in our project but we currently use 0.51. |
Hi, I was trying to debug this issue and so far I found two ways that are partially fixing the problem.
I ran benchmarks for few days about 10k app restarts and it crashed once.
For now, I tested the new version with When I benchmark build I usually add The direct reason for a crash is gc issue, for a crash with stacktrace ending with Internally in gc there are MarkedAllocator objects that have MarkedBlocks that are essentially some memory fragments, every allocator has few bit vectors that represents a state of those blocks. When a crash occurs one allocator is "stealing" block from another one. When I log operations on that block(before stealing) I see when a block was allocated, I see where block is set not empty and later while it's stolen by another allocator it's already marked as empty but setter for that block was never called. It's possible that I missed something there and the problem is deeper. I suspect that there is some race condition and fixes like disable inlining are masking this race by slowing down certain part fo code, also there are some cases where locking is optional depending on build and platform that may explain way arm64 is the only platform where this occurs. |
@rajivshah3 happens on newer RN as well. It's not related to RN at all it seems, something with JSC+arm64. If you use the 32 version you should be fine. @wkozyra95 Thanks for that! Passing the correct arch looks to be unrelated to the crash directly, but fixes other build issues according to #55 . Looking forward to hearing if still crashes in 2.22.2 FYI I'm no longer actively maintaining this project but will help as best I can |
After over 20k restarts version 2.22.2 didn't crash, so I'm assuming that the issue is fixed there. I prepared PR with required changes. |
🎉 |
@DanielZlotin This crash produced in version 225067.0.0! I upgrade version to 236355.1.1, still crash!
|
hey @luoyushouchai ! Would you be able to provide repro steps for the crash you posted? I'd also appreciate if you decided to open a new issue for your crash as based on the stacktrace you posted it appears like it happens on ARMv7 and not ARM64 (which this issue was all about) |
Hi @wkozyra95 I'm having all sorts of unpredictable and intermittent crashes trying to build a release version of my Android RN project. I usually don't get any stack trace at all but the last time it gave me the following. Is it related to the problem you're discussing here? I'm afraid the chat is way over my head but it seemed like the fix was "changing arm64 to aarch64 in DCMAKE_SYSTEM_PROCESSOR=$ARCH_NAME". Where exactly do I set that? Thanks
|
If you are using the latest jsc it's probably a different bug. This crash happened maybe once per hundred app lunches (only on start), fault address usually had a lot of zeros (garbage collector issue allocated two objects in the same place in memory and code accessing those fragments was potentially reading random stuff). Simplest fix we found was to disable code inlining (it was to large perfomance drop to use that solution), you can try that. |
Running on a custom compiled react-native based on 0.51.0 version for x64 Android.
Once the bundle is loaded about 50% of times crashes with:
I can't seem to reproduce it in our measure app, just in the Wix app (which is very big)
The text was updated successfully, but these errors were encountered: