ndk: Requre all off-thread dyn Fn* callback types to implement Send

In all these cases the NDK either documents or implements the callback
to be invoked from a single, separate thread.  For this to be allowed
by Rust thread safety, the closures and their (moved) contents are
effectively  "moved" to a different thread (`Send`) but not accessed
concurrently (`Sync`), but the type definitions were never requiring
this marker trait which could lead to undefined/invalid behaviour.

In addition some `Box`ed callbacks may have been dropped before a new
callback is passed to the NDK, leading to a possible race condition.
By storing the new callback _after_ registering it with the NDK, the
previous callback is now dropped later to ensure such race conditions
no longer happen (assuming AOSP callback invocations are properly
synchronized with the setters).

Multiple requests have been filed to solidify this "behavioural
contract" in the documentation where not done so yet:
https://issuetracker.google.com/issues/300602767#comment12
https://issuetracker.google.com/issues/318944941

ndk/CHANGELOG.md

@@ -3,6 +3,16 @@
 - Move `MediaFormat` from `media::media_codec` to its own `media::media_format` module. (#442)
 - media_format: Expose `MediaFormat::copy()` and `MediaFormat::clear()` from API level 29. (#449)
 - **Breaking:** media_format: Mark all `fn set_*()` and `fn str()` as taking `self` by `&mut`. (#452)
+- **Breaking:** Require all `dyn Fn*` types to implement `Send` when the FFI implementation invokes them on a separate thread: (#455)
+  - `audio::AudioStreamDataCallback`;
+  - `audio::AudioStreamErrorCallback`;
+  - `media::image_reader::BufferRemovedListener`;
+  - `media::image_reader::ImageListener`;
+  - `media::media_codec::ErrorCallback`;
+  - `media::media_codec::FormatChangedCallback`;
+  - `media::media_codec::InputAvailableCallback`;
+  - `media::media_codec::OutputAvailableCallback`.
+- Drop previous `Box`ed callbacks _after_ registering new ones, instead of before. (#455)
 - input_queue: Add `from_java()` constructor, available since API level 33. (#456)
 - event: Add `from_java()` constructors to `KeyEvent` and `MotionEvent`, available since API level 31. (#456)
 - event: Implement `SourceClass` `bitflag` and provide `Source::class()` getter. (#458)

ndk/src/audio.rs

@@ -500,9 +500,9 @@ impl fmt::Debug for AudioStreamBuilder {
 
 #[doc(alias = "AAudioStream_dataCallback")]
 pub type AudioStreamDataCallback =
-    Box<dyn FnMut(&AudioStream, *mut c_void, i32) -> AudioCallbackResult>;
+    Box<dyn FnMut(&AudioStream, *mut c_void, i32) -> AudioCallbackResult + Send>;
 #[doc(alias = "AAudioStream_errorCallback")]
-pub type AudioStreamErrorCallback = Box<dyn FnMut(&AudioStream, AudioError)>;
+pub type AudioStreamErrorCallback = Box<dyn FnMut(&AudioStream, AudioError) + Send>;
 
 impl AudioStreamBuilder {
     fn from_ptr(inner: NonNull<ffi::AAudioStreamBuilder>) -> Self {
@@ -643,7 +643,6 @@ impl AudioStreamBuilder {
     pub fn data_callback(mut self, callback: AudioStreamDataCallback) -> Self {
         let mut boxed = Box::new(callback);
         let ptr: *mut AudioStreamDataCallback = &mut *boxed;
-        self.data_callback = Some(boxed);
 
         unsafe extern "C" fn ffi_callback(
             stream: *mut ffi::AAudioStreamStruct,
@@ -672,6 +671,8 @@ impl AudioStreamBuilder {
             )
         };
 
+        self.data_callback = Some(boxed);
+
         self
     }
 
@@ -727,7 +728,6 @@ impl AudioStreamBuilder {
     pub fn error_callback(mut self, callback: AudioStreamErrorCallback) -> Self {
         let mut boxed = Box::new(callback);
         let ptr: *mut AudioStreamErrorCallback = &mut *boxed;
-        self.error_callback = Some(boxed);
 
         unsafe extern "C" fn ffi_callback(
             stream: *mut ffi::AAudioStreamStruct,
@@ -755,6 +755,8 @@ impl AudioStreamBuilder {
             )
         };
 
+        self.error_callback = Some(boxed);
+
         self
     }
 

ndk/src/media/image_reader.rs

@@ -44,10 +44,10 @@ pub enum ImageFormat {
     DEPTH_JPEG = ffi::AIMAGE_FORMATS::AIMAGE_FORMAT_DEPTH_JPEG.0,
 }
 
-pub type ImageListener = Box<dyn FnMut(&ImageReader)>;
+pub type ImageListener = Box<dyn FnMut(&ImageReader) + Send>;
 
 #[cfg(feature = "api-level-26")]
-pub type BufferRemovedListener = Box<dyn FnMut(&ImageReader, &HardwareBuffer)>;
+pub type BufferRemovedListener = Box<dyn FnMut(&ImageReader, &HardwareBuffer) + Send>;
 
 /// A native [`AImageReader *`]
 ///
@@ -122,16 +122,14 @@ impl ImageReader {
     pub fn set_image_listener(&mut self, listener: ImageListener) -> Result<()> {
         let mut boxed = Box::new(listener);
         let ptr: *mut ImageListener = &mut *boxed;
-        // keep listener alive until Drop or new listener is assigned
-        self.image_cb = Some(boxed);
 
         unsafe extern "C" fn on_image_available(
             context: *mut c_void,
             reader: *mut ffi::AImageReader,
         ) {
             abort_on_panic(|| {
                 let reader = ImageReader::from_ptr(NonNull::new_unchecked(reader));
-                let listener: *mut ImageListener = context as *mut _;
+                let listener: *mut ImageListener = context.cast();
                 (*listener)(&reader);
                 std::mem::forget(reader);
             })
@@ -142,6 +140,10 @@ impl ImageReader {
             onImageAvailable: Some(on_image_available),
         };
         let status = unsafe { ffi::AImageReader_setImageListener(self.as_ptr(), &mut listener) };
+
+        // keep listener alive until Drop or new listener is assigned
+        self.image_cb = Some(boxed);
+
         MediaError::from_status(status)
     }
 
@@ -150,8 +152,6 @@ impl ImageReader {
     pub fn set_buffer_removed_listener(&mut self, listener: BufferRemovedListener) -> Result<()> {
         let mut boxed = Box::new(listener);
         let ptr: *mut BufferRemovedListener = &mut *boxed;
-        // keep listener alive until Drop or new listener is assigned
-        self.buffer_removed_cb = Some(boxed);
 
         unsafe extern "C" fn on_buffer_removed(
             context: *mut c_void,
@@ -161,7 +161,7 @@ impl ImageReader {
             abort_on_panic(|| {
                 let reader = ImageReader::from_ptr(NonNull::new_unchecked(reader));
                 let buffer = HardwareBuffer::from_ptr(NonNull::new_unchecked(buffer));
-                let listener: *mut BufferRemovedListener = context as *mut _;
+                let listener: *mut BufferRemovedListener = context.cast();
                 (*listener)(&reader, &buffer);
                 std::mem::forget(reader);
             })
@@ -173,6 +173,10 @@ impl ImageReader {
         };
         let status =
             unsafe { ffi::AImageReader_setBufferRemovedListener(self.as_ptr(), &mut listener) };
+
+        // keep listener alive until Drop or new listener is assigned
+        self.buffer_removed_cb = Some(boxed);
+
         MediaError::from_status(status)
     }
 

ndk/src/media/media_codec.rs

@@ -94,10 +94,10 @@ impl fmt::Debug for AsyncNotifyCallback {
     }
 }
 
-pub type InputAvailableCallback = Box<dyn FnMut(usize)>;
-pub type OutputAvailableCallback = Box<dyn FnMut(usize, &BufferInfo)>;
-pub type FormatChangedCallback = Box<dyn FnMut(&MediaFormat)>;
-pub type ErrorCallback = Box<dyn FnMut(MediaError, ActionCode, &CStr)>;
+pub type InputAvailableCallback = Box<dyn FnMut(usize) + Send>;
+pub type OutputAvailableCallback = Box<dyn FnMut(usize, &BufferInfo) + Send>;
+pub type FormatChangedCallback = Box<dyn FnMut(&MediaFormat) + Send>;
+pub type ErrorCallback = Box<dyn FnMut(MediaError, ActionCode, &CStr) + Send>;
 
 impl MediaCodec {
     fn as_ptr(&self) -> *mut ffi::AMediaCodec {