Skip to content
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

[HermesABI][C API] Creating PropName from string triggers assertion failure #1560

Closed
robik opened this issue Nov 6, 2024 · 6 comments
Closed
Assignees
Labels
need more info Awating additional info before proceeding

Comments

@robik
Copy link
Contributor

robik commented Nov 6, 2024

Bug Description

  • [?] I have run gradle clean and confirmed this bug does not occur with JSC
  • [?] The issue is reproducible with the latest version of React Native.

Hermes git revision (if applicable): main (f2970e7)
React Native version: none
OS: MacOs
Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64): arm64-v8a

Steps To Reproduce

Run below code repro

#include <stdio.h>
#include <hermes_abi.h>
#include <hermes_vtable.h>

int main() {
    const struct HermesABIVTable *global_vt = get_hermes_abi_vtable();
    struct HermesABIRuntime *rt = global_vt->make_hermes_runtime(NULL);

    struct HermesABIStringOrError strOrError = rt->vt->create_string_from_utf8(rt, "test", 4);
    if ((strOrError.ptr_or_error & 1) == 1) {
        printf("Failed to create str");
        return 1;
    }
    struct HermesABIString* hermesStr = (struct HermesABIString*)strOrError.ptr_or_error;

    // FAILS HERE
    struct HermesABIPropNameIDOrError propNameOrError = rt->vt->create_propnameid_from_string(rt, *hermesStr);

    // Never reached
    if ((propNameOrError.ptr_or_error & 1) == 1) {
        printf("Failed to create propname");
        return 1;
    }

    struct HermesABIPropNameID* propName = (struct HermesABIPropNameID*)propNameOrError.ptr_or_error;

    rt->vt->release(rt);
    return 0;
}

The Expected Behavior

Expected: PropName is created
Current: Assertion failure

The assertion failure is related to ref counting I guess:

Assertion failed: (!isFree() && "Value not present"), function value, file hermes_vtable.cpp, line 95.
@robik robik added the bug Something isn't working label Nov 6, 2024
@neildhar
Copy link
Contributor

neildhar commented Nov 6, 2024

Hey @robik, it's great to see that you're experimenting with the C API. Like with JSI, the C-API requires that all references you create are freed before the runtime is destroyed. You can do this by calling the invalidate vtable method on HermesABIManagedPointer.

In your example, both propName and hermesStr need to be freed before calling release on the runtime.

@neildhar neildhar added need more info Awating additional info before proceeding and removed bug Something isn't working labels Nov 6, 2024
@robik
Copy link
Contributor Author

robik commented Nov 7, 2024

Hey @neildhar, thanks for clarifying!

I skipped invalidation as it is not relevant in this case.

The app crashes at call to the create_propname_from_string function, never reaching runtime release call

@neildhar
Copy link
Contributor

neildhar commented Nov 8, 2024

Ah, I see, there seems to be another issue. The line

    struct HermesABIString* hermesStr = (struct HermesABIString*)strOrError.ptr_or_error;

Should actually be

    struct HermesABIString hermesStr{(struct HermesABIManagedPointer*)strOrError.ptr_or_error};

ptr_or_error is a HermesABIManagedPointer *, which is supposed to be a field of a HermesABIString, not a pointer to a HermesABIString. (similarly for the PropNameID)

@tmikov
Copy link
Contributor

tmikov commented Nov 8, 2024

In our defense, this C ABI wasn't meant to be used directly, but to build other APIs, JSI in particular, on top of it :-)

Do we need to improve the documentation?

@robik
Copy link
Contributor Author

robik commented Nov 8, 2024

Okay, thank you very much! This makes sense.

I did not use documentation, just (poorly) reverse engineered it. Thanks for the clarification!

@robik robik closed this as completed Nov 8, 2024
@neildhar
Copy link
Contributor

neildhar commented Nov 8, 2024

@robik You may also find it helpful to refer to the JSI implementation in HermesABIRuntime.cpp, and to use the C++ helper functions in HermesABIHelpers.h. Apart from that, it's exciting that you're trying out the C-API, and we're keen to hear how it goes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need more info Awating additional info before proceeding
Projects
None yet
Development

No branches or pull requests

3 participants