-
Notifications
You must be signed in to change notification settings - Fork 26
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
Crash in android release mode during runtime in example app #314
Comments
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) |
|
Hanno can you try to run each test individually to find out which test is actually crashing? |
And second thing we can try is to edit Then run the app in debug and see if it still crashes. If no, then maybe there is something happening in Hermes with NativeState. |
sure, let me test! |
Its the first two tests that are crashing:
The other tests seem to all work: untitled.mp4 |
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 cc @tmikov anything I'm doing wrong here? |
@mrousavy if i remove |
wait whaaat This function didn't throw when nitro/packages/react-native-nitro-modules/cpp/core/HybridFunction.hpp Lines 170 to 221 in 6682b2d
..or do you think there's some kind of flattening going on? The object has a single nitro/packages/react-native-nitro-modules/cpp/core/HybridObject.cpp Lines 74 to 77 in 6682b2d
|
I think this isn't the place where its crashing: nitro/packages/react-native-nitro-modules/cpp/core/HybridFunction.hpp Lines 170 to 221 in 6682b2d
The place where its actually crashing is here: |
Yes I know that it's crashing in I'm trying to figure out why Is there maybe something happening with the nitro/packages/react-native-nitro-modules/cpp/core/HybridObject.cpp Lines 74 to 77 in 6682b2d
|
Its still working in debug with the mentioned lines removed 🤔 |
Hm, maybe it is trying to call |
I finally figured it out after hours of debugging. ContextIn the Example app, we run tests and each tests then logs the result of the test. All HybridObjects can be logged by calling The way it works is simple - it gets the This is a problem though - for HybridObjects (objects with
|
try { | |
if ('toString' in value) { | |
const string = value.toString() | |
if (string !== '[object Object]') return string | |
} | |
return `{ ${value} ${Object.keys(value).join(', ')} }` | |
} catch { | |
// toString() threw - maybe because we accessed it on a prototype. | |
return `{ [Object] ${Object.keys(value).join(', ')} }` | |
} |
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?
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
The text was updated successfully, but these errors were encountered: