Commit Graph
1695 Commits
Author SHA1 Message Date
Joshua Gross 5abe5843e2 Add C++ AndroidTextInput component for backwards-compatible Fabric support of TextInput on Android
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
2019-08-30 19:04:14 -07:00
Valentin Shergin c80192c2ab Fabric: EventBeat::Owner to help with crashes
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
2019-08-30 18:24:27 -07:00
Valentin Shergin 5c39c22187 Fabric: Moving ShadowTreeRegistry from Scheduler to UIManager
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
2019-08-30 18:24:27 -07:00
Valentin Shergin dd7f99b1ae Fabric: Changing retaining configuration among UIManager, UIManagerBindging, Scheduler and EventDisaptcher
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
2019-08-30 18:24:27 -07:00
Valentin Shergin e6c6ebf08e Fabric: Removing EventBeat::setFailCallback
Summary: We don't use it anymore. (And that was debug only concept.)

Reviewed By: sammy-SC

Differential Revision: D17115538

fbshipit-source-id: 20aac5457e37666cbf9ca9f62cdfca411026c219
2019-08-30 18:24:27 -07:00
Valentin Shergin 4ffe5b6f6d Fabric: Farewell EventBeatBasedExecutor
Summary: We don't use it anymore.

Reviewed By: JoshuaGross

Differential Revision: D17115539

fbshipit-source-id: e1fa5cc023cd27e53307aa0ea8eae0bad5413be3
2019-08-30 18:24:26 -07:00
Valentin Shergin 6a10feacda Fabric: Passing EventDispatcher as a weak pointer everywhere
Summary: Storing a strong shared pointer to `ComponentDescriptor` can cause a memory leak. Therefore we enforce all call sides and params to be weak pointers. The only Scheduler preserves a retaining pointer to it (to prevent preliminary deallocation).

Reviewed By: sammy-SC

Differential Revision: D17115540

fbshipit-source-id: fdea7d19f742ff04d5ba5470dd9748a5b226aa7c
2019-08-30 18:24:26 -07:00
Ram N a58dd9683d Update the version of yoga podspec to match the actual version of Yoga published
Summary: Yoga is currently published in cocoapods. While we don't use the Yoga from Cocoapods in React Native, we should atleast try to follow that version, so that other integrations with Yoga are possible

Reviewed By: shergin

Differential Revision: D17127625

fbshipit-source-id: bca2e1e33ad775e2a0d7a6f1e4177c3b480c023a
2019-08-30 07:12:34 -07:00
Ram N 82a8080f07 Change podspec name of yoga to Yoga
Summary:
Needed to capitalize the name, since this is the convention used elsewhere too

## Changelog:

[iOS] [Changed] - Renamed yoga podspec to Yoga

Reviewed By: shergin

Differential Revision: D17127104

fbshipit-source-id: 14047bf452edda000037701f4ba7f4a02a0e717b
2019-08-30 07:12:34 -07:00
Valentin Shergin 33112a1c20 Fabric: Making State::getMostRecentState public
Summary: That was always part of design, that's okay to get a newer state from any other state. We will need it in the future diffs.

Reviewed By: sammy-SC

Differential Revision: D17053429

fbshipit-source-id: 2174e7d6e3a1ed231f7f6e238d216d0b09ec8797
2019-08-28 21:10:24 -07:00
Valentin Shergin 876a2789db Fabric: Preventing an obsolete state to be committed
Summary:
It's okay in Fabric model to commit some subtrees which are kinda obsolete (that's strange but technically it's not against the model), but it's not okay to commit some states that were already committed before.
This diff prevents that.

Reviewed By: JoshuaGross

Differential Revision: D17053428

fbshipit-source-id: fb3536312163b7b57011647b4ba7b931c2179d89
2019-08-28 21:10:24 -07:00
Valentin Shergin 59c3d05058 Fabric: Support for <ScrollView scrollEventThrottle={...}> on iOS
Summary: This diff adds support from `ScrollView::scrollEventThrottle` property on iOS.

Reviewed By: JoshuaGross

Differential Revision: D17000397

fbshipit-source-id: 93f23919a6a2588000c0eeca869171d1036348b6
2019-08-26 21:01:46 -07:00
glevi@fb.com 97b42bb142 Deploy v0.106.0 to xplat/js
Reviewed By: mroch

Differential Revision: D16979246

fbshipit-source-id: 995fbd391823eaf0c9e3a673cf8fbe061ce0545a
2019-08-23 08:06:44 -07:00
Peter Argany e94f2c3625 Adhere to main queue requirements of native modules
Summary:
Certain turbomodules set `requiresMainQueueSetup` to true. This is b/c they use some fancy APIs in setup that need main queue.

TurboModuleManager mostly adhered to this restriction, the only case it didn't is when setting bridge. There is possibility that this happens on JS thread, which would crash the app for these certain TM. This diff fixes that.

Reviewed By: RSNara

Differential Revision: D16921644

fbshipit-source-id: 69b2410550360d3ccb03c0b71fb7dfccb889eda4
2019-08-21 14:31:02 -07:00
Joshua Gross 219e1972d6 Remove hack in C++ that prevents any real TextInputs from being rendered
Summary: There's an old hack in the C++ code that prevents TextInputs from being rendered by forcing them to render as Views instead. We don't need this anymore.

Reviewed By: shergin

Differential Revision: D16932795

fbshipit-source-id: 347df106f638c2bf936e2312f42bcb9310d72c6d
2019-08-20 22:28:55 -07:00
SachinTotale b8d6ef3726 Memory Leak due to JSStringRelease not called in multiple places in JSCRuntime.cpp (#25884)
Summary:
Memory Leak due to JSStringRelease not called in multiple places in JSCRuntime.cpp
Issue: https://github.com/facebook/react-native/issues/25664
Reproducible repo: https://github.com/bhandarijiwan/memory_issue_repro

## Changelog
[JSC] [JSCRuntime.cpp] - Added missing JSStringRelease calls in missing places
Pull Request resolved: https://github.com/facebook/react-native/pull/25884

Test Plan: Tested that is no memory leak with various NativeModule to JS call flows

Reviewed By: JoshuaGross

Differential Revision: D16928985

Pulled By: TheSavior

fbshipit-source-id: 65ce15ae32482d0db39bad7e22a2fed9ee04f230
2019-08-20 17:14:01 -07:00
Ramanpreet Nara bf78d7969a Migrate RCTImage NativeModules to CoreModules
Summary:
This diff moves RCTImageLoader, RCTImageEditingManager, and RCTImageStoreManager to CoreModules. This is necessary for us to convert all these NativeModules to TurboModules.

**Note:** As a part of this diff, I had to break apart `RCTImageLoader.h`. All the protocols that were in `RCTImageLoader` are now in their own headers. Furthermore, `RCTImageLoader`'s methods are defined in `RCTImageLoaderProtocol`, so that we can call them from classes like `RCTImageViewManager` in `RCTImage`.

Reviewed By: PeteTheHeat

Differential Revision: D16805827

fbshipit-source-id: 89f6728b0766c30b74e25f7af1be8e6b8a7e6397
2019-08-14 13:39:30 -07:00
Logan Daniels 91f139b941 xplat/js/react-native-github
Reviewed By: panagosg7

Differential Revision: D16657770

fbshipit-source-id: 4e260842c838a35317515044c54ccf55a083da33
2019-08-09 10:11:15 -07:00
Scott Rice 99487d61f2 Codemod fbandroid// => //fbandroid/ in xplat/js/
Reviewed By: zertosh

Differential Revision: D16710441

fbshipit-source-id: 610e0330c486e716a61b31a8198c05aa50a261cf
2019-08-09 09:47:09 -07:00
Ramanpreet Nara c237794236 Format code in ReactCommon/turbomodule/core
Summary: Just ran `arc f ReactCommon/turbomodule/core/**/*`.

Reviewed By: ejanzer

Differential Revision: D16691807

fbshipit-source-id: 3f499ffeffaae47bda550c0071c93cd7f48e2a23
2019-08-08 10:52:26 -07:00
Sidharth Guglani 8830698657 use array for passing measure callback reasons count
Summary:
Use an array for counting measure callbacks due to each reason.
and this is now added as qpl metadata in Layout Calculation qpl event

Reviewed By: davidaurelio

Differential Revision: D16666786

fbshipit-source-id: ff85fba835148f06b9c5d90c4604e552a813777a
2019-08-08 07:28:26 -07:00
Kevin Gozali d2e18a1c5c iOS: Revert RCT->RN prefix renaming to avoid confusion
Summary: The previous rename from RCT->RN prefix ended up causing some confusions on which prefix to use for which files and under what circumstances. To avoid further confusion before we're done with the re-architecture project, let's keep them as RCT.

Reviewed By: mdvacca

Differential Revision: D16705566

fbshipit-source-id: 395bff771c84e5ded6b2261a84c7549df1e6c5e5
2019-08-08 07:21:25 -07:00
Adlai Holler 4c11b55053 Don't copy children in YGNodeComputeFlexBasisForChildren (#919)
Summary:
No need for a copy here.
Pull Request resolved: https://github.com/facebook/yoga/pull/919

Differential Revision: D16701461

Pulled By: davidaurelio

fbshipit-source-id: 3a90adbb2b5c43d5aefe693a8525aa3a37e53b3d
2019-08-08 00:25:00 -07:00
David Aurelio c9b757f513 Back out "[Yoga] Experiment: double invocations of measure callbacks"
Summary:
Removes the double measure callbacks experiment
Original commit changesets: c6cf9c01a173,  b157d8137c72

Reviewed By: SidharthGuglani

Differential Revision: D16687367

fbshipit-source-id: 9649f8731bd1b27f4d291cee4fa30153165cea02
2019-08-07 18:27:40 -07:00
David Aurelio f94360a377 Use Bitfield in YGLayout
Summary: Replaces the usage of C++ bitfields with our portable `Bitfield` class.

Reviewed By: SidharthGuglani

Differential Revision: D16656361

fbshipit-source-id: 05f679e2e994e109b2bd1090c879d6850fabdc40
2019-08-07 16:22:01 -07:00
David Aurelio 2c73dbdc9b Use Bitfield in YGNode and YGStyle
Summary:
@public

Replaces the usage of C++ bitfields with our portable `Bitfield` class.

Reviewed By: SidharthGuglani

Differential Revision: D16649875

fbshipit-source-id: 539f016d5e1c9a8c48cc9bacbbf6ed985e385e69
2019-08-07 16:22:01 -07:00
David Aurelio f33a623c92 Remove style property bitmask
Summary:
@public

Removes the style properties bitmask. We have used this for experimentation, and it's no longer necessary.

This simplifyies the code, and allows us to cut over to `Bitfield.h` more easily.

Reviewed By: astreet

Differential Revision: D16648862

fbshipit-source-id: 17c0899807af976f4ba34db54f8f0f6a3cd92519
2019-08-07 16:22:00 -07:00
David Aurelio d1d48ef915 Add portable bit field implementation
Summary:
@public
Our usage of C++ bit fields has lead to quite some problems with different compiler setups. Problems include sign bits, alignment, etc.

Here we introduce a portable implementation as a variadic template, allowing the user to store a number of booleans and enums (defined with `YG_ENUM_SEQ_DECL`) in an unsigned integer type of their choice.

This will replace all usages of bit fields across the Yoga code base.

Differential Revision: D16647801

fbshipit-source-id: 230ffab500885a3ad662ea8f19e35a5e9357a563
2019-08-07 16:22:00 -07:00
sunnylqm c21e36db45 Bump hermes to v0.1.1 (#25908)
Summary:
Hermes has been updated to [v0.1.1](https://github.com/facebook/hermes/releases/tag/v0.1.1) and [renamed from 'hermesvm' to 'hermes-engine'](https://github.com/facebook/hermes/commit/c74842ee5c4d11dc9fe3bf012f97a0e3fde6d54f)

## Changelog

[Android] [Changed] - Bump hermes to v0.1.1
Pull Request resolved: https://github.com/facebook/react-native/pull/25908

Test Plan: RNTester builds and runs as expected

Differential Revision: D16645811

Pulled By: cpojer

fbshipit-source-id: 4fb6a3160df2c6d08140dd1fee51acf9ff8baffc
2019-08-05 12:58:25 -07:00
Samuel Susla e557901267 Fix crash when tapping full screen button on video ad
Summary:
Fragment was assigned incorrect `tag` and `surfaceID` (`surfaceID` is the important one).

Wrong `surfaceID` means that `navigationCoordinator` is never resolved. As a result of navigationCoordinator not being assigned, tapping a video ad on Marketplace results in showing video ad overlay rather than showing full screen video.

Reviewed By: JoshuaGross

Differential Revision: D16646492

fbshipit-source-id: 0da5c56ecb7c81e9f4a9469a3626ccd430a01558
2019-08-05 10:17:16 -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 08e4537680 Expose JS Responder handler in Scheduler API
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
2019-08-02 16:38:45 -07:00
Ramanpreet Nara 82e4d28e25 Move ownership of TurboModule jni ref to JavaTurboModule
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
2019-08-02 10:08:40 -07:00
Ramanpreet Nara 7ab517b5fe Make TurboModules long-lived on Android
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
2019-08-02 10:08:39 -07:00
Eric Williamson 283a9dcc11 Revert D16553363: [RN][TurboModule] Make TurboModules long-lived on Android
Differential Revision:
D16553363

Original commit changeset: c95e150d6967

fbshipit-source-id: d035724ce131b560abc70e94410d727951a83241
2019-08-01 21:59:45 -07:00
Eric Williamson af4862ce32 Revert D16555673: [RN][TurboModule] Move ownership of TurboModule jni ref to JavaTurboModule
Differential Revision:
D16555673

Original commit changeset: 2778fc5a372c

fbshipit-source-id: fac3a6ea185acaa750f58e19d24c194668749636
2019-08-01 21:59:45 -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
Kevin Gozali 9420de6860 iOS: codemod react-native-github: RCT->RN prefix for Fabric
Summary: Fabric ObjC(++) files will be prefixed by RN* for the time being, this codemod is a simple rename. This includes `interface` and `protocol` definition

Reviewed By: PeteTheHeat, yungsters

Differential Revision: D16611524

fbshipit-source-id: 868d2571ea2414dde4cbb3b75b1334b779b5d832
2019-08-01 20:06:04 -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
Ramanpreet Nara 88e03fa4db Move ownership of TurboModule jni ref to JavaTurboModule
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
2019-08-01 16:36:47 -07:00
Ramanpreet Nara e876cc68e7 Make TurboModules long-lived on Android
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
2019-08-01 16:36:46 -07:00
Sidharth Guglani c36737204e Added counts for measure callbacks reasons in an array inside qpl annotations
Summary:
Added an array to maintain the counts of each of the reason of measure callbacks
and this is now added as qpl metadata in Layout Calculation qpl event

Reviewed By: davidaurelio

Differential Revision: D16516379

fbshipit-source-id: 201c5d2463f0a921841a0bbfec8f4d5e007000c8
2019-07-31 14:38:38 -07:00
Sidharth Guglani 6ce985463b create two layout pass reason flexLayout and flexMeasure instead of flex
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
2019-07-31 14:38:38 -07:00
David Vacca 8f055a0f8e Unify naming of dispatchCommand methods in UIManagerDelegate and Scheduler
Summary:
This diff changes the name of the DispatchCommand methods in the UIManagerDelegate and Scheduler classes.
The purpose of this change is to use a consistent naming with the rest of the methods od these classes.
We might re-name these interfaces later, but for now we want to keep a consistent naming. For more details see discussion in D16543437

Reviewed By: JoshuaGross

Differential Revision: D16575403

fbshipit-source-id: 8335be8dc3367372a8ef2d7e8ed78665f4c02699
2019-07-30 22:42:51 -07:00
Joshua Gross 62591ac840 Set scheduler delegate during construction
Summary: I think it's possible that there's a race condition between creating the scheduler and setting the delegate leading to bugs like T47272192.

Reviewed By: mdvacca

Differential Revision: D16537737

fbshipit-source-id: 9c579537658be5a9aeed37c0e4935c997cabb6aa
2019-07-29 10:42:31 -07:00
Samuel Susla 678e833e01 Fix creation of new ComponentDescriptorRegistry when switching surface
Summary:
# Problem:
I enabled Fabric for "Profile About", when I navigate to "Marketplace Home" from "Profile About", I get following warning "<Component> is not Fabric compatible yet". Even though the component has been migrated to Fabric and is getting registered. The component works if I navigate straight to "Marketplace Home".

{F172219008}

# How it was solved:

It turns out, there was a missing call that added newly created registry to `componentDescriptorRegistries_`.

Reviewed By: shergin

Differential Revision: D16542109

fbshipit-source-id: 22a315333cfff0895ce77e8439816f4e7bf34d35
2019-07-29 09:52:28 -07:00
Championrunner 9115b61083 Update RCTTurboModule.mm (#25831)
Summary:
## Changelog

[CATEGORY] [TYPE] - Typo Error
Pull Request resolved: https://github.com/facebook/react-native/pull/25831

Differential Revision: D16514794

Pulled By: cpojer

fbshipit-source-id: 36a267fe53f78b93de418d1f0b04f04353bf2195
2019-07-26 01:36:32 -07:00
cpojer d7f5153cd8 Add Hermes support to React Native on Android (#25613)
Summary:
Yesterday we shipped hermesengine.dev as part of the current 0.60 release. This PR brings those changes to master.

## Changelog

[General] [Added] - Added support for Hermes
Pull Request resolved: https://github.com/facebook/react-native/pull/25613

Test Plan:
* CI is green both on GitHub and at FB
* Creating a new app from source can use Hermes on Android

Reviewed By: cpojer

Differential Revision: D16221777

Pulled By: willholen

fbshipit-source-id: aa6be10537863039cb666292465ba2e1d44b64ef
2019-07-25 23:05:53 -07:00
James Treanor ca9e108110 Remove 's.static_framework = true' requirement for podspec (#25816)
Summary:
As part of the fix for https://github.com/facebook/react-native/issues/25349 I added `s.static_framework = true` to each podspec in repo (see https://github.com/facebook/react-native/pull/25619#discussion_r306993309 for more context).

This was required to ensure the existing conditional compilation with `#if RCT_DEV` and `__has_include` still worked correctly when `use_frameworks!` is enabled.

However, fkgozali pointed out that it would be ideal if we didn't have this requirement as it could make life difficult for third-party libraries.

This removes the requirement by moving `React-DevSupport.podspec` and `React-RCTWebSocket.podspec` into `React-Core.podspec` as subspecs. This means the symbols are present when `React-Core.podspec` is built dynamically so `s.static_framework = true` isn't required.

This means that any `Podfile` that refers to `React-DevSupport` or `React-RCTWebSocket` will need to be updated to avoid errors.

## Changelog

I don't think this needs a changelog entry since its just a refinement of https://github.com/facebook/react-native/pull/25619.
Pull Request resolved: https://github.com/facebook/react-native/pull/25816

Test Plan:
Check `RNTesterPods` still works both with and without `use_frameworks!`:

1. Go to the `RNTester` directory and run `pod install`.
2. Run the tests in `RNTesterPods.xcworkspace` to see that everything still works fine.
3. Uncomment the `use_frameworks!` line at the top of `RNTester/Podfile` and run `pod install` again.
4. Run the tests again and see that it still works with frameworks enabled.

Reviewed By: hramos

Differential Revision: D16495030

Pulled By: fkgozali

fbshipit-source-id: 2708ac9fd20cd04cb0aea61b2e8ab0d931dfb6d5
2019-07-25 11:46:43 -07:00
Moti Zilberman 71c84cf6be Implement globalEvalWithSourceUrl
Summary:
Implements a new host function on the global object in debug builds, called `globalEvalWithSourceUrl`. This performs a global `eval()` and attaches a URL/filename to the evaluated script (in stack traces, debuggers, etc).

It serves a similar purpose to the `//# sourceURL=` directive (which most JS engines support, but JSC doesn't) and to the old `nativeInjectHMRUpdate` function which was dropped in the JSC->JSI migration.

Reviewed By: cpojer

Differential Revision: D16491506

fbshipit-source-id: bd9a89311dcbb1d0baece77ead16b9ecfb13bfe3
2019-07-25 10:23:42 -07:00