From 229e3820dce98f64fd4834d5f421faec9a9d7026 Mon Sep 17 00:00:00 2001 From: Jay Shrauner Date: Fri, 15 Aug 2014 09:23:07 -0700 Subject: [PATCH] Prevent ConcurrentModificationExceptions Use sets backed by ConcurrentHashMaps instead of HashSets, and CopyOnWriteArrayLists instead of ArrayLists, to prevent concurrent exceptions if listeners try to remove themselves in callbacks while iterating over the listeners. Bug:16325026 Change-Id: I55e081eda6ba19fa466bbf019c648bbdaf833c33 --- telecomm/java/android/telecomm/Call.java | 57 ++++++++----------- .../java/android/telecomm/Connection.java | 23 ++++---- .../telecomm/ConnectionServiceAdapter.java | 9 ++- telecomm/java/android/telecomm/Phone.java | 27 +++++---- .../android/telecomm/RemoteConnection.java | 16 ++++-- 5 files changed, 69 insertions(+), 63 deletions(-) diff --git a/telecomm/java/android/telecomm/Call.java b/telecomm/java/android/telecomm/Call.java index f988ac8a759b7..7223574b28897 100644 --- a/telecomm/java/android/telecomm/Call.java +++ b/telecomm/java/android/telecomm/Call.java @@ -27,6 +27,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.CopyOnWriteArrayList; /** * Represents an ongoing phone call that the in-call app should present to the user. @@ -364,7 +365,7 @@ public final class Call { private final InCallAdapter mInCallAdapter; private final List mChildren = new ArrayList<>(); private final List mUnmodifiableChildren = Collections.unmodifiableList(mChildren); - private final List mListeners = new ArrayList<>(); + private final List mListeners = new CopyOnWriteArrayList<>(); private final List mConferenceableCalls = new ArrayList<>(); private final List mUnmodifiableConferenceableCalls = Collections.unmodifiableList(mConferenceableCalls); @@ -589,7 +590,9 @@ public final class Call { * @param listener A {@code Listener}. */ public void removeListener(Listener listener) { - mListeners.remove(listener); + if (listener != null) { + mListeners.remove(listener); + } } /** {@hide} */ @@ -709,72 +712,62 @@ public final class Call { } private void fireStateChanged(int newState) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onStateChanged(this, newState); + for (Listener listener : mListeners) { + listener.onStateChanged(this, newState); } } private void fireParentChanged(Call newParent) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onParentChanged(this, newParent); + for (Listener listener : mListeners) { + listener.onParentChanged(this, newParent); } } private void fireChildrenChanged(List children) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onChildrenChanged(this, children); + for (Listener listener : mListeners) { + listener.onChildrenChanged(this, children); } } private void fireDetailsChanged(Details details) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onDetailsChanged(this, details); + for (Listener listener : mListeners) { + listener.onDetailsChanged(this, details); } } private void fireCannedTextResponsesLoaded(List cannedTextResponses) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onCannedTextResponsesLoaded(this, cannedTextResponses); + for (Listener listener : mListeners) { + listener.onCannedTextResponsesLoaded(this, cannedTextResponses); } } private void fireVideoCallChanged(InCallService.VideoCall videoCall) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onVideoCallChanged(this, videoCall); + for (Listener listener : mListeners) { + listener.onVideoCallChanged(this, videoCall); } } private void firePostDialWait(String remainingPostDialSequence) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onPostDialWait(this, remainingPostDialSequence); + for (Listener listener : mListeners) { + listener.onPostDialWait(this, remainingPostDialSequence); } } private void fireStartActivity(PendingIntent intent) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onStartActivity(this, intent); + for (Listener listener : mListeners) { + listener.onStartActivity(this, intent); } } private void fireCallDestroyed() { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onCallDestroyed(this); + for (Listener listener : mListeners) { + listener.onCallDestroyed(this); } } private void fireConferenceableCallsChanged() { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onConferenceableCallsChanged(this, mUnmodifiableConferenceableCalls); + for (Listener listener : mListeners) { + listener.onConferenceableCallsChanged(this, mUnmodifiableConferenceableCalls); } } diff --git a/telecomm/java/android/telecomm/Connection.java b/telecomm/java/android/telecomm/Connection.java index 3ecb4cb70b8b8..78c34a117500e 100644 --- a/telecomm/java/android/telecomm/Connection.java +++ b/telecomm/java/android/telecomm/Connection.java @@ -32,7 +32,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Set; -import java.util.concurrent.CopyOnWriteArraySet; +import java.util.concurrent.ConcurrentHashMap; /** * Represents a connection to a remote endpoint that carries voice traffic. @@ -448,7 +448,13 @@ public abstract class Connection { } }; - private final Set mListeners = new CopyOnWriteArraySet<>(); + /** + * ConcurrentHashMap constructor params: 8 is initial table size, 0.9f is + * load factor before resizing, 1 means we only expect a single thread to + * access the map so make only a single shard + */ + private final Set mListeners = Collections.newSetFromMap( + new ConcurrentHashMap(8, 0.9f, 1)); private final List mChildConnections = new ArrayList<>(); private final List mUnmodifiableChildConnections = Collections.unmodifiableList(mChildConnections); @@ -587,7 +593,9 @@ public abstract class Connection { * @hide */ public final Connection removeConnectionListener(Listener l) { - mListeners.remove(l); + if (l != null) { + mListeners.remove(l); + } return this; } @@ -874,13 +882,8 @@ public abstract class Connection { * Tears down the Connection object. */ public final void destroy() { - // It is possible that onDestroy() will trigger the listener to remove itself which will - // result in a concurrent modification exception. To counteract this we make a copy of the - // listeners and iterate on that. - for (Listener l : new ArrayList<>(mListeners)) { - if (mListeners.contains(l)) { - l.onDestroyed(this); - } + for (Listener l : mListeners) { + l.onDestroyed(this); } } diff --git a/telecomm/java/android/telecomm/ConnectionServiceAdapter.java b/telecomm/java/android/telecomm/ConnectionServiceAdapter.java index bfcb5c3218be8..4144b81e5c844 100644 --- a/telecomm/java/android/telecomm/ConnectionServiceAdapter.java +++ b/telecomm/java/android/telecomm/ConnectionServiceAdapter.java @@ -36,8 +36,13 @@ import java.util.concurrent.ConcurrentHashMap; * @hide */ final class ConnectionServiceAdapter implements DeathRecipient { + /** + * ConcurrentHashMap constructor params: 8 is initial table size, 0.9f is + * load factor before resizing, 1 means we only expect a single thread to + * access the map so make only a single shard + */ private final Set mAdapters = Collections.newSetFromMap( - new ConcurrentHashMap(2)); + new ConcurrentHashMap(8, 0.9f, 1)); ConnectionServiceAdapter() { } @@ -53,7 +58,7 @@ final class ConnectionServiceAdapter implements DeathRecipient { } void removeAdapter(IConnectionServiceAdapter adapter) { - if (mAdapters.remove(adapter)) { + if (adapter != null && mAdapters.remove(adapter)) { adapter.asBinder().unlinkToDeath(this, 0); } } diff --git a/telecomm/java/android/telecomm/Phone.java b/telecomm/java/android/telecomm/Phone.java index 79e777a4eb4e4..03a86763ce878 100644 --- a/telecomm/java/android/telecomm/Phone.java +++ b/telecomm/java/android/telecomm/Phone.java @@ -24,6 +24,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.CopyOnWriteArrayList; /** * A unified virtual device providing a means of voice (and other) communication on a device. @@ -89,7 +90,7 @@ public final class Phone { private AudioState mAudioState; - private final List mListeners = new ArrayList<>(); + private final List mListeners = new CopyOnWriteArrayList<>(); /** {@hide} */ Phone(InCallAdapter adapter) { @@ -171,7 +172,9 @@ public final class Phone { * @param listener A {@code Listener} object. */ public final void removeListener(Listener listener) { - mListeners.remove(listener); + if (listener != null) { + mListeners.remove(listener); + } } /** @@ -236,30 +239,26 @@ public final class Phone { } private void fireCallAdded(Call call) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onCallAdded(this, call); + for (Listener listener : mListeners) { + listener.onCallAdded(this, call); } } private void fireCallRemoved(Call call) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onCallRemoved(this, call); + for (Listener listener : mListeners) { + listener.onCallRemoved(this, call); } } private void fireAudioStateChanged(AudioState audioState) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onAudioStateChanged(this, audioState); + for (Listener listener : mListeners) { + listener.onAudioStateChanged(this, audioState); } } private void fireBringToForeground(boolean showDialpad) { - Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]); - for (int i = 0; i < listeners.length; i++) { - listeners[i].onBringToForeground(this, showDialpad); + for (Listener listener : mListeners) { + listener.onBringToForeground(this, showDialpad); } } diff --git a/telecomm/java/android/telecomm/RemoteConnection.java b/telecomm/java/android/telecomm/RemoteConnection.java index d3972d31fa39c..f1cee10e4ea16 100644 --- a/telecomm/java/android/telecomm/RemoteConnection.java +++ b/telecomm/java/android/telecomm/RemoteConnection.java @@ -184,8 +184,13 @@ public final class RemoteConnection { private IConnectionService mConnectionService; private final String mConnectionId; + /** + * ConcurrentHashMap constructor params: 8 is initial table size, 0.9f is + * load factor before resizing, 1 means we only expect a single thread to + * access the map so make only a single shard + */ private final Set mListeners = Collections.newSetFromMap( - new ConcurrentHashMap(2)); + new ConcurrentHashMap(8, 0.9f, 1)); private final Set mConferenceableConnections = new HashSet<>(); private int mState = Connection.STATE_NEW; @@ -248,7 +253,9 @@ public final class RemoteConnection { * @param listener A {@code Listener}. */ public void removeListener(Listener listener) { - mListeners.remove(listener); + if (listener != null) { + mListeners.remove(listener); + } } /** @@ -588,11 +595,10 @@ public final class RemoteConnection { setDisconnected(DisconnectCause.ERROR_UNSPECIFIED, "Connection destroyed."); } - Set listeners = new HashSet(mListeners); - mListeners.clear(); - for (Listener l : listeners) { + for (Listener l : mListeners) { l.onDestroyed(this); } + mListeners.clear(); mConnected = false; }