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

[tomekzaw]

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.

[javache]

The entire ShadowNodeWrapper was removed in #40864, but reintroduced in #44772

Thanks for catching this!

[facebook-github-bot]

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

[tomekzaw]

@javache Thanks, I've updated the PR description.

[tomekzaw]

btw I'm afraid this static_assert does not work as intented, even when I remove ~ShadowNode(), it still holds as jsi::NativeState (the base class) has a virtual destructor

react-native/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h

Lines 277 to 279 in d1bf828

	static_assert(
	std::has_virtual_destructor<ShadowNode>::value,
	"ShadowNode must have a virtual destructor");

react-native/packages/react-native/ReactCommon/jsi/jsi/jsi.h

Lines 149 to 152 in d1bf828

	class JSI_EXPORT NativeState {
	public:
	virtual ~NativeState();
	};

edit: actually, I suspect that ShadowNode suffers from the same problem as the virtual destructor is inline (so it's not a key function)

react-native/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h

Line 89 in d1bf828

virtual ~ShadowNode() override = default;

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	21,285,099	+5
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	24,481,856	-9
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: a7f5963
Branch: main

[facebook-github-bot]

@javache merged this pull request in ea958c6.

[github-actions]

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?}

[tomekzaw]

@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?

[javache]

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?

[tomekzaw]

@javache No worries, it's fine as-is for us, but anyway I submitted #45357 for the sake of completeness :)