-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you!
There was a problem hiding this 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.
This pull request was successfully merged by @Kudo in ce80a67. When will my fix make it into a release? | Upcoming Releases |
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
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:
Native crash backtrace:
Java exception code from ViewManagerRegistry.java :
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.In the verifyReference(), there is a GetObjectRefType() call which is an invalid call at exception pending state.
The fix moves
adopt_local()
wrapping afterExceptionClear()
and we could callGetObjectRefType()
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