Synchronize in LazyValue.get()

When we use Bundle's copy constructor we perform a shallow copy of the
underlying map, so lazy values are the same across the original and the
clone. So, if we're accessing the same item backed by a lazy value in 2
different threads, each one accessing a different bundle (the original
and the clone), both would have to synchronize on a common lock.

Although Bundle is not thread-safe, we should keep the expectation that
the client only needs to properly synchronize access to one bundle, not
more. To achieve this, we synchronize on the original parcel held by
LazyValue in get(), this removes the need for the above.

Note that after we produce the lazy values and put them in the map, we
don't have a reference to mParcelledData anymore inside the Bundle, the
only objects holding a reference to that parcel are the lazy values.

Also fixed some stuff in LazyValue:
* Fixed condition inside LazyValue that used mObject == null as
  signal for "not deserialized" state since mObject can be null, so
  switched to mSource != null.
* Made mSource volatile and adjusted operations to always check it
  before, ensuring visibility of itself and mObject via happens-before.
  This works because after checking mSource, if that's null, mObject is
  guaranteed to be visible and valid, if it's not null, we can use it
  since we're in the serialized state and this works even if another
  thread deserializes in between as long as we hold on to a reference of
  mSource in a local because the parcel is still valid.
* Remove the recycling of mValueParcel, otherwise we'd need locking to
  ensure whatever was using it was still using a valid object. Decided
  to just remove recycling for now since the plan is to remove
  mValueParcel entirely when we add comparison and hasFileDescriptor()
  methods that operate on a range to Parcel.

Bug: 195622897
Test: atest -d android.os.cts.ParcelTest android.os.cts.BundleTest
      android.os.BundleTest android.os.ParcelTest
Test: More tests coming
Change-Id: I501c91f505950338b23e7837e55f89b2f63ff93a

2 files changed, 61 insertions, 38 deletions

diff --git a/core/java/android/os/BaseBundle.java b/core/java/android/os/BaseBundle.java
index 6da02f5c9ff5..fb21ce30415f 100644
--- a/core/java/android/os/BaseBundle.java
+++ b/core/java/android/os/BaseBundle.java
@@ -260,6 +260,9 @@ public class BaseBundle {
     /**
      * Returns the value for key {@code key}.
      *
+     * This call should always be made after {@link #unparcel()} or inside a lock after making sure
+     * {@code mMap} is not null.
+     *
      * @hide
      */
     final Object getValue(String key) {
@@ -270,15 +273,15 @@ public class BaseBundle {
     /**
      * Returns the value for a certain position in the array map.
      *
+     * This call should always be made after {@link #unparcel()} or inside a lock after making sure
+     * {@code mMap} is not null.
+     *
      * @hide
      */
     final Object getValueAt(int i) {
         Object object = mMap.valueAt(i);
         if (object instanceof Supplier<?>) {
-            Supplier<?> supplier = (Supplier<?>) object;
-            synchronized (this) {
-                object = supplier.get();
-            }
+            object = ((Supplier<?>) object).get();
             mMap.setValueAt(i, object);
         }
         return object;
diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java
index a2716d211bbf..7f2e2b115986 100644
--- a/core/java/android/os/Parcel.java
+++ b/core/java/android/os/Parcel.java
@@ -16,6 +16,8 @@
 
 package android.os;
 
+import static java.util.Objects.requireNonNull;
+
 import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.annotation.TestApi;
@@ -3412,12 +3414,19 @@ public final class Parcel {
         private final int mLength;
         private final int mType;
         @Nullable private final ClassLoader mLoader;
-        @Nullable private Parcel mSource;
         @Nullable private Object mObject;
-        @Nullable private Parcel mValueParcel;
+        @Nullable private volatile Parcel mValueParcel;
+
+        /**
+         * This goes from non-null to null once. Always check the nullability of this object before
+         * performing any operations, either involving itself or mObject since the happens-before
+         * established by this volatile will guarantee visibility of either. We can assume this
+         * parcel won't change anymore.
+         */
+        @Nullable private volatile Parcel mSource;
 
         LazyValue(Parcel source, int position, int length, int type, @Nullable ClassLoader loader) {
-            mSource = source;
+            mSource = requireNonNull(source);
             mPosition = position;
             mLength = length;
             mType = type;
@@ -3426,38 +3435,41 @@ public final class Parcel {
 
         @Override
         public Object get() {
-            if (mObject == null) {
-                int restore = mSource.dataPosition();
-                try {
-                    mSource.setDataPosition(mPosition);
-                    mObject = mSource.readValue(mLoader);
-                } finally {
-                    mSource.setDataPosition(restore);
-                }
-                mSource = null;
-                if (mValueParcel != null) {
-                    mValueParcel.recycle();
-                    mValueParcel = null;
+            Parcel source = mSource;
+            if (source != null) {
+                synchronized (source) {
+                    int restore = source.dataPosition();
+                    try {
+                        source.setDataPosition(mPosition);
+                        mObject = source.readValue(mLoader);
+                    } finally {
+                        source.setDataPosition(restore);
+                    }
+                    mSource = null;
                 }
             }
             return mObject;
         }
 
         public void writeToParcel(Parcel out) {
-            if (mObject == null) {
-                out.appendFrom(mSource, mPosition, mLength + 8);
+            Parcel source = mSource;
+            if (source != null) {
+                out.appendFrom(source, mPosition, mLength + 8);
             } else {
                 out.writeValue(mObject);
             }
         }
 
         public boolean hasFileDescriptors() {
-            return getValueParcel().hasFileDescriptors();
+            Parcel source = mSource;
+            return (source != null)
+                    ? getValueParcel(source).hasFileDescriptors()
+                    : Parcel.hasFileDescriptors(mObject);
         }
 
         @Override
         public String toString() {
-            return mObject == null
+            return (mSource != null)
                     ? "Supplier{" + valueTypeToString(mType) + "@" + mPosition + "+" + mLength + '}'
                     : "Supplier{" + mObject + "}";
         }
@@ -3476,41 +3488,49 @@ public final class Parcel {
                 return false;
             }
             LazyValue value = (LazyValue) other;
-            // Check if they are either both serialized or both deserialized
-            if ((mObject == null) != (value.mObject == null)) {
+            // Check if they are either both serialized or both deserialized.
+            Parcel source = mSource;
+            Parcel otherSource = value.mSource;
+            if ((source == null) != (otherSource == null)) {
                 return false;
             }
-            // If both are deserialized, compare the live objects
-            if (mObject != null) {
-                return mObject.equals(value.mObject);
+            // If both are deserialized, compare the live objects.
+            if (source == null) {
+                // Note that here it's guaranteed that both mObject references contain valid values
+                // (possibly null) since mSource will have provided the memory barrier for those and
+                // once deserialized we never go back to serialized state.
+                return Objects.equals(mObject, value.mObject);
             }
-            // Better safely fail here since this could mean we get different objects
+            // Better safely fail here since this could mean we get different objects.
             if (!Objects.equals(mLoader, value.mLoader)) {
                 return false;
             }
-            // Otherwise compare metadata prior to comparing payload
+            // Otherwise compare metadata prior to comparing payload.
             if (mType != value.mType || mLength != value.mLength) {
                 return false;
             }
-            // Finally we compare the payload
-            return getValueParcel().compareData(value.getValueParcel()) == 0;
+            // Finally we compare the payload.
+            return getValueParcel(source).compareData(value.getValueParcel(otherSource)) == 0;
         }
 
         @Override
         public int hashCode() {
-            return Objects.hash(mObject, mLoader, mType, mLength);
+            // Accessing mSource first to provide memory barrier for mObject
+            return Objects.hash(mSource == null, mObject, mLoader, mType, mLength);
         }
 
         /** This extracts the parcel section responsible for the object and returns it. */
-        private Parcel getValueParcel() {
-            if (mValueParcel == null) {
-                mValueParcel = Parcel.obtain();
+        private Parcel getValueParcel(Parcel source) {
+            Parcel parcel = mValueParcel;
+            if (parcel == null) {
+                parcel = Parcel.obtain();
                 // mLength is the length of object representation, excluding the type and length.
                 // mPosition is the position of the entire value container, right before the type.
                 // So, we add 4 bytes for the type + 4 bytes for the length written.
-                mValueParcel.appendFrom(mSource, mPosition, mLength + 8);
+                parcel.appendFrom(source, mPosition, mLength + 8);
+                mValueParcel = parcel;
             }
-            return mValueParcel;
+            return parcel;
         }
     }