-
Notifications
You must be signed in to change notification settings - Fork 36
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 Node-API calling conventions #122
Conversation
It was nice to have NodeApiJsiRuntime.h/.cpp alongside the napi implementation; we could test it at the source, and make the package consumable stand-alone (without RNW). But if we add napi+jsi tests in RNW that should provide some coverage as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -28,13 +28,13 @@ typedef struct napi_ext_env_scope__ *napi_ext_env_scope; | |||
typedef struct napi_ext_ref__ *napi_ext_ref; | |||
|
|||
// A callback to return buffer synchronously | |||
typedef void (*napi_ext_buffer_callback)(napi_env env, uint8_t const *buffer, size_t buffer_length, void *buffer_hint); | |||
typedef void (__cdecl *napi_ext_buffer_callback)(napi_env env, uint8_t const *buffer, size_t buffer_length, void *buffer_hint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot think about any such test. Any ideas?
I see your point and it was not an easy decision. Currently we have three copies of this code: here, hermes-windows, and RNW. I want to have a single copy instead. Having a full repo just for these few lines seems to be too much. My current thoughts to have all this code inside of RNW, but like you said it makes it difficult to use v8jsi.dll package on its own. |
I am going to restore the JSI for Node-API implementation before merging this PR. |
d489b45
to
965f8e4
Compare
Hello @vmoroz! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
The issue
When we tried to adopt the Node-API based code in ReactNative for Windows we have got runtime crashes in x86 Release mode.
The issue is that some function pointers in Node-API did not have explicit calling conventions and since
react-native-win32.dll
produced by ReactNative for Windows uses__stdcall
calling conventions by default while most of Node-API is__cdecl
, we have got a mismatch in calling conventions for all function pointers passed to Node-API. The Node-API is expecting__cdecl
function pointer and given a__stdcall
function pointer instead. It causes a runtime crash.The solution
We set the
__cdecl
explicitly for function pointers injs_native_api_types.h
andjs_native_ext_api.h
.The NapiJsiRuntime implementation is updated to use explicit
__cdecl
decorator for the Node-API function pointers.Microsoft Reviewers: Open in CodeFlow