Summary:
This PR fixes RTTI (run-time type information) for ShadowNodeWrapper and ShadowNodeListWrapper classes, i.e., calls to dynamic_cast and dynamic_pointer_cast that are called via JSI's getHostObject calls.
The fix is simply to add a so-called "key function" in a form of virtual destructor. Key functions needs to be a virtual non-pure and non-inlined functions that points the compiler as to which library contains the vtable/type information for a given class (see https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable and https://developer.android.com/ndk/guides/common-problems#rttiexceptions_not_working_across_library_boundaries)
Without the "key function", calls to dynamic_cast for ShadowNodeWrapper instances won't work across library boundaries because the class will have separate definitions in each separate library, therefore objects created in one of those libraries won't be recognized as the same type by the other library. This has been a problem in reanimated and gesture-handler libraries where we call `object.getHostObject<ShadowNodeWrapper>(rt)` (this is a method from JSI) in order to access ShadowNode instance from a handle we have in JS. I think, this issue is going to be relevant to more libraries that cope with view instances. In this scenario, we have a separate library, say "libreanimated.so" that calls to `getHostObject` which is an inline function that calls `dynamic_cast` for the `ShadowNodeWrapper` class. On the other hand, the instances of `ShadowNodeWrapper` are created by the code from `libreact_render_uimanager.so`. Because of that `dynamic_cast` fails even though it is called on instance of `ShadowNodeWrapper` because the class has separate vtable/type info: one in `libreanimated.so` and one in `libreact_render_uimanager.so` (by "fails" I mean that it actually returns `nullptr`).
This problem has been documented here: https://developer.android.com/ndk/guides/common-problems#rttiexceptions_not_working_across_library_boundaries where the solution is for the class to have a so-called "key function". The key function makes it so that compiler sees that one of the implementation for a given class is missing and therefore can safely assume that a vtable/type info for a given class is embedded into some library we link to.
This change adds a virtual destructor that is declared in the header file but defined in file that gets compiled as a part of `libreact_render_uimanager`. As a result, the compiler only creates one vtable/type info and calls to dynamic_cast works as expected in all libraries for `ShadowNodeWrapper` and `ShadowNodeListWrapper` classes.
This issue would only surface on Android, because on iOS all libraries by default are bundled together via Pods, whereas on Android each library is loaded separately using dynamic loading.
## Changelog
[Fabric][Android specific] - Fix dynamic_cast (RTTI) for ShadowNodeWrapper and similar classes when accessed by third-party libraries.
Pull Request resolved: https://github.com/facebook/react-native/pull/33500
Test Plan:
1. In order to test this you need to add a library that'd include `<react/renderer/uimanager/primitives.h>` (i.e. use this branch of reanimated library: https://github.com/software-mansion/react-native-reanimated/tree/fabric)
2. After compiling the app inspect libreact_render_uimanager.so and libreanimated.so artifacts with `nm` tool
3. Notice that symbols like `vtable for facebook::react::ShadowNodeWrapper` and `typeinfo for facebook::react::ShadowNodeWrapper` are only present in the former and not in the latter library (before this change you'd see them both)
Reviewed By: ShikaSD
Differential Revision: D35143600
Pulled By: javache
fbshipit-source-id: 5fb25a02365b99a515edc81e5485a77017c56eb8
Summary:
The fix in this diff seems simple, but it took some time to understand why this change fixed the issue that views animated use native driver out from their parent's layout are not getting touch events.
We introduced `overflowInset` to RN Android a while back to give each shadow node extra information to cover all its children's layout. These values (left, top, right, bottom extensions from the view's own layout) help us improve hit-testing algorithm used in touch events. We could ignore all subtrees that the touch point not in their parent's overflowInset box.
However, this was not working for native animation. When `userNativeDriver` is on, all animation happens without Fabric knows anything about them. The overflowInset is out of date with the final animated layout, which caused the issue that we ignored the animated view as we thought it's not under the pointer.
Here is a playground demo (P476407654) for the issue:
https://pxl.cl/1XfPL
We've tried to fix this by passing the final animated values via `passthroughAnimatedPropExplicitValues` added in D32539976. This is a prop that will get merged into `style` prop for [animation component](https://fburl.com/code/jybzfgu5). The transform values were already applied when measuring layout in [Pressability](https://fburl.com/code/5mect2k3), which uses [LayoutableShadowNode](https://fburl.com/code/qh8fufrw). However, this is not the case for overflowInset calculation. Hence, the fix here is to apply the transform matrix in Yoga before calculating the overflowInset.
Changelog:
[Android][Fixed] - Fix overflowInset calculation by using transform values
Reviewed By: ShikaSD
Differential Revision: D33806030
fbshipit-source-id: e438618e3d6e5b0333cff9ff9919b841d73b2e9d
Summary:
This was a bug, we are fixing it.
Move pointerEvents from formsStacking -> formsView and we are also removing "onLayout" from formsStackingContext
changelog: [internal] internal
Reviewed By: sammy-SC
Differential Revision: D33846660
fbshipit-source-id: 6b65a9a7815972e34dafbc48b3d732d9b02d5e9f
Summary:
Changelog: [internal]
Add comments and avoid using shared_ptr unnecessarily.
Why note shared_ptr? Using shared_ptr suggests we are transferring ownership of object, which we are not.
Reviewed By: javache
Differential Revision: D33741836
fbshipit-source-id: 56ebb098e6185793f05e2bb587005bb0f093c0c9
Summary:
changelog: [internal]
Yielding in RuntimeScheduler is shipped. This diff removes the gating around it.
Reviewed By: sshic
Differential Revision: D33740360
fbshipit-source-id: 267372e81e66dda96e451435954a7c3546cc6fbe
Summary:
Refactors MapBuffer-related `primitives.h` to be namespaced with `MapBuffer` class, to avoid name collisions and confusion later on.
Most of the size constants are moved to relevant .cpp files or updated to use `sizeof`.
Additionally, adds a little bit of documentation about `MapBuffer` serialization format.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33662487
fbshipit-source-id: 5a7a2b1c7f2bb13ee1edfc5fae51ba88c34f0d3c
Summary:
Serializes type information along with key/value in MapBuffer, asserting the data type on Java side during read. At the moment, accessing value with incorrect will result in a crash.
Other changes:
Adds a `getType` method to verify types before accessing them.
Removes `null` as a type, as just not inserting value and checking for its existence with `hasKey` is more optimal.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33656841
fbshipit-source-id: 23a78daa0d84704aab141088b5dfe881e9609472
Summary:
Removes the need to store keys in incremental order by sorting them after before inserting into MapBuffer.
Updates MapBuffer to support random access on C++ side, actually retrieving values by keys and not bucket index.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33611758
fbshipit-source-id: c81e970613c8ecc688bfacb29ba038bf081a0c0f
Summary:
Rename `_header` to `header_` to align with the C++ naming scheme we use.
Rename `readKey` to `readUnsignedShort` as purpose of the method have changed.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D33637127
fbshipit-source-id: a82f4d6c1b753b21e0567fbe919af98e4c78105d
Summary:
The struct is copied directly to the buffer, so it is safe to read it back.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D33624036
fbshipit-source-id: 7ee2b28b815690ab471832c0c622a075b5dd7d78
Summary:
Replaces dynamic manually managed array with a vector of bytes, removing the need for managing memory manually.
This approach is a little bit slower than before, as vector is allocating memory more conservatively. This can be improved in the future by providing an estimate for the data size.
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D33611759
fbshipit-source-id: a0e5e57c4e413206a9f891cd5630ecc9088a9bf7
Summary:
Instead of copying integer values into key/value data array, this implementation keeps a structured vector of `Bucket` structs (which is sized to be serialized later).
This approach provides multiple benefits to modify and access buckets (e.g. sort them based on the key, allowing for out of order inserts (D33611758))
Changelog: [Internal]
Reviewed By: javache
Differential Revision: D33601231
fbshipit-source-id: 62ace1374936cb504836d6eae672e909ea404e3f
Summary:
Benchmarks performance/correctness of mapbuffer with several different types of data
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33608299
fbshipit-source-id: 81b1e195aecac60ad1cc3ca416d1cf7b09feab32
Summary:
This method can produce unnecessary copy of the string, replacing it with reference to optimize.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33607765
fbshipit-source-id: e04eddffec063c0c8e173bfdf690e7b8e1050525
Summary:
Cleans up `ReadableMapBuffer` APIs and migrates to use `std::vector` instead of raw pointer array.
Also uses `fbjni` utility to allocate the bytes instead of making manual JNI calls.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D33513027
fbshipit-source-id: afe7d320d12830503de4c9994117d849f0b25245
Summary:
Replaces raw array pointer inside `MapBuffer` with `std::vector`, which is more conventional "safer" way of storing dynamically sized arrays.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D33512115
fbshipit-source-id: d875a2240f7938fd35c4c3ae0e7e91dc7456ff32
Summary:
TurboModules: use weakSelf for block sent to main thread.
Changelog:
[iOS][Fixed] - Use weakSelf in objc block instead of self.
Reviewed By: RSNara
Differential Revision: D33488938
fbshipit-source-id: d03c98fb59fbd1fc4b67686251ebec541e5d3e6c
Summary:
changelog: [internal]
You can read more about this rule on https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
# Isn't it wasteful to copy? Isn't reference more efficient?
This rule of thumb is no longer true since C++11 with move semantics. Let's look at some examples.
# Option one
```
class TextHolder
{
public:
TextBox(std::string const &text) : text_(text) {}
private:
std::string text_;
};
```
By using reference here, we prevent the caller from using rvalue to and avoiding copy. Regardless of what the caller passes in, copy always happens.
# Option two
```
class TextHolder
{
public:
TextBox(std::string const &text) : text_(text) {}
TextBox(std::string &&text) : text_(std::move(text)) {}
private:
std::string text_;
};
```
Here, we provide two constructors, one for const reference and one for rvalue reference. This gives the caller option to avoid copy. But now we have two constructors, which is not ideal.
# Option three (what we do in this diff)
```
class TextHolder
{
public:
TextBox(std::string text) : text_(std::move(text)) {}
private:
std::string text_;
};
```
Here, the caller has option to avoid copy and we only have single constructor.
Reviewed By: fkgozali, JoshuaGross
Differential Revision: D33276841
fbshipit-source-id: 619d5123d2e28937b22874650366629f24f20a63
Summary:
changelog: [internal]
For some reason, using `TextLayoutManager::Shared` in `TextInputShadowNode` trips up clang tidy linter. We have a plan to move away from `*::Shared` anyway, so let's remove it from `TextInputShadowNode` now.
Why do we want to move away from `*::Shared`?
Using `TextLayoutManager::Shared` is confusing for people unfamiliar with Fabric's codebase. It expresses a concept of immutability but uses term `shared`. Term shared is already used in C++ `std::shared_ptr`.
Reviewed By: fkgozali
Differential Revision: D33186422
fbshipit-source-id: 10ee588735997f5fedc372a1d1e3d9cd9684178a
Sparsha Saha
Anandraj
Krzysztof Magiera
Chiara Mooney
Xin Chen
David Vacca
Pieter De Baets
Samuel Susla
Andrei Shikov
Moses DeJong
Andres Suarez