Summary: This allows to implement `findNodeHandle` in Fabric world (temporary).
Reviewed By: zackargyle, JoshuaGross, mdvacca
Differential Revision: D17175953
fbshipit-source-id: c88bd1c58608450812799d4ecb4a6bf2c027c5f3
Summary: The mehtod implements the core functionality of finding a shadow node by given tag.
Reviewed By: zackargyle, JoshuaGross, mdvacca
Differential Revision: D17175955
fbshipit-source-id: 5cfbb80b2d4e54a33ca9c20a21988df4de54c800
Summary: The new methods provides a way to iterate over all registered shadow trees.
Reviewed By: zackargyle, JoshuaGross, mdvacca
Differential Revision: D17175954
fbshipit-source-id: e4e6ee5a0e0a4e4e36b99d546f8724b99559c8c2
Summary:
Fix flow test case input in react-native-codegen package that has syntax errors.
Missing ";" after export type,
Missing ";" or "," after interface/object type member signature,
Missing ":" in method signature,
etc
## Changelog
[General] [Fixed] - Fix flow test case input in react-native-codegen package
Pull Request resolved: https://github.com/facebook/react-native/pull/26316
Test Plan: Parsed files using flow parser, but some test cases are intended to have syntax error, in this case I fixed unexpected ones.
Reviewed By: rickhanlonii
Differential Revision: D17194969
Pulled By: osdnk
fbshipit-source-id: 262d0af4d9e7e74f7ba3eb30c4fd9753c08f3bf7
Summary:
The README.md was slightly out of date.
## Changelog
[Android] [Fixed] - Fixed RNTester instructions
Pull Request resolved: https://github.com/facebook/react-native/pull/26140
Test Plan: Ran `./gradlew :RNTester:android:app:installJscDebug` to build and install RNTester on my device.
Differential Revision: D17178074
Pulled By: mdvacca
fbshipit-source-id: 677f91833170af54eeb1f0b5431dd94eae7668ac
Summary:
Trying to build the Android RNTester app using Gradle fails with the following error:
```
Errors found:
[...]/RNTester/android/app/src/main/AndroidManifest.xml:2: Error: Hardware feature android.hardware.touchscreen not explicitly marked as optional [ImpliedTouchscreenHardware]
<manifest
~~~~~~~~
[...]/RNTester/android/app/src/main/AndroidManifest.xml:2: Error: Expecting <uses-feature android:name="android.software.leanback" android:required="false" /> tag. [MissingLeanbackSupport]
<manifest
~~~~~~~~
```
This PR adds the two missing `uses-feature` tags to AndroidManifest of RNTester.
## Changelog
[Android] [Fixed] - Fix RNTester Lint errors
Pull Request resolved: https://github.com/facebook/react-native/pull/26279
Test Plan: After adding the tags, the build is successful again.
Differential Revision: D17177952
Pulled By: mdvacca
fbshipit-source-id: c81dce9c6642fc8551f3d0eeb84c984ed06b18e3
Summary: Fixes the crash when calling setCameraDistance with NaN on Android Q.
Reviewed By: JoshuaGross, mdvacca
Differential Revision: D17170045
fbshipit-source-id: 386f969e70c56bca6ae5b8ffead62453ebb72857
Summary:
For iOS < 13, default to "light" appearance, just like in Android (instead of `nil`).
This fixes the following redbox:
{F205818545}
Reviewed By: cpojer
Differential Revision: D17147970
fbshipit-source-id: b2adccd349a0a0ff7668c2f30e93164d23c3eea6
Summary:
For now, disable TM completely in test environment, like RNTester integration/unit tests. See details in T53341772
This also fixes the failure discussed in https://github.com/facebook/react-native/pull/26151
Reviewed By: PeteTheHeat
Differential Revision: D17147915
fbshipit-source-id: 1c48ebb9c3b81fc08bc33606dcc38c29297d6010
Summary: This diff migrates `FbReactTTRCStepRenderFlagManager` to use the generated `TTRCStepRenderFlagManagerDelegate` for setting its properties. A bit more work was required to migrate this view manager since it doesn't extend `BaseViewManager`, so I had to add an adapter which made it possible to pass an instance of `FbReactTTRCStepRenderFlagManager` to `TTRCStepRenderFlagManagerDelegate`.
Reviewed By: mdvacca
Differential Revision: D16984172
fbshipit-source-id: 7bf8c1342435c4e615eb9e7ca711f53c63ed3438
Summary:
Our JS codegen assumes that all Android view managers extend `BaseViewManager` which allows generated delegates to set base view props using `BaseViewManagerDelegate`, see and example of the generated delegate for `FbReactTTRCStepRenderFlagManager`:
```
public class TTRCStepRenderFlagManagerDelegate<T extends View, U extends BaseViewManager<T, ? extends LayoutShadowNode> & TTRCStepRenderFlagManagerInterface<T>>{
public TTRCStepRenderFlagManagerDelegate(U viewManager) {
super(viewManager);
}
Override
public void setProperty(T view, String propName, Nullable Object value) {
switch (propName) {
case "traceId":
mViewManager.setTraceId(view, value == null ? null : (String) value);
break;
case "stepName":
mViewManager.setStepName(view, value == null ? null : (String) value);
break;
default:
super.setProperty(view, propName, value);
}
}
}
```
The problem is that `FbReactTTRCStepRenderFlagManager` doesn't extend `BaseViewManager`, but `ViewManager`, which means that we cannot use it with the generated delegate. We cannot use `ViewManager` instead of `BaseViewManager` in our JS codegen either, otherwise we will not be able to set base view props.
This diff makes it possible for delegates generated by JS to be used by Android view managers that do not extend `BaseViewManager`. By having a `BaseViewManagerInterface` we will be able to introduce a no-op base view manager implementation and wrap our original view manager in it so that we can pass as a constructor parameter to `BaseViewManagerDelegate`.
See an example of this approach for `FbReactTTRCStepRenderFlagManager`:
```
public class FbReactTTRCStepRenderFlagManager extends ViewManager<FbReactTTRCStepRenderFlag, ReactShadowNodeImpl> implements TTRCStepRenderFlagManagerInterface<FbReactTTRCStepRenderFlag> {
private final ViewManagerDelegate<FbReactTTRCStepRenderFlag> mDelegate;
public FbReactTTRCStepRenderFlagManager() {
mDelegate = new TTRCStepRenderFlagManagerDelegate<>(new ViewManagerWrapper(this));
}
...
private static class ViewManagerWrapper extends BaseViewManagerAdapter<FbReactTTRCStepRenderFlag> implements TTRCStepRenderFlagManagerInterface<FbReactTTRCStepRenderFlag> {
private final TTRCStepRenderFlagManagerInterface<FbReactTTRCStepRenderFlag> mViewManager;
private FbReactTTRCStepRenderFlagManagerAdapter(TTRCStepRenderFlagManagerInterface<FbReactTTRCStepRenderFlag> viewManager) {
mViewManager = viewManager;
}
Override
public void setTraceId(FbReactTTRCStepRenderFlag view, Nullable String traceId) {
mViewManager.setTraceId(view, traceId);
}
Override
public void setStepName(FbReactTTRCStepRenderFlag view, Nullable String stepName) {
mViewManager.setStepName(view, stepName);
}
}
}
```
Reviewed By: mdvacca
Differential Revision: D16984121
fbshipit-source-id: ea2761dda68a96ff3ba6ac64153bc4e56e396774
Summary:
blairv recently brought an issue with heap snapshots to our attention: When
captured from iOS, the viewer would quite regularly hang forever instead of
capturing the snapshot.
Further investigation showed that it was because the message that Hermes sends
to indicate that it had finished sending chunks of the heap snapshot was
arriving to chrome before the last chunk of the snapshot. This caused Chrome
to believe the snapshot was malformed.
I confirmed that:
- Sends from the Hermes Inspector were occurring in the correct order.
- The WebSocket protocol, being built on top of TCP, also preserves order.
- The Inspector Proxy in Metro was preserving the order of requests it was
given, but the responses were already in the wrong order when they reached
it.
This left the code that Hermes' Inspector calls into to send its messages. On
iOS, the logic to send the request is run on the global dispatch queue which
can and does schedule jobs concurrently with each other. Because the messages
containing heap snapshot chunks are much larger than the "Ok" message used to
indicate the end of the snapshot, the earlier chunk message would often lose
the race with the later "Ok" message.
There was already a serial background queue used by the websocket server to
schedule the actual sending of the message. I re-use it in this diff to
process the message as well.
The architecture of the [same code in Android](https://our.intern.facebook.com/intern/diffusion/FBS/browse/master/xplat/js/react-native-github/ReactAndroid/src/main/java/com/facebook/react/devsupport/InspectorPackagerConnection.java?commit=64c8954ec7be555509437db6944b9a71a350e87c&lines=283) initially seems to be broken in
the same way, but it is not, because AsyncTask does not run tasks concurrently
any more because Google found it exposed too many concurrency bugs to do so.
pakoito, it seems like you might be somewhat familiar with what's going on
here. Let me know if what I've done is sensible.
Reviewed By: pakoito
Differential Revision: D17106960
fbshipit-source-id: af85ff1753324340bb55fc63048e8bd424c8a983
Summary:
We probably don't always need to crash in these cases.
This should replace all prod/dev crashes with logged SoftExceptions.
Note that this is currently only used for Fabric.
Changelog:
[Internal]
Reviewed By: mdvacca
Differential Revision: D17170459
fbshipit-source-id: 70757ae88deb8c893b36971d879174e25a194fb9
Summary:
This diff replaces the usage of UIManagerModule.playTouchSound() in Pressability and Touchable for the SoundManager.playTouchSound()
Previously landed and unladed: D16543433
Reviewed By: rickhanlonii, JoshuaGross
Differential Revision: D17170140
fbshipit-source-id: 33a8ca508ec31f034c76fb0ac4107150d43c608b
Summary:
Fix Android gradle build error for native debug build.
`NATIVE_BUILD_TYPE=Debug ./gradlew :ReactAndroid:installArchives`
The root cause is `folly::Future<bool>` not declared.
As we don't include true folly::Future implementation in OSS build but to use a forward declaration,
the fix is pretty much like to original declaration as `folly::Future<folly::Unit>`.
## Changelog
[Android] [Fixed] - Fix compile error for native debug build
This change could be ignored from changelog which is only for internal development.
Pull Request resolved: https://github.com/facebook/react-native/pull/26248
Test Plan: Makes sure `NATIVE_BUILD_TYPE=Debug ./gradlew :ReactAndroid:installArchives` build without errors.
Differential Revision: D17169696
Pulled By: mdvacca
fbshipit-source-id: 42e8b84b7ee0d1bd99d913702df98bc030965f63
Summary:
This is part of enabling types-first mode in xplat/js ([context](https://fb.workplace.com/groups/rn.engineering/permalink/2293015867694617/)).
This diff adds `React.Node` as the return type of the `render` method of react components.
Differential Revision: D17137432
fbshipit-source-id: 415e902d87b6be5c26e4a0af3884a43a89c9be78
Summary:
This diff fixes a redbox happens on every single hot-reload practically saying that ImageLoader is misconfigured. The issue happened because after reloading Fabric used the previous obsolete instance of `ImageLoader` created and stored inside old (and already destroyed) instance of Bridge. The fix is simple: We update all dependencies after the Bridge was reloaded.
See https://fb.workplace.com/groups/rn.support/permalink/2677343372314261/ for more details.
Reviewed By: mdvacca
Differential Revision: D17173702
fbshipit-source-id: 5ff0c418feae10ede9b76c184cd24ad06ee008b7
Summary: `installDebug` is no longer working, `installJscDebug` is the right command. ALso removed Genymotion recommendation.
Reviewed By: mdvacca
Differential Revision: D13912729
fbshipit-source-id: 1ed5eccc8460c48ee417efbd360f42783010d376
Summary:
@public
With this guard removed, the functionality is the same in RCT_DEV and non-RCT_DEV builds because RCTDevSettings will return NO (or not exist) in non-RCT_DEV builds, so this code will not be run.
Pros to this approach:
- Hot loading can be enabled in a non-RCT_DEV build
Cons:
- all builds now have this extra block of code, even if it won't be run
If the cons are too strong, I can move this code into a new RCT_DEFINE that inherits from RCT_DEV. (but notably, can be overridden)
Reviewed By: shergin
Differential Revision: D17118516
fbshipit-source-id: cc6c01cca6b2450c35274eccc939c9a2123e6b93
Summary:
@public
RCTDevMenu and RCTDevSettings are used to display the dev menu you see when you shake the device.
With this change in build flags, it's possible to build them into a production version of the app without pulling in all the RCT_DEV logic.
Reviewed By: shergin
Differential Revision: D17116992
fbshipit-source-id: 71458c49affe5bb94c52c9d8bb0f793b16d35828
Summary:
It looks like the codegen output for OSS NativeModule specs was modified in D17152891. In this diff, I run `js1 build oss-native-modules-specs -p ios` to unbreak stable.
build-break
Differential Revision:
D17171834
Ninja: master broken
fbshipit-source-id: 3eb14e555030dc3cd50ae6f9f946c75710c0f141
Summary:
## Summary
When an exception is raised by Java in a synchronous call from JS to Java, the Java portion of the stack trace is simply ignored when the exception is forwarded to JS. This problem doesn't exist for async calls. I did some digging to figure out why this works with async calls, but not sync calls, to get to the bottom of T52585087.
In T52585087, `TurboModuleRegistry.get<Spec>('I18nAssets')` fails because of a `NullPointerException` in Java. However, since there's no stack trace associated with the `NullPointerException`, it's hard to diagnose the problem.
## Asynchronous calls
ReactNative's NativeModules thread is a background thread on which we schedule asynchronous NativeModule method invocations. We spawn our background threads in Java using the [`MessageQueueThreadImpl`](https://fburl.com/diffusion/u0vdm5k3). Each thread is associated with a queue on which you can schedule some work. These `MessageQueueThread`s have a [C++ API](https://fburl.com/diffusion/596740d8) that we can use to schedule some work from C++.
NativeModule method invocations in JS produce a C++/Java boundry, because our JS executes C++, which executes the Java NativeModule method. But since the method is asynchronous, instead of invoking it immediately, we wrap it in a C++ lambda and use the C++ API of `MessageQueueThread` to schedule it to be invoked later. Therefore, when it actually invokes, we'll get Java invoking C++, which invokes Java.
When the NativeModule method (implemented in Java) throws an exception, fbjni will convert that exception into a `jni::JniException` and start unwinding the C++ stack. Eventually, this exception will reach the outermost Java/C++ boundry. Typically, at this point, the program would crash, but because we used fbjni to register all our native functions, our `jni::JniException` will automatically be converted into a regular Java exception. This exception will be caught by MessageQueueThreadHandler [here](https://fburl.com/diffusion/c4thoca7), and handled using our ExceptionHandler NativeModule.
## Synchronous calls
In synchronous execution, JS uses a `JSI::HostObject` to call the Java method directly. If the Java method throws an error, because we're using fbjni, that Java exception will be converted to a `jni::JniException` object, which will contain the stack tract of the Java object. However, from what I could gather, Hermes doesn't know about `jni::JniException`. So, simply ignore the stack trace associated with the Java exception, understanding only the exception message. Hence, all synchronous calls into Java only display the JS stack trace. What we really want is to build an platform-agnostic abstraction that understands `jni::JniException` (and whatever its analogue is in iOS, if any) and use that to bridge between Native and JS.
## Temporary Solution
We know that when NativeModules are created, we do a synchronous call from JS to C++ to Java. This synchronous call happens when you do a property access on the `NativeModules` object. Therefore, at the very least, to get to the bottom of T52585087, we could log all exceptions that are thrown whenever a NativeModule is created. This should help us get to the bottom of T52585087.
Reviewed By: mdvacca
Differential Revision: D17126667
fbshipit-source-id: a6fb27aaea094b9559939ddcc260d3a2c6e71492
Summary:
We don't need to have those methods virtual anymore.
Nowadays we have a pointer to a `ComponentDescriptor` in `ShadowNode`, it's simpler to get that value from there.
That's also more code-size friendly because we now can remove those methods from a template.
Reviewed By: sammy-SC
Differential Revision: D17142173
fbshipit-source-id: 9f54fbe6c1ebad3ed49843ed75a01ca5ee48c34f
Summary: We merge `events` module into `core` long time ago, so we don't need this anymore.
Reviewed By: mdvacca
Differential Revision: D17142174
fbshipit-source-id: 6e63f7f22dc3d65ed2f9cc3bd4f4776404dfe788
Summary:
We added the accessibilityState property as a more semantically rich way for components to describe information about their state to accessibility services. This PR removes the old accessibilityStates property.
<!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? -->
## Changelog
[General] [Change] - Remove accessibilityStates property.
Pull Request resolved: https://github.com/facebook/react-native/pull/26168
Test Plan: Ensure that RNTester accessibility examples function properly on both iOS and Android.
Differential Revision: D17152891
Pulled By: cpojer
fbshipit-source-id: d71d3cf0f2e0846979d2ba104b6c69e4e5725252
Summary:
This fixes an error when using RN's version of yoga with Flipper.
```
[!] The following Swift pods cannot yet be integrated as static libraries:
The Swift pod `YogaKit` depends upon `Yoga`, which does not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.
```
Taken from https://github.com/facebook/yoga/blob/master/Yoga.podspec#L25
## Changelog
[Internal] [Fixed] - Add `DEFINES_MODULE=YES` to the yoga podspec
Pull Request resolved: https://github.com/facebook/react-native/pull/26276
Reviewed By: priteshrnandgaonkar
Differential Revision: D17149819
Pulled By: axe-fb
fbshipit-source-id: 5060b8e7111ba411f6e26e3479ad4fb55a552b4e
Summary: The progress bar on Android was disabled in D5329011 because of T19653381, but was never enabled again. I spent some time trying to reproduce the issue of the bundle being stuck while loading, but didn't succeed. So let's enable the progress bar and monitor whether people would start seeing this bug again.
Reviewed By: cpojer
Differential Revision: D17148134
fbshipit-source-id: 5130b809460bc743d26a6e88961f81061089fe1d
Summary: Prior to Android P things like setScaleX() allowed passing float values that were bogus such as Float.NaN. If the app is targeting Android P or later then passing these values will result in an exception being thrown. Since JS might still send Float.NaN, we want to keep the code backward compatible and continue using the fallback value if an invalid float is passed. `sanitizeFloatPropertyValue` is an exact copy of the private method with the same name in `android.view.View.java`.
Reviewed By: cpojer
Differential Revision: D17153279
fbshipit-source-id: 036acc4baa6f0b7f206488991b428a84374fa453
Summary:
Since react-native 0.58, I encountered some issues about snapshot with animated components. I opened an issue here : https://github.com/facebook/react-native/issues/25653
After hours of debugging, I finally found the problem:
In RN 0.57, an Animated View was created like this :
`View: AnimatedImplementation.createAnimatedComponent(View)`
And `AnimatedImplementation` was mock like this :
```
mock('../Libraries/Animated/src/AnimatedImplementation', () => {
const AnimatedImplementation = jest.requireActual('../Libraries/Animated/src/AnimatedImplementation');
const oldCreate = AnimatedImplementation.createAnimatedComponent;
AnimatedImplementation.createAnimatedComponent = function(
Component,
defaultProps,
) {
const Wrapped = oldCreate(Component, defaultProps);
Wrapped.__skipSetNativeProps_FOR_TESTS_ONLY = true;
return Wrapped;
};
return AnimatedImplementation;
})
```
So thanks to this mock, the animated component had a props `__skipSetNativeProps_FOR_TESTS_ONLY` set to `true` and this was used to forceUpdate the component
```
if (AnimatedComponent.__skipSetNativeProps_FOR_TESTS_ONLY || typeof this._component.setNativeProps !== 'function') {
this.forceUpdate();
}
```
But since RN 0.58, the way we create an animated component as changed into :
```
const View = require('View');
const createAnimatedComponent = require('createAnimatedComponent');
module.exports = createAnimatedComponent(View);
```
As you can see, we directly use `createAnimatedComponent`, we don't use it through `AnimatedComponent` like before.
This caused the animated component had not anymore the props `__skipSetNativeProps_FOR_TESTS_ONLY`, so the component doesn't forceUpdate during the animation and breaks the snapshot.
Mocking `createAnimatedComponent` fix the problem
## Changelog
[General] [Fixed] - Mock createAnimatedComponent to fix snapshot with animated component
Pull Request resolved: https://github.com/facebook/react-native/pull/26109
Test Plan: See the issue
Differential Revision: D17155134
Pulled By: cpojer
fbshipit-source-id: 892efc7e820e3db4eb670ddec8fcbf7702bb69bf
Summary:
Android implementation of the Appearance native module. Exposes the user's preferred color scheme: "dark" for Night theme ON, "light" for Night theme OFF.
Emits a `appearanceChanged` event when the current uiMode configuration changes.
To make your app handle Night mode changes, make sure to do the following:
* Declare your Activity can handle uiMode configuration changes (https://developer.android.com/preview/features/darktheme#java):
```
android:configChanges="uiMode"
```
* Make sure to pass the configuration changed activity lifecycle callback from your ReactActivity:
```
Override
protected void onConfigurationChanged(Configuration newConfig) {
super.onConfigurationChanged();
if (mReactInstanceManager != null) {
mReactInstanceManager.onConfigurationChanged(newConfig);
}
}
```
### RNTester
Adds the AppearanceExample to RNTester on Android.
Changelog:
[Android][Added] - New Appearance module exposes the user's current Night theme preference
Reviewed By: makovkastar
Differential Revision: D16942161
fbshipit-source-id: d24a8ff800a1c5f70f4efdec6891396c2078067e
Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/26143
A new useColorScheme hook is provided as the preferred way of accessing the user's preferred color scheme (aka Dark Mode).
Changelog:
[General] [Added] - useColorScheme hook
Reviewed By: yungsters
Differential Revision: D16860954
fbshipit-source-id: 8a2b6c2624ed7cf431ab331158bc5456cde1f185
Summary: Export font weight type to use in typing
Reviewed By: panagosg7
Differential Revision: D17128236
fbshipit-source-id: baf9d5e5c5fa0b8aad4cf29ea94430adfe1e8b5f
Summary:
Initial conversion of RNTester to support light and dark themes. Theming is implemented by providing the desired color theme via context. Example:
```
const ThemedContainer = props => (
<RNTesterThemeContext.Consumer>
{theme => {
return (
<View
style={{
paddingHorizontal: 8,
paddingVertical: 16,
backgroundColor: theme.SystemBackgroundColor,
}}>
{props.children}
</View>
);
}}
</RNTesterThemeContext.Consumer>
);
```
As RNTester's design follows the base iOS system appearance, I've chosen light and dark themes based on the actual iOS 13 semantic colors. The themes are RNTester-specific, however, and we'd expect individual apps to build their own color palettes.
## Examples
The new Appearance Examples screen demonstrates how context can be used to force a theme. It also displays the list of colors in each RNTester theme.
https://pxl.cl/HmzW (screenshot: Appearance Examples screen on RNTester with Dark Mode enabled. Displays useColorScheme hook, and context examples.)
https://pxl.cl/HmB3 (screenshot: Same screen, with light and dark RNTester themes visible)
Theming support in this diff mostly focused on the main screen and the Dark Mode examples screen. This required updating the components used by most of the examples, as you can see in this Image example:
https://pxl.cl/H0Hv (screenshot: Image Examples screen in Dark Mode theme)
Note that I have yet to go through every single example screen to update it. There's individual cases, such as the FlatList example screen, that are not fully converted to use a dark theme when appropriate. This can be taken care later as it's non-blocking.
Reviewed By: zackargyle
Differential Revision: D16681909
fbshipit-source-id: e47484d4b3f0963ef0cc3d8aff8ce3e9051ddbae
Summary: Flow type for AndroidTextInput. This could theoretically be used for the interface codegen in the future, and I did use this to codegen the scaffolding for AndroidTextInput (see previous diffs).
Reviewed By: mdvacca
Differential Revision: D16926831
fbshipit-source-id: d01c2e041efb4151f6091dd0fea191989d133881
Summary:
The type is wrong in the constructor.
build-break
Differential Revision: D17145039
fbshipit-source-id: f6b80e38c05e60f04d029aa34baa0c55c237a39a
Summary: Support existing, backwards-compatible AndroidTextInput component for minimal support of TextInput on Android.
Reviewed By: shergin, mdvacca
Differential Revision: D17086758
fbshipit-source-id: 25726f22229e0d5dfe96eb36b386a5317601283d
Summary: Support for `sendAccessibilityEvent` in the FabricUIManager.
Reviewed By: shergin
Differential Revision: D17142507
fbshipit-source-id: 5c131d7caa1e4189fd41ecfb558d0027394b6a15
Summary:
The purpose of `EventBeat` is handling an asynchronous callback to itself which is being delivered on some different thread. That brings a challenge of ensuring that the `EventBeat` object stays valid during the timeframe of callback execution. The concept of Owner helps with that.
The owner is a shared pointer that retains (probably indirectly) the `EventBeat` object. To ensure the correctness of the call, `EventBeat` retains the owner (practically creating a retain cycle) during executing the callback. In case if the pointer to the owner already null, `EventBeat` skips executing the callback.
It's impossible to retain itself directly or refer to the shared pointer to itself from a constructor. `OwnerBox` is designed to work around this issue; it allows to store the pointer later, right after the creation of some other object that owns an `EventBeat`.
Reviewed By: JoshuaGross
Differential Revision: D17128549
fbshipit-source-id: 7ed34fd865430975157fd362f51c4a3d64214430
Summary:
Seems it's more logical and safe to store ShadowTreeRegistry in UIManager than in Scheduler. We also probably will move some functionality dealing with the registry to UIManager.
But most importantly we need it to manage the ownership properly. UIManager might overlive Scheduler and still get a call to process a shadow tree. In the current configuration, it causes a crash because the registry owns by Scheduler. Moving that to UIManager solves that.
Reviewed By: JoshuaGross
Differential Revision: D17128550
fbshipit-source-id: e6735acaa11f2ed82ca17f18a45e389d79aa1a08
Summary:
This diff changes the way how `UIManager`, `UIManagerBindging`, `Scheduler` and `EventDisaptcher` refer to each other which should help with stability and reliability.
Here is the node that describes the details:
# Retaining dilemma
# Players
We have many logical of moving pieces but most of them can be abstracted by following high-level components.
* **Scheduler**
`Scheduler` is the main representation of running React Native infrastructure. Creation of it means the creation of all React Native C++ subsystems (excluding RuntimeExecutor) and destruction of that means the destruction of all dependent parts. Both processes must be thread-safe.
* **UIManager**
UIManager is a module that contains the most high-level logic of managing shadow trees. All React Renderer calls are practically implemented there.
* **UIManagerBinding**
UIManagerBinding is a representation (aka `HostObject`) of UIManager in the JavaScript world.
* **EventDispatcher**
EventDispatcher is a class that implements all logic related to dispatching events: from calling event on any thread anywhere to executing a particular JavaScript handler responsible for handling that event.
Instances of those classes have complex relationships in terms of owning each other, order of creation and destruction. The configuration of these relationships is dictated by a set of constraints that those classes need to satisfy to be constructed, accessed, and destructed in a hostile multithreaded environment. Messing with that can cause deadlocks, random crashes, suboptimal performance or memory leaks. Make sure you consider all constraints and requirements before changing that.
# Goal
We need to have a safe and reliable way to construct and destroy those objects (on any thread, in any random moment). Keep in mind that all of those objects are being accessed from random threads and have random states in any particular moment. Switching threads happens all the time, so having some state in one place does not guarantee any state in other places.
# Caveats
Let's discuss all concrete constrains that the moving pieces have to satisfy.
* **UIManagerBinding is a HostObject**
Practically that means:
1. It must be constructed "on JavaScript thread" (with exclusive access to JavaScript VM);
2. It must not be retained by other parts of the system because overliving the VM will cause a crash.
3. It can be destructed on any thread (VM does not give any guarantees here). The particular configuration guarantees that the destruction cannot be run concurrently with any JS execution though (because we never clear the reference to the host object from JavaScript side).
* **UIManager needs to be connected with UIManagerBinding and vice-versa**
Those to modules call each other to perform some UI updates or deliver events.
* **Scheduler can be deallocated on any thread at any time**
Timing and thread are up to the application side. The Scheduler must be resilient to that.
* **EventDispatcher can call UIManager at any time**
Luckily, that happens only on JavaScript thread.
* **Using weak pointers cames at a cost**
`std::weak_ptr` is a concept for managing the non-owning relationships in a safe manner. Dereferencing such pointers cames at a cost (additional object construction and atomic counters bumps). So, we should use that carefully; we cannot use shared and week pointers everywhere and assume that will work magically.
# How does this blow up?
Without describing the current configuration, here are a variety of cases that we currently observe.
1. `Scheduler` was deallocated and destroyed UIManager but VM is still running. The VM calls the UIManagerBinding, UIManagerBinding calls a method on already deallocated UIManager. Boom.
2. VM is being deallocated and deletes all host object as part of this process. Some UI event is sill in flight on some thread. The event retains UIManagerBinding via UIManager. VM cannot destroy UIManagerBinding because it's being retained. Boom.
3. VM was deallocated. `Scheduler` was deallocated. But some native state update is still in flight. It retains EventDispatcher and eventually trying to access some shadow tree that was retained by Scheduler and already dead. Boom.
That's pretty much routine endless nightmare of any low-level framework. Luckily, the good proper decisions (and iterating on that!) can solve that.
# Proposed configuration
The configuration is based on those ideas:
1. Never retain `UIManagerBinging`.
2. Never recreate `UIManagerBinging`. Create once and load lazily from JS environment on demand.
3. Consider UIManager as an object with shared ownership between JS and native. That object must be able to overlive native infra or JS VM.
4. Use EventDispatcher as a single weak representation of the JavaScript world; Never retain it strongly except by the Scheduler.
5. `UIManagerBinging` and `UIManager` can be attached or detached. `UIManagerBinging` retains `UIManager`, `UIManager` does not retain `UIManagerBinging` back. Destroying `UIManagerBinging` nulls the raw pointer to that from `UIManager`.
6. All calls from native to JavaScript can validate the pointer from `UIManager` to `UIManagerBinging` to check that the call is possible. All that calls happen on JavaScript thread.
## Stages
* **Creation process**
Creation Scheduler creates `UIManager` and scheduler asynchronous call to JavaScript to create `UIManagerBinding` and attach them. At the same time `Scheduler` creates `EventDispatcher` and makes it retains `UIManager`.
* **JavaScript-to-native invocation**
`UIManagerBinding` has a shared pointer to `UIManager` and can cheaply and safely verify that the pointer is not nullptr. Any mutation of this pointer happens on the JavaScript thread or effectively on VM destruction (non-concurrently).
* **Native-to-JavaScript invocation**
The invocation starts from retaining `EventDispatcher` (converting a weak pointer to strong one), that retains `UIManager`. Later, on the JavaScript thread, `UIManager` checks the raw pointer to `UIManagerBinding` to verify that the call can be performed safely.
# Easy ways to break the fragile balance
- Never retain `EventDispatcher` as a shared pointer. That causes a leak of UIManager and associated resources.
- Access a shared pointer to `UIManager` by value only. The simple way to break that is to specify `[=]` capture block for a lambda and access an instance variable pointing to a `UIManager` (that does not retain the pointer; make a copy on the stack and copy that to the lambda).
Reviewed By: JoshuaGross
Differential Revision: D17120333
fbshipit-source-id: 83138657683e91ceb2f48f18f30e745199c83e82
Valentin Shergin
Dulmandakh Sukhbaatar
Rick Hanlon
Zihan Chen (MSFT)
Héctor Ramos
Frieder Bluemle
Oleg Lokhvitsky
Oleksandr Melnykov
Kevin Gozali
David Vacca
Ashok Menon
Joshua Gross
Kudo Chien
Logan Daniels
Ram N
Mehdi Mulani
Ramanpreet Nara
Marc Mulcahy
Janic Duplessis
Joan Saum
Héctor Ramos
Itamar Ostricher
--fbcode@fb.com