Android window handle invalidation

[maroider]

Window handles on Android can be invalidated between a pair of "suspend" and "resume" events. This makes it difficult to safely use window handles without asking end users to wire up the missing event handling. Thus, an API naïvely accepting a T: HasRawWindowHandle is inherently unsafe on Android.

[aloucks]

Has there been any investigation into ndk-glue to see if it's actually doing things properly? For example, the window pointer is pulled out of a an RwLock and I didn't see any usage of ANativeWindow_acquire or ANativeWindow_release. I don't know much about how andriod windows work but it looks a little suspicious on first glance.

[MarijnS95]

@aloucks Thanks for the reminder, I almost forgot that I still had rust-mobile/ndk#207 sitting around locally.

Fwiw I don't think _acquire needs to be called when there's only ever one "owned copy" of that pointer floating around - it's for refcounting/lifetime purposes only:

https://developer.android.com/ndk/reference/group/a-native-window#anativewindow_acquire

Acquire a reference on the given ANativeWindow object.

This prevents the object from being deleted until the reference is removed.

In other words, it's better to look at when we zero out that RwLock'ed window, and when winit receives and handles WindowCreated vs WindowDestroyed. We had a nice PR related to this recently: rust-mobile/ndk#134

[aloucks]

If we call acquire after the window is created, won't that prevent it from being destroyed and keep our copy of the pointer valid?

[MarijnS95]

@aloucks I still want to test that before merging rust-mobile/ndk#207. As far as I understand Android gives us a window in on_window_created that it keeps "a ref" on, then after on_window_destroyed it'll give up that reference and should theoretically destroy the window assuming we have never acquired it.

If we however acquire it, I don't know what happens after Android "says" it's going to be destroyed since the window seems/should be useless after that point?

EDIT: We should not access it at all after returning from that callback - no mention of acquired windows though: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks#struct_a_native_activity_callbacks_1a6aaafbbfae4c0ac066b9d61ebb3ab97f

You MUST ensure that you do not touch the window object after returning from this function: in the common case of drawing to the window from another thread, that means the implementation of this callback must properly synchronize with the other thread to stop its drawing before returning from here.

Likewise, what'd happen if we call release immediately without ever calling acquire? Would that free the window and result in Android having no window to draw the activity from anymore?

All things to test out :)

EDIT: In hindsight these functions seem more relevant when creating an ANativeWindow from elsewhere, where they are not managed by the activity lifecycle: https://developer.android.com/ndk/reference/group/native-activity#anativewindow_fromsurface

[MarijnS95]

Turns out I tested NativeWindow "validity" after on_window_destroyed but never got back to forward the results here to complement my own questions: rust-mobile/ndk#207 (comment)

Take these with a grain of salt as they are not specified in the reference (as quoted above: "You MUST ensure that you do not touch the window object after returning from this function") and might differ between Android versions and (:vomiting_face: please forbid) vendor modifications.

• We don't have to perform any additional refcounting (Android system owns the NativeWinodw and informs us of changes via on_window_destroyed so to speak; i.e. a borrow);
• We shouldn't use the NativeWindow after that, as its refcount should have been reduced to zero and the handles/pointers are invalid);
• If we do wish to keep using it however, we should attain explicit ownership via _acquire() (implemented as Clone);
• However, this hardware buffer is not presented to the screen anymore, so its usefulness is debatable (and again: this conflicts with the documentation for onNativeWindowDestroyed).

[notgull]

Borrowed window handles should handle this properly, see #111

[rib]

I think this issue might be a bit misleading/out-of-date and perhaps at the time it was filed it may have been a reflection of the synchronization issues that used to exist with ndk-glue that were addressed with android-activity.

Here's a recent summary of how I think this looks on Android: #111 (comment)

As far as I know there isn't a safety issue - it's just that on Android applications need to be aware that they have to recreate their graphics API surfaces when they resume (E.g. after onNativeWindowCreated with NativeActivity) so the graphics API will have a fresh BufferQueue which they can get buffers from to render to the surface (otherwise the app will stop being able to present anything).

So long as we hold a reference to the ANativeWindow (even if it's no longer in use) we shouldn't have a safety issue afik.

[notgull]

Thank you for the summary, @rib!

Closing because as of #118 I believe that we handle this properly.