Sometimes running apps panicked on the Android Emulator

[Gordon-F]

Sometimes running apps panicked on the Android Emulator with following error:

01-20 22:05:38.816  3402  3421 I RustStdoutStderr: thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/ndk-glue-0.2.1/src/lib.rs:255:34
01-20 22:05:38.816  3402  3421 I RustStdoutStderr: stack backtrace:
01-20 22:05:38.816  3402  3421 I RustStdoutStderr:    0:     0x7b6194f50110 - <unknown>

This error is constantly repeated in CI: https://github.com/gfx-rs/gfx/runs/1738010575?check_suite_focus=true

Sometimes I saw this error on my dev machine (MacOS host), but restart the Android Emulator fixed it. And I never saw this error on a real device.

[MarijnS95]

That is apparently the looper which has not been initialized yet:

https://github.com/rust-windowing/android-ndk-rs/blob/7d36455f8b850ce716debcef551f86fc7fa9ecaf/ndk-glue/src/lib.rs#L255

I don't think we should be returning from init() until the thread has finished assigning it, here:

https://github.com/rust-windowing/android-ndk-rs/blob/7d36455f8b850ce716debcef551f86fc7fa9ecaf/ndk-glue/src/lib.rs#L163-L172

[Gordon-F]

I don't think we should be returning from init() until the thread has finished assigning it

@MarijnS95 Thank you for suggestion! How can we do this in the best way?

[MarijnS95]

@Gordon-F There are numerous ways to solve this; for example wrapping the static global LOOPER in a Mutex and using a Condvar to signal when it is ready. We can either block fn init() (which iirc is desired by Android) or wait in on_input_queue_created when it is trying to access the looper (or in a generic way, in both, wrapping the mutex+condvar dance in a fn get_looper).

Another, simpler-to-write solution is to block fn init() and send a message through a channel to unblock it, after the looper has been set up: master...MarijnS95:glue-wait-for-thread
That's not so elegant as a mutex+condvar combination though, which is purposely built for this case. LOOPER anyway isn't accessed outside on_input_queue_created so we shouldn't worry about any overhead anyway.

Finally we can also create a temporary mutex inside fn init() that the read will pass the looper into, block a condvar on that, and assign it to the unsafe static global LOOPER before returning. Downside is that calling the users main() may at some point get access to the LOOPER if such functionality is ever needed/added (though they should likely just use ForeignLooper::for_thread()).

[Gordon-F]

@MarijnS95 Thank you for the detailed explanation!

I've tried your solution with Android Emulator, but I get another unrelated error from winit:

RustStdoutStderr: thread '<unnamed>' panicked at 'Cannot get the native window, it's null and will always be null before Event::Resumed and after Event::Suspended. Make sure you only call this function between those events.', /Users/indish/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.24.0/src/platform_impl/android/mod.rs:530:13
RustStdoutStderr: note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

https://github.com/rust-windowing/winit/blob/86748fbc681ea070bc37e90a569c192555b402cd/src/platform_impl/android/mod.rs#L565-L573

This is strange since at the beginning of the application I am waiting for native_window:

fn main() {
    {
        log::info!("App started. Waiting for NativeScreen");
        loop {
            match ndk_glue::native_window().as_ref() {
                Some(_) => {
                    log::info!("NativeScreen Found:{:?}", ndk_glue::native_window());
                    break;
                }
                None => (),
            }
        }
    }

    run();
}

[MarijnS95]

@Gordon-F I assume you changed your ndk-glue dependency to = { path = "..." } where the patch above was applied?

You should instead add that to a patch section like:

[patch.crates-io]
ndk-glue = { path = "..../android-ndk-rs/ndk-glue" }

Right now there are likely two versions of ndk-glue compiled into your application, both with their own NATIVE_WINDOW static. The one you use from fn main() is initialized (because of the macro), the one used by winit is not. A patch section makes sure every use of ndk-glue (do make sure you leave the original dependency in your own code in place, too!) that shares the same version is overridden to the specified dependency (a path, but could be a git remote as well).

[msiglreith]

For acquiring the window handle it should be done during the event loop only, see for example https://github.com/msiglreith/wgpu-rs/blob/android/examples/hello-triangle/main.rs#L108-L119

At any other place it may or may not give you proper values resulting in a panic

[MarijnS95]

Yes, that too - you should not need to wait for a native window before starting your application, use the Suspended and Resumed callbacks for that so that your application can be properly put to the background.

Also note that fn native_window() returns a RwLockReadGuard to prevent on_window_destroyed from cleaning it up and returning to Android, signaling that the application is done with the window. The linked fn raw_window_handle() violates that principle by taking the raw pointer and dropping the readlock. See the discussion following #117 (comment); we should probably revisit that though I suspect a proper solution involves providing raw window handles to the application through the Resumed event (also outside Android) and assume or assert the application gives it back or stops using it in Suspended.

[MarijnS95]

@Gordon-F The mutex+condvar solution seemes cleanest to me and has been submitted in #131, please test with it :)