Closed hannojg closed 2 weeks ago
hannojg
commented
2 weeks ago Update: at first i thought this would be only an issue when building react native from source, but I just realized that its always happening in release mode on android (for the example app at least)
mrousavy
commented
2 weeks ago mrousavy
commented
2 weeks ago Hanno can you try to run each test individually to find out which test is actually crashing?
mrousavy
commented
2 weeks ago And second thing we can try is to edit NitroDefines.hpp to just always set NITRO_DEBUG to 0/undef'd, even in debug mode.
Then run the app in debug and see if it still crashes. If no, then maybe there is something happening in Hermes with NativeState.
hannojg
commented
2 weeks ago sure, let me test!
hannojg
commented
2 weeks ago Its the first two tests that are crashing:
HyrbidObject.prototype is validHyrbidObject.prototype.prototype is validThe other tests seem to all work:
https://github.com/user-attachments/assets/fe27694e-aefc-4746-beef-b1bea660f8a3
mrousavy
commented
2 weeks ago Hm, could this be a Hermes bug? 🙈
The code that this test essentially runs is:
const prototype = Object.getPrototypeOf(testObject)
typeof prototype === 'object'
prototype[simpleFunc] != null || Object.keys(prototype).includes("simpleFunc")Where testObject is an object created by Nitro via Object.create(XXX) (XXX being the prototype we want to test here) with NativeState.
cc @tmikov anything I'm doing wrong here?
hannojg
commented
2 weeks ago @mrousavy if i remove NITRO_DEBUG and run in debug mode the same tests are crashing!
mrousavy
commented
2 weeks ago wait whaaat
This function didn't throw when NITRO_DEBUG was true, so why should it suddenly fail if we remove those checks?
..or do you think there's some kind of flattening going on? The object has a single __type property in debug, which is not available on release: https://github.com/mrousavy/nitro/blob/6682b2d6fde2ecd4c479ed13e17d7d325c82dfc2/packages/react-native-nitro-modules/cpp/core/HybridObject.cpp#L74-L77
hannojg
commented
2 weeks ago I think this isn't the place where its crashing:
The place where its actually crashing is here:
mrousavy
commented
2 weeks ago Yes I know that it's crashing in getNativeState because apparently hasNativeState is false here.
I'm trying to figure out why hasNativeState is false; for that I was wondering where we actually have a different code-path than before (NITRO_DEBUG on vs off).
The only place where there is actually a different code-path is where I set a __type property on the object that has a NativeState or Prototype - only here we have a different code-path in NITRO_DEBUG than in release.
Is there maybe something happening with the NativeState when the object does not have any properties?
You can check this by reverting all changes again and just removing those 4 lines here and run it again: https://github.com/mrousavy/nitro/blob/6682b2d6fde2ecd4c479ed13e17d7d325c82dfc2/packages/react-native-nitro-modules/cpp/core/HybridObject.cpp#L74-L77
hannojg
commented
2 weeks ago Its still working in debug with the mentioned lines removed 🤔
mrousavy
commented
2 weeks ago Hm, maybe it is trying to call toString() on the Prototype which does not have a bound this/NativeState?
mrousavy
commented
2 weeks ago I finally figured it out after hours of debugging.
In the Example app, we run tests and each tests then logs the result of the test.
The first two tests of the example app tested the prototype of a HybridObject - so that's Object.getPrototypeOf(hybridObject).
All HybridObjects can be logged by calling hybridObject.toString(), and toString() is a method defined on the highest Prototype of the HybridObject prototype chain - HybridObject itself.
The way it works is simple - it gets the NativeState, unwraps it to the C++ class HybridObject, and calls HybridObject::toString() - which just returns the C++ _name.
This is a problem though - for HybridObjects (objects with NativeState) this works, but if we call it on the prototype directly it does not have a NativeState - because it's the prototype, not an actual instance of it.
toString() throws in debugCalling toString() on an unbound prototype (the prototype itself, without NativeState) this throws , and in release it fully crashes the app.
The reason this worked before in debug is because of this code; https://github.com/mrousavy/nitro/blob/328a6e12519424e326550c1589d1bd546040952c/example/src/utils.ts#L20-L29
We caught the error, and just returned { [Object] ... } in this case. The error was silently swallowed.
assert(...) didn't catch this in releaseWell, dumb mistake on my end - assert is compiled out in release.
value.toString() workedApparently this also threw, which silently caught the error. No idea why.
Now, I added some checks to my toString() function in the example to prevent calling toString() on an unbound prototype (so a object without NativeState).
This now no longer crashes and just logs something different for prototypes, which is absolutely desireable.
Alternatively I could've made toString() work without NativeState, but that would've been a lot of code complexity, and potentially also a minor performance hit for an absolutely unnecessary feature; who in their right mind would stringify an object's prototype in a release build?
What's happening?
Running the example app in release mode causes a crash
Reproduceable Code
Relevant log output
Device
android emulator / device doesn't matter
Nitro Modules Version
0.15.0
Nitrogen Version
0.15.0
Can you reproduce this issue in the Nitro Example app here?
Yes, I can reproduce the same issue in the Example app here
Additional information