upgrade jni v0.21 #63
Conversation
|
Thanks for the PR! It looks like the changes here aren't currently compiling for Android. Once CI is passing I'm happy to take a look through this.
That's very strange, and definitely something I'll look into on the side. We use this exact same JNI-abstraction inside 1Password's Android app (this code was copy/pasted out) and we've never seen this issue AFAICT. If you have any other JNI code in your app, it might be worth checking if its having a poor interaction with |
We're using |
|
It would be great if |
| @@ -74,7 +74,7 @@ fn global() -> &'static Global { | |||
| /// nothing else in your application needs access the Android runtime. | |||
| /// | |||
| /// Initialization must be done before any verification is attempted. | |||
| pub fn init_hosted(env: &JNIEnv, context: JObject) -> Result<(), JNIError> { | |||
| pub fn init_hosted(env: &mut JNIEnv, context: &JObject) -> Result<(), JNIError> { | |||
There was a problem hiding this comment.
Note: This is a breaking change and is probably going to require a new release.
cc @cpu
| pub(super) fn application_context(&self) -> GlobalRef { | ||
| self.context.clone() |
There was a problem hiding this comment.
Question: Can this use as_obj instead for clarity? The result of application_context() is passed as an object.
There was a problem hiding this comment.
I'm afraid not. The context.env is a mutable reference, so other context fields must be full owned.
| } | ||
|
|
||
| /// Load a class from the application class loader | ||
| /// | ||
| /// This should be used instead of `JNIEnv::find_class` to ensure all classes | ||
| /// in the application can be found. | ||
| pub(super) fn load_class(&self, name: &str) -> Result<JClass<'a>, Error> { | ||
| pub(super) fn load_class(&mut self, name: &str) -> Result<JClass<'a>, Error> { | ||
| let loader = self.loader.clone(); |
There was a problem hiding this comment.
Suggestion: This should be able to stay inline? Its .as_ref() call should produce the JObject that's needed.
There was a problem hiding this comment.
I think it's the same problem as the above question.
|
|
||
| env.pop_local_frame(JObject::null())?; | ||
| unsafe { context.env.pop_local_frame(&JObject::null()) }?; |
There was a problem hiding this comment.
This should have a // SAFETY: comment attached to it now, but I'm attempting to determine the soundness requirements. The documentation says local references become invalid, and I assume those are JVM references and not anything related to native code.
Given that we only return native Rust objects from f, I think we follow that rule correctly. If you happen to know, does that sound reasonable to you as well?
There was a problem hiding this comment.
Yes, I agree. Maybe it's better to declare T: 'static to make sure it's native rust object?
There was a problem hiding this comment.
That sounds like a good idea to me 👍
We also use a multi-threaded Tokio runtime at work and haven't encountered this, so I am curious where the difference is. @flisky Are you able to/do you mind sharing your Android context initialization code? While looking at the SO post in your linked GitHub comment, some things stood out to me as strange. Notably its unclear why they are wrangling the list of JVMs and current thread instead of passing an environment from Kotlin/Java and using |
|
I create a demo project for this, and you can take a look at https://github.com/flisky/jnidemo/blob/main/src/android.rs |
|
Hey @complexspaces, I'm back on this issue, and I finally find the root case. During several days debugging, it turns out We're using
Now, we're calling Thanks this bug we find out something to optimized. Hope it help:) (By the way, |
jni v0.19 doesn't work for me.I use tokio multi threads runtime, and our app is killed by system -It turns outjava_vm::get_env()goes null even after callingAttachCurrentThreadin a few threads, while other threads are okay.I have no idea. However, upgrade to jni v0.21.0 eliminates the problem for me.fixes #22