getNativeState(runtime) asserts sometimes in release builds even tho it has a NativeState

[mrousavy]

Bug Description

Hey!

I'm working on Nitro, which uses NativeState for every object.
An object in Nitro has a proper prototype chain as well, which just consists of objects with HostFunctions, those HostFunctions unwrap the NativeState from the this object (which we set before).

This works perfectly fine in Debug, and works also for pretty much every production app in Release.

But in the Nitro Example app (example/) I have one test which does Object.getPrototypeOf(object) and checks if the protoytpe contains a function we expect it to have.

For some reason, this one test crashes in Release because getNativeState(object) asserts at hasNativeState(object). But the object definitely has NativeState.

11:55:07.913 libc                     A  Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x15ba3ffff0 in tid 24735 (mqt_v_js), pid 24686 (lo.nitroexample)
11:55:07.981 DEBUG                    A  pid: 24686, tid: 24735, name: mqt_v_js  >>> com.margelo.nitroexample <<<
11:55:08.073 DEBUG                    A      #00 pc 00000000000b176c  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #01 pc 000000000005fe24  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x5fc000) (_ZN7margelo5nitro14HybridFunction26getHybridObjectNativeStateINS0_12HybridObjectEEENSt6__ndk110shared_ptrIT_EERN8facebook3jsi7RuntimeERKNS9_5ValueENS0_12FunctionKindERKNS4_12basic_stringIcNS4_11char_traitsIcEENS4_9allocatorIcEEEE+104)
11:55:08.073 DEBUG                    A      #02 pc 000000000005f7b0  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x5fc000) (_ZZN7margelo5nitro14HybridFunction20createHybridFunctionINS0_12HybridObjectENSt6__ndk112basic_stringIcNS4_11char_traitsIcEENS4_9allocatorIcEEEEJEEES1_RKSA_MT_FT0_DpT1_ENS0_12FunctionKindEENKUlRN8facebook3jsi7RuntimeERKNSL_5ValueEPSP_mE_clESN_SQ_SR_m+84)
11:55:08.073 DEBUG                    A      #03 pc 00000000000bb7f0  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #04 pc 00000000000bb450  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #05 pc 00000000000c2528  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #06 pc 00000000000d2874  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #07 pc 00000000000d4224  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #08 pc 00000000000d38e0  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #09 pc 00000000000c2e38  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #10 pc 0000000000136c90  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #11 pc 00000000000c2528  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #12 pc 00000000000d2874  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #13 pc 00000000000d4224  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #14 pc 00000000000d38e0  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #15 pc 00000000000c2628  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #16 pc 00000000000c0f04  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #17 pc 0000000000109254  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #18 pc 00000000000af970  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x83c000)
11:55:08.073 DEBUG                    A      #19 pc 00000000004de26c  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0xbac000) (facebook::react::RuntimeScheduler_Modern::performMicrotaskCheckpoint(facebook::jsi::Runtime&)+60)
11:55:08.073 DEBUG                    A      #20 pc 00000000004dd8d4  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0xbac000) (_ZN8facebook5react23RuntimeScheduler_Modern16runEventLoopTickERNS_3jsi7RuntimeERNS0_4TaskENSt6__ndk16chrono10time_pointINS8_12steady_clockENS8_8durationIxNS7_5ratioILl1ELl1000000000EEEEEEE+128)
11:55:08.073 DEBUG                    A      #21 pc 00000000004ddc2c  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0xbac000) (facebook::react::RuntimeScheduler_Modern::runEventLoop(facebook::jsi::Runtime&, bool)+144)
11:55:08.073 DEBUG                    A      #22 pc 0000000000349e28  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0xbac000)
11:55:08.073 DEBUG                    A      #23 pc 000000000051b7b4  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0xbac000)
11:55:08.073 DEBUG                    A      #24 pc 0000000000019804  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x80c000) (_ZN8facebook3jni6detail13MethodWrapperIMNS0_15JNativeRunnableEFvvEXadL_ZNS3_3runEvEES3_vJEE8dispatchENS0_9alias_refIPNS1_8JTypeForINS0_11HybridClassIS3_NS0_9JRunnableEE8JavaPartESA_vE11_javaobjectEEE+72)
11:55:08.073 DEBUG                    A      #25 pc 0000000000019744  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/base.apk (offset 0x80c000) (_ZN8facebook3jni6detail15FunctionWrapperIPFvNS0_9alias_refIPNS1_8JTypeForINS0_11HybridClassINS0_15JNativeRunnableENS0_9JRunnableEE8JavaPartES7_vE11_javaobjectEEEESC_vJEE4callEP7_JNIEnvP8_jobjectSF_+60)
11:55:08.073 DEBUG                    A      #26 pc 00000000000342bc  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/oat/arm64/base.odex (offset 0x33000) (com.facebook.jni.NativeRunnable.run [DEDUPED]+124)
11:55:08.073 DEBUG                    A      #34 pc 0000000000277ea4  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/oat/arm64/base.vdex (com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage)
11:55:08.073 DEBUG                    A      #45 pc 00000000002782a0  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/oat/arm64/base.vdex (com.facebook.react.bridge.queue.MessageQueueThreadImpl.lambda$startNewBackgroundThread$2+70)
11:55:08.073 DEBUG                    A      #51 pc 0000000000277f0a  /data/app/com.margelo.nitroexample-JPuPElpp1_I62d5ADOoIWA==/oat/arm64/base.vdex (com.facebook.react.bridge.queue.MessageQueueThreadImpl$$ExternalSyntheticLambda1.run+4)

Here's some interesting observations:

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 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):
React Native version: 0.76
OS: Android 9
Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64): arm64-v8a (Huawei P10)

Steps To Reproduce

Make sure to do this in a RELEASE Build:

1. Create a jsi::HostFunction that calls getNativeState on thisObject (second argument)
2. Add jsi::HostFunction to an Object, create a new Object from that Object as prototype (Object.create(...)) which binds it's this
3. Set NativeState on the new object which has a prototype
4. Call jsi::HostFunction - this should now call getNativeState on this (which is the object that 100% has NativeState)

code example:

auto ObjectCreateFunc = runtime.global().……;
auto hostFunc = [](…, const jsi::Value& thisValue, …) {
  auto nativeState = thisValue.getNativeState(runtime);
};
jsi::Object prototype(runtime);
prototype.setProperty(runtime, "someFunc", hostFunc);

jsi::Object object = ObjectCreateFunc(runtime, prototype);
object.setNativeState(runtime, myNativeState);
// now call object.someFunc(..) from JS!

The Expected Behavior

I expect it to behave the same in debug and in release.

[mrousavy]

I'm sorry for not having a minimal reproduction example, but you can run Nitro Example (example/) in release and just hit run on the first or second test.

This PR highlights the change required to make it work fine in release; mrousavy/nitro#321 - and without it it will just crash in release.

[neildhar]

Hey @mrousavy, from your description, it sounds like one possibility is that there's some uninitialised memory access happening somewhere, which would explain the unpredictable behaviour you're seeing.

Two things that would be useful:

1. What happens if you remove the hasNativeState check in getHybridObjectNativeState in debug mode? Does Hermes then assert?
2. Have you tried building with ASan?

[mrousavy]

I'll work on dumbing down the repro to figure this one out.

The only possible scenario I can think of right now is that maybe my example tried to console.log the actual prototype object which contains a jsi::HostFunction for toString() - so it called toString(), but toString() is also implemented with unwrapping NativeState.

But that doesn't explain why assert(false) didn't crash the app. I'll investigate further.

[neildhar]

assert(false) would typically be compiled out in release mode. Are you doing something special to ensure it remains in the final build? If not, abort() would be more appropriate.

[mrousavy]

God dammit, I finally figured it out after hours of debugging.

TL;DR: It's not a Hermes bug - it's my stupidity. Sorry for creating an issue.

Context

In the Nitro 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 debug

Calling 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.

Why assert(...) didn't catch this in release

Well, dumb mistake on my end - assert is compiled out in release.

Why value.toString() worked

Apparently this also threw, which silently caught the error. No idea why.

Solution

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?