Crashes on re-launch

[matthunz]

When my app is relaunched I see:

01-15 01:03:27.582 22941 23008 I RustStdoutStderr: thread '<unnamed>' panicked at
'assertion failed: previous.is_none()', /Users/matt/.cargo/registry/src/github.com-1ecc6299db9ec823/ndk-context-0.1.1/src/lib.rs:87:5

After some digging around it looks like that line of code in idk-context is called in this crate's onCreate. Should we check to see if there is already a context created beforehand? (if that function is supposed to be called after a suspend)

[MarijnS95]

That is because neither ndk-glue nor android-activity support multiple activities, the design of ndk-context just doesn't work for that.

[rib]

@matthunz, do you know if your app tries to create multiple native Activities at the same time? I'd guess you aren't trying to do that because I'd also guess in that case that you wouldn't be able to launch the app even once.

As @MarijnS95 mentions, it's not possible to run multiple native Activities in the same process with the current ndk-context design, but maybe you can clarify a bit what you mean by "re-launch" here?

Are you just trying open, then close and then re-open one application based on a single Activity using android-activity - that's certainly supposed to work.

Currently android-activity calls ndk_context::release_android_context() after your native android_main() function returns which should usually mean that restarting that Activity in the same process would be ok if the first one exited cleanly.

Just trying to think of how your app might not be cleanly exiting from android_main() (and therefore not clearing the context pointers) - does your app maybe override panic handling and then exit with a panic? That's one possible way I could see how the context pointers might not be cleared properly.

As an experiment maybe you could build against a local patched version of android_activity that adds some println debugging here to check whether you see the context state get cleared before it panics.

It looks like we can't currently guard against this outside of ndk-context because if we read the context and it hasn't been set that will also panic: https://github.com/rust-mobile/ndk-context/blob/0eb252bf2306f6ca477430a35e88298e634c0b24/ndk-context/src/lib.rs#L84

That's pretty awkward to fix too because it's conceptually an infallible api that doesn't return an Option or Result.

The longer term idea had been that we should aim to depreciate the use of ndk-context and middleware libraries would need to have a way to be explicitly initialised with jvm/activity pointers on Android.

A good step toward this would probably be for android-activity to expose it's own AndroidApp APIs for accessing the jvm and Activity pointers that can be used for JNI.

[MarijnS95]

FWIW for me an app is relaunched when changing how it is shown, e.g. changing to freeform window or split-screen mode.

@rib looking at https://crates.io/crates/ndk-context/reverse_dependencies there are only a handful of apps making use of this, we might still stand a chance to back out of ndk-context and propose this "explicitly pass your AndroidApp object" API as PRs to them (and ndk-context might host the shared struct/helpers/documentation for this)?

[rib]

@rib looking at https://crates.io/crates/ndk-context/reverse_dependencies there are only a handful of apps making use of this

Yeah we can probably look at opening PRs for projects depending on ndk-context currently.

I briefly started looking at adding APIs to android-activity to expose the jvm pointer and activity object pointer and hopefully I can get around to finishing that soonish too.

[MarijnS95]

I briefly started looking at adding APIs to android-activity to expose the jvm pointer and activity object pointer and hopefully I can get around to finishing that soonish too.

Cool! That'd only be the start of this though, right? We probably don't want to make any crate depend on the full android-activity only for context pointer types, a wrapper in ndk-context seems most trivial. When we get rid of statics I also don't mind having dependencies on NDK/JNI crates again for better type safety (though that may pose additional strain on propagating updates through the ecosystem...).

[rib]

I see it as two separate issues:

1. middleware libs, e.g egui or winit or some random crate for permissions or file open dialog etc should stop using ndk-context and should instead provide a way to be explicitly initialised on Android with a vm pointer and activity pointer. (So in this way they dont care where these come from and they also dont necessarily care if they are being used as part of a standalone Rust app or as part of an embedded library that might be loaded from a Java app)

2. android-activity should provide some way to get the vm and activity pointers, for apps that happen to use android-activity.

There are also some other thoughts I have about making it easier to access the correct classLoader when using JNI in standalone apps which are kind of related.

Other kinds of frameworks, not based on android-activity will also need some way to access the same pointers.

[MarijnS95]

1. Yes. I don't mind whether or not we provide a standard crate to wrap or type the VM and Activity (or base Context, or both) pointers;
2. That's what you'll be implementing, right?

[MarijnS95]

Fwiw we had one issue on app_dirs2 where someone combined a compiled utility for <arch>-linux-android and expected it to run under Termux using XDG env vars. This however initially triggered the cfg logic to read Java pointers from ndk-context, but has now been worked around by looking for the respective Linuxy env vars before even trying to read the VM/context pointers; we could make that explicit with this builder-like pattern.

[matthunz]

Sorry for the late reply!
Thanks so much for the info on this, I think I understand what's going wrong now.

Having this crate be the source of the JNIEnv would be a great solution for outside crates that work with android. For example I've been working on android-intent and it would be great to have a way to get the current env or VM to make ndk calls.

Seems complex to explicitly pass the context around, unless there's some big benefits to it. The way ndk-context does this implicitly with a static variable does seem simpler

[rib]

For reference here, I've just opened #61 which adds APIs that make it possible to get the JavaVM and Activity pointers directly from an AndroidApp without necessarily using ndk-context.

Seems complex to explicitly pass the context around, unless there's some big benefits to it. The way ndk-context does this implicitly with a static variable does seem simpler

Yeah it's a bit of an awkward one. Its certainly convenient to just have a global static but that also makes it impossible to track more than one Activity per process which is something that Android supports generally.

We could potentially investigate whether we could update ndk-context to track state via thread-local storage so that it's possible to track multiple activities, and then the main limitation would be that the ndk-context would only be accessible (by default at least) from the android_main() thread.

Dealing with breaking API changes with ndk-context will also be "fun" if go that route, since we will need to ensure each incompatible version gets initialized properly from android-activity.

[rib]

Taking a quick look at android-intent linked above, and seeing the with_current_env API reminds me that I'd been thinking that if I were to add more ergonomic integration with the jni crate to android-activity that it could be good to add something similar to AndroidApp - basically a utility for using JNIEnv::with_local_frame() something like:

let result = app.jni_with_local_frame(|env| {
    Intent::new(env, Action::Send)
            .with_type("text/plain")
            .with_extra(Extra::Text, "Hello World!")
            .into_chooser()
            .start_activity()
});

Maybe the closure could also be passed the Activity reference as a &JObject

That would avoid the need to resort to unsafe to make JNI calls.

I'm currently just a bit unsure about exposing jni types via the android-activity crate since it's an unstable API and we'd need to sync releases which might be fiddly to deal with.

[matthunz]

I think that's a solid plan

Passing an AndroidApp through an app might be cumbersome but I see what you're saying about what to do with multiple activities. If there's a simple way to just grab the current activity in a closure that would be super simple to use.

Maybe something like AndroidApp::with_env with a struct that wraps references to Activity and Vm, so this crate's API could be stable, could work

[MarijnS95]

One of my ideas:

For some crates the base application context might just be enough, which we could still safely store in a process-wide static (after the first Activity was started). TLS is complicated as you're stuck with the main thread that spawned that Activity instance; any other thread would have to be explicitly associated at which point it might be desirable to just make it explicit in code as suggested above.

[matthunz]

Oh that's interesting I would like something like that. I think I understand what you're saying with multiple activities having to access their current env with TLS or a static would be really complicated as well. Having a shared base application context sounds great but I hope it's possible to get the Vm or JNIEnv that way as well

[MarijnS95]

Right, yes, I forgot to mention the VM/JNIEnv and am assuming the former to also globally exist to the process, otherwise an Application context won't work like that.

[rib]

I think if you only need the global application context you may already be able to access that just via a JavaVM/env, e.g. similar to this: https://stackoverflow.com/a/46871051/8021127 so I guess we probably don't need to introduce any static variables on the Rust/native side for that. A JNI utility somewhere could be handy if that's useful for multiple projects though.

I would guess that crates are more likely to need the Activity than global app Context so I guess we still want to make sure that it's easy to access the Activity as the first priority.

[rib]

Right, yes, I forgot to mention the VM/JNIEnv and am assuming the former to also globally exist to the process, otherwise an Application context won't work like that.

So for this are you imagining keeping something like ndk-context for at least passing around the JavaVM pointer?

[rib]

The issue at the moment with ndk-context is only really with the context/Activity pointer, so it could still be reasonable to continue sharing the JavaVM pointer via a standard crate like ndk-context. This would cover the needs of code that would have historically relied on JNI_OnLoad (which isn't a good hook to rely on for Rust code when there's only a single implementation of JNI_OnLoad per dylib)

[rib]

TLS is complicated as you're stuck with the main thread that spawned that Activity instance; any other thread would have to be explicitly associated at which point it might be desirable to just make it explicit in code as suggested above.

Yeah, if we're trying to come up with a pattern for crates like app_dirs2 that might be embedded as part of an app that's primarily written in Java instead of assuming it's being used via a standalone Rust application then we can't assume that some glue layer like android-activity would have a good opportunity to initialize that TLS state.

It seems like the Activity at least needs to be passed to these crates explicitly so they don't bake in any assumptions about what kind of framework / architecture they can be used with.

[MarijnS95]

I think if you only need the global application context you may already be able to access that just via a JavaVM/env, e.g. similar to this: https://stackoverflow.com/a/46871051/8021127 so I guess we probably don't need to introduce any static variables on the Rust/native side for that. A JNI utility somewhere could be handy if that's useful for multiple projects though.

Okay we can get static access to the Application Context without ever going through an Activity Context. Can we also get the VM in a similar static way?

I would guess that crates are more likely to need the Activity than global app Context so I guess we still want to make sure that it's easy to access the Activity as the first priority.

Perhaps, I'm only familiar with app_dirs2 (and winit which already uses the builder pattern with a typed AndroidApp), hence suggested to make an overview containing this info based on reverse deps on ndk-context.

So for this are you imagining keeping something like ndk-context for at least passing around the JavaVM pointer?

That seems like a necessity unless this is associated with an entire process at once and can be retrieved statically? Again, not my area of expertise but it seems like we need this for basic functionality...

It seems like the Activity at least needs to be passed to these crates explicitly so they don't bake in any assumptions about what kind of framework / architecture they can be used with.

I'm all for explicitly passing it (Context should be enough, might even be the base context in some areas) to solve issues like Termux mentioned elsewhere. TLS is tricky when you don't know which thread(s) might be associated with an Activity, especially if they're spawned ad-hoc.

[MarijnS95]

Quick testing shows that at least app_dirs2 can get away with the Application context, either acquired via getApplicationContext() or with https://stackoverflow.com/a/46871051/8021127. All we then need is the VM :)

[rib]

Can we also get the VM in a similar static way?

I don't think so no.

The generic mechanism JNI provides is the ability to export a JNI_OnLoad symbol from your shared library which will be called with a VM pointer but that's not a good general solution for Rust crates.

Having a crate like ndk-context is probably good to have for the VM pointer - even though it's not well suited to sharing the Activity pointer. This way the crate can be initialized in a number of ways (e.g. via android-activity or some JNI_OnLoad implementation) and the consumers don't have to care where exactly the pointer came from.

[rib]

Quick testing shows that at least app_dirs2 can get away with the Application context

I suppose that makes sense, directory constants are generally app centric.

On the other hand I'd guess that enabling something like rfd for Android and using an Intent to let the user choose a directory would require the Activity instead.

[rib]

Maybe something like AndroidApp::with_env with a struct that wraps references to Activity and Vm, so this crate's API could be stable, could work

yep, this sounds like it'd be a pretty nice approach, that could avoid needing unsafe code

I have some nagging concern about exposing the jni API as part of the public API for android-activity but it might be worth it.

If the jni API gets exposed publicly and if this kind of ::with_env utility is used that will have a knock on consequence of tightly coupling the jni version with the android-activity version - so we'll have to synchronize releases. It'll probably also make sense to re-export the full jni API, similar to why we should probably do that for the ndk API too (ref: #45)

The jni API is currently more volatile that the android-activity API (lots of breaking changes were e.g. required for 0.21 to address long standing safety issues and bugs) and I can see lots of good reasons why the jni API needs to continue evolving - whereas the android-activity API is currently pretty tiny and there aren't currently any obvious reasons to plan for API breakages.

[MarijnS95]

I suppose that makes sense, directory constants are generally app centric.

Yup, I expected/hoped for that.

On the other hand I'd guess that enabling something like rfd for Android and using an Intent to let the user choose a directory would require the Activity instead.

That could mostly go straight to an android-intent crate, as long as it gets the Activity JObject hooked up. Don't think there's a way around it, these are usually associated with Activitys and not entire Applications (but I'm rather Rusty on Android internals...).

Having a crate like ndk-context is probably good to have for the VM pointer - even though it's not well suited to sharing the Activity pointer. This way the crate can be initialized in a number of ways (e.g. via android-activity or some JNI_OnLoad implementation) and the consumers don't have to care where exactly the pointer came from.

Fair enough, so we'd keep ndk-context exclusively for the VM so that in the specific case of app_dirs2 we pull the Application Context out of a static function and it should all behave properly behind users' backs (or not, if we want to make things explicit; Termux as a preliminary fallback based on env vars is working just fine now).

[rib]

"ndk-context" does end up being a pretty terrible name for the crate if we remove the Context pointer. It's also not related to the NDK :)

[MarijnS95]

Yes and yes :)

[rib]

I wonder if it'd make sense to create a tiny replacement under the jni-rs org for this.

Not exactly related but jni-sys was recently also transferred to the jni-rs org and I'm thinking of merging it into a common repo (still as a separate crate) and it could also make some sense to have a crate here that's just for sharing the JavaVM pointer - regardless of what specific version of the jni crate your using (or if your using a different crate entirely for JNI)

Being able to share the JavaVM pointer isn't something that's specific to Android, it's a generic issue for modular / orthogonal Rust crates that wants to use JNI

[MarijnS95]

And that JavaVM pointer is (typically) global to an entire process? Sounds like a good plan :)