Summary:
We noticed a repro-able crash in Ride in T52804960 on Android Q due to NaN being passed into setCameraDistance
on View
see Oleg's related post: https://fb.workplace.com/groups/rn.support/permalink/2682537011794897/
It looks like a generic fix or wrapper around View setCameraDistance might be planned in T48580247
But in the meantime, it kind of maybe seems reasonable-ish to say, ~~if the value of an input node is NaN, don't use it in the math for this node?~~ if a one of the inputs for this node evaluates to NaN, update that input node first? But I'm not super familiar with the Animations library so maybe that's not a good idea, idk.
From what I can tell in our specific error, it's coming from an InterpolatedNode A based off an AdditionNode B which tried to add a ValueNode C + a InterpolatedNode D, but D had only just been created and not had it's first update, so it's value was NaN, and so when B runs it's update value of C + NaN means B's new values is also NaN, and A's subsequent update based on that now comes out to NaN. Atleast that's what it seems like based on Log statements.
Reviewed By: olegbl
Differential Revision: D16960177
fbshipit-source-id: 99c8ca35be4b5e99f7c21db6733ebd622ae39d07
Summary:
After some thought, we decided we don't need the flexibility of
separate horizontal and vertical props - it would be much nicer
to just have a single prop for the edge length and then the native
code can enable the booleans as appropriate.
Original PR: https://github.com/facebook/react-native/pull/26163
Original commit changeset: f72a9a890d90
Reviewed By: TheSavior
Differential Revision: D16997468
fbshipit-source-id: 7973262287a7ec2cee5957f8dc1806a0f28c1432
Summary: [Android] [Fixed] - Make sure the default accessibility hint is read for buttons on Android.
Reviewed By: PeteTheHeat
Differential Revision: D16974987
fbshipit-source-id: e7b932041f82d41271d9393e651967789c05f38a
Summary: When debugging supporting a Fabric component, a redbox was being shown without a title which resulted in an infinite redbox loop, followed by OOM. That isn't helpful so we just safely work around the null title while indicating what happened to aid in debugging.
Reviewed By: lunaleaps
Differential Revision: D16921053
fbshipit-source-id: df1f4a691df0c10de53c91e4a9356a04a9005a97
Summary: This was an experiment to patch individual deltas in development instead of reloading the whole JS bundle. With improvements such as Fast Refresh that reduces the need for reloads and bundle splitting that reduces the number of modules and memory by 10x, we won't be needing this complex optimization that we never properly made work. This diff removes that code and I will be removing the JS side of things in Metro in a follow-up diff.
Reviewed By: fkgozali
Differential Revision: D16832709
fbshipit-source-id: 46596a3126d52d7d74f4b9ffc9a6ee9d82ec9522
Summary:
There have been multiple complaints about combining RN with various
other FB libraries, including Litho and Flipper, because of bundled dependencies
that can't be deduplicated by Gradle.
This is one of three current conflicts:
1) Proguard annotations (this PR)
2) Yoga
3) fbjni
While the Yoga group name doesn't make a massive amount of sense
it was the easiest existing package to use and since we don't
have a better namespace for this, we might as well use this.
A similar change to Litho is landing right now.
## Changelog
[Android] [Changed] - Use centralized package for DoNotStrip annotation
Pull Request resolved: https://github.com/facebook/react-native/pull/26069
Test Plan:
```
yarn
./gradlew :RNTester:android:app:assembleDebug
```
Reviewed By: cpojer
Differential Revision: D16827430
Pulled By: passy
fbshipit-source-id: 87542b5422fee598d8e635651441f0ecd42eb9d7
Summary: The OnLoad.cpp file is needed since it actually registers the jni functions, it wasn't included in the build so it wasn't working correctly.
Reviewed By: jbower-fb
Differential Revision: D16826230
fbshipit-source-id: 0243e456c4015879d17650737a6a27a58a3d0d9a
Summary: This diff adds a feature flag which must be enabled if view managers should be allowed to use a delegate for setting their properties.
Reviewed By: mdvacca
Differential Revision: D16762876
fbshipit-source-id: ae3466d7f02ed02f203dbb79f5e0843e6d9fdd45
Summary: ModuleRegistryBuilder does as assertion to verify that all `cxxModules` are instances of `CxxModuleWrapperBase::javaStaticClass()`. This assertion is causing the crash in T48554656. Since we don't know which NativeModule is causing this problem, it's difficult to get to the bottom of this. So, for now, I'm improving the assertion message in the hopes that it helps us get to the bottom of this issue.
Reviewed By: mdvacca
Differential Revision: D16774711
fbshipit-source-id: 82318b8ff5ab735ae642da81777c1b5588e8a483
Summary:
This diff introduces an interface `ViewManagerDelegate` and its base implementation `BaseViewManagerDelegate`, which is used as a parent class for all view manager delegates generated by the JS codegen. Before the changes in this diff, generated delegates didn't support setting the base view properties such as background color, rotation, opacity, etc. Now it's possible to do by using `BaseViewManagerDelegate.setProperty(...)`, and since all generated delegates extend BaseViewManagerDelegate, they can just call `super.setProperty(...)` for properties they don't want to handle.
This diff also introduced a new method `ViewManager.getDelegate()`. This will allow view managers to return an instance of the delegate generated by JS and ensure that the view properties are set in a type-safe manner. If this method returns null (it does by default), we fall back to the default implementation of setting view properties using Java-generated `$$PropsSetter`
classes.
This is an example of an interface class generated by JS:
```
public interface RCTAxialGradientViewViewManagerInterface<T extends View> {
void setColors(T view, Nullable ReadableArray value);
void setLocations(T view, Nullable ReadableArray value);
void setEndX(T view, Float value);
void setEndY(T view, Float value);
void setStartX(T view, Float value);
void setStartY(T view, Float value);
}
```
This is an example of a delegate class generated by JS:
```
public class RCTAxialGradientViewManagerDelegate<T extends View, U extends BaseViewManager<T, ? extends LayoutShadowNode> & RCTAxialGradientViewManagerInterface<T>> extends BaseViewManagerDelegate<T, U> {
public RCTAxialGradientViewManagerDelegate(U viewManager) {
super(viewManager);
}
Override
public void setProperty(T view, String propName, Nullable Object value) {
switch (propName) {
case "colors":
mViewManager.setColors(view, (ReadableArray) value);
break;
case "locations":
mViewManager.setLocations(view, (ReadableArray) value);
break;
case "endX":
mViewManager.setEndX(view, value == null ? Float.NaN : ((Double) value).floatValue());
break;
case "endY":
mViewManager.setEndY(view, value == null ? Float.NaN : ((Double) value).floatValue());
break;
case "startX":
mViewManager.setStartX(view, value == null ? Float.NaN : ((Double) value).floatValue());
break;
case "startY":
mViewManager.setStartY(view, value == null ? Float.NaN : ((Double) value).floatValue());
break;
default:
super.setProperty(view, propName, value);
}
}
}
```
NOTE: What if a view manager, for instance ReactAxialGradientManager, wanted to add support for the borderRadius prop? In the old Java codegen, it would just need to create a method and annotate it with ReactProp (name = ViewProps.BORDER_RADIUS) and $$PropsSetter would call this method when a property with this name must be set. With the new JS codegen, borderRadius is a part of the basic view props, so setBorderRadius is not generated as a part of the ViewManagerInterface, so it’s not possible to set this value. I see two options: 1) add a method boolean setProperty (String propName, Object value) and let the view manager handle it in a non-type safe way (return true if it’s been handled). 2) Generate BaseViewManagerInterface which will include all basic view props and make BaseViewManager implement this interface, leaving all methods empty so that it stays compatible with the current implementation. Override these methods in a view manager that needs to handle a specific property in a custom way (so we would override setBorderRadius in ReactAxialGradientManager).
Reviewed By: mdvacca
Differential Revision: D16667686
fbshipit-source-id: 06a15a92f8af55640b7a53c5a34f40366d1be2a8
Summary:
Changing showSoftKeyboard and hideSoftKeyboard to be protected, as we need this change for an internal control that extends ReactEditText.
## Changelog
[Android] [Changed] - part of our react native platform, we have a control that extends ReactEditText and we need to be able to override these 2 methods.
Pull Request resolved: https://github.com/facebook/react-native/pull/25964
Test Plan: The change has been in Microsoft's branch of RN for almost 2 years, and since it's a relatively small change we've done a quick sanity check in RNTester prior to this PR, making sure the TextInput page loads fine and it's functional.
Differential Revision: D16686878
Pulled By: cpojer
fbshipit-source-id: 63035ee9c58e93bc0fa40e5bec318df05322c6c5
Summary: Some surfaces throw ConcurrentModificationException when logging detailed perf for Fabric. I've refactored the ReactMarker class to use a threadsafe ArrayList and removed synchronization, which is safer and should improve perf everywhere the markers are used, even if there are zero listeners.
Reviewed By: mdvacca
Differential Revision: D16656139
fbshipit-source-id: 34572f9ad19028a273e0837b0b895c5e8a47976a
Summary:
## The Problem
1. `CatalystInstanceImpl` indirectly holds on to the `jsi::Runtime`. When you destroy `CatalystInstanceImpl`, you destroy the `jsi::Runtime`. As a part of reloading React Native, we destroy and re-create `CatalystInstanceImpl`, which destroys and re-creates the `jsi::Runtime`.
2. When JS passes in a callback to a TurboModule method, we take that callback (a `jsi::Function`) and wrap it in a Java `Callback` (implemented by `JCxxCallbackImpl`). This Java `Callback`, when executed, schedules the `jsi::Function` to be invoked on a Java thread at a later point in time. **Note:** The Java NativeModule can hold on to the Java `Callback` (and, by transitivity, the `jsi::Function`) for potentially forever.
3. It is a requirement of `jsi::Runtime` that all objects associated with the Runtime (ex: `jsi::Function`) must be destroyed before the Runtime itself is destroyed. See: https://fburl.com/m3mqk6wt
### jsi.h
```
/// .................................................... In addition, to
/// make shutdown safe, destruction of objects associated with the Runtime
/// must be destroyed before the Runtime is destroyed, or from the
/// destructor of a managed HostObject or HostFunction. Informally, this
/// means that the main source of unsafe behavior is to hold a jsi object
/// in a non-Runtime-managed object, and not clean it up before the Runtime
/// is shut down. If your lifecycle is such that avoiding this is hard,
/// you will probably need to do use your own locks.
class Runtime {
public:
virtual ~Runtime();
```
Therefore, when you delete `CatalystInstanceImpl`, you could end up with a situation where the `jsi::Runtime` is destroyed before all `jsi::Function`s are destroyed. In dev, this leads the program to crash when you reload the app after having used a TurboModule method that uses callbacks.
## The Solution
If the only reference to a `HostObject` or a `HostFunction` is in the JS Heap, then the `HostObject` and `HostFunction` destructors can destroy JSI objects. The TurboModule cache is the only thing, aside from the JS Heap, that holds a reference to all C++ TurboModules. But that cache (and the entire native side of `TurboModuleManager`) is destroyed when we call `mHybridData.resetNative()` in `TurboModuleManager.onCatalystInstanceDestroy()` in D16552730. (I verified this by commenting out `mHybridData.resetNative()` and placing a breakpoint in the destructor of `JavaTurboModule`). So, when we're cleaning up `TurboModuleManager`, the only reference to a Java TurboModule is the JS Heap. Therefore, it's safe and correct for us to destroy all `jsi::Function`s created by the Java TurboModule in `~JavaTurboModule`. So, in this diff, I keep a set of all `CallbackWrappers`, and explicitly call `destroy()` on them in the `JavaTurboModule` destructor. Note that since `~JavaTurboModule` accesses `callbackWrappers_`, it must be executed on the JS Thread, since `createJavaCallbackFromJSIFunction` also accesses `callbackWrappers_` on the JS Thread.
For additional safety, I also eagerly destroyed the `jsi::Function` after it's been invoked once. I'm not yet sure if we only want JS callbacks to only ever be invoked once. So, I've created a Task to document this work: T48128233.
Reviewed By: mdvacca
Differential Revision: D16623340
fbshipit-source-id: 3a4c3efc70b9b3c8d329f19fdf4b4423c489695b
Summary: This diff implements the JSResponderHandler methods in the core of RN (scheduler API and friends)
Reviewed By: ejanzer
Differential Revision: D16543437
fbshipit-source-id: dac03e30c4330d182ecf134f3174ba942dbf7289
Summary:
This diff adds the new API required to implment JSResponderHandler in FabricUIManager
The new API differs from the old API, but since setJSResponder is called ONLY from JS it's not necessary to have this method as part of UIManager interface.
Reviewed By: JoshuaGross
Differential Revision: D16543440
fbshipit-source-id: ca4bd4c1e4df706cda0eb16798e01f3350558d06
Summary: When you create a TurboModule from the JS side, we instantiate its Java class and simply make this `javaobject` a `jni::global_ref` in C++. But the reason why we need to make this a global ref is because `JavaTurboModule` needs it to be a global reference for method calls. Making this a `jni::global_ref` from the perspective to TurboModuleManager doesn't really make any sense. So, this diff refactors that bit of code.
Differential Revision: D16622133
fbshipit-source-id: 6a5c20bb405b945c06378a3423d5e7eb38ef244c
Summary:
## Description
When we reload the app, `ReactApplicationContext.destroy()` is called. This method calls `CatalystInstanceImpl.destroy()`, which (on the NativeModules thread) calls `NativeModuleRegistry.notifyJSInstanceDestroy()`, which loops over all its `ModuleHolder`s, and calls `ModuleHolder.destroy()`, each of which call their NativeModule's `onCatalystInstanceDestroy()` method.
TurboModuleManager is a `JSIModule`, so it also has its `onCatalystInstanceDestroy()` method called when the `ReactApplicationContext` is destroyed. But `TurboModuleManager.onCatalystInstanceDestroy()` doesn't do anything. Instead, at the very least, it should call `onCatalystInstanceDestroy()` of all NativeModules.
Differential Revision: D16622132
fbshipit-source-id: da29e698b5217232cfaa01e6f817aab18ceb526f
Summary: On iOS, calling the `__turboModuleProxy` function with the same name returns the same instance of the TurboModule. Adding this behaviour to Andorid as well.
Differential Revision: D16622131
fbshipit-source-id: 472011ac3356e7c30497f848be0c888596c449b1
Summary: Same as title. Changing as per suggestion.
Reviewed By: furdei
Differential Revision: D16615807
fbshipit-source-id: 1c35ae1471beb2460e975841f367ffd49ce34494
Summary: This diff registers the new Turbo Module SoundManager in Catalyst, FB4A and Workplace
Reviewed By: JoshuaGross
Differential Revision: D16543432
fbshipit-source-id: de3ab175befd5cc11403159e3aa42e5f17d0a280
Summary: This diff implements the Turbo Module SoundManager, this will be used by following diffs of the stack
Reviewed By: JoshuaGross
Differential Revision: D16543430
fbshipit-source-id: 34ba545f54b759fe4e49d4e3c5f8867205de907c
Summary:
## The Problem
1. `CatalystInstanceImpl` indirectly holds on to the `jsi::Runtime`. When you destroy `CatalystInstanceImpl`, you destroy the `jsi::Runtime`. As a part of reloading React Native, we destroy and re-create `CatalystInstanceImpl`, which destroys and re-creates the `jsi::Runtime`.
2. When JS passes in a callback to a TurboModule method, we take that callback (a `jsi::Function`) and wrap it in a Java `Callback` (implemented by `JCxxCallbackImpl`). This Java `Callback`, when executed, schedules the `jsi::Function` to be invoked on a Java thread at a later point in time. **Note:** The Java NativeModule can hold on to the Java `Callback` (and, by transitivity, the `jsi::Function`) for potentially forever.
3. It is a requirement of `jsi::Runtime` that all objects associated with the Runtime (ex: `jsi::Function`) must be destroyed before the Runtime itself is destroyed. See: https://fburl.com/m3mqk6wt
### jsi.h
```
/// .................................................... In addition, to
/// make shutdown safe, destruction of objects associated with the Runtime
/// must be destroyed before the Runtime is destroyed, or from the
/// destructor of a managed HostObject or HostFunction. Informally, this
/// means that the main source of unsafe behavior is to hold a jsi object
/// in a non-Runtime-managed object, and not clean it up before the Runtime
/// is shut down. If your lifecycle is such that avoiding this is hard,
/// you will probably need to do use your own locks.
class Runtime {
public:
virtual ~Runtime();
```
Therefore, when you delete `CatalystInstanceImpl`, you could end up with a situation where the `jsi::Runtime` is destroyed before all `jsi::Function`s are destroyed. In dev, this leads the program to crash when you reload the app after having used a TurboModule method that uses callbacks.
## The Solution
If the only reference to a `HostObject` or a `HostFunction` is in the JS Heap, then the `HostObject` and `HostFunction` destructors can destroy JSI objects. The TurboModule cache is the only thing, aside from the JS Heap, that holds a reference to all C++ TurboModules. But that cache (and the entire native side of `TurboModuleManager`) is destroyed when we call `mHybridData.resetNative()` in `TurboModuleManager.onCatalystInstanceDestroy()` in D16552730. (I verified this by commenting out `mHybridData.resetNative()` and placing a breakpoint in the destructor of `JavaTurboModule`). So, when we're cleaning up `TurboModuleManager`, the only reference to a Java TurboModule is the JS Heap. Therefore, it's safe and correct for us to destroy all `jsi::Function`s created by the Java TurboModule in `~JavaTurboModule`. So, in this diff, I keep a set of all `CallbackWrappers`, and explicitly call `destroy()` on them in the `JavaTurboModule` destructor. Note that since `~JavaTurboModule` accesses `callbackWrappers_`, it must be executed on the JS Thread, since `createJavaCallbackFromJSIFunction` also accesses `callbackWrappers_` on the JS Thread.
For additional safety, I also eagerly destroyed the `jsi::Function` after it's been invoked once. I'm not yet sure if we only want JS callbacks to only ever be invoked once. So, I've created a Task to document this work: T48128233.
Reviewed By: mhorowitz
Differential Revision: D16589168
fbshipit-source-id: a1c0786999c22bef55d416beb0fc40261447a807
Summary: When you create a TurboModule from the JS side, we instantiate its Java class and simply make this `javaobject` a `jni::global_ref` in C++. But the reason why we need to make this a global ref is because `JavaTurboModule` needs it to be a global reference for method calls. Making this a `jni::global_ref` from the perspective to TurboModuleManager doesn't really make any sense. So, this diff refactors that bit of code.
Reviewed By: mdvacca
Differential Revision: D16555673
fbshipit-source-id: 2778fc5a372c41847e8296c2e22bb9a8826fcc52
Summary:
## Description
When we reload the app, `ReactApplicationContext.destroy()` is called. This method calls `CatalystInstanceImpl.destroy()`, which (on the NativeModules thread) calls `NativeModuleRegistry.notifyJSInstanceDestroy()`, which loops over all its `ModuleHolder`s, and calls `ModuleHolder.destroy()`, each of which call their NativeModule's `onCatalystInstanceDestroy()` method.
TurboModuleManager is a `JSIModule`, so it also has its `onCatalystInstanceDestroy()` method called when the `ReactApplicationContext` is destroyed. But `TurboModuleManager.onCatalystInstanceDestroy()` doesn't do anything. Instead, at the very least, it should call `onCatalystInstanceDestroy()` of all NativeModules.
Reviewed By: mdvacca
Differential Revision: D16552730
fbshipit-source-id: 04459185262e92d69570facb368578a848cc5fdb
Summary: On iOS, calling the `__turboModuleProxy` function with the same name returns the same instance of the TurboModule. Adding this behaviour to Andorid as well.
Reviewed By: mdvacca
Differential Revision: D16553363
fbshipit-source-id: c95e150d6967604a808cfb49877b7a633e33d729
Summary: This fixes the lint that was broken by D16543439
Reviewed By: fkgozali
Differential Revision: D16607092
fbshipit-source-id: 70c3989d25f446ffe0877e58253acef5ada8e010
Summary:
View Pooling is not currently being used in Fabric Android, this diff removes all the extra abstractions that are being used becuase of the unused ViewPooling.
We might add this in the future when we re-implement view correctly.
Reviewed By: JoshuaGross
Differential Revision: D16543439
fbshipit-source-id: f41b6e02fddc36c7ef7a1052399d2e6b2041fcfb
Summary:
We had flex as a reason for both layout and measure. Now creating separating reason flexLayout and flexMeasure in this diff.
Also changed ordering of items in Enum to group layout and measure reasons
Reviewed By: davidaurelio
Differential Revision: D16562350
fbshipit-source-id: 75501f9d4dde0974009193b3991a8acc97b02ad0
Summary: In Dev Settings, we used to have an `Start Sampling Profiler on init` option, which was defunct. This diff re-enables that option. We can now start the Sampling Profiler on app start
Reviewed By: yinghuitan
Differential Revision: D7022382
fbshipit-source-id: 1db85d8a324e401c71187ba5871a91abcc18acf9
Joshua Gross
André Krüger
Peter Laraia
Andres Suarez
Spencer Ahrens
Anda Bereczky
glevi@fb.com
Christoph Nakazawa
Marc Horowitz
Pascal Hartig
Rodrigo Salazar
Oleksandr Melnykov
Ram N
Ramanpreet Nara
Jakob Krigovsky
Logan Daniels
Scott Rice
Onti Vals
sunnylqm
Florian Cargoët
David Vacca
Eric Williamson
Sensen Chen
Sidharth Guglani
