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

3 files changed, 75 insertions, 63 deletions

diff --git a/core/java/android/os/BaseBundle.java b/core/java/android/os/BaseBundle.java
index 2a344f6c2b6b..ad3de25fecc2 100644
--- a/core/java/android/os/BaseBundle.java
+++ b/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) {
-        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) {
+    private 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} */
diff --git a/core/java/android/os/Bundle.java b/core/java/android/os/Bundle.java
index feca81e1aea1..b2bbfd6c163d 100644
--- a/core/java/android/os/Bundle.java
+++ b/core/java/android/os/Bundle.java
@@ -102,6 +102,18 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable {
     }
 
     /**
+     * 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}
      * unset, because scanning a map is slower.  We'll do it lazily in
@@ -167,13 +179,6 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable {
     }
 
     /**
-     * Constructs a Bundle without initializing it.
-     */
-    Bundle(boolean doInit) {
-        super(doInit);
-    }
-
-    /**
      * Make a Bundle for a single key/value pair.
      *
      * @hide
@@ -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);
-        b.copyInternal(this, true);
-        return b;
+        return new Bundle(this, /* deep */ true);
     }
 
     /**
diff --git a/core/java/android/os/PersistableBundle.java b/core/java/android/os/PersistableBundle.java
index 7b55e710caeb..f4edcb1317f4 100644
--- a/core/java/android/os/PersistableBundle.java
+++ 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) {
-        super(doInit);
+    PersistableBundle(PersistableBundle from, boolean deep) {
+        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);
-        b.copyInternal(this, true);
-        return b;
+        return new PersistableBundle(this, /* deep */ true);
     }
 
     /**