Skip to content

Commit

Permalink
[android] Swap std::ostringstream usage for std::to_string (#8542)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
sjchmiela committed May 29, 2020
1 parent a5bec1f commit 0edcd4f
Show file tree
Hide file tree
Showing 15 changed files with 13 additions and 31 deletions.
11 changes: 1 addition & 10 deletions android/ReactCommon/jsi/JSCRuntime.cpp
Expand Up @@ -319,15 +319,6 @@ JSStringRef getIsArrayString() {
#endif
} // namespace

// std::string utility
namespace {
std::string to_string(void* value) {
std::ostringstream ss;
ss << value;
return ss.str();
}
} // namespace

JSCRuntime::JSCRuntime()
: JSCRuntime(JSGlobalContextCreateInGroup(nullptr, nullptr)) {
JSGlobalContextRelease(ctx_);
Expand Down Expand Up @@ -405,7 +396,7 @@ jsi::Object JSCRuntime::global() {

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_;
}
Expand Down
@@ -1,4 +1,4 @@
#NOTE: This is a Maven Resolver internal implementation file, its format can be changed without prior notice.
#Tue Feb 25 19:27:11 CET 2020
#Thu May 28 17:48:15 CEST 2020
reactandroid-abi35_0_0-1.0.0.pom>=
reactandroid-abi35_0_0-1.0.0.aar>=
Binary file not shown.
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
<modelVersion>4.0.0</modelVersion>
<groupId>host.exp</groupId>
Expand Down
Expand Up @@ -7,6 +7,6 @@
<versions>
<version>1.0.0</version>
</versions>
<lastUpdated>20200225182711</lastUpdated>
<lastUpdated>20200528154815</lastUpdated>
</versioning>
</metadata>
@@ -1,4 +1,4 @@
#NOTE: This is a Maven Resolver internal implementation file, its format can be changed without prior notice.
#Wed Feb 26 11:40:22 CET 2020
#Thu May 28 17:06:23 CEST 2020
reactandroid-abi36_0_0-1.0.0.pom>=
reactandroid-abi36_0_0-1.0.0.aar>=
Binary file not shown.
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
<modelVersion>4.0.0</modelVersion>
<groupId>host.exp</groupId>
Expand Down
Expand Up @@ -7,6 +7,6 @@
<versions>
<version>1.0.0</version>
</versions>
<lastUpdated>20200226104022</lastUpdated>
<lastUpdated>20200528150623</lastUpdated>
</versioning>
</metadata>
@@ -1,4 +1,4 @@
#NOTE: This is a Maven Resolver internal implementation file, its format can be changed without prior notice.
#Wed Mar 11 14:39:15 CET 2020
reactandroid-abi37_0_0-1.0.0.aar>=
#Thu May 28 15:04:45 CEST 2020
reactandroid-abi37_0_0-1.0.0.pom>=
reactandroid-abi37_0_0-1.0.0.aar>=
Binary file not shown.
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
<modelVersion>4.0.0</modelVersion>
<groupId>host.exp</groupId>
Expand Down
Expand Up @@ -7,6 +7,6 @@
<versions>
<version>1.0.0</version>
</versions>
<lastUpdated>20200311133915</lastUpdated>
<lastUpdated>20200528130445</lastUpdated>
</versioning>
</metadata>
11 changes: 1 addition & 10 deletions android/versioned-react-native/ReactCommon/jsi/JSCRuntime.cpp
Expand Up @@ -319,15 +319,6 @@ JSStringRef getIsArrayString() {
#endif
} // namespace

// std::string utility
namespace {
std::string to_string(void* value) {
std::ostringstream ss;
ss << value;
return ss.str();
}
} // namespace

JSCRuntime::JSCRuntime()
: JSCRuntime(JSGlobalContextCreateInGroup(nullptr, nullptr)) {
JSGlobalContextRelease(ctx_);
Expand Down Expand Up @@ -405,7 +396,7 @@ jsi::Object JSCRuntime::global() {

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_;
}
Expand Down
2 changes: 1 addition & 1 deletion react-native-lab/react-native

0 comments on commit 0edcd4f

Please sign in to comment.