Hold on to Android NativeWindow lock to prevent races #1892

maciejgodek · 2021-03-21T23:36:12Z

The current Android implementation of winit aims to guarantee the validity of pointers handed out by Window::raw_window_handle() between a Resumed event and a matching Suspended event. I don't think this guarantee is actually upheld.

The design of the ndk_glue crate requires that users hold on to a read lock on the NativeWindow handle received from ndk_glue::native_window() while they intend to use it, including through implicitly dependent objects such as RawWindowHandle (or EGLSurface, and so on). Otherwise, Android may destroy the underlying resource before dependent resources are cleaned up, leading to a race condition and UB.

Unfortunately, due to a subtle bug, such user behavior would cause a deadlock in the current version of ndk_glue. This should no longer be the case once rust-mobile/ndk#117 is resolved.

From that point on, in order to ensure validity of the raw pointers to Android's ANativeWindow handed out by Window::raw_window_handle(), winit's Android EventLoop should hold on to a read lock on the NativeWindow returned by ndk_glue::native_window() between receiving the WindowCreated event and the matching WindowDestroyed event from ndk_glue's event pipe.

Other designs are possible, of course, but as long as raw pointers are being handed out, some similar protection will be necessary.

Depends on: rust-mobile/ndk#134

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
cargo doc builds successfully
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

src/platform_impl/android/mod.rs

MarijnS95

That's pretty much how I had it in mind! Let's get a Depends on: https://github.com/rust-windowing/android-ndk-rs/pull/134 in the PR description and pump out a new ndk-glue release quick so that the version can be bumped and this can make its way in, too.

Thanks!

EDIT:

Note that this commit depends on recent fixes to ndk_glue

Feel free to (or rather, please) link to this PR directly from the commit for future reference :). Perhaps wise to add the same link to the changelog, too.

In order to ensure validity of the raw pointers to Android's `ANativeWindow` handed out by `Window::raw_window_handle()`, the `EventLoop` will hold on to a read lock on the `NativeWindow` returned by `ndk_glue::native_window()` between receiving the `WindowCreated` event and the matching `WindowDestroyed` event from `ndk-glue`'s event pipe. Note that this commit depends on recent fixes to `ndk-glue`, specifically this PR rust-mobile/ndk#134. Previous versions of `ndk-glue` will cause this code to deadlock due to a concurrency bug.

src/platform_impl/android/mod.rs

MarijnS95 · 2021-03-23T23:53:47Z

src/platform_impl/android/mod.rs

-                            control_flow,
-                            event::Event::Resumed
-                        );
+                        let native_window_lock = ndk_glue::native_window();


There are two calls remaining to ndk_glue::native_window(). IMO these should use self.native_window_lock and panic if it was None (implying it was called outside a Resumed - Suspended "block") instead of trying to grab the window on their own regard.

That might require some refactoring as fn raw_window_handle() only has access to struct Window which is currently empty, with EventLoopWindowTarget supposedly tying them together. I'm not sure what the desired flow is here, the other platform implementations should be able to help out here.

I tried something like that first. My attempt ran into RwLockReadGuard being !Send and I gave that approach up. Will give it another try though.

Ok, I came up with something. Let me know what you think.

I would have preferred to keep the entire thing in one place without two mutexes/locks. It looks like other backends like Wayland store modifyable data in EventLoopWindowTarget through a RefCell:

winit/src/platform_impl/linux/wayland/event_loop/mod.rs

Lines 50 to 51 in 86748fb

/// State that we share across callbacks.

pub state: RefCell<WinitState>,

That's where I'd put native_window_lock instead. Perhaps in a similar *State struct and/or with a type alias because it is getting quite long 😅

Maybe the winit-Android developers have a better overview of where to place (mutable) data shared between the EventLoop and Window; @msiglreith @dvc94ch?

I would prefer that too, but I assumed that Window needs to be Send, which complicates things. I'll take another look at the other backends and see if I can glean anything more.

Desktop platforms seem to make sure Window is Send and synchronize state with the event loop (?). It's not clear how important this is on Android, but if it is, then the fact that you can't put RwLockReadGuard behind an Arc<Mutex<T>> becomes a problem.

winit/src/platform_impl/linux/wayland/window/mod.rs

Line 46 in 86748fb

size: Arc<Mutex<LogicalSize<u32>>>,

winit/src/platform_impl/linux/x11/window.rs

Line 105 in 86748fb

pub shared_state: Mutex<SharedState>,

@MarijnS95 Still no idea how to solve this without extra synchronization. Some input from a maintainer might be useful.

Unfortunately I have not had the time to look at any alternatives either, seems like wrapping our state (either the readlock or raw window handle) inside yet another mutex is the only option - given Send limits this might only be possible with the window handle which has to be unconditionally retrieved and stored like this PR does currently.

It would have also been nice if the lock could be transposed to remove the Option (only want to hold the lock if it contains a valid window) but that is not possible either.

Co-authored-by: Marijn Suijten <[email protected]>

dvc94ch

Great work @maciejgodek and @MarijnS95 . Thanks for thinking through the subleties of the api and coming up with a simple solution.

This updates the Android backend to use the android_activity create instead of ndk-glue. This solves a few issues: 1. The backend is agnostic of the application's choice of Activity base class 2. Winit is no longer responsible for handling any Java synchronization details, since these are encapsulated by the design of android_activity 3. The backend no longer depends on global / static getters for state such as the native_window() which puts it in a better position to support running multiple activities within a single Android process. 4. Redraw requests are flagged, not queued, in a way that avoids taking priority over user events (resolves rust-windowing#2299) Addresses: PR rust-windowing#1892 Addresses: PR rust-windowing#2307 Addresses: PR rust-windowing#2343 Addresses: rust-windowing#2293 Resolves: rust-windowing#2299

This updates the Android backend to use the android_activity crate instead of ndk-glue. This solves a few issues: 1. The backend is agnostic of the application's choice of Activity base class 2. Winit is no longer responsible for handling any Java synchronization details, since these are encapsulated by the design of android_activity 3. The backend no longer depends on global / static getters for state such as the native_window() which puts it in a better position to support running multiple activities within a single Android process. 4. Redraw requests are flagged, not queued, in a way that avoids taking priority over user events (resolves rust-windowing#2299) Addresses: PR rust-windowing#1892 Addresses: PR rust-windowing#2307 Addresses: PR rust-windowing#2343 Addresses: rust-windowing#2293 Resolves: rust-windowing#2299

This updates the Android backend to use the android-activity crate instead of ndk-glue. This solves a few issues: 1. The backend is agnostic of the application's choice of Activity base class 2. Winit is no longer responsible for handling any Java synchronization details, since these are encapsulated by the design of android_activity 3. The backend no longer depends on global / static getters for state such as the native_window() which puts it in a better position to support running multiple activities within a single Android process. 4. Redraw requests are flagged, not queued, in a way that avoids taking priority over user events (resolves rust-windowing#2299) To make it possible for application crates to avoid explicitly depending on the `android-activity` crate (and avoid version conflicts) this re-exports the android-activity crate under: `winit::platform::android::activity::*` This also adds `android-native-activity` and `android-game-activity` features that set the corresponding android-activity features. Addresses: PR rust-windowing#1892 Addresses: PR rust-windowing#2307 Addresses: PR rust-windowing#2343 Addresses: rust-windowing#2293 Resolves: rust-windowing#2299

This updates the Android backend to use the android-activity crate instead of ndk-glue. This solves a few issues: 1. The backend is agnostic of the application's choice of Activity base class 2. Winit is no longer responsible for handling any Java synchronization details, since these are encapsulated by the design of android_activity 3. The backend no longer depends on global / static getters for state such as the native_window() which puts it in a better position to support running multiple activities within a single Android process. 4. Redraw requests are flagged, not queued, in a way that avoids taking priority over user events (resolves rust-windowing#2299) To make it possible for application crates to avoid explicitly depending on the `android-activity` crate (and avoid version conflicts) this re-exports the android-activity crate under: `winit::platform::android::activity::*` This also adds `android-native-activity` and `android-game-activity` features that set the corresponding android-activity features. Addresses: PR rust-windowing#1892 Addresses: PR rust-windowing#2307 Addresses: PR rust-windowing#2343 Addresses: rust-windowing#2293 Resolves: rust-windowing#2299 Co-authored-by: Markus Siglreithmaier <[email protected]>

This updates the Android backend to use the android-activity crate instead of ndk-glue. This solves a few issues: 1. The backend is agnostic of the application's choice of Activity base class 2. Winit is no longer responsible for handling any Java synchronization details, since these are encapsulated by the design of android_activity 3. The backend no longer depends on global / static getters for state such as the native_window() which puts it in a better position to support running multiple activities within a single Android process. 4. Redraw requests are flagged, not queued, in a way that avoids taking priority over user events (resolves #2299) To make it possible for application crates to avoid explicitly depending on the `android-activity` crate (and avoid version conflicts) this re-exports the android-activity crate under: `winit::platform::android::activity::*` This also adds `android-native-activity` and `android-game-activity` features that set the corresponding android-activity features. Addresses: PR #1892 Addresses: PR #2307 Addresses: PR #2343 Addresses: #2293 Resolves: #2299 Co-authored-by: Markus Siglreithmaier <[email protected]> Co-authored-by: Markus Siglreithmaier <[email protected]>

This updates the Android backend to use the android_activity crate instead of ndk-glue. This solves a few issues: 1. The backend is agnostic of the application's choice of Activity base class 2. Winit is no longer responsible for handling any Java synchronization details, since these are encapsulated by the design of android_activity 3. The backend no longer depends on global / static getters for state such as the native_window() which puts it in a better position to support running multiple activities within a single Android process. 4. Redraw requests are flagged, not queued, in a way that avoids taking priority over user events (resolves rust-windowing#2299) Addresses: PR rust-windowing#1892 Addresses: PR rust-windowing#2307 Addresses: PR rust-windowing#2343 Addresses: rust-windowing#2293 Resolves: rust-windowing#2299

This updates the Android backend to use the android-activity crate instead of ndk-glue. This solves a few issues: 1. The backend is agnostic of the application's choice of Activity base class 2. Winit is no longer responsible for handling any Java synchronization details, since these are encapsulated by the design of android_activity 3. The backend no longer depends on global / static getters for state such as the native_window() which puts it in a better position to support running multiple activities within a single Android process. 4. Redraw requests are flagged, not queued, in a way that avoids taking priority over user events (resolves rust-windowing#2299) To make it possible for application crates to avoid explicitly depending on the `android-activity` crate (and avoid version conflicts) this re-exports the android-activity crate under: `winit::platform::android::activity::*` This also adds `android-native-activity` and `android-game-activity` features that set the corresponding android-activity features. Addresses: PR rust-windowing#1892 Addresses: PR rust-windowing#2307 Addresses: PR rust-windowing#2343 Addresses: rust-windowing#2293 Resolves: rust-windowing#2299 Co-authored-by: Markus Siglreithmaier <[email protected]>

MarijnS95 self-requested a review March 22, 2021 00:07

MarijnS95 added the DS - android label Mar 22, 2021

MarijnS95 reviewed Mar 22, 2021

View reviewed changes

src/platform_impl/android/mod.rs Show resolved Hide resolved

MarijnS95 approved these changes Mar 22, 2021

View reviewed changes

maciejgodek force-pushed the keep-nativewindow-lock branch from 5a1dbb3 to b211b5f Compare March 23, 2021 00:11

maciejgodek force-pushed the keep-nativewindow-lock branch from b211b5f to c04011d Compare March 23, 2021 00:14

MarijnS95 reviewed Mar 23, 2021

View reviewed changes

src/platform_impl/android/mod.rs Outdated Show resolved Hide resolved

MarijnS95 requested changes Mar 23, 2021

View reviewed changes

MarijnS95 requested a review from dvc94ch March 23, 2021 23:54

maciejgodek and others added 2 commits March 24, 2021 10:03

Convert comment into doc comment

cbeddb0

Co-authored-by: Marijn Suijten <[email protected]>

No longer obtain multiple read locks on Android NativeWindow

b079b26

maciejgodek force-pushed the keep-nativewindow-lock branch from efe7f94 to b079b26 Compare March 24, 2021 10:53

dvc94ch approved these changes Mar 24, 2021

View reviewed changes

MarijnS95 mentioned this pull request Jul 5, 2022

android: Hold NativeWindow lock until after notifying the user with Event::Suspended #2307

Merged

5 tasks

rib mentioned this pull request Aug 26, 2022

Android: rework backend to use android_activity crate #2444

Merged

5 tasks

maciejgodek closed this Mar 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hold on to Android NativeWindow lock to prevent races #1892

Hold on to Android NativeWindow lock to prevent races #1892

maciejgodek commented Mar 21, 2021 •

edited

Loading

MarijnS95 left a comment •

edited

Loading

MarijnS95 Mar 23, 2021

maciejgodek Mar 24, 2021

maciejgodek Mar 24, 2021

MarijnS95 Mar 24, 2021

maciejgodek Mar 24, 2021

maciejgodek Mar 24, 2021 •

edited

Loading

maciejgodek Apr 6, 2021

MarijnS95 Apr 6, 2021

dvc94ch left a comment

	/// State that we share across callbacks.
	pub state: RefCell<WinitState>,

Hold on to Android NativeWindow lock to prevent races #1892

Hold on to Android NativeWindow lock to prevent races #1892

Conversation

maciejgodek commented Mar 21, 2021 • edited Loading

MarijnS95 left a comment • edited Loading

Choose a reason for hiding this comment

MarijnS95 Mar 23, 2021

Choose a reason for hiding this comment

maciejgodek Mar 24, 2021

Choose a reason for hiding this comment

maciejgodek Mar 24, 2021

Choose a reason for hiding this comment

MarijnS95 Mar 24, 2021

Choose a reason for hiding this comment

maciejgodek Mar 24, 2021

Choose a reason for hiding this comment

maciejgodek Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

maciejgodek Apr 6, 2021

Choose a reason for hiding this comment

MarijnS95 Apr 6, 2021

Choose a reason for hiding this comment

dvc94ch left a comment

Choose a reason for hiding this comment

maciejgodek commented Mar 21, 2021 •

edited

Loading

MarijnS95 left a comment •

edited

Loading

maciejgodek Mar 24, 2021 •

edited

Loading