Merge "binder_rs: Make ParcelableHolder thread-safe"android-s-v2-preview-2android-s-v2-preview-1android-s-v2-beta-2android-s-v2-preview-1

1 files changed, 37 insertions, 33 deletions

diff --git a/libs/binder/rust/src/parcel/parcelable_holder.rs b/libs/binder/rust/src/parcel/parcelable_holder.rs
index 3e75d1b11d..bccfd2d25e 100644
--- a/libs/binder/rust/src/parcel/parcelable_holder.rs
+++ b/libs/binder/rust/src/parcel/parcelable_holder.rs
@@ -16,13 +16,12 @@
 
 use crate::binder::Stability;
 use crate::error::{Result, StatusCode};
-use crate::parcel::{Parcel, Parcelable};
+use crate::parcel::{OwnedParcel, Parcel, Parcelable};
 use crate::{impl_deserialize_for_parcelable, impl_serialize_for_parcelable};
 
-use downcast_rs::{impl_downcast, Downcast};
+use downcast_rs::{impl_downcast, DowncastSync};
 use std::any::Any;
-use std::cell::RefCell;
-use std::rc::Rc;
+use std::sync::{Arc, Mutex};
 
 /// Metadata that `ParcelableHolder` needs for all parcelables.
 ///
@@ -40,18 +39,18 @@ pub trait ParcelableMetadata {
     }
 }
 
-trait AnyParcelable: Downcast + Parcelable + std::fmt::Debug {}
-impl_downcast!(AnyParcelable);
-impl<T> AnyParcelable for T where T: Downcast + Parcelable + std::fmt::Debug {}
+trait AnyParcelable: DowncastSync + Parcelable + std::fmt::Debug {}
+impl_downcast!(sync AnyParcelable);
+impl<T> AnyParcelable for T where T: DowncastSync + Parcelable + std::fmt::Debug {}
 
 #[derive(Debug, Clone)]
 enum ParcelableHolderData {
     Empty,
     Parcelable {
-        parcelable: Rc<dyn AnyParcelable>,
+        parcelable: Arc<dyn AnyParcelable>,
         name: String,
     },
-    Parcel(Parcel),
+    Parcel(OwnedParcel),
 }
 
 impl Default for ParcelableHolderData {
@@ -67,15 +66,15 @@ impl Default for ParcelableHolderData {
 /// `ParcelableHolder` is currently not thread-safe (neither
 /// `Send` nor `Sync`), mainly because it internally contains
 /// a `Parcel` which in turn is not thread-safe.
-#[derive(Debug, Default, Clone)]
+#[derive(Debug, Default)]
 pub struct ParcelableHolder {
-    // This is a `RefCell` because of `get_parcelable`
+    // This is a `Mutex` because of `get_parcelable`
     // which takes `&self` for consistency with C++.
     // We could make `get_parcelable` take a `&mut self`
-    // and get rid of the `RefCell` here for a performance
+    // and get rid of the `Mutex` here for a performance
     // improvement, but then callers would require a mutable
     // `ParcelableHolder` even for that getter method.
-    data: RefCell<ParcelableHolderData>,
+    data: Mutex<ParcelableHolderData>,
     stability: Stability,
 }
 
@@ -83,7 +82,7 @@ impl ParcelableHolder {
     /// Construct a new `ParcelableHolder` with the given stability.
     pub fn new(stability: Stability) -> Self {
         Self {
-            data: RefCell::new(ParcelableHolderData::Empty),
+            data: Mutex::new(ParcelableHolderData::Empty),
             stability,
         }
     }
@@ -93,20 +92,20 @@ impl ParcelableHolder {
     /// Note that this method does not reset the stability,
     /// only the contents.
     pub fn reset(&mut self) {
-        *self.data.get_mut() = ParcelableHolderData::Empty;
+        *self.data.get_mut().unwrap() = ParcelableHolderData::Empty;
         // We could also clear stability here, but C++ doesn't
     }
 
     /// Set the parcelable contained in this `ParcelableHolder`.
-    pub fn set_parcelable<T>(&mut self, p: Rc<T>) -> Result<()>
+    pub fn set_parcelable<T>(&mut self, p: Arc<T>) -> Result<()>
     where
-        T: Any + Parcelable + ParcelableMetadata + std::fmt::Debug,
+        T: Any + Parcelable + ParcelableMetadata + std::fmt::Debug + Send + Sync,
     {
         if self.stability > p.get_stability() {
             return Err(StatusCode::BAD_VALUE);
         }
 
-        *self.data.get_mut() = ParcelableHolderData::Parcelable {
+        *self.data.get_mut().unwrap() = ParcelableHolderData::Parcelable {
             parcelable: p,
             name: T::get_descriptor().into(),
         };
@@ -127,12 +126,12 @@ impl ParcelableHolder {
     /// * `Ok(None)` if the holder is empty or the descriptor does not match
     /// * `Ok(Some(_))` if the object holds a parcelable of type `T`
     ///   with the correct descriptor
-    pub fn get_parcelable<T>(&self) -> Result<Option<Rc<T>>>
+    pub fn get_parcelable<T>(&self) -> Result<Option<Arc<T>>>
     where
-        T: Any + Parcelable + ParcelableMetadata + Default + std::fmt::Debug,
+        T: Any + Parcelable + ParcelableMetadata + Default + std::fmt::Debug + Send + Sync,
     {
         let parcelable_desc = T::get_descriptor();
-        let mut data = self.data.borrow_mut();
+        let mut data = self.data.lock().unwrap();
         match *data {
             ParcelableHolderData::Empty => Ok(None),
             ParcelableHolderData::Parcelable {
@@ -143,12 +142,13 @@ impl ParcelableHolder {
                     return Err(StatusCode::BAD_VALUE);
                 }
 
-                match Rc::clone(parcelable).downcast_rc::<T>() {
+                match Arc::clone(parcelable).downcast_arc::<T>() {
                     Err(_) => Err(StatusCode::BAD_VALUE),
                     Ok(x) => Ok(Some(x)),
                 }
             }
-            ParcelableHolderData::Parcel(ref parcel) => {
+            ParcelableHolderData::Parcel(ref mut parcel) => {
+                let parcel = parcel.borrowed();
                 unsafe {
                     // Safety: 0 should always be a valid position.
                     parcel.set_data_position(0)?;
@@ -160,10 +160,10 @@ impl ParcelableHolder {
                 }
 
                 let mut parcelable = T::default();
-                parcelable.read_from_parcel(parcel)?;
+                parcelable.read_from_parcel(&parcel)?;
 
-                let parcelable = Rc::new(parcelable);
-                let result = Rc::clone(&parcelable);
+                let parcelable = Arc::new(parcelable);
+                let result = Arc::clone(&parcelable);
                 *data = ParcelableHolderData::Parcelable { parcelable, name };
 
                 Ok(Some(result))
@@ -184,7 +184,8 @@ impl Parcelable for ParcelableHolder {
     fn write_to_parcel(&self, parcel: &mut Parcel) -> Result<()> {
         parcel.write(&self.stability)?;
 
-        match *self.data.borrow() {
+        let mut data = self.data.lock().unwrap();
+        match *data {
             ParcelableHolderData::Empty => parcel.write(&0i32),
             ParcelableHolderData::Parcelable {
                 ref parcelable,
@@ -212,9 +213,10 @@ impl Parcelable for ParcelableHolder {
 
                 Ok(())
             }
-            ParcelableHolderData::Parcel(ref p) => {
+            ParcelableHolderData::Parcel(ref mut p) => {
+                let p = p.borrowed();
                 parcel.write(&p.get_data_size())?;
-                parcel.append_all_from(p)
+                parcel.append_all_from(&p)
             }
         }
     }
@@ -229,7 +231,7 @@ impl Parcelable for ParcelableHolder {
             return Err(StatusCode::BAD_VALUE);
         }
         if data_size == 0 {
-            *self.data.get_mut() = ParcelableHolderData::Empty;
+            *self.data.get_mut().unwrap() = ParcelableHolderData::Empty;
             return Ok(());
         }
 
@@ -240,9 +242,11 @@ impl Parcelable for ParcelableHolder {
             .checked_add(data_size)
             .ok_or(StatusCode::BAD_VALUE)?;
 
-        let mut new_parcel = Parcel::new();
-        new_parcel.append_from(parcel, data_start, data_size)?;
-        *self.data.get_mut() = ParcelableHolderData::Parcel(new_parcel);
+        let mut new_parcel = OwnedParcel::new();
+        new_parcel
+            .borrowed()
+            .append_from(parcel, data_start, data_size)?;
+        *self.data.get_mut().unwrap() = ParcelableHolderData::Parcel(new_parcel);
 
         unsafe {
             // Safety: `append_from` checks if `data_size` overflows