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
diff --git a/telecomm/java/android/telecomm/Call.java b/telecomm/java/android/telecomm/Call.java
index f988ac8..7223574 100644
--- a/telecomm/java/android/telecomm/Call.java
+++ b/telecomm/java/android/telecomm/Call.java
@@ -27,6 +27,7 @@
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 @@
private final InCallAdapter mInCallAdapter;
private final List<Call> mChildren = new ArrayList<>();
private final List<Call> mUnmodifiableChildren = Collections.unmodifiableList(mChildren);
- private final List<Listener> mListeners = new ArrayList<>();
+ private final List<Listener> mListeners = new CopyOnWriteArrayList<>();
private final List<Call> mConferenceableCalls = new ArrayList<>();
private final List<Call> mUnmodifiableConferenceableCalls =
Collections.unmodifiableList(mConferenceableCalls);
@@ -589,7 +590,9 @@
* @param listener A {@code Listener}.
*/
public void removeListener(Listener listener) {
- mListeners.remove(listener);
+ if (listener != null) {
+ mListeners.remove(listener);
+ }
}
/** {@hide} */
@@ -709,72 +712,62 @@
}
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<Call> 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<String> 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 3ecb4cb..78c34a1 100644
--- a/telecomm/java/android/telecomm/Connection.java
+++ b/telecomm/java/android/telecomm/Connection.java
@@ -32,7 +32,7 @@
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 @@
}
};
- private final Set<Listener> 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<Listener> mListeners = Collections.newSetFromMap(
+ new ConcurrentHashMap<Listener, Boolean>(8, 0.9f, 1));
private final List<Connection> mChildConnections = new ArrayList<>();
private final List<Connection> mUnmodifiableChildConnections =
Collections.unmodifiableList(mChildConnections);
@@ -587,7 +593,9 @@
* @hide
*/
public final Connection removeConnectionListener(Listener l) {
- mListeners.remove(l);
+ if (l != null) {
+ mListeners.remove(l);
+ }
return this;
}
@@ -874,13 +882,8 @@
* 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 bfcb5c3..4144b81 100644
--- a/telecomm/java/android/telecomm/ConnectionServiceAdapter.java
+++ b/telecomm/java/android/telecomm/ConnectionServiceAdapter.java
@@ -36,8 +36,13 @@
* @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<IConnectionServiceAdapter> mAdapters = Collections.newSetFromMap(
- new ConcurrentHashMap<IConnectionServiceAdapter, Boolean>(2));
+ new ConcurrentHashMap<IConnectionServiceAdapter, Boolean>(8, 0.9f, 1));
ConnectionServiceAdapter() {
}
@@ -53,7 +58,7 @@
}
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 79e777a..03a8676 100644
--- a/telecomm/java/android/telecomm/Phone.java
+++ b/telecomm/java/android/telecomm/Phone.java
@@ -24,6 +24,7 @@
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 @@
private AudioState mAudioState;
- private final List<Listener> mListeners = new ArrayList<>();
+ private final List<Listener> mListeners = new CopyOnWriteArrayList<>();
/** {@hide} */
Phone(InCallAdapter adapter) {
@@ -171,7 +172,9 @@
* @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 @@
}
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 d3972d31..f1cee10 100644
--- a/telecomm/java/android/telecomm/RemoteConnection.java
+++ b/telecomm/java/android/telecomm/RemoteConnection.java
@@ -184,8 +184,13 @@
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<Listener> mListeners = Collections.newSetFromMap(
- new ConcurrentHashMap<Listener, Boolean>(2));
+ new ConcurrentHashMap<Listener, Boolean>(8, 0.9f, 1));
private final Set<RemoteConnection> mConferenceableConnections = new HashSet<>();
private int mState = Connection.STATE_NEW;
@@ -248,7 +253,9 @@
* @param listener A {@code Listener}.
*/
public void removeListener(Listener listener) {
- mListeners.remove(listener);
+ if (listener != null) {
+ mListeners.remove(listener);
+ }
}
/**
@@ -588,11 +595,10 @@
setDisconnected(DisconnectCause.ERROR_UNSPECIFIED, "Connection destroyed.");
}
- Set<Listener> listeners = new HashSet<Listener>(mListeners);
- mListeners.clear();
- for (Listener l : listeners) {
+ for (Listener l : mListeners) {
l.onDestroyed(this);
}
+ mListeners.clear();
mConnected = false;
}