Merge "Handle Concurrency issues in Connection" into nyc-dev

telecomm/java/android/telecom/Conference.java

@@ -82,6 +82,7 @@ public abstract class Conference extends Conferenceable {
     private StatusHints mStatusHints;
     private Bundle mExtras;
     private Set<String> mPreviousExtraKeys;
+    private final Object mExtrasLock = new Object();
 
     private final Connection.Listener mConnectionDeathListener = new Connection.Listener() {
         @Override
@@ -686,11 +687,13 @@ public abstract class Conference extends Conferenceable {
      * @param extras The extras associated with this {@code Conference}.
      */
     public final void setExtras(@Nullable Bundle extras) {
+        // Keeping putExtras and removeExtras in the same lock so that this operation happens as a
+        // block instead of letting other threads put/remove while this method is running.
+        synchronized (mExtrasLock) {
             // Add/replace any new or changed extras values.
             putExtras(extras);
-
+            // If we have used "setExtras" in the past, compare the key set from the last invocation
-        // If we have used "setExtras" in the past, compare the key set from the last invocation to
+            // to the current one and remove any keys that went away.
-        // the current one and remove any keys that went away.
             if (mPreviousExtraKeys != null) {
                 List<String> toRemove = new ArrayList<String>();
                 for (String oldKey : mPreviousExtraKeys) {
@@ -704,8 +707,8 @@ public abstract class Conference extends Conferenceable {
                 }
             }
 
-        // Track the keys the last time set called setExtras.  This way, the next time setExtras is
+            // Track the keys the last time set called setExtras.  This way, the next time setExtras
-        // called we can see if the caller has removed any extras values.
+            // is called we can see if the caller has removed any extras values.
             if (mPreviousExtraKeys == null) {
                 mPreviousExtraKeys = new ArraySet<String>();
             }
@@ -714,6 +717,7 @@ public abstract class Conference extends Conferenceable {
                 mPreviousExtraKeys.addAll(extras.keySet());
             }
         }
+    }
 
     /**
      * Adds some extras to this {@link Conference}.  Existing keys are replaced and new ones are
@@ -730,13 +734,19 @@ public abstract class Conference extends Conferenceable {
             return;
         }
 
+        // Creating a Bundle clone so we don't have to synchronize on mExtrasLock while calling
+        // onExtrasChanged.
+        Bundle listenersBundle;
+        synchronized (mExtrasLock) {
             if (mExtras == null) {
                 mExtras = new Bundle();
             }
             mExtras.putAll(extras);
+            listenersBundle = new Bundle(mExtras);
+        }
 
         for (Listener l : mListeners) {
-            l.onExtrasChanged(this, extras);
+            l.onExtrasChanged(this, new Bundle(listenersBundle));
         }
     }
 
@@ -790,17 +800,17 @@ public abstract class Conference extends Conferenceable {
             return;
         }
 
+        synchronized (mExtrasLock) {
             if (mExtras != null) {
                 for (String key : keys) {
                     mExtras.remove(key);
                 }
-            if (mExtras.size() == 0) {
-                mExtras = null;
             }
         }
 
+        List<String> unmodifiableKeys = Collections.unmodifiableList(keys);
         for (Listener l : mListeners) {
-            l.onExtrasRemoved(this, keys);
+            l.onExtrasRemoved(this, unmodifiableKeys);
         }
     }
 
@@ -833,7 +843,13 @@ public abstract class Conference extends Conferenceable {
      * @hide
      */
     final void handleExtrasChanged(Bundle extras) {
+        Bundle b = null;
+        synchronized (mExtrasLock) {
             mExtras = extras;
-        onExtrasChanged(mExtras);
+            if (mExtras != null) {
+                b = new Bundle(mExtras);
+            }
+        }
+        onExtrasChanged(b);
     }
 }

telecomm/java/android/telecom/Connection.java

@@ -1239,6 +1239,7 @@ public abstract class Connection extends Conferenceable {
     private Conference mConference;
     private ConnectionService mConnectionService;
     private Bundle mExtras;
+    private final Object mExtrasLock = new Object();
 
     /**
      * Tracks the key set for the extras bundle provided on the last invocation of
@@ -1388,7 +1389,13 @@ public abstract class Connection extends Conferenceable {
      * @return The extras associated with this connection.
      */
     public final Bundle getExtras() {
-        return mExtras;
+        Bundle extras = null;
+        synchronized (mExtrasLock) {
+            if (mExtras != null) {
+                extras = new Bundle(mExtras);
+            }
+        }
+        return extras;
     }
 
     /**
@@ -1924,14 +1931,20 @@ public abstract class Connection extends Conferenceable {
         if (extras == null) {
             return;
         }
-
+        // Creating a duplicate bundle so we don't have to synchronize on mExtrasLock while calling
+        // the listeners.
+        Bundle listenerExtras;
+        synchronized (mExtrasLock) {
             if (mExtras == null) {
                 mExtras = new Bundle();
             }
             mExtras.putAll(extras);
-
+            listenerExtras = new Bundle(mExtras);
+        }
         for (Listener l : mListeners) {
-            l.onExtrasChanged(this, extras);
+            // Create a new clone of the extras for each listener so that they don't clobber
+            // each other
+            l.onExtrasChanged(this, new Bundle(listenerExtras));
         }
     }
 
@@ -1981,18 +1994,16 @@ public abstract class Connection extends Conferenceable {
      * @hide
      */
     public final void removeExtras(List<String> keys) {
+        synchronized (mExtrasLock) {
             if (mExtras != null) {
                 for (String key : keys) {
                     mExtras.remove(key);
                 }
-
-            if (mExtras.size() == 0) {
-                mExtras = null;
             }
         }
-
+        List<String> unmodifiableKeys = Collections.unmodifiableList(keys);
         for (Listener l : mListeners) {
-            l.onExtrasRemoved(this, keys);
+            l.onExtrasRemoved(this, unmodifiableKeys);
         }
     }
 
@@ -2274,8 +2285,14 @@ public abstract class Connection extends Conferenceable {
      * @hide
      */
     final void handleExtrasChanged(Bundle extras) {
+        Bundle b = null;
+        synchronized (mExtrasLock) {
             mExtras = extras;
-        onExtrasChanged(mExtras);
+            if (mExtras != null) {
+                b = new Bundle(mExtras);
+            }
+        }
+        onExtrasChanged(b);
     }
 
     /**