[expo-gl-cpp] fix crash in react-native 0.62 #8352
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Native Component List for this branch is ready |
sjchmiela
approved these changes
May 19, 2020
|
bbarthec
approved these changes
May 19, 2020
tsapeta
approved these changes
May 19, 2020
wkozyra95
force-pushed
the
@wkozyra95/fix-expo-gl-crash-rn-0.62
branch
from
8aa9613
to
b65910f
Compare
sjchmiela
added a commit
that referenced
this pull request
May 29, 2020
# Why Eric reported native crashes when running older experiences on Expo client after RN 0.62 upgrade. # How The first problem I had was reproducing it — after some testing @wkozyra95, @tsapeta and I realized it was only happening on Android 10. [The stacktrace](https://gist.github.com/sjchmiela/5cb3c90bb65847c42cfcbe7a88f8d812) didn't show too much information, but there was the most interesting part: ``` 2020-05-28 11:19:42.552 25672-25672/? A/DEBUG: #7 pc 000b6d6c /data/app/host.exp.exponent-ikw7l6wjtBDnnLdjc-HnfQ==/lib/x86/libfbjni.so (__cxa_throw+108) (BuildId: 3546c1351f83366cb88b3802c4c63ee907e2db98) 2020-05-28 11:19:42.552 25672-25672/? A/DEBUG: #8 pc 0007f919 /data/app/host.exp.exponent-ikw7l6wjtBDnnLdjc-HnfQ==/lib/x86/libc++_shared.so (std::__ndk1::locale::use_facet(std::__ndk1::locale::id&) const+217) (BuildId: 4824c9732515c02a2d10dbd0aff9cfee79138cbe) 2020-05-28 11:19:42.552 25672-25672/? A/DEBUG: #9 pc 00027924 /data/app/host.exp.exponent-ikw7l6wjtBDnnLdjc-HnfQ==/lib/x86/libjscexecutor_abi37_0_0.so (std::__ndk1::basic_ostream<char, std::__ndk1::char_traits<char>>::operator<<(void const*)+148) (BuildId: 6f33b70e6648e4230566cae72b44d88a308d2dd8) 2020-05-28 11:19:42.552 25672-25672/? A/DEBUG: #10 pc 00024557 /data/app/host.exp.exponent-ikw7l6wjtBDnnLdjc-HnfQ==/lib/x86/libjscexecutor_abi37_0_0.so (facebook::jsc::JSCRuntime::description()+407) (BuildId: 6f33b70e6648e4230566cae72b44d88a308d2dd8) ``` It looked like it threw a native exception when… building a description for `JSCRuntime`. The implementation of this method is: ```cpp std::string JSCRuntime::description() { if (desc_.empty()) { desc_ = std::string("<JSCRuntime@") + to_string(this) + ">"; } return desc_; } ``` where `to_string` is defined as ```cpp // std::string utility namespace { std::string to_string(void* value) { std::ostringstream ss; ss << value; return ss.str(); } } // namespace ``` The `use_facet` methods from `c++_shared` aren't very helpful either: ```cpp const locale::facet* locale::__imp::use_facet(long id) const { #ifndef _LIBCPP_NO_EXCEPTIONS if (!has_facet(id)) throw bad_cast(); #endif // _LIBCPP_NO_EXCEPTIONS return facets_[static_cast<size_t>(id)]; } const locale::facet* locale::use_facet(id& x) const { return __locale_->use_facet(x.__get()); } ``` Then, fortunately, I realized I saw a PR removing `std::stream` usage recently — #8352. I decided to follow its example and see whether changing: ```diff std::string JSCRuntime::description() { if (desc_.empty()) { - desc_ = std::string("<JSCRuntime@") + to_string(this) + ">"; + desc_ = std::string("<JSCRuntime@") + std::to_string(reinterpret_cast<std::uintptr_t>(this)) + ">"; } return desc_; } ``` could let the crash go, so I checked out the `sdk-37` branch, applied the change, rebuilt the `host.exp.reactandroid-abi37_0_0`, copied `maven` folder, checked out `master`, pasted `maven` folder, rebuilt Expo client and lo and behold, SDK 37 didn't crash anymore! Opening SDK36 experience didn't crash anymore either! Then I started testing more and I realized Expo client still crashed if the first experience opened after running the app was a not-patched SDK (eg. SDK36). So I went on and applied the fix for all other SDKs. I also tried building projects with newer NDK (RN 0.62 uses r19c, this didn't help either). If I understand the problem correctly, only one version of some _symbol_ is used in Expo client—the one that is loaded first. For some reason the version loaded by unversioned Home doesn't count. Moreover, `<<`-ing to `std::ostringstream` may cause bugs in some circumstances. I understand that this may fix the problem and nobody will experience any crash whatsoever anymore or this may fix only one probable cause of crashes and we only QA and users may figure out whether it's safe now. Thanks to @wkozyra95 for assistance and answers during the whole day! # Test Plan Running Expo client on Android 10 emulator does not cause a crash anymore no matter experience of which SDK is run first.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
std::stringstream is causing crashes on react-native 0.62
closes #7575
closes #7623
How
replace use of
std::stringstreamwithstd::to_stringTest Plan
Run ncl on client with 0.62.2 and 0.61.5 react-native
Run repro from users that crashed before