-
Notifications
You must be signed in to change notification settings - Fork 576
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
JNI error on Android when passing Enum to native method #481
Comments
Seems that crash is caused by CheckJNI mode that is enabled for debuggable apps and in Android Emulator: |
Looks like Android doesn't like the hack that I've come up with for enum classes. @HGuillemet was already thinking of enhancing that to avoid duplicate instances with the same enum value using maybe a |
The reflection trick to modify the value list of an enum is really hacky. It won't surprise me if it failed on Android too (maybe because of Could we limit the use of |
We don't need to modify the list of values, just create new instances with arbitrary values, and it should work fine. Now, this issue here is more about reading the values from enum objects in JNI. We'd have to treat each enum separately, which complicates things, but it should work fine. If someone wants to work on this right now, I'd be glad to accept patches, but I'm not putting this on my "to-do list". |
CheckJNI also affect passing enums from native to Java, since javacpp creates a new instance of enum class and sets And this behaviour is really confusing too, to say the least. There is no way to compare two enums returned from native method unless you called
Granted, specification talks about Java code specifically, but it means that Java developers always expect enum comparisons to just work (including reference comparison via |
It prevents creating new instances of enum classes?? If that's the case, I don't see any way to work around that limitation. We'd need to speak to the Android team about that.
Right, that's what @HGuillemet wants to fix by avoiding duplicate instances as I mention above at #481 (comment). |
No, I meant that it prevents setting field with field id from wrong class. Sorry for confusion. I tried to rewrite enums code in Generator to use the same singleton enum values created in Java in JNI. It hardcodes It has one major difference with current implementation, though. Since it can only return (or take) Java enum constants, if unknown enum value is passed from native to Java, it will have to throw or return null. Current implementation always returns new instance of enum, i.e. it is always non-null. That makes it a breaking change. |
Right, we still need a way to create new instances when the value we need to return isn't part of the list at compile time. We can't return null, we need to return the value! |
I also have started to modify how enum is handled. Please hold on. No need to make the work twice. |
Here is my attempt. |
Note that we could push the logic a bit further and add a Map Enum => value. This would remove the need for a value field in the Enum and for the LongEnum, IntEnum,... proxy classes. But i'm not sure it's worth the slight overhead when getting the value from an Enum, and the possible breaking of existing codes accessing the value field. |
IntEnum and its family doesn't work on Android with CheckJNI enabled so we need to find another way to get native value from Java enum object (and we still can keep value field for compatibility). |
Does it fail when reading the field too ? Not just when writing it ? |
Yes, that's what original error message is about. When GetIntField is called on enum object with method ID from IntEnum, CheckJNI crashes app. |
I just checked or your solution for instantiating enum object with Unsafe on Android, and it works. Weirdly enough, doing it via normal reflection works too even though Java spec prohibits this ( |
Ok sorry, I missed this part. Or we need to get the FieldID for each Enum class in JNI like you did in your solution, if I understood well. |
It's not a slight overhead, calling Java methods is the most expensive JNI operation by far. That's not a solution. |
Since enum just doesn't work well at all with JNI, how about we move everything related to enum out of JNI? Parser already creates non-trivial enum classes from scratch that work without JNI, so it's not something that people do without the parser anyway, and the only additional thing we would need to produce are wrappers like this: MyEnum someFunction(MyEnum input) { return MyEnum.valueOf(someFunction(input.getValue())); }
@Cast("MyEnum") native int someFunction(@Cast("MyEnum") int input); How does that look? I'm not sure yet how we'd go on implementing that, but I think the result would be acceptable. And we wouldn't need to remove anything from Generator. We can just leave what's there as deprecated functionality for backward compatibility. |
It looks like the way to go. If this mechanism of wrapping/unwrapping arguments and return values Java-side is implementable, I believe it could be interesting to generalize it to other non-enum cases. If it happens to not be easily implementable, we can always stick with the good old abstract class with constants such as |
As an alternative, we still could implement this in C++ by extending my solution (or similar one) to add allocation of new enum objects with values that don't exist on Java enum side. It could be even easier than in Java, since in C++ we can directly invoke enum's private constructors using NewObject() insead of using sun.misc.Unsafe. This will, however, mean that we will have to keep cache of allocated instances on C++ side (can we use std containers such as std::map?) unless we are ok with always creating new instances for unknown values. |
When we always have overloads with corresponding primitive types, we don't need to worry about allocating new instances. Users can just pass and get values that are not in enum classes with those, and that's it. I think that sounds like the most usable strategy. |
just ran into the same issue, tried to do |
I have enum class that was created by Parser by using
new Info("name").enumerate()
. When I pass it to native method (field setter), my app crashes with following error:That's what generated JNI method looks like:
This happens on Android 11 on Pixel 4A 5G.
The text was updated successfully, but these errors were encountered: