Commit Graph

434 Commits

Author SHA1 Message Date
Emily Janzer 3963c7aa56 Clean up unused functions on ReactContext
Summary: Removing the methods I recently added for storing/retrieving an instance key on a ReactContext.

Reviewed By: PeteTheHeat, mdvacca

Differential Revision: D18710637

fbshipit-source-id: d34683ec660bd999db8112865e15392606fc9237
2019-12-03 11:33:00 -08:00
David Vacca 4ad852c137 Add extra logging in the Bridge exception handling
Summary:
This diff adds extra logging in the method that handles exceptions for RN Android
Changelog: internal

Reviewed By: JoshuaGross

Differential Revision: D18694123

fbshipit-source-id: e275445b65473ed55eec9d4b823938e32fa805e5
2019-11-27 15:56:52 -08:00
Ramanpreet Nara e69be0ae55 Give ReactContextBaseJavaModule a 0 arg ctor
Summary:
We need to make our ExceptionsManagers into TurboModules. To do so, we need to first make them type-safe. This requires extending the base class code-generated from the NativeModule JavaScript spec. That base class extends `ReactContextBaseJavaModule`, which requires `ReactApplicationContext` to instantiate. Unfortunatley, our ExceptionsManagers need to be created before the `ReactInstanceManager`. This means that the `ReactApplicationContext` isn't available by the time they're created.

In this diff, I make the `ReactContextBaseJavaModule` have a 0 argument constructor that simply sets its ReactApplicationContext to null. This will allow us to eventually migrate our ExceptionsManagers to the TurboModule system.

Changelog:
[Android][Added] - Give ReactContextBaseJavaModule a 0 arg ctor

Reviewed By: mdvacca

Differential Revision: D18717908

fbshipit-source-id: 203ffc49f2ec0b32a809402801795879d3b3a64b
2019-11-27 10:38:54 -08:00
Ramanpreet Nara 29b99720b2 Make TurboModuleRegistry in CatalystInstanceImpl.java volatile
Summary:
We read from / write to `CatalystInstanceImpl.mTurboModuleRegistry` from multiple threads. To ensure that we directly read/write from memory, and not a stale cache, we should make this variable `volatile`.

Changelog:
[Android][Fixed] Make TurboModuleRegistry in CatalystInstanceImpl.java volatile

Reviewed By: fkgozali

Differential Revision: D18542954

fbshipit-source-id: 0a47f05e0653b4f7f58503c678bdf31c68eab9bf
2019-11-15 17:31:31 -08:00
Joshua Gross cc6b430c3a Annotate UIManager methods to document thread semantics
Summary:
Document which methods can be called on UI thread or ANY thread.

In the future we should see if we can use only `ThreadConfined` or the AndroidX annotations instead of using both / choosing between them at each site.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D18532542

fbshipit-source-id: 3b5406ea5035615a0ebf83484bf8ec0747a6b6f7
2019-11-15 17:01:02 -08:00
Joshua Gross ce226c1f28 Fix T54997838
Summary:
Fixes T54997838 by preventing any view mutations during `onMeasure` calls.

There might still be places where this is possible, but this is where I'm seeing all the crashes currently.

See comments in ReactRootView for why views were mutated during onMeasure.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D18518591

fbshipit-source-id: 1406af8a6b0bfcc86f4cc5b451b3967f312dfd85
2019-11-14 21:23:32 -08:00
Emily Janzer 6598292ede Add a field + getter for instanceKey to ReactContext
Summary:
Adding a new String field for `instanceKey` to ReactContext, which is set via a new constructor on ReactApplicationContext. Also adding getters to ReactContext and ThemedReactContext so that it's accessible from any instance/subclass of ReactContext.

This will only be used in bridgeless mode.

Reviewed By: mdvacca

Differential Revision: D18316556

fbshipit-source-id: 9757da72fde4ba36034c1e129326461fed496229
2019-11-05 19:01:53 -08:00
Joshua Gross b1b97b8b45 UiThreadUtil.assertX should only throw in DEBUG mode
Summary:
Turns out that `SoftAssertions.java` has always been a lie - it actually always throws exceptions. Migrate it to using `ReactSoftException`.

This file hasn't been touched at all since it was originally open-sourced, besides codemods!

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D18336020

fbshipit-source-id: cba3db25a9f9d61325dd3f7843e92e984ae56281
2019-11-05 15:36:29 -08:00
Ramanpreet Nara 56ad1bd38a Assert TurboModuleRegistry is not null
Summary:
Looking at the crash reports from T46487253:
1. This crash happens only with TurboModule-compatible NativeModules.
2. Users who experience this crash are in the TurboModules test group.

Therefore, the crash happens while trying to load TurboModules.

The stack trace of the crash includes [this lookup via the NativeModule system](https://fburl.com/diffusion/vxj9goz5). When TurboModules are enabled, we can only start executing this line if one of two things are true:
1. The TurboModuleRegistry is null in CatalystInstanceImpl.
2. The TurboModuleRegistry isn't null but the NativeModule returned by the TurboModuleRegistry is null.

We can protect against 1 by asserting that when `ReactFeatureFlags.useTurboModules` is `true`, `mTurboModuleRegistry` is not null. Once this check lands, unless there's a race with setting `ReactFeatureFlags.useTurboModules`, we should be able to rule out 1.

Changelog:
[Added][Android] - Assert TurboModuleRegistry isn't null before using it in CatalystInstanceImpl

Reviewed By: PeteTheHeat

Differential Revision: D18211935

fbshipit-source-id: de88c033425c474ef80b73386b7182b1d3bb382f
2019-11-01 19:24:09 -07:00
Joshua Gross 426868b6c2 Add enable_nullify_catalyst_instance_on_destroy MC and gate setting mCatalystInstance to null in ReactContext
Summary:
Mostly for easing open-source migration and not making a backwards-incompatible change (yet), we'll set this to false by default. Every app can opt-in to this if wanted but it's not necessary. This change is part of experiments surrounding more-aggressive teardown for Fabric and Bridgeless mode.

Changelog: [Internal] - This has the effect of (by default) disabling the previous diff which caused ReactContext teardown to always set mCatalystInstance to null. Now that is opt-in behavior and off by default, so it's not longer a breaking change.

Reviewed By: mdvacca

Differential Revision: D18207302

fbshipit-source-id: 7acfc894415e966f652c7049849eef79c440a135
2019-10-29 16:21:12 -07:00
Joshua Gross 9446277fc1 Simplify API of getReactApplicationContextIfActiveOrWarn
Summary:
Simplify the API of `getReactApplicationContextIfActiveOrWarn`. We don't need to pass so much information into this method to collect good SoftExceptions.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D18134400

fbshipit-source-id: 0a250ab0252a44121f3339a31506a0a6c4c7cd35
2019-10-25 16:16:00 -07:00
Joshua Gross 94cb4bf90c In modules or classes that call ReactApplicationContext.getJSModule, ensure that there's still a CatalystInstance alive
Summary:
In D18032458 we introduce getReactApplicationContextIfActiveOrWarn. In this diff, modules that access a JS or Native module through ReactApplicationContext need to check if the CatalystInstance is still alive before continuing.

Modules that don't derive from `ReactContextBaseJavaModule` manually check for the catalyst impl and log their own SoftExceptions.

In this diff we also introduce SoftExceptions that by contract never cause crashes, even in debug mode.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D18112989

fbshipit-source-id: 868f01f388aa2db3518db9f873f2afc2a62eed45
2019-10-24 17:29:08 -07:00
Joshua Gross b12a29cfcb Implement getReactApplicationContextIfActiveOrWarn in ReactContextBaseJavaModule to generalize checking hasActiveCatalystInstance and warning when it's dead
Summary:
In three previous diffs (D18020359 D17998627 D17969056), I implemented this logic in three different modules. There are potentially hundreds of modules where we should be implementing this check, so I'm moving the important logic into ReactContextBaseJavaModule.

Additionally, `WebSocketModule` was retaining its own copy of ReactApplicationContext instead of using the built-in `getReactApplicationContext`, so I removed that ivar from `WebSocketModule`.

Changelog:
[Internal]

Reviewed By: mdvacca

Differential Revision: D18032458

fbshipit-source-id: 9114120d3b80334df8d2e0813e36d21c667fc1bd
2019-10-21 15:59:21 -07:00
Joshua Gross 1e9d4cde4b Wait until everything is destroyed before returning from CatalystInstanceImpl.destroy
Summary:
CatalystInstanceImpl.destroy does a bunch of stuff in each of the relevant threads (Native Module thread, JS thread, UI thread).

This change creates a V1 destroy method (unchanged) and a V2 destroy method. The goal is to resolve (and catch!) race conditions in native modules and JSI modules that could occur during teardown; and mitigate race conditions that occur in RN teardown, like deallocation of C++ objects (scheduler, JS VM, and UIManager for Fabric).

Changelog:
[Internal] Experiment to fix deallocation race conditions

Reviewed By: mdvacca

Differential Revision: D18001677

fbshipit-source-id: 5955da0a7b726491c7d749642475f0fba74cce5a
2019-10-19 02:30:09 -07:00
Joshua Gross 95a43f3366 Set mCatalystInstance to null when destroying ReactContext
Summary:
We want ReactContext to blow up if it tries to use `mCatalystInstance` after `destroy` is called.

Changelog:
[[Internal]]

Reviewed By: mdvacca

Differential Revision: D17944723

fbshipit-source-id: cfe8a8b98473f53dd68bbcd91a71f58bd7a0c503
2019-10-19 02:30:08 -07:00
Joshua Gross 526a010ad7 Add ThreadConfined(UI) to CatalystInstanceImpl.destroy
Summary:
This already has an assert that `destroy` should only be called on the UI thread. Add an annotation.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D17989113

fbshipit-source-id: fd44f321cbcb7f0a18383ca6226cce72e5991eea
2019-10-18 15:08:53 -07:00
Andres Suarez 3b31e69e28 Tidy up license headers [2/n]
Summary: Changelog: [General] [Fixed] - License header cleanup

Reviewed By: yungsters

Differential Revision: D17952694

fbshipit-source-id: 17c87de7ebb271fa2ac8d00af72a4d1addef8bd0
2019-10-16 10:06:34 -07:00
Andres Suarez 722feeb02b Tidy up license headers [1/n]
Summary: Changelog: [General] [Fixed] - License header cleanup

Reviewed By: yungsters

Differential Revision: D17952695

fbshipit-source-id: 81aa607612ba1357ef7814ef20371335151afe7e
2019-10-16 10:06:33 -07:00
Joshua Gross a440613654 Add ThreadConfined(UI) annotations to relevant methods in ReactContext
Summary:
Add ThreadConfined(UI) annotations to relevant methods in ReactContext.

Changelog:
[[Internal]]

Reviewed By: mdvacca

Differential Revision: D17944686

fbshipit-source-id: ce93c6ffa2a523532cbe709650054638412da59e
2019-10-16 00:19:34 -07:00
Emily Janzer b5ea49f132 Move sDidInit to the end of ReactBridge.staticInit()
Summary: Oleksandr made the point that making `sDidInit` volatile isn't quite enough because the flag is set at the beginning of the function; it's possible that it will return true while it's still in the process of initializing the bridge. Moving where we set the flag to the end of the function should address the issue.

Reviewed By: JoshuaGross

Differential Revision: D17770128

fbshipit-source-id: 695d3edc582d9dce1884d8698d400dd147ca5cae
2019-10-07 16:23:36 -07:00
Emily Janzer c3a07b6dcc Use volatile for sDidInit in ReactBridge
Summary: In D17747958 I added a function to check the value of a static variable, but forgot to make sure it was accessed in a thread-safe manner. Adding `volatile` to the flag

Reviewed By: makovkastar

Differential Revision: D17763888

fbshipit-source-id: f227b69de1a0df6493424da3b276529555999f70
2019-10-04 11:00:08 -07:00
Emily Janzer 7d8b7ff3aa Don't call ReactBridge.staticInit() on the main thread
Summary: Adding a hook to check if ReactBridge is initialized so we can avoid loading the .so on the main thread.

Reviewed By: fkgozali

Differential Revision: D17747958

fbshipit-source-id: 5969afa57dc1b446c03bc68eaa9e1385fe17c461
2019-10-03 16:21:28 -07:00
masaaki1915 231d2f95cd fix typo in comment about package (#26638)
Summary:
This PR only fixes a typo in `ModuleHolder.java`.

## Changelog

[Internal] [Fixed] - Fix typo in comment about package
Pull Request resolved: https://github.com/facebook/react-native/pull/26638

Differential Revision: D17661204

Pulled By: cpojer

fbshipit-source-id: 77ab92b7bfff300961e0a32183188a6e57e99b3d
2019-09-29 20:49:51 -07:00
Petter Hesselberg 4b9350061f Petterh/support parcelable array args (#26379)
Summary:
`ReactRootView.startReactApplication` takes a `Bundle` argument called `initialProperties`. This is translated to a `ReadableMap` via `Arguments.fromBundle`. If the bundle contains an array of bundles, this gets translated by `Arguments.fromArray`.

If the bundle was passed from one activity to another via intent extras, however, there is a problem. After the bundle has been marshaled and unmarshaled, the array of `Bundle`s come out the other end as an arrap of `Parcelable`s, although each array element remains a `Bundle`. This results in an "Unknown array type" exception.

This PR fixes this by adding support for `Parcelable` arrays – provided they contain only members of type `Bundle`.

## Changelog

[Android] [Fixed] - Don't throw "Unknown array type" exception when passing serialized bundle arrays in ReactRootView.startReactApplication's initialProperties parameter
Pull Request resolved: https://github.com/facebook/react-native/pull/26379

Test Plan: Added test class `ArgumentsTest`. The test method `testFromMarshaledBundle` fails when used on the old version of `Arguments`.

Differential Revision: D17661203

Pulled By: cpojer

fbshipit-source-id: 63612d78f49bdf9cc53f6f21ae883dba6cebce84
2019-09-29 19:22:22 -07:00
Ramanpreet Nara 42dcfab2a9 Stop requiring TurboModuleManager JSIModule on CatalystInstance cleanup
Summary:
In `CatalystInstanceImpl.destroy()`, we require the TurboModuleManager using the [following lines](https://fburl.com/diffusion/a4y6wbft):

```
final JSIModule turboModuleManager =
    ReactFeatureFlags.useTurboModules
        ? mJSIModuleRegistry.getModule(JSIModuleType.TurboModuleManager)
        : null;
```

For some strange reason, even though `ReactFeatureFlags.useTurboModules` is true, the TurboModuleManager isn't registered with mJSIModuleRegistry. I spent some time looking through the code, but I couldn't figure out why. These lines actually aren't necessary, so it's possible to fix the issue by simply working around it, which is what this diff does. We shouldn't have been double requiring the TurboModuleManager anyways, since `CatalystInstance.java` has a method to set the TurboModuleManager, which we call in `ReactInstanceManager.createReactContext`.

## Alternative approach
I could push this diff to the next cut, and instead land a diff that adds debug information to the native crash. At the cost of a week, it may help us figure out why we're seeing the crash. Thoughts? cc fkgozali

Reviewed By: fkgozali

Differential Revision: D17636604

fbshipit-source-id: ecfff593dc6eb4ec4d5e331348b308bc7ab37966
2019-09-27 18:20:28 -07:00
Ramanpreet Nara 77fe4f087d Introduce CatalystInstance.getNativeCallInvokerHolder()
Summary: CatalystInstanceImpl is responsible for creating the NativeModules thread. We therefore expose a method `getNativeCallInvokerHolder()` on this hybrid class to create and give us access to the `CallInvokerHolder` for the NativeModules thread.

Reviewed By: PeteTheHeat

Differential Revision: D17422164

fbshipit-source-id: 316423847518124115643549fa73a8533d493cd0
2019-09-20 10:52:57 -07:00
Ramanpreet Nara 4c998fd05d Rename JSCallInvoker{,Holder} to CallInvoker{,Holder}
Summary:
## Motivation
The concept behind JSCallInvoker doesn't necessarily have to apply only to the JS thread. On Android, we need to re-use this abstraction to allow execution of async method calls on the NativeModules thread.

Reviewed By: PeteTheHeat

Differential Revision: D17377313

fbshipit-source-id: 3d9075cbfce0b908d800a366947cfd16a3013d1c
2019-09-20 10:52:56 -07:00
Joshua Gross 15b38958b0 Log soft exceptions but don't crash when viewState is missing for commands or view deletion
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
2019-09-03 21:45:49 -07:00
Ramanpreet Nara 7756114250 Print stacktraces of exceptions thrown when creating NativeModules
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
2019-09-03 13:58:52 -07:00
Joshua Gross 898124541c Support sendAccessibilityEvent in Fabric
Summary: Support for `sendAccessibilityEvent` in the FabricUIManager.

Reviewed By: shergin

Differential Revision: D17142507

fbshipit-source-id: 5c131d7caa1e4189fd41ecfb558d0027394b6a15
2019-08-30 19:04:14 -07:00
Christoph Nakazawa bb625e5238 Remove the native delta client from Android
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
2019-08-16 10:02:46 -07:00
Joshua Gross 751d0e75b0 Fabric PerfLogger: prevent ConcurrentModificationException
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
2019-08-05 15:38:04 -07:00
Ramanpreet Nara bc7c85f153 Delete jsi::Functions before jsi::Runtime gets deleted
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
2019-08-02 17:08:19 -07:00
David Vacca 470ace0932 Expose JSResponderHandler API in Fabric UIManager
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
2019-08-02 16:38:44 -07:00
Eric Williamson 6bc0c108eb Revert D16589168: [RN][TurboModule] Delete jsi::Functions before jsi::Runtime gets deleted
Differential Revision:
D16589168

Original commit changeset: a1c0786999c2

fbshipit-source-id: 8048d62e958c0b58aface00dae8447b8c2d5d2dc
2019-08-01 21:59:45 -07:00
Ramanpreet Nara 2a8c188701 Delete jsi::Functions before jsi::Runtime gets deleted
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
2019-08-01 16:36:47 -07:00
Joshua Gross c075a240cd Granularly track perf of Fabric
Summary: Enable granular perf measurements under Fabric.

Reviewed By: mdvacca

Differential Revision: D16021797

fbshipit-source-id: c25a8f7cebf53abfcfc39c8f6d50900813214abb
2019-07-20 01:57:20 -07:00
Emily Janzer 5f8c129f19 Remove unused removeRootView() method from FabricUIManager
Summary:
FabricUIManager.removeRootView() isn't currently used, removing it from the UIManager interface.

It looks like this is called from JS in paper renderers, but not Fabric, so we should be good to delete it.

Reviewed By: shergin, mdvacca

Differential Revision: D16275118

fbshipit-source-id: b8f3ae1dc7574ce17d8cc9e7fee72ef5dcc9b323
2019-07-17 11:05:33 -07:00
Emily Janzer 83969c26fb Decouple GuardedRunnable + friends from ReactContext
Summary:
Step 1 in removing the dependency on ReactContext from GuardedRunnable and other related classes. These are extensively by native modules and view managers, so in order to remove the bridge dependency from those modules we'll need to first decouple these classes from ReactContext. It turns out they only need ReactContext for its handleException method, which delegates out to product code. For backwards compatibility I'm exposing another NativeModuleExceptionManager in ReactContext that simply wraps its handleException method (since this interface already does everything we need).

I figured I'd keep around an extra constructor that still uses ReactContext for now instead of trying to migrate everything over at once.

Reviewed By: makovkastar

Differential Revision: D16270995

fbshipit-source-id: c9a8714bea7ac2a98e78234a0bae49140c00980d
2019-07-16 14:05:19 -07:00
David Vacca 1914d9a4c0 Use AndroidX GuardedBy annotation in favor of Javax GuardedBy annotation
Summary: Use AndroidX GuardedBy annotation in favor of Javax GuardedBy annotation

Reviewed By: ejanzer

Differential Revision: D16234167

fbshipit-source-id: 7f818d20b332a866926f80275b4c8a7489d4c6d3
2019-07-12 18:51:39 -07:00
Ram N 6c362a7b19 Enabling Sampling Profiler for all apps via Dev Menu
Reviewed By: makovkastar

Differential Revision: D16141959

fbshipit-source-id: 3a9964961a6af4bc7d4650526031db564ec2dd27
2019-07-11 23:32:25 -07:00
David Vacca aa5edca0e2 Migrate Nullable and NonNull annotations to AndroidX
Summary:
This diff migrates the usages Nullable and NonNull annotations to AndroidX instead of javax.

The purpose of this change is to bring consistency in the annotations used by the core of RN

Reviewed By: makovkastar

Differential Revision: D16054504

fbshipit-source-id: 21d888854da088d2a14615a90d4dc058e5286b91
2019-07-11 16:23:29 -07:00
Moti Zilberman 1a2937151b Make writable arrays and maps only shallowly writable
Summary:
@public

The `WritableArray` and `WritableMap` interfaces currently require that nested arrays and maps also be writable. Nothing in our code actually relies on this, so we can relax this restriction and get useful properties.

For instance, it is now possible to construct a `JavaOnlyMap` (or array) that reuses `ReadableMap` and `ReadableArray` values by reference ( = structural sharing) instead of forcing a deep copy.

Reviewed By: kathryngray

Differential Revision: D16132580

fbshipit-source-id: 9f41189ebea2a82e775a7a4da8c357a5ce9c5b9d
2019-07-08 13:25:02 -07:00
Moti Zilberman e0ae655787 Remove JsonWriter, support bridge types in JsonWriterHelper
Summary:
@public

* Removes `JsonWriter`; it's apparently a buggy fork of [`android.util.JsonWriter`](https://developer.android.com/reference/android/util/JsonWriter) which has existed since API level 11. Our version doesn't insert commas before objects or arrays within an array. Instead of fixing it, we can just use the Android one.
* Extends `JsonWriterHelper` to support serialising `ReadableMap`, `ReadableArray` and `Dynamic` values into a `JsonWriter`.

Reviewed By: kathryngray

Differential Revision: D16131713

fbshipit-source-id: d258af42b669f10218cae8b086e7adc3226d16c0
2019-07-08 13:25:01 -07:00
Oleksandr Melnykov cd05a85fe5 Fix Javadocs broken by google-java-format
Summary: After we ran google-java-format D16071725, some Javadocs which weren't properly written broke. This includes putting unordered and ordered lists not using <ul> and <ol>, putting code blocks and pseudo-graphics not using <pre>. I ran through all the changed classes and tried to fix the broken Javadocs.

Reviewed By: cpojer

Differential Revision: D16090087

fbshipit-source-id: f31971cbc0e367a04814ff90bbfb2192751d5e16
2019-07-02 09:39:21 -07:00
Oleksandr Melnykov 6c0f73b322 Format Java code in xplat/js/react-native-github
Summary:
This diff formats the Java class files inside xplat/js/react-native-github. Since google-java-format was enabled in D16071401 we want to codemode the existing code so that users don't have to deal with formatter lint noise at diff-time.

```arc f --paths-cmd 'hg files -I "**/*.java"'```

drop-conflicts

Reviewed By: cpojer

Differential Revision: D16071725

fbshipit-source-id: fc6e3852e45742c109f0c5ac4065d64201c74204
2019-07-02 04:16:46 -07:00
Eli White 3cae6fa950 RN Android: Support View Manager Commands that are strings
Summary:
Right now JS triggers a view manager command with the following code:

```
UIManager.dispatchViewManagerCommand(
  ReactNative.findNodeHandle(this),
  UIManager.getViewManagerConfig('RCTView').Commands.hotspotUpdate,
  [destX || 0, destY || 0],
);
```

As we want to get rid of calls to UIManager, we need to stop looking for the integer defined in native from JavaScript. We will be changing methods like this to be:

```
UIManager.dispatchViewManagerCommand(
  ReactNative.findNodeHandle(this),
  'hotspotUpdate',
  [destX || 0, destY || 0],
);
```

We need to support ints and Strings to be backwards compatible, but ints will be deprecated.

Reviewed By: shergin

Differential Revision: D15955444

fbshipit-source-id: d1c488975ae03404f8f851a7035b58a90ed34163
2019-06-24 18:47:16 -07:00
Kevin Gozali e6f28bb4f7 Android TM: create TurboModuleManager earlier
Summary: Some TM lookup from native will fail assertion if done too early, because TM Manager is not initialized yet.

Reviewed By: mdvacca

Differential Revision: D15872776

fbshipit-source-id: 7616c1424816f73a45aa1d9723e7807ae10392a7
2019-06-18 16:22:17 -07:00
Kevin Gozali 2df90738d5 Android: Use enum type for looking up JSIModule's
Summary:
To avoid unnecessary class loads, and better modularity, let's use string keys (enum) to access JSIModule's. For now all JSIModule's are all known inside the core infra (only FabricUIManager and TurboModuleManager right now), so let's keep it simple and explicitly list them out.

The only problem here is we lose some form of type safety...

Reviewed By: JoshuaGross

Differential Revision: D15872777

fbshipit-source-id: 9c2de7ef1e88ef3a6dff5888d644f9d8963af2a3
2019-06-18 16:22:17 -07:00
Luis Miguel Alvarado 3bf068ac93 Fix a typo in BaseJavaModule (#25245)
Summary:
This PR only fixes a one small orthographic error
Pull Request resolved: https://github.com/facebook/react-native/pull/25245

Differential Revision: D15796478

Pulled By: hramos

fbshipit-source-id: ccb811b43b0d2efc5d97ba335b60531a0fcbda10
2019-06-12 18:22:59 -07:00