Summary:
As per commit: https://github.com/facebook/react-native/commit/4f3b17412018a10f9293247c802598d2b94a844b which states that "React Native requires that the RootView id be managed entirely by React Native, and will crash in addRootView/startSurface if the native View id isn't set to NO_ID."
This behaviour can not be guaranteed in **hybrid** apps that have a native android layer over which ReactRootViews are added and the native views need to have ids on them in order to work. **The control of views can jump back and forth between native android and react-native (fabric). As the ReactRootView is added to ViewGroups (or layouts) in Android Fragments and Activities, they contain ids on their views which might get passed down to the reactRootView by features like DataBinding**
Hence this can cause unnecessary crashes at runtime for hybrid apps even when they are not changing the id of the reactRootView object they are adding to their ViewGroups.
Our app is a hybrid app that uses both native android and react-native on different screens and on one such screen that has a Fragment adding a ReactRootView to its FrameLayout to render native android views to render in ReactNative, this crash occurs on pressing the back button as well as on unlocking the screen while staying on the same screen.
The app was running fine on more than a 100 million devices on React Native 0.63.4 but after updating to 0.67.2, that features this commit, it crashes on the very first device it was tested on.
Refer to the issue: https://github.com/facebook/react-native/issues/33121 for more information on the crash
The fragment in which this issues arises is like this:
```binding.frameLayout.addView(getReactRootView())```
where getReactRootView() is like this:
```
private var mReactRootView: ReactRootView? = null
private var mReactInstanceManager: ReactInstanceManager? = null
mReactRootView = ReactRootView(context)
if (activity != null) {
val application = activity?.application
if (application is MainApplication) {
mReactInstanceManager = application.reactInstanceManager
}
}
fun getReactRootView():View?{
return mReactRootView
}
```
So converting this to a soft exception such that pure react-native devs can still see the error while hybrid apps continue to run without crashes.
### Snippet of the change:
```
if (getId() != View.NO_ID) {
ReactSoftExceptionLogger.logSoftException(
TAG,
new IllegalViewOperationException(
"Trying to attach a ReactRootView with an explicit id already set to ["
+ getId()
+ "]. React Native uses the id field to track react tags and will overwrite this"
+ " field. If that is fine, explicitly overwrite the id field to View.NO_ID."));
}
```
## Changelog
[GENERAL] [ADDED] - A ReactSoftException log instead of a direct exception being thrown:
```
if (getId() != View.NO_ID) {
ReactSoftExceptionLogger.logSoftException(
TAG,
new IllegalViewOperationException(
"Trying to attach a ReactRootView with an explicit id already set to ["
+ getId()
+ "]. React Native uses the id field to track react tags and will overwrite this"
+ " field. If that is fine, explicitly overwrite the id field to View.NO_ID."));
}
```
[GENERAL] [REMOVED]- Directly throwing an exception even when the code is not responsible for this issue:
```
if (getId() != View.NO_ID) {
throw new IllegalViewOperationException(
"Trying to attach a ReactRootView with an explicit id already set to ["
+ getId()
+ "]. React Native uses the id field to track react tags and will overwrite this"
+ " field. If that is fine, explicitly overwrite the id field to View.NO_ID.");
}
```
Pull Request resolved: https://github.com/facebook/react-native/pull/33133
Test Plan:
This crash is hard to reproduce but when it occurs, this is the only way to fix it.
If any app used to crash with this exact error, it will no longer crash but show an error log in Logcat for developers to be informed about the issue.
Reviewed By: ShikaSD
Differential Revision: D34304212
Pulled By: cortinico
fbshipit-source-id: f0eaeef2e905a6e0587df088b43cc49cabda397a
Summary:
Fixes a potential crash was introduced by https://github.com/facebook/react-native/issues/30919 that aimed to get the keyboard height on devices with a Notch. The problem is that it considers that any ReactRootView will have an insets available.
When using [react-native-navigation](https://github.com/wix/react-native-navigation) and assigning a Navigation button to the TopBar as a component, the component gets registered as a RootView but won't have any insets attach to the view.
[getRootWindowInsets()](https://developer.android.com/reference/android/view/View#getRootWindowInsets()) in fact return a `WindowInset` only available if the view is attached, so when executing `checkForKeyboardEvents` method from ReactRootView, is trying to access the `DisplayCutout` of a null object, leading to a crash.
## Changelog
[Android] [Fixed] - Fix potential crash if ReactRootView does not have insets attached.
Pull Request resolved: https://github.com/facebook/react-native/pull/32989
Test Plan:
Without the code change: Notice how the second screen being push contains a React Component on the top right of the navigation bar, and when component is unmounted (going back) the app crashes.
https://user-images.githubusercontent.com/6757047/151558235-39b9a8b5-be73-4c31-8053-02ce188637b8.mp4
crash log
```
2022-01-28 10:27:52.902 15600-15600/com.mattermost.rnbeta E/AndroidRuntime: FATAL EXCEPTION: main
Process: com.mattermost.rnbeta, PID: 15600
java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.DisplayCutout android.view.WindowInsets.getDisplayCutout()' on a null object reference
at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.checkForKeyboardEvents(ReactRootView.java:778)
at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.onGlobalLayout(ReactRootView.java:769)
at android.view.ViewTreeObserver.dispatchOnGlobalLayout(ViewTreeObserver.java:1061)
at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3214)
at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:2143)
at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8665)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1037)
at android.view.Choreographer.doCallbacks(Choreographer.java:845)
at android.view.Choreographer.doFrame(Choreographer.java:780)
at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1022)
at android.os.Handler.handleCallback(Handler.java:938)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loopOnce(Looper.java:201)
at android.os.Looper.loop(Looper.java:288)
at android.app.ActivityThread.main(ActivityThread.java:7839)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
```
After applying the patch which is only a null check validation and does not change any previous behavior
https://user-images.githubusercontent.com/6757047/151558429-9ff1a608-abb6-4168-8db9-df0c3c71d79e.mp4
Reviewed By: cortinico
Differential Revision: D33844955
Pulled By: ShikaSD
fbshipit-source-id: ed5579ad3afeed009c61cc1851eee45c70087cf5
Summary:
The current JSLoader implementation (on Android) will copy the buffer into a JSBigBufferString during startup. This duplicates work Android is already doing (it allocates creates a copy of the bundle in memory while decompressing the asset from the APK) or worse, if the APK is mmap'ed this will unnecessarily create dirty memory.
The risk with this approach is that the bundle loaded from disk is no longer \0 terminated, but that's also the case for the bundles we load with `JSBigFileString` and is not an issue as far as I can tell.
Changelog: [Internal]
Reviewed By: mhorowitz
Differential Revision: D33792735
fbshipit-source-id: 61fc089a223f3602d3575340d79a8de2ec92d8a0
Summary:
This diff deletes the flag ReactFeatureFlags.enableReactContextCleanupFix, the flag was disabled for many months, I just disable it.
changelog: [internal] internal
Reviewed By: genkikondo, makovkastar
Differential Revision: D33781628
fbshipit-source-id: 4b5e22adf9d30da5b85bbbde8bdc98d98f5e8891
Summary:
This diff deletes the ReactFeatureFlag enableFabricInLogBox, from now on Logbox will be rendered in Fabric when Fabric is enabled
changelog: [internal] internal
Reviewed By: genkikondo
Differential Revision: D33781626
fbshipit-source-id: 3187a22fec80125afd27860995637564640dab8d
Summary:
The overflowInset uses negative values to indicate extending from parent view. This diff fixes the math so that it's correctly check if the point is within overflowInset.
Changelog
[Android][Fixed] - Fix math for detecting if children views are in parent's overflowInset area.
Reviewed By: genkikondo
Differential Revision: D33750129
fbshipit-source-id: 1a5a33a227280c687b158b4a81a56017b6f4f3e0
Summary:
changelog: [internal]
Yielding in RuntimeScheduler is shipped. This diff removes the gating around it.
Reviewed By: sshic
Differential Revision: D33740360
fbshipit-source-id: 267372e81e66dda96e451435954a7c3546cc6fbe
Summary:
Enable eager intialization of FabricUIManager during intiialization of React Native.
This feature highly improved TTRC in Markeptlace Home
changelog: [internal] internal
Reviewed By: genkikondo
Differential Revision: D33585099
fbshipit-source-id: 0ffbc720bcb1edd1b04180189a52c82e9e2fa800
Summary:
Per discussion in the previous diff D33672110, it's ok to add the `pointerEvents` prop to scrollview. This will help prevent scrolling on the ScrollView if pointerEvents is set to `box-none`, or `none`.
Corresponding doc changes are in https://github.com/facebook/react-native-website/pull/2936
Changelog:
[Android][Added] - Add new API in ScrollView and HorizontalScrollView to process pointerEvents prop.
Reviewed By: javache
Differential Revision: D33699223
fbshipit-source-id: 1cae5113e9e7d988fc4c4765c41d817a321804c4
Summary:
Making the native ViewConfig for RCTModalHostView match its static ViewConfig.
Changelog: [Internal]
Reviewed By: JoshuaGross
Differential Revision: D33303419
fbshipit-source-id: 6fac237d670ee221ad867f79e54d5a3576c156da
Summary:
Serializes type information along with key/value in MapBuffer, asserting the data type on Java side during read. At the moment, accessing value with incorrect will result in a crash.
Other changes:
Adds a `getType` method to verify types before accessing them.
Removes `null` as a type, as just not inserting value and checking for its existence with `hasKey` is more optimal.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33656841
fbshipit-source-id: 23a78daa0d84704aab141088b5dfe881e9609472
Summary:
This fixes https://github.com/facebook/react-native/issues/32898 by reseting ReactInstanceManager state when `createReactContext` throws. By resetting the state, we allow future attempts at creating the React context.
## Changelog
[Android] [Fixed] - Fixed empty screen after retrying a BundleDownloader failure in dev mode
Pull Request resolved: https://github.com/facebook/react-native/pull/32901
Test Plan: Go through the steps to reproduce listed in https://github.com/facebook/react-native/issues/32898 and see that the React Native application starts.
Reviewed By: cortinico
Differential Revision: D33634178
Pulled By: ShikaSD
fbshipit-source-id: e54d12d5f33c9c7c0ca213113871b88c2f1dc261
Summary:
Rename `_header` to `header_` to align with the C++ naming scheme we use.
Rename `readKey` to `readUnsignedShort` as purpose of the method have changed.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D33637127
fbshipit-source-id: a82f4d6c1b753b21e0567fbe919af98e4c78105d
Summary:
In the case of T110060790, there is a JavaScript crash that causes RN teardown to race with a ScrollView event being emitted, which ends up being reported as the "real" cause of the crash.
This is not correct, and it's better to just ignore this case. If there's no EventDispatcher, it means something has gone wrong (usually teardown) before, RN is in an inconsistent state, and we should
not crash here or report here.
Changelog: [Android][Fixed] Fix crash from ScrollView that occurs while reporting an error from JS
Reviewed By: mdvacca
Differential Revision: D33644692
fbshipit-source-id: 41c3defde795b804239cc8401c8aff71d017d59d
Summary:
Minor optimization, but spotted this while reviewing the implementation for D33622997
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33622996
fbshipit-source-id: 8712753803fc46e6a046d50f77454a813e4a641a
Summary:
`onPointerMove` events get dispatched with an `offset: [x, y]` attribute (API is not yet available in OSS, so subject to change), but `EventAnimationDriver` in RN Android is not able to extract values with such keys (even though the equivalent JS implementation does allow it).
(TODO: verify iOS behaviour)
Changelog:
[General][Added] - Animated.event can be used to extract values with numeric keys from native events
Reviewed By: mdvacca
Differential Revision: D32531117
fbshipit-source-id: 918a5443c5d8f5f8200d86bb67f84e8bc175c1d3
Summary:
MapBuffer uses unsigned short in C++, but Java doesn't really have a type that represents that. That means that MapBuffer would be limited to max 32768 values instead of 65536, which doesn't make much sense as a limitation.
Considering weird (and usually not performant) handling of short values in Java in general, this change replaces them with ints, converting keys from short when needed with `key & 0xFFFF`.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D33595308
fbshipit-source-id: a7adde8a207bb4aa1d81d367ab5d7b41ace2e291
Summary:
Adds a check to verify that we are not trying to insert a new view id into a non-existent set.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D33621031
fbshipit-source-id: 8468af69bea250a70d656789ea819c39b55a9de6
Summary:
Cleans up `ReadableMapBuffer` APIs and migrates to use `std::vector` instead of raw pointer array.
Also uses `fbjni` utility to allocate the bytes instead of making manual JNI calls.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D33513027
fbshipit-source-id: afe7d320d12830503de4c9994117d849f0b25245
Summary:
`WritableNativeMap` has two constructors:
```c++
WritableNativeMap();
WritableNativeMap(folly::dynamic &&val);
```
but `WritableNativeMap` has only one constructor:
```c++
WritableNativeMap();
```
Without a second constructor, I am not able to create `WritableNativeMap` from the existing array. I added this second constructor because I need it for `react-native-reanimated`.
## Changelog
[Android] [Added] - Added missing constructor to WritableNativeArray
Pull Request resolved: https://github.com/facebook/react-native/pull/32796
Test Plan:
```c++
std::shared_ptr<jsi::Runtime> rt = facebook::hermes::makeHermesRuntime();
jsi::Value value;
WritableNativeMap::newObjectCxxArgs(jsi::dynamicFromValue(*rt.get(), value));
```
Reviewed By: cortinico
Differential Revision: D33285316
Pulled By: lunaleaps
fbshipit-source-id: 1bbd816892f34ae6fef747066893fe3b803c088d
Summary:
Adds an experimental replacement for preallocation revision checks on Android mounting layer. Instead of checking revisions of props, we remember tags of preallocated/previously allocated views and use this as a filter to avoid issuing extra create and preallocate commands on copy.
This method is a bit more expensive on memory, but is more precise than relying on revisions.
Changelog: [Internal]
Reviewed By: sammy-SC, mdvacca
Differential Revision: D33038768
fbshipit-source-id: fc98c2a7912e162a3845ca80488635bffaf8cc7f
Summary:
Quick refactor of ReactRootView where I extract 'Fabric' check into a method
changelog: [internal] internal
Reviewed By: fkgozali
Differential Revision: D33269025
fbshipit-source-id: 2647d5870f3b5fcc5a940a5924a39ca43b5bebac
Kunal Farmah
Distiller
Anandraj
Distiller
Distiller
Distiller
Distiller
David Vacca
Distiller
Alfonso Curbelo
Distiller
Elias Nahum
Distiller
Pieter De Baets
Xin Chen
owjsub
Samuel Susla
Ramanpreet Nara
Andrei Shikov
CodemodService FBSourceClangFormatLinterBot
Janic Duplessis
Sam Kline
Joshua Gross
grgr-dkrk
Lulu Wu
Mengke Ding
Krzysztof Piaskowy
Andres Suarez
