JNI error on Android when passing Enum to native method

[equeim]

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:

A/queim.tremotes: java_vm_ext.cc:577] JNI DETECTED ERROR IN APPLICATION: attempt to access field int org.bytedeco.javacpp.tools.Generator$IntEnum.value from an object argument of type org.equeim.libtremotesf.Server$ProxyType: 0x6b
    java_vm_ext.cc:577]     in call to GetIntField
    java_vm_ext.cc:577]     from org.equeim.libtremotesf.Server org.equeim.libtremotesf.Server.proxyType(org.equeim.libtremotesf.Server$ProxyType)
A/queim.tremotes: runtime.cc:655] Runtime aborting...

That's what generated JNI method looks like:

JNIEXPORT jobject JNICALL Java_org_equeim_libtremotesf_Server_proxyType__Lorg_equeim_libtremotesf_Server_00024ProxyType_2(JNIEnv* env, jobject obj, jobject arg0) {
    libtremotesf::Server* ptr = (libtremotesf::Server*)jlong_to_ptr(env->GetLongField(obj, JavaCPP_addressFID));
    if (ptr == NULL) {
        env->ThrowNew(JavaCPP_getClass(env, 7), "This pointer address is NULL.");
        return 0;
    }
    jlong position = env->GetLongField(obj, JavaCPP_positionFID);
    ptr += position;
    if (arg0 == NULL) {
        env->ThrowNew(JavaCPP_getClass(env, 7), "Enum for argument 0 is NULL.");
        return 0;
    }
    libtremotesf::Server::ProxyType val0 = (libtremotesf::Server::ProxyType)env->GetIntField(arg0, JavaCPP_intValueFID);
    jobject rarg = obj;
    ptr->proxyType = val0;
    return rarg;
}

This happens on Android 11 on Pixel 4A 5G.

[equeim]

Seems that crash is caused by CheckJNI mode that is enabled for debuggable apps and in Android Emulator: https://developer.android.com/training/articles/perf-jni#extended-checking

[saudet]

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 ConcurrentHashMap in a factory method like <E extends Enum<E>> E Loader.enumof(Class<E> e, long value). This issue with Android would be another good reason to move all that there and use reflection to get things done instead.

[HGuillemet]

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 sun.reflect missing package. We thus probably have to use Unsafe).

Could we limit the use of enumerate() to the enum which values are, by design, all known at compile time ? This should cover 95% of cases. This would also allow us to use a faster normal HashMap.

[saudet]

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

[equeim]

Now, this issue here is more about reading the values from enum objects in JNI.

CheckJNI also affect passing enums from native to Java, since javacpp creates a new instance of enum class and sets value field from Generator.IntEnum class (or other) in generated JNI method.

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 intern() on both of them, and it is very easy to forget to do so, especially considering that Java specification says that:

An enum type has no instances other than those defined by its enum constants. It is a compile-time error to attempt to explicitly instantiate an enum type (§15.9.1).

In addition to the compile-time error, three further mechanisms ensure that no instances of an enum type exist beyond those defined by its enum constants:

• The final clone method in Enum ensures that enum constants can never be cloned.
• Reflective instantiation of enum types is prohibited.
• Special treatment by the serialization mechanism ensures that duplicate instances are never created as a result of deserialization.

Granted, specification talks about Java code specifically, but it means that Java developers always expect enum comparisons to just work (including reference comparison via ==) and javacpp break that expectation.

[saudet]

Now, this issue here is more about reading the values from enum objects in JNI.

CheckJNI also affect passing enums from native to Java, since javacpp creates a new instance of enum class and sets value field from Generator.IntEnum class (or other) in generated JNI method.

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.

Granted, specification talks about Java code specifically, but it means that Java developers always expect enum comparisons to just work (including reference comparison via ==) and javacpp break that expectation.

Right, that's what @HGuillemet wants to fix by avoiding duplicate instances as I mention above at https://github.com/bytedeco/javacpp/issues/481#issuecomment-850064285.

[equeim]

It prevents creating new instances of enum classes??

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 value field values from Java enums and their names into native code, and then adds a couple of internal native functions to lookup Java enum objects from native value (lazy initializing them using GetStaticObjectField) and to lookup hardcoded native value from Java enum object.

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.

[saudet]

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!

[HGuillemet]

I also have started to modify how enum is handled. Please hold on. No need to make the work twice. Concerning @saudet comment: The enumerate info tells the parser to map a C++ enum to a Java enum. So it won't bother me if its use must be limited to C++ enums which fullfull the Java enum contract (all values known at compile time). But I don't know how ofen "live" enum values occur in C++. Anyway I'll try to implement it in Java with Unsafe.

[HGuillemet]

Here is my attempt. A new class EnumValueMap register the mapping (Long, Class) => Enum and the changes in Generator are mostly limited to the replacement of the JNI enum instantiations by a call to EnumValueMap.getEnum which returns an existing instance. If there is none, one is created with Unsafe. I haven't tested on Android.

[HGuillemet]

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.

[equeim]

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.

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

[HGuillemet]

Does it fail when reading the field too ? Not just when writing it ?

[equeim]

Yes, that's what original error message is about. When GetIntField is called on enum object with method ID from IntEnum, CheckJNI crashes app.

[equeim]

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 (I have no idea how portable it though. Doesn't work wiith OpenJDK). BTW, we can also call private enum constructors from JNI since JNI ignores access modifiers (it works on Android with CheckJNI too), and this is kinda legal (at least for normal classes, not sure about enums).

[HGuillemet]

Yes, that's what original error message is about. When GetIntField is called on enum object with method ID from IntEnum, CheckJNI crashes app.

Ok sorry, I missed this part. We could then add a EnumValueMap.getValue(Enum e). It's just too bad we have to do these Java/C roundtrips when conceptually the enum conversion work on the arguments and on the return value could be done in some Java stub method.

Or we need to get the FieldID for each Enum class in JNI like you did in your solution, if I understood well.

[saudet]

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.

It's not a slight overhead, calling Java methods is the most expensive JNI operation by far. That's not a solution.

[saudet]

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.

[HGuillemet]

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 final static int VALUE = 2 as a replacement for Enums.

[equeim]

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.

[saudet]

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.

[Clement-Devevey]

just ran into the same issue, tried to do adb shell setprop dalvik.vm.checkjni false but no matter what, it still crashes. The only fix as of now is to run the APK in release mode?