-
Notifications
You must be signed in to change notification settings - Fork 24.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper again #45290
Fix dynamic_cast (RTTI) by adding key function to ShadowNodeWrapper again #45290
Conversation
@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@javache Thanks, I've updated the PR description. |
btw I'm afraid this react-native/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h Lines 277 to 279 in d1bf828
react-native/packages/react-native/ReactCommon/jsi/jsi/jsi.h Lines 149 to 152 in d1bf828
edit: actually, I suspect that
|
packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp
Outdated
Show resolved
Hide resolved
…de.cpp Co-authored-by: Wojciech Lewicki <wojciech.lewicki@swmansion.com>
Base commit: a7f5963 |
This pull request was successfully merged by @tomekzaw in ea958c6. When will my fix make it into a release? | How to file a pick request? |
@javache I've pushed some more commits and they have not been merged. Nothing important (just minor changes in the comments). Do I need to submit another PR with these changes? |
…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: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [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
Sorry, I missed this and the internal copy of this landed already. I think it's fine as-is, unless you believe the comments are worth a separate PR? |
Summary:
This PR restores the virtual destructor for
ShadowNodeWrapper
which was added in #33500. The class itself was removed in #40864 and reintroduced in #44772, however this time without a virtual destructor.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
Test Plan:
This patch fixes an issue in Reanimated's fabric-example app.