mrousavy / nitro

🔥 Insanely fast native C++, Swift or Kotlin modules with a statically compiled binding layer to JSI
https://nitro.margelo.com
MIT License
642 stars 22 forks source link

fix: Fix hard crash on Android in Prototype tests in release #321

Closed mrousavy closed 1 week ago

mrousavy commented 1 week ago

I honestly don't know why this crashes.

But now we check for hasNativeState() also in release builds, not only debug. This will then throw if the function is called on an unbound object (no this/NativeState).

I can only explain this being called by console.log'ing an unbound object (so just the Prototype) - which has an overridden toString function but that requires it to be bound. I guess if console.log calls toString() and toString() throws, it will catch the error internally.. idk man.

no real production app will call toString() on a prototype in release, so I'm not sure if this is the right approach.

vercel[bot] commented 1 week ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **nitro-docs** | ⬜️ Skipped ([Inspect](https://vercel.com/margelo/nitro-docs/6C7PQnujrC5pNuXaoan65EcKPMYQ)) | | | Nov 12, 2024 10:22am |
mrousavy commented 1 week ago

This code here works:

Screenshot 2024-11-12 at 11 43 44

and this code here crashes:

Screenshot 2024-11-12 at 11 44 11

..same as if I would just not call hasNativeState at all, this also crashes:

Screenshot 2024-11-12 at 11 44 22

So what makes it work? The assert(..) can't be hit, can it? It would crash the app...

mrousavy commented 1 week ago

Okay some more insights:

  1. This code here crashes:
    auto nativeState = object.getNativeState(runtime);
  2. This code here also crashes:
    object.hasNativeState(runtime);
    auto nativeState = object.getNativeState(runtime);
  3. This code here WORKS, but it doesn't assert/crash the app???:
    if (!object.hasNativeState(runtime)) {
      assert(false);
    }
    auto nativeState = object.getNativeState(runtime);
  4. This code here WORKS:
    if (!object.hasNativeState(runtime)) {
       value.toString(runtime);
    }
    auto nativeState = object.getNativeState(runtime);

So maybe the object is somehow flattened or lazy evaluated in a sense that NativeState isn't properly available?

I am still kinda baffled that the assert does not crash the app, maybe object.hasNativeState(...) is not the same as the internal assert of object.getNativeState(...)?

cc @hannojg @tmikov

mrousavy commented 1 week ago

Created an issue in the Hermes repo for now: https://github.com/facebook/hermes/issues/1564

mrousavy commented 1 week ago

I think this can be fixed by just adjusting the code in the example app. No need to slow down all function calls for users by leaving this check in release builds