ndk-glue does not handle NativeWindow lifecycle correctly

[maciejgodek]

As far as I understand, ndk-glue's NativeActivity callbacks make it impossible or at least very hard to handle the NativeWindow lifecycle in a manner that conforms with the Android documentation. The chief offending callback is on_window_destroyed.

ANativeActivityCallbacks documentation seems to demand that NativeWindow cleanup code be executed in a synchronized manner, i.e. before the callback returns. For example, one might wish to ensure OpenGL is done drawing and destroy the associated EGLSurface.

NDK android_native_app_glue appears to follow this guideline, cf. especially how lifecycle event handling is split into pre-user-callback and post-user-callback parts (lines 89 and 146 respectively).

EDIT: for added context, compare example usage of NDK android_native_app_glue feature.

[MarijnS95]

Correct!

I think the most interesting lines are 306-308 though, showing that android_app_set_window (called from onNativeWindowDestroyed) indeed blocks and waits until the application thread processed the destroy message (android_app_post_exec_cmd indeed).

At the same time it seems unfair to pull NATIVE_WINDOW from under the application, native_window() would panic in that case.

I've been thinking about fixing this before, and it probably comes down to delivering this event to the application in a different way that allows us to make sure their cleanup handler has ran, surrounded by similar synchronization primitives.

[MarijnS95]

Interesting to note that their on_window_created handler first checks if the previous window was cleaned up, and sends a destroy message for it otherwise.

[maciejgodek]

Thanks for the quick confirmation! I'd love to try to put a pull request together, but: I'm pretty new at Rust, and as you say, it would be necessary to come up with better synchronization with user code, impacting e.g. winit. I'd probably need a fair amount of hand-holding.

[maciejgodek]

Interesting to note that their on_window_created handler first checks if the previous window was cleaned up, and sends a destroy message for it otherwise.

I'm not sure that's the case. It seems that the function android_app_set_window() just gets reused in onNativeWindowCreated and onNativeWindowDestroyed (with NULL pointer argument). It relies on some undocumented invariants (especially on the value of android_app->pendingWindow) to do the right thing. It's just unnecessarily dense code.

However this code seems to ensure that if onNativeWindowDestroyed fires before the user event loop has processed the INIT_WINDOW message, the window is preemptively NULLed out and won't be used or drawn to at all. (This requires a NULL check on the user side.) It seems to mostly be an optimization, so as to ensure no expensive and useless drawing path is run while the NativeActivity loop waits inside onNativeWindowDestroyed.

[maciejgodek]

As far as solutions for ndk-glue go — assuming there is no actual black magic involved: might it not suffice to add a one-shot channel / fence of some kind that user code must notify when done with NativeWindow cleanup? There are actually only two types of events that need this kind of synchronization (on_window_destroyed and on_save_instance_state, which is currently unimplemented anyway), so it seems like a simple hack might be enough?

[MarijnS95]

@maciejgodek Correct, in hindsight it seems more of a simplification than an actual paranoid attempt to make sure no window is leaked. For the same reasons I'd either like to see the same behaviour (clean up the window if non-null before assigning), or an assertion.

No matter the implementation it seems like onInputQueueDestroyed and onNativeWindowRedrawNeeded need synchronization as well; we might want to spend some time flushing out a high-level representation of these rather than applying a simple hack. I don't want to restrict API users too much; at the same time I don't want an application to deadlock because someone forgot to signal ndk-glue that one of the few "synchronous" events has been processed. That usually ends in some closure running within some locked context, though.

[maciejgodek]

It took me an evening or three to get a minimal "hello triangle" app running on Android to have a basic debugging environment & get a better handle on EGL behavior. (I hacked bits of glutin together with glow-rs.) I can now try to implement your suggestion, should have something to share in a few days.

I'm going to give up on the UNIX pipe of single byte enums as event queue. Instead I suggest a queue of Rust enums, much like what winit uses for user events. For events providing a new resource and/or requiring synchronization with the callback, the enum variant will contain a SyncEventContext, something along the lines of:

pub struct SyncEventContext<T> {
    done_notifier: Sender<()>,
    payload: Arc<Mutex<T>> 
}

impl<T> SyncEventContext<T> {
    pub fn handle<F: FnOnce(&T)>(self, f: F) {
        {
            let lock = self.payload.lock().unwrap();
            f(&lock);
        }
        let _ = self.done_notifier.send(());
    }
}

Any thoughts?

[MarijnS95]

@maciejgodek The advantage of the pipe is that can easily integrate with Androids Looper, immediately making the source clear. With this approach you'll need to .wake() the Looper and figure out where that wake came from; similar to the issue discussed in rust-windowing/winit#1822 (comment).

At this point we should probably create a higher-level wrapper in ndk-glue around .wake() and retrieving event source + possible extra data, taking generics that can be passed through an atomic queue or channel.

Otherwise these events might be triggered by a single-byte write on the original UNIX pipe (or a new one with distinct ident) in the form of a new Event::SyncEvent that the consumer should retrieve/process from that queue. From an ndk-glue perspective that could be wrapped in a struct that signals done_notifier when dropped. You could possibly mix the Mutex with a Condvar to achieve this.

[maciejgodek]

Both options make sense. I think I prefer the first one.

Here's what I've been thinking though: all you need to plug into the Looper is a file descriptor that can be slept on/polled, right? I thought we could build a higher-level generic event source around eventfd instead of pipe. As for event storage, you could use Arc<Mutex<VecDeque>> to start with and then maybe something cleverer / more tightly integrated with the waking mechanism. For example a lockless queue if a suitable implementation can be found.

[maciejgodek]

I've put together something matching the above description in PR #121. Not sure if it's changing too much in the API. I also accidentally added a new method of building another event queue attached to the looper (e.g. user event queue in winit) and a method of sending redraw events.

I've got a rough winit branch that works with this API, will push my fork tomorrow. I might try to adapt glutin for it next week.

[dvc94ch]

Why is there a problem? native_window returns a guard and on_window_destroyed takes a write lock, which will block until all read locks are dropped.

I think the problem is with glutin when trying to write correct code (no one seems interested in the new glutin that fixes these issues in the next branch) or people trying to cheat the very clear api in some way (keeping raw dangling pointers to native window around)

[maciejgodek]

Huh. That... might work. I'm not (yet) sure it does.

Just to be clear, for that to work the way you describe, the user (e.g. glutin) would have to hang on to their RwLockReadGuard from the time they first started using the NativeWindow for as long as they wish to continue using it. This is where I might be wrong: "using" here is very general and includes any derived objects such as EGLSurface, as well as stuff that's happening asynchronously on the GPU, or in yet another thread if someone insists on that. Compare the docs again.

But in that case the user needs a way of knowing that the NativeWindow is about to go away to perform all requisite cleanup and drop the ReadLockGuard. Currently, on_window_destroyed sends the Destroyed event after obtaining write access to the RwLock and None-ing out the static NativeWindow slot. What's worse, it sends the notification to the winit even loop, which does not seem to notify glutin in any way (at least on respective master branches).

But let me dig around some more, I genuinely might be misunderstanding a thing or two about the whole ecosystem. 😄

[dvc94ch]

goddessfreya the previous glutin maintainer last made contact in feb 2020, never to be heard from again, and taking the glutin improvements with her to the graveyard of dead code.

• https://github.com/rust-windowing/glutin/tree/v0.23_reworks
• Glutin0.23 android rust-windowing/glutin#1274
• Implementation tracking and API discussions for v0.24 rust-windowing/glutin#1271

[maciejgodek]

Ok, I'm going to read it all carefully and close my PR for now to reduce confusion.

FWIW I still think there's either a race condition or a potential deadlock in on_window_destroyed. The notification should be sent before obtaining a write lock, so clients have a chance to drop their long-lived read locks, which they should be holding if they have created new objects dependent on NativeWindow.

[MarijnS95]

@maciejgodek Did you manage to look into this yet?

I indeed agree that the notification should be sent before grabbing the write lock. Winit should hold on to the read lock as soon as raw_window_handle is called, and drop it on WindowDestroyed after it finished handling that event in user code. Alternatively Winit can grab and cache the lock in WindowCreated and serve from there if raw_window_handle is called. At that point we can only assume user code stops using the window and any implicit dependencies like Vulkan surfaces/swapchains. Reworking Winit to serve the window in ::Resumed and coming up with complicated structures to prevent using it after ::Suspended isn't worth the effort (if properly done for all platforms), nor completely preventing UB at all.

[maciejgodek]

I've tried digging into last year's attempted glutin rework but it seems sufficiently different from master that it's mostly a curiosity. :( I'm still not sure how exactly it allows users to keep an EGLContext alive while windows come and go. I'll give it another read some day — I had trouble locating all the dependencies.

As for the more immediate problem, I agree. The reasonable thing is for winit to extend the lifetime of the native window until after the user handles its Suspended event. In that case, android-ndk-rs just needs to make that possible without a race or deadlock, which should require changing approximately two lines of code. :) I will try to put together the two PRs this weekend.

What worries me is whether various winit wrappers (including but not limited to glutin's) are currently capable of intercepting the Suspended event and correctly destroying the implicit dependencies of the NativeWindow without forcing the user to recreate the entire graphics context, which can be quite expensive. But that should probably be discussed elsewhere.