upgrade jni v0.21 #63

flisky · 2024-01-16T11:26:29Z

~~jni v0.19 doesn't work for me.~~

~~I use tokio multi threads runtime, and our app is killed by system -~~

e.android.debug: java_vm_ext.cc:594] JNI DETECTED ERROR IN APPLICATION: a thread (tid 5917 is making JNI calls without being attached
e.android.debug: java_vm_ext.cc:594]     in call to PushLocalFrame

~~It turns out java_vm::get_env() goes null even after calling AttachCurrentThread in 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

complexspaces · 2024-01-16T21:44:05Z

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.

It turns out java_vm::get_env() goes null even after calling AttachCurrentThread in a few threads, while other threads are okay.
I have no idea. However, upgrade to jni v0.21.0 eliminates the problem for me.

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 rustls-platform-verifier.

flisky · 2024-01-17T02:16:42Z

If you have any other JNI code in your app, it might be worth checking if its having a poor interaction with rustls-platform-verifier.

We're using mozilla/uniffi which uses JNA actually. We have to configure JNI environment mannually, and mozilla/uniffi-rs#1778 (comment) inspires us.

complexspaces · 2024-01-22T22:51:04Z

It would be great if jni could make another release before this merges. I don't want to push a very outdated version of windows-sys into everyone's dependency trees.

complexspaces · 2024-01-22T22:51:56Z

rustls-platform-verifier/src/android.rs

@@ -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> {


Note: This is a breaking change and is probably going to require a new release.

cc @cpu

complexspaces · 2024-01-22T22:56:42Z

rustls-platform-verifier/src/android.rs

+    pub(super) fn application_context(&self) -> GlobalRef {
+        self.context.clone()


Question: Can this use as_obj instead for clarity? The result of application_context() is passed as an object.

I'm afraid not. The context.env is a mutable reference, so other context fields must be full owned.

complexspaces · 2024-01-22T22:59:19Z

rustls-platform-verifier/src/android.rs

    }

    /// 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();


Suggestion: This should be able to stay inline? Its .as_ref() call should produce the JObject that's needed.

I think it's the same problem as the above question.

complexspaces · 2024-01-23T00:01:35Z

rustls-platform-verifier/src/android.rs


-    env.pop_local_frame(JObject::null())?;
+    unsafe { context.env.pop_local_frame(&JObject::null()) }?;


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?

Yes, I agree. Maybe it's better to declare T: 'static to make sure it's native rust object?

That sounds like a good idea to me 👍

complexspaces · 2024-01-23T00:19:09Z

It turns out java_vm::get_env() goes null even after calling AttachCurrentThread in a few threads, while other threads are okay.
I have no idea. However, upgrade to jni v0.21.0 eliminates the problem for 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 JniEnv::get_java_vm (which calls GetJavaVM) instead. You can see an example of this here for this crate's tests.

flisky · 2024-01-23T15:25:53Z

I create a demo project for this, and you can take a look at https://github.com/flisky/jnidemo/blob/main/src/android.rs
Unfortunately, I cannot reproduce with that project. And I'll tweak it in next few days.

flisky · 2024-05-22T15:47:11Z

Hey @complexspaces, I'm back on this issue, and I finally find the root case.

During several days debugging, it turns out jna will also attach & detach thread if current thread is not attached.

We're usinguniffi callback to log tracing events, and the race happens:

logging start (jna attached)
verifier start (jni attached )
logging finished (jna detached)
verifying (jni calling).

Now, we're calling attach_current_thread_permanently manully when https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.on_thread_start, and make sure jni's logging disabled.

Thanks this bug we find out something to optimized. Hope it help:)

(By the way, jna & jni can be mixed used, because java loads native library only once.)

cpu · 2024-09-16T17:53:01Z

I'm not sure I understand the current state of this PR? Are changes required? Is the update no longer a priority since a workaround was discovered?

flisky · 2024-09-17T01:51:58Z

I think it's waiting for upstreaming dependency coordination: jni-rs/jni-rs#513, and yes, it's not a priority.
We could revisit it when upsteam goes ready.

flisky changed the title ~~update jni v0.21~~ upgrade jni v0.21 Jan 16, 2024

chore: update jni v0.21

4a65853

flisky force-pushed the rel-0.1 branch from 8dd5ee4 to 4a65853 Compare January 16, 2024 11:45

fix tests

3040d5c

run cargo fmt

69795c5

complexspaces requested changes Jan 23, 2024

View reviewed changes

chore: add safety note

77dc137

voczi mentioned this pull request Mar 22, 2024

[BUG]: Self Signed TLS certificate is no longer trusted for AnkiDroid after new update (works well with 2.16.5) ankidroid/Anki-Android#14686

Closed

4 tasks

This was referenced May 29, 2024

docs(kotlin): mention AttachCurrentThread mozilla/uniffi-rs#2133

Merged

Reuse existing tokio runtime if one exists smol-rs/async-compat#30

Merged

complexspaces mentioned this pull request Aug 23, 2024

Consider migration from winapi-rs -> windows #128

Closed

flisky closed this Sep 17, 2024

Cyannide mentioned this pull request Nov 9, 2024

Update jni to 0.21 #151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade jni v0.21 #63

upgrade jni v0.21 #63

flisky commented Jan 16, 2024 •

edited

Loading

complexspaces commented Jan 16, 2024

flisky commented Jan 17, 2024

complexspaces commented Jan 22, 2024

complexspaces Jan 22, 2024

complexspaces Jan 22, 2024

flisky Jan 24, 2024

complexspaces Jan 22, 2024

flisky Jan 24, 2024

complexspaces Jan 23, 2024

flisky Jan 24, 2024

complexspaces Jan 24, 2024

complexspaces commented Jan 23, 2024

flisky commented Jan 23, 2024

flisky commented May 22, 2024 •

edited

Loading

cpu commented Sep 16, 2024

flisky commented Sep 17, 2024

		pub(super) fn application_context(&self) -> GlobalRef {
		self.context.clone()


		env.pop_local_frame(JObject::null())?;
		unsafe { context.env.pop_local_frame(&JObject::null()) }?;

upgrade jni v0.21 #63

upgrade jni v0.21 #63

Conversation

flisky commented Jan 16, 2024 • edited Loading

complexspaces commented Jan 16, 2024

flisky commented Jan 17, 2024

complexspaces commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

complexspaces commented Jan 23, 2024

flisky commented Jan 23, 2024

flisky commented May 22, 2024 • edited Loading

cpu commented Sep 16, 2024

flisky commented Sep 17, 2024

flisky commented Jan 16, 2024 •

edited

Loading

flisky commented May 22, 2024 •

edited

Loading