Merge "Refactor Bundle for thread-safety of Bundle.hasFileDescriptors()" am: f5cc73d25f

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

Change-Id: I43dff41f44fb904c6c4d44b870c9de88945f04f3

core/java/android/os/BaseBundle.java

@@ -103,7 +103,7 @@ public class BaseBundle {
      * are unparcelled, mParcelledData willbe set to null.
      */
     @UnsupportedAppUsage
-    Parcel mParcelledData = null;
+    volatile Parcel mParcelledData = null;
 
     /**
      * Whether {@link #mParcelledData} was generated by native code or not.
@@ -182,13 +182,56 @@ public class BaseBundle {
      * @param b a Bundle to be copied.
      */
     BaseBundle(BaseBundle b) {
-        copyInternal(b, false);
+        this(b, /* deep */ false);
     }
 
     /**
-     * Special constructor that does not initialize the bundle.
+     * Constructs a {@link BaseBundle} containing a copy of {@code from}.
+     *
+     * @param from The bundle to be copied.
+     * @param deep Whether is a deep or shallow copy.
+     *
+     * @hide
      */
-    BaseBundle(boolean doInit) {
+    BaseBundle(BaseBundle from, boolean deep) {
+        synchronized (from) {
+            mClassLoader = from.mClassLoader;
+
+            if (from.mMap != null) {
+                if (!deep) {
+                    mMap = new ArrayMap<>(from.mMap);
+                } else {
+                    final ArrayMap<String, Object> fromMap = from.mMap;
+                    final int n = fromMap.size();
+                    mMap = new ArrayMap<>(n);
+                    for (int i = 0; i < n; i++) {
+                        mMap.append(fromMap.keyAt(i), deepCopyValue(fromMap.valueAt(i)));
+                    }
+                }
+            } else {
+                mMap = null;
+            }
+
+            final Parcel parcelledData;
+            if (from.mParcelledData != null) {
+                if (from.isEmptyParcel()) {
+                    parcelledData = NoImagePreloadHolder.EMPTY_PARCEL;
+                    mParcelledByNative = false;
+                } else {
+                    parcelledData = Parcel.obtain();
+                    parcelledData.appendFrom(from.mParcelledData, 0,
+                            from.mParcelledData.dataSize());
+                    parcelledData.setDataPosition(0);
+                    mParcelledByNative = from.mParcelledByNative;
+                }
+            } else {
+                parcelledData = null;
+                mParcelledByNative = false;
+            }
+
+            // Keep as last statement to ensure visibility of other fields
+            mParcelledData = parcelledData;
+        }
     }
 
     /**
@@ -323,8 +366,8 @@ public class BaseBundle {
             } else {
                 mMap.erase();
             }
-            mParcelledData = null;
             mParcelledByNative = false;
+            mParcelledData = null;
             return;
         }
 
@@ -358,8 +401,8 @@ public class BaseBundle {
             if (recycleParcel) {
                 recycleParcel(parcelledData);
             }
-            mParcelledData = null;
             mParcelledByNative = false;
+            mParcelledData = null;
         }
         if (DEBUG) {
             Log.d(TAG, "unparcel " + Integer.toHexString(System.identityHashCode(this))
@@ -501,44 +544,7 @@ public class BaseBundle {
         mMap.clear();
     }
 
-    void copyInternal(BaseBundle from, boolean deep) {
+    private Object deepCopyValue(Object value) {
-        synchronized (from) {
-            if (from.mParcelledData != null) {
-                if (from.isEmptyParcel()) {
-                    mParcelledData = NoImagePreloadHolder.EMPTY_PARCEL;
-                    mParcelledByNative = false;
-                } else {
-                    mParcelledData = Parcel.obtain();
-                    mParcelledData.appendFrom(from.mParcelledData, 0,
-                            from.mParcelledData.dataSize());
-                    mParcelledData.setDataPosition(0);
-                    mParcelledByNative = from.mParcelledByNative;
-                }
-            } else {
-                mParcelledData = null;
-                mParcelledByNative = false;
-            }
-
-            if (from.mMap != null) {
-                if (!deep) {
-                    mMap = new ArrayMap<>(from.mMap);
-                } else {
-                    final ArrayMap<String, Object> fromMap = from.mMap;
-                    final int N = fromMap.size();
-                    mMap = new ArrayMap<>(N);
-                    for (int i = 0; i < N; i++) {
-                        mMap.append(fromMap.keyAt(i), deepCopyValue(fromMap.valueAt(i)));
-                    }
-                }
-            } else {
-                mMap = null;
-            }
-
-            mClassLoader = from.mClassLoader;
-        }
-    }
-
-    Object deepCopyValue(Object value) {
         if (value == null) {
             return null;
         }
@@ -570,7 +576,7 @@ public class BaseBundle {
         return value;
     }
 
-    ArrayList deepcopyArrayList(ArrayList from) {
+    private ArrayList deepcopyArrayList(ArrayList from) {
         final int N = from.size();
         ArrayList out = new ArrayList(N);
         for (int i=0; i<N; i++) {
@@ -1717,9 +1723,9 @@ public class BaseBundle {
         if (length < 0) {
             throw new RuntimeException("Bad length in parcel: " + length);
         } else if (length == 0) {
+            mParcelledByNative = false;
             // Empty Bundle or end of data.
             mParcelledData = NoImagePreloadHolder.EMPTY_PARCEL;
-            mParcelledByNative = false;
             return;
         } else if (length % 4 != 0) {
             throw new IllegalStateException("Bundle length is not aligned by 4: " + length);
@@ -1757,8 +1763,8 @@ public class BaseBundle {
                 + ": " + length + " bundle bytes starting at " + offset);
         p.setDataPosition(0);
 
-        mParcelledData = p;
         mParcelledByNative = isNativeBundle;
+        mParcelledData = p;
     }
 
     /** {@hide} */

core/java/android/os/Bundle.java

@@ -101,6 +101,18 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable {
         maybePrefillHasFds();
     }
 
+    /**
+     * Constructs a {@link Bundle} containing a copy of {@code from}.
+     *
+     * @param from The bundle to be copied.
+     * @param deep Whether is a deep or shallow copy.
+     *
+     * @hide
+     */
+    Bundle(Bundle from, boolean deep) {
+        super(from, deep);
+    }
+
     /**
      * If {@link #mParcelledData} is not null, copy the HAS FDS bit from it because it's fast.
      * Otherwise (if {@link #mParcelledData} is already null), leave {@link #FLAG_HAS_FDS_KNOWN}
@@ -166,13 +178,6 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable {
         mFlags = FLAG_HAS_FDS_KNOWN | FLAG_ALLOW_FDS;
     }
 
-    /**
-     * Constructs a Bundle without initializing it.
-     */
-    Bundle(boolean doInit) {
-        super(doInit);
-    }
-
     /**
      * Make a Bundle for a single key/value pair.
      *
@@ -260,9 +265,7 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable {
      * are referenced as-is and not copied in any way.
      */
     public Bundle deepCopy() {
-        Bundle b = new Bundle(false);
+        return new Bundle(this, /* deep */ true);
-        b.copyInternal(this, true);
-        return b;
     }
 
     /**

core/java/android/os/PersistableBundle.java

@@ -156,10 +156,15 @@ public final class PersistableBundle extends BaseBundle implements Cloneable, Pa
     }
 
     /**
-     * Constructs a PersistableBundle without initializing it.
+     * Constructs a {@link PersistableBundle} containing a copy of {@code from}.
+     *
+     * @param from The bundle to be copied.
+     * @param deep Whether is a deep or shallow copy.
+     *
+     * @hide
      */
-    PersistableBundle(boolean doInit) {
+    PersistableBundle(PersistableBundle from, boolean deep) {
-        super(doInit);
+        super(from, deep);
     }
 
     /**
@@ -190,9 +195,7 @@ public final class PersistableBundle extends BaseBundle implements Cloneable, Pa
      * are referenced as-is and not copied in any way.
      */
     public PersistableBundle deepCopy() {
-        PersistableBundle b = new PersistableBundle(false);
+        return new PersistableBundle(this, /* deep */ true);
-        b.copyInternal(this, true);
-        return b;
     }
 
     /**