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

Fix Android native debug build assert crash #25263

Closed
wants to merge 1 commit into from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Jun 14, 2019

Summary

The problem happens only from native debug build, i.e.
NATIVE_BUILD_TYPE=Debug ./gradlew ...
During Java exception happens, only limited methods are allowed.
RN uses smart pointer to manage JNI reference.
There is an assert() in smart pointer constructor that will call other JNI method and lead to VM crash.
Since assert() do things in debug build by default, the problem will not happens on native release build.

Crash backtrace

Java exception happens:

java_vm_ext.cc:534] JNI DETECTED ERROR IN APPLICATION: JNI GetObjectRefType called with pending exception com.facebook.react.uimanager.IllegalViewOperationException: No ViewManager defined for class Text
java_vm_ext.cc:534]   at com.facebook.react.uimanager.ViewManager com.facebook.react.uimanager.ViewManagerRegistry.get(java.lang.String) (ViewManagerRegistry.java:57)
java_vm_ext.cc:534]   at com.facebook.react.uimanager.ViewManager com.facebook.react.uimanager.UIImplementation.resolveViewManager(java.lang.String) (UIImplementation.java:134)
java_vm_ext.cc:534]   at com.facebook.react.bridge.WritableMap com.facebook.react.uimanager.UIManagerModule.computeConstantsForViewManager(java.lang.String) (UIManagerModule.java:325)
java_vm_ext.cc:534]   at com.facebook.react.bridge.WritableMap com.facebook.react.uimanager.UIManagerModule.getConstantsForViewManager(java.lang.String) (UIManagerModule.java:319)
java_vm_ext.cc:534]   at void com.facebook.react.bridge.queue.NativeRunnable.run() (NativeRunnable.java:-2)
java_vm_ext.cc:534]   at void android.os.Handler.handleCallback(android.os.Message) (Handler.java:790)
java_vm_ext.cc:534]   at void android.os.Handler.dispatchMessage(android.os.Message) (Handler.java:99)
java_vm_ext.cc:534]   at void com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(android.os.Message) (MessageQueueThreadHandler.java:29)
java_vm_ext.cc:534]   at void android.os.Looper.loop() (Looper.java:164)
java_vm_ext.cc:534]   at void com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run() (MessageQueueThreadImpl.java:232)
java_vm_ext.cc:534]   at void java.lang.Thread.run() (Thread.java:764)

Native crash backtrace:

#00 pc 00000000000276f8  /system/lib64/libc.so (syscall+24)
#01 pc 0000000000027905  /system/lib64/libc.so (abort+101)
#02 pc 000000000052f32e  /system/lib64/libart.so (art::Runtime::Abort(char const*)+558)
#03 pc 00000000006341a8  /system/lib64/libart.so (android::base::LogMessage::~LogMessage()+1016)
#04 pc 0000000000399339  /system/lib64/libart.so (art::JavaVMExt::JniAbort(char const*, char const*)+1657)
#05 pc 00000000003994c2  /system/lib64/libart.so (art::JavaVMExt::JniAbortV(char const*, char const*, __va_list_tag*)+82)
#06 pc 0000000000178aab  /system/lib64/libart.so (art::ScopedCheck::AbortF(char const*, ...)+187)
#07 pc 00000000001785b8  /system/lib64/libart.so (art::ScopedCheck::CheckThread(_JNIEnv*)+472)
#08 pc 0000000000176871  /system/lib64/libart.so (art::ScopedCheck::Check(art::ScopedObjectAccess&, bool, char const*, art::JniValueType*)+801)
#09 pc 000000000017614a  /system/lib64/libart.so (art::CheckJNI::GetObjectRefType(_JNIEnv*, _jobject*)+778)
#10 pc 0000000000123831  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (_JNIEnv::GetObjectRefType(_jobject*)+49)
#11 pc 00000000001236b1  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (facebook::jni::LocalReferenceAllocator::verifyReference(_jobject*) const+81)
#12 pc 0000000000046f40  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libfb.so (facebook::jni::base_owned_ref<_jthrowable*, facebook::jni::LocalReferenceAllocator>::base_owned_ref(_jthrowable*)+64)
#13 pc 0000000000046ef7  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libfb.so (facebook::jni::basic_strong_ref<_jthrowable*, facebook::jni::LocalReferenceAllocator>::basic_strong_ref(_jthrowable*)+39)
#14 pc 000000000003648b  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libfb.so (_ZN8facebook3jni11adopt_localIP11_jthrowableEENS0_16basic_strong_refIT_NS0_23LocalReferenceAllocatorEEES5_+27)
#15 pc 0000000000036333  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libfb.so (facebook::jni::throwPendingJniExceptionAsCppException()+99)
#16 pc 000000000018652f  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (facebook::react::MethodInvoker::invoke(std::__ndk1::weak_ptr<facebook::react::Instance>&, facebook::jni::alias_ref<facebook::jni::detail::JTypeFor<facebook::react::JBaseJavaModule, facebook::jni::JObject, void>::_javaobject*>, folly::dynamic const&)+4559)
#17 pc 00000000001616fa  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (_ZN8facebook5react16JavaNativeModule26callSerializableNativeHookEjON5folly7dynamicE+954)
#18 pc 000000000021bbd6  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (_ZN8facebook5react14ModuleRegistry26callSerializableNativeHookEjjON5folly7dynamicE+486)
#19 pc 000000000022d90b  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (_ZN8facebook5react16JsToNativeBridge26callSerializableNativeHookERNS0_10JSExecutorEjjON5folly7dynamicE+75)
#20 pc 000000000004a6fa  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libjscexecutor.so (facebook::react::JSIExecutor::nativeCallSyncHook(facebook::jsi::Value const*, unsigned long)+202)
#21 pc 000000000004b7a2  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libjscexecutor.so
#22 pc 0000000000056475  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libjscexecutor.so
#23 pc 00000000000c3a0a  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libjsc.so

Java exception code from ViewManagerRegistry.java :

  public ViewManager get(String className) {
    ViewManager viewManager = mViewManagers.get(className);
    if (viewManager != null) {
      return viewManager;
    }
    if (mViewManagerResolver != null) {
      viewManager = mViewManagerResolver.getViewManager(className);
      if (viewManager != null) {
        mViewManagers.put(className, viewManager);
        return viewManager;
      }
    }
    throw new IllegalViewOperationException("No ViewManager defined for class " + className);
  }

It is an expected exception as legacy JS try to find view manager and Text has no ViewManager registered in fact.

From C++ assert:
adopt_local will call to base_owned_ref() and there is an assert to ensure JNI reference type.

template<typename T, typename Alloc>
inline facebook::jni::base_owned_ref<T, Alloc>::base_owned_ref(
    javaobject reference) noexcept
  : storage_(reference) {
  assert(Alloc{}.verifyReference(reference));
  internal::dbglog("New wrapped ref=%p this=%p", get(), this);
}

In the verifyReference(), there is a GetObjectRefType() call which is an invalid call at exception pending state.

inline bool LocalReferenceAllocator::verifyReference(jobject reference) const noexcept {
  if (!reference || !internal::doesGetObjectRefTypeWork()) {
    return true;
  }
  return internal::getEnv()->GetObjectRefType(reference) == JNILocalRefType;
}

The fix moves adopt_local() wrapping after ExceptionClear() and we could call GetObjectRefType() without problems.

Changelog

[Android] [Fixed] - Fix Android native debug build assert crash
NOTE this might be ignored in RN release changelog.
Since the problem should exist for years and no user tried native debug build.

Test Plan

Try to run RNTester from native debug build and make sure no crash happens.
(It is a launch crash and should be easy to verify)
NATIVE_BUILD_TYPE=Debug ./gradlew clean :RNTester:android:app:installDebug

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Jun 14, 2019
@react-native-bot react-native-bot added Platform: Android Android applications. Bug labels Jun 14, 2019
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Kudo in ce80a67.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jun 18, 2019
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
The problem happens only from native debug build, i.e.
`NATIVE_BUILD_TYPE=Debug ./gradlew ...`
During Java exception happens, [only limited methods are allowed](https://developer.android.com/training/articles/perf-jni#exceptions_1).
RN uses smart pointer to manage JNI reference.
There is an assert() in smart pointer constructor that will call other JNI method and lead to VM crash.
Since assert() do things in debug build by default, the problem will not happens on native release build.

### Crash backtrace
Java exception happens:

```
java_vm_ext.cc:534] JNI DETECTED ERROR IN APPLICATION: JNI GetObjectRefType called with pending exception com.facebook.react.uimanager.IllegalViewOperationException: No ViewManager defined for class Text
java_vm_ext.cc:534]   at com.facebook.react.uimanager.ViewManager com.facebook.react.uimanager.ViewManagerRegistry.get(java.lang.String) (ViewManagerRegistry.java:57)
java_vm_ext.cc:534]   at com.facebook.react.uimanager.ViewManager com.facebook.react.uimanager.UIImplementation.resolveViewManager(java.lang.String) (UIImplementation.java:134)
java_vm_ext.cc:534]   at com.facebook.react.bridge.WritableMap com.facebook.react.uimanager.UIManagerModule.computeConstantsForViewManager(java.lang.String) (UIManagerModule.java:325)
java_vm_ext.cc:534]   at com.facebook.react.bridge.WritableMap com.facebook.react.uimanager.UIManagerModule.getConstantsForViewManager(java.lang.String) (UIManagerModule.java:319)
java_vm_ext.cc:534]   at void com.facebook.react.bridge.queue.NativeRunnable.run() (NativeRunnable.java:-2)
java_vm_ext.cc:534]   at void android.os.Handler.handleCallback(android.os.Message) (Handler.java:790)
java_vm_ext.cc:534]   at void android.os.Handler.dispatchMessage(android.os.Message) (Handler.java:99)
java_vm_ext.cc:534]   at void com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(android.os.Message) (MessageQueueThreadHandler.java:29)
java_vm_ext.cc:534]   at void android.os.Looper.loop() (Looper.java:164)
java_vm_ext.cc:534]   at void com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run() (MessageQueueThreadImpl.java:232)
java_vm_ext.cc:534]   at void java.lang.Thread.run() (Thread.java:764)
```

Native crash backtrace:

```
#00 pc 00000000000276f8  /system/lib64/libc.so (syscall+24)
facebook#1 pc 0000000000027905  /system/lib64/libc.so (abort+101)
facebook#2 pc 000000000052f32e  /system/lib64/libart.so (art::Runtime::Abort(char const*)+558)
facebook#3 pc 00000000006341a8  /system/lib64/libart.so (android::base::LogMessage::~LogMessage()+1016)
facebook#4 pc 0000000000399339  /system/lib64/libart.so (art::JavaVMExt::JniAbort(char const*, char const*)+1657)
facebook#5 pc 00000000003994c2  /system/lib64/libart.so (art::JavaVMExt::JniAbortV(char const*, char const*, __va_list_tag*)+82)
facebook#6 pc 0000000000178aab  /system/lib64/libart.so (art::ScopedCheck::AbortF(char const*, ...)+187)
facebook#7 pc 00000000001785b8  /system/lib64/libart.so (art::ScopedCheck::CheckThread(_JNIEnv*)+472)
facebook#8 pc 0000000000176871  /system/lib64/libart.so (art::ScopedCheck::Check(art::ScopedObjectAccess&, bool, char const*, art::JniValueType*)+801)
facebook#9 pc 000000000017614a  /system/lib64/libart.so (art::CheckJNI::GetObjectRefType(_JNIEnv*, _jobject*)+778)
facebook#10 pc 0000000000123831  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (_JNIEnv::GetObjectRefType(_jobject*)+49)
facebook#11 pc 00000000001236b1  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (facebook::jni::LocalReferenceAllocator::verifyReference(_jobject*) const+81)
facebook#12 pc 0000000000046f40  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libfb.so (facebook::jni::base_owned_ref<_jthrowable*, facebook::jni::LocalReferenceAllocator>::base_owned_ref(_jthrowable*)+64)
facebook#13 pc 0000000000046ef7  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libfb.so (facebook::jni::basic_strong_ref<_jthrowable*, facebook::jni::LocalReferenceAllocator>::basic_strong_ref(_jthrowable*)+39)
facebook#14 pc 000000000003648b  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libfb.so (_ZN8facebook3jni11adopt_localIP11_jthrowableEENS0_16basic_strong_refIT_NS0_23LocalReferenceAllocatorEEES5_+27)
facebook#15 pc 0000000000036333  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libfb.so (facebook::jni::throwPendingJniExceptionAsCppException()+99)
facebook#16 pc 000000000018652f  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (facebook::react::MethodInvoker::invoke(std::__ndk1::weak_ptr<facebook::react::Instance>&, facebook::jni::alias_ref<facebook::jni::detail::JTypeFor<facebook::react::JBaseJavaModule, facebook::jni::JObject, void>::_javaobject*>, folly::dynamic const&)+4559)
facebook#17 pc 00000000001616fa  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (_ZN8facebook5react16JavaNativeModule26callSerializableNativeHookEjON5folly7dynamicE+954)
facebook#18 pc 000000000021bbd6  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (_ZN8facebook5react14ModuleRegistry26callSerializableNativeHookEjjON5folly7dynamicE+486)
facebook#19 pc 000000000022d90b  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libreactnativejni.so (_ZN8facebook5react16JsToNativeBridge26callSerializableNativeHookERNS0_10JSExecutorEjjON5folly7dynamicE+75)
facebook#20 pc 000000000004a6fa  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libjscexecutor.so (facebook::react::JSIExecutor::nativeCallSyncHook(facebook::jsi::Value const*, unsigned long)+202)
facebook#21 pc 000000000004b7a2  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libjscexecutor.so
facebook#22 pc 0000000000056475  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libjscexecutor.so
facebook#23 pc 00000000000c3a0a  /data/app/com.facebook.react.uiapp-NEO_tjqQpFriZwGleZxJeQ==/lib/x86_64/libjsc.so
```

Java exception code from [ViewManagerRegistry.java](https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManagerRegistry.java#L45-L58) :

```java
  public ViewManager get(String className) {
    ViewManager viewManager = mViewManagers.get(className);
    if (viewManager != null) {
      return viewManager;
    }
    if (mViewManagerResolver != null) {
      viewManager = mViewManagerResolver.getViewManager(className);
      if (viewManager != null) {
        mViewManagers.put(className, viewManager);
        return viewManager;
      }
    }
    throw new IllegalViewOperationException("No ViewManager defined for class " + className);
  }
```
It is an expected exception as [legacy JS try to find view manager](https://github.com/facebook/react-native/blob/master/Libraries/ReactNative/PaperUIManager.js#L44-L50) and Text has no ViewManager registered in fact.

From C++ assert:
`adopt_local`  will call to base_owned_ref() and [there is an assert to ensure JNI reference type](https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/jni/first-party/fb/include/fb/fbjni/References-inl.h#L203-L209).

```C++
template<typename T, typename Alloc>
inline facebook::jni::base_owned_ref<T, Alloc>::base_owned_ref(
    javaobject reference) noexcept
  : storage_(reference) {
  assert(Alloc{}.verifyReference(reference));
  internal::dbglog("New wrapped ref=%p this=%p", get(), this);
}
```

In the verifyReference(), [there is a GetObjectRefType() call which is an invalid call at exception pending state](https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/jni/first-party/fb/include/fb/fbjni/ReferenceAllocators-inl.h#L57-L62).

```C++
inline bool LocalReferenceAllocator::verifyReference(jobject reference) const noexcept {
  if (!reference || !internal::doesGetObjectRefTypeWork()) {
    return true;
  }
  return internal::getEnv()->GetObjectRefType(reference) == JNILocalRefType;
}
```

The fix moves `adopt_local()` wrapping after `ExceptionClear()` and we could call `GetObjectRefType()` without problems.

## Changelog

[Android] [Fixed] - Fix Android native debug build assert crash
NOTE this might be ignored in RN release changelog.
Since the problem should exist for years and no user tried native debug build.
Pull Request resolved: facebook#25263

Test Plan:
Try to run RNTester from native debug build and make sure no crash happens.
(It is a launch crash and should be easy to verify)
`NATIVE_BUILD_TYPE=Debug ./gradlew clean :RNTester:android:app:installDebug`

Differential Revision: D15873691

Pulled By: cpojer

fbshipit-source-id: fb93b85daa1136cdf44e7fd7f217a2767391d8dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants