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.

Sign up for GitHub

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

Jump to bottom

[expo-gl-cpp] fix crash in react-native 0.62 #8352

Merged
merged 2 commits into from
May 20, 2020
Merged

[expo-gl-cpp] fix crash in react-native 0.62 #8352

merged 2 commits into from
May 20, 2020

Conversation

wkozyra95
Copy link
Contributor

@wkozyra95 wkozyra95 commented May 18, 2020 •
edited
Loading

Why

std::stringstream is causing crashes on react-native 0.62
closes #7575
closes #7623

How

replace use of std::stringstream with std::to_string

Test Plan

Run ncl on client with 0.62.2 and 0.61.5 react-native
Run repro from users that crashed before

@wkozyra95 wkozyra95 requested review from nikki93 and sjchmiela May 18, 2020 17:20
@github-actions
Copy link
Contributor

Native Component List for this branch is ready

sjchmiela
sjchmiela approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@sjchmiela sjchmiela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why tho? (this works and the former thing doesn't)
  2. Just to be safe — it is expected that one of the sos got smaller and all other ones got bigger?

@wkozyra95
Copy link
Contributor Author

  1. Why tho? (this works and the former thing doesn't)
  2. Just to be safe — it is expected that one of the sos got smaller and all other ones got bigger?
  1. Bug in std::stringstream
  2. expected: no, plausible: I think so

tsapeta
tsapeta approved these changes May 19, 2020
View reviewed changes
@wkozyra95 wkozyra95 force-pushed the @wkozyra95/fix-expo-gl-crash-rn-0.62 branch from 8aa9613 to b65910f Compare May 19, 2020 19:10
@wkozyra95 wkozyra95 merged commit 7fbde60 into master May 20, 2020
@wkozyra95 wkozyra95 deleted the @wkozyra95/fix-expo-gl-crash-rn-0.62 branch May 20, 2020 13:05
@sjchmiela sjchmiela mentioned this pull request May 28, 2020
Merged
sjchmiela added a commit that referenced this pull request May 29, 2020
@sjchmiela
[android] Swap std::ostringstream usage for std::to_string (#8542)
0edcd4f 
# 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
Reviewers

@sjchmiela sjchmiela sjchmiela approved these changes

@tsapeta tsapeta tsapeta approved these changes

@bbarthec bbarthec bbarthec approved these changes

@nikki93 nikki93 Awaiting requested review from nikki93

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@wkozyra95 @sjchmiela @tsapeta @bbarthec