Merge "Synchronize in LazyValue.get()" am: 92b71e564f am: 278a00a8d3 am: df6618821f am: e807d7e73e

Original change: https://android-review.googlesource.com/c/platform/frameworks/base/+/1829879

Change-Id: I4dbff7d08a3c5a88e1ac466ef65d38aa3ec849ea

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;
+            object = ((Supplier<?>) object).get();
-            synchronized (this) {
-                object = supplier.get();
-            }
             mMap.setValueAt(i, object);
         }
         return object;

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;
@@ -3453,12 +3455,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;
@@ -3467,38 +3476,41 @@ public final class Parcel {
 
         @Override
         public Object get() {
-            if (mObject == null) {
+            Parcel source = mSource;
-                int restore = mSource.dataPosition();
+            if (source != null) {
+                synchronized (source) {
+                    int restore = source.dataPosition();
                     try {
-                    mSource.setDataPosition(mPosition);
+                        source.setDataPosition(mPosition);
-                    mObject = mSource.readValue(mLoader);
+                        mObject = source.readValue(mLoader);
                     } finally {
-                    mSource.setDataPosition(restore);
+                        source.setDataPosition(restore);
                     }
                     mSource = null;
-                if (mValueParcel != null) {
-                    mValueParcel.recycle();
-                    mValueParcel = null;
                 }
             }
             return mObject;
         }
 
         public void writeToParcel(Parcel out) {
-            if (mObject == null) {
+            Parcel source = mSource;
-                out.appendFrom(mSource, mPosition, mLength + 8);
+            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 + "}";
         }
@@ -3517,41 +3529,49 @@ public final class Parcel {
                 return false;
             }
             LazyValue value = (LazyValue) other;
-            // Check if they are either both serialized or both deserialized
+            // Check if they are either both serialized or both deserialized.
-            if ((mObject == null) != (value.mObject == null)) {
+            Parcel source = mSource;
+            Parcel otherSource = value.mSource;
+            if ((source == null) != (otherSource == null)) {
                 return false;
             }
-            // If both are deserialized, compare the live objects
+            // If both are deserialized, compare the live objects.
-            if (mObject != null) {
+            if (source == null) {
-                return mObject.equals(value.mObject);
+                // 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
+            // Finally we compare the payload.
-            return getValueParcel().compareData(value.getValueParcel()) == 0;
+            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() {
+        private Parcel getValueParcel(Parcel source) {
-            if (mValueParcel == null) {
+            Parcel parcel = mValueParcel;
-                mValueParcel = Parcel.obtain();
+            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;
         }
     }