Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper and related classes #33500

kmagiera · 2022-03-25T13:56:10Z

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.

Test Plan

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)
After compiling the app inspect libreact_render_uimanager.so and libreanimated.so artifacts with nm tool
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)

…nd related classes

pull-bot · 2022-03-25T14:00:52Z

	Messages
📖	📋 Verify Changelog Format - A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against e5f8330

facebook-github-bot · 2022-03-25T14:10:25Z

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

analysis-bot · 2022-03-25T14:15:15Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 96c611b
Branch: main

javache · 2022-03-25T14:27:14Z

Hey @kmagiera :) No concerns with adding these methods, but just FYI, internally we've been compiling most React Native code without RTTI since it gives significant binary size wins, and instead use trait fields instead of dynamic_cast (see d291a7e)

kmagiera · 2022-03-25T14:28:40Z

ReactCommon/react/renderer/uimanager/primitives.h

@@ -30,13 +30,21 @@ struct ShadowNodeWrapper : public jsi::HostObject {
  ShadowNodeWrapper(SharedShadowNode shadowNode)
      : shadowNode(std::move(shadowNode)) {}

+  // The below method needs to be 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)
+  virtual ~ShadowNodeWrapper();


apparently, it needs to not be inlined and exists in one of the binaries to be treated a s a key function

javache · 2022-03-25T14:30:48Z

ReactCommon/react/renderer/uimanager/primitives.h

@@ -30,13 +30,21 @@ struct ShadowNodeWrapper : public jsi::HostObject {
  ShadowNodeWrapper(SharedShadowNode shadowNode)
      : shadowNode(std::move(shadowNode)) {}

+  // The below method needs to be 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)
+  virtual ~ShadowNodeWrapper();


This should use override, which implies virtual.

Yup that's also one of the reason the internal build is failing. Thanks for pointing this out 👍

javache · 2022-03-25T14:30:52Z

ReactCommon/react/renderer/uimanager/primitives.h

  ShadowNode::Shared shadowNode;
 };

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

+  // The below method needs to be 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)
+  virtual ~ShadowNodeListWrapper();


analysis-bot · 2022-03-25T14:47:18Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,951,054	-28,788
android	hermes	armeabi-v7a	7,546,214	+193,083
android	hermes	x86	8,305,362	-10,006
android	hermes	x86_64	8,257,241	-27,150
android	jsc	arm64-v8a	9,619,655	-7,942
android	jsc	armeabi-v7a	8,599,222	+196,900
android	jsc	x86	9,570,058	-6,733
android	jsc	x86_64	10,166,067	-7,704

Base commit: 96c611b
Branch: main

kmagiera · 2022-03-25T15:51:50Z

Thanks for prompt review @javache – AFAIK dynamic_cast should work ok w/o rtti compiler flag enabled so long there is a only one library that has a vtable for the class.I'm not entirely sure I understand the traits approach nevertheless I think an alternative solution here would be to replace dynamic_cast from the jsi implementation with reinterpret_cast as it like with getHostObject you should know what type to cast to anyways.

facebook-github-bot · 2022-03-28T10:03:44Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

javache · 2022-03-28T10:04:35Z

The traits solution only applies to subclasses of ShadowView, not in scenarios where you have a HostObject which could be anything.

cortinico · 2022-03-28T10:36:19Z

I'm handing this over to @javache for driving it to landing 👍

react-native-bot · 2022-03-28T12:05:04Z

This pull request was successfully merged by @kmagiera in 58a2eb7.

^{When will my fix make it into a release? | Upcoming Releases}

…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: facebook/react-native#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

…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

…nd related classes (facebook#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: facebook#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

…gain (#45290) Summary: This PR restores the virtual destructor for `ShadowNodeWrapper` which was added in #33500 and unfortunately removed in #40864. The virtual destructor here serves as a key function. Without a key function, `obj.hasNativeState<ShadowNodeWrapper>(rt)` **does not** work correctly between shared library boundaries on Android and always returns false. We need this pretty badly in third-party libraries like react-native-reanimated or react-native-gesture-handler. ## Changelog:  [ANDROID] [FIXED] - Fix dynamic_cast (RTTI) for ShadowNodeWrapper when accessed by third-party libraries again Pull Request resolved: #45290 Test Plan: This patch fixes an issue in Reanimated's fabric-example app. Reviewed By: fabriziocucci Differential Revision: D59375554 Pulled By: javache fbshipit-source-id: 09f3eda89a67c26d6dacca3428e08d1b7138d350

Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper a…

90618d2

…nd related classes

kmagiera commented Mar 25, 2022

View reviewed changes

javache reviewed Mar 25, 2022

View reviewed changes

Add override for destructors to address review comments

e5f8330

facebook-github-bot closed this in 58a2eb7 Mar 28, 2022

react-native-bot added the Merged This PR has been merged. label Mar 28, 2022

snyk-bot mentioned this pull request May 5, 2022

[Snyk] Upgrade react-native from 0.63.3 to 0.68.1 NOUIY/react-native-macos#99

Open

snyk-bot mentioned this pull request May 30, 2022

[Snyk] Upgrade react-native from 0.63.3 to 0.68.2 NOUIY/react-native-macos#220

Open

This was referenced Nov 14, 2022

i18n fallback testing discord/react-native#30

Closed

Alter font loading code to support fallback fonts in fontFamily discord/react-native#31

Merged

stevenpetryk mentioned this pull request Nov 21, 2022

Remove RN Android's bizarre font weight extension pattern discord/react-native#32

Merged

tomekzaw mentioned this pull request Jul 4, 2024

Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper again #45290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper and related classes #33500

Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper and related classes #33500

kmagiera commented Mar 25, 2022 •

edited

Loading

pull-bot commented Mar 25, 2022 •

edited

Loading

facebook-github-bot commented Mar 25, 2022

analysis-bot commented Mar 25, 2022 •

edited

Loading

javache commented Mar 25, 2022 •

edited

Loading

kmagiera Mar 25, 2022

javache Mar 25, 2022

cortinico Mar 25, 2022

javache Mar 25, 2022

analysis-bot commented Mar 25, 2022 •

edited

Loading

kmagiera commented Mar 25, 2022

facebook-github-bot commented Mar 28, 2022

javache commented Mar 28, 2022

cortinico commented Mar 28, 2022

react-native-bot commented Mar 28, 2022

Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper and related classes #33500

Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper and related classes #33500

Conversation

kmagiera commented Mar 25, 2022 • edited Loading

Summary

Changelog

Test Plan

pull-bot commented Mar 25, 2022 • edited Loading

facebook-github-bot commented Mar 25, 2022

analysis-bot commented Mar 25, 2022 • edited Loading

javache commented Mar 25, 2022 • edited Loading

kmagiera Mar 25, 2022

Choose a reason for hiding this comment

javache Mar 25, 2022

Choose a reason for hiding this comment

cortinico Mar 25, 2022

Choose a reason for hiding this comment

javache Mar 25, 2022

Choose a reason for hiding this comment

analysis-bot commented Mar 25, 2022 • edited Loading

kmagiera commented Mar 25, 2022

facebook-github-bot commented Mar 28, 2022

javache commented Mar 28, 2022

cortinico commented Mar 28, 2022

react-native-bot commented Mar 28, 2022

kmagiera commented Mar 25, 2022 •

edited

Loading

pull-bot commented Mar 25, 2022 •

edited

Loading

analysis-bot commented Mar 25, 2022 •

edited

Loading

javache commented Mar 25, 2022 •

edited

Loading

analysis-bot commented Mar 25, 2022 •

edited

Loading