Skip to content

Commit

Permalink
Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper a…
Browse files Browse the repository at this point in the history
…nd related classes (#33500)

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: #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
  • Loading branch information
kmagiera authored and facebook-github-bot committed Mar 28, 2022
1 parent d31d83f commit 58a2eb7
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 0 deletions.
7 changes: 7 additions & 0 deletions ReactCommon/react/renderer/uimanager/UIManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@

namespace facebook::react {

// Explicitly define destructors here, as they to exist in order to act as a
// "key function" for the ShadowNodeWrapper class -- this allow for RTTI to work
// properly across dynamic library boundaries (i.e. dynamic_cast that is used by
// isHostObject method)
ShadowNodeWrapper::~ShadowNodeWrapper() = default;
ShadowNodeListWrapper::~ShadowNodeListWrapper() = default;

static std::unique_ptr<LeakChecker> constructLeakCheckerIfNeeded(
RuntimeExecutor const &runtimeExecutor) {
#ifdef REACT_NATIVE_DEBUG
Expand Down
10 changes: 10 additions & 0 deletions ReactCommon/react/renderer/uimanager/primitives.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,23 @@ struct ShadowNodeWrapper : public jsi::HostObject {
ShadowNodeWrapper(SharedShadowNode shadowNode)
: shadowNode(std::move(shadowNode)) {}

// The below method needs to be implemented out-of-line in order for the class
// to have at least one "key function" (see
// https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable)
~ShadowNodeWrapper() override;

ShadowNode::Shared shadowNode;
};

struct ShadowNodeListWrapper : public jsi::HostObject {
ShadowNodeListWrapper(SharedShadowNodeUnsharedList shadowNodeList)
: shadowNodeList(shadowNodeList) {}

// The below method needs to be implemented out-of-line in order for the class
// to have at least one "key function" (see
// https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable)
~ShadowNodeListWrapper() override;

SharedShadowNodeUnsharedList shadowNodeList;
};

Expand Down

0 comments on commit 58a2eb7

Please sign in to comment.