Prevent ConcurrentModificationExceptions
Use sets backed by ConcurrentHashMaps instead of HashSets to prevent
concurrent exceptions if listeners try to remove themselves in callbacks
while iterating over the listeners.
Bug:16325026
Change-Id: I5089eac83154f8d71b759a891c0fbf3d3a9a4e6a
diff --git a/src/com/android/telecomm/Call.java b/src/com/android/telecomm/Call.java
index 6d3da71..48fb21c 100644
--- a/src/com/android/telecomm/Call.java
+++ b/src/com/android/telecomm/Call.java
@@ -53,7 +53,7 @@
import java.util.Locale;
import java.util.Objects;
import java.util.Set;
-import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.concurrent.ConcurrentHashMap;
/**
* Encapsulates all aspects of a given phone call throughout its lifecycle, starting
@@ -248,8 +248,14 @@
/** Info used by the connection services. */
private Bundle mExtras = Bundle.EMPTY;
- /** Set of listeners on this call. */
- private Set<Listener> mListeners = new CopyOnWriteArraySet<>();
+ /** Set of listeners on this call.
+ *
+ * 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 CreateConnectionProcessor mCreateConnectionProcessor;
@@ -322,7 +328,9 @@
}
void removeListener(Listener listener) {
- mListeners.remove(listener);
+ if (listener != null) {
+ mListeners.remove(listener);
+ }
}
/** {@inheritDoc} */
@@ -659,14 +667,12 @@
setDisconnectCause(code, null);
setState(CallState.DISCONNECTED);
- Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]);
- for (int i = 0; i < listeners.length; i++) {
- listeners[i].onFailedIncomingCall(this);
+ for (Listener listener : mListeners) {
+ listener.onFailedIncomingCall(this);
}
} else {
- Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]);
- for (int i = 0; i < listeners.length; i++) {
- listeners[i].onFailedOutgoingCall(this, code, msg);
+ for (Listener listener : mListeners) {
+ listener.onFailedOutgoingCall(this, code, msg);
}
clearConnectionService();
}
@@ -679,14 +685,12 @@
clearConnectionService();
setDisconnectCause(DisconnectCause.OUTGOING_CANCELED, null);
- Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]);
- for (int i = 0; i < listeners.length; i++) {
- listeners[i].onFailedIncomingCall(this);
+ for (Listener listener : mListeners) {
+ listener.onFailedIncomingCall(this);
}
} else {
- Listener[] listeners = mListeners.toArray(new Listener[mListeners.size()]);
- for (int i = 0; i < listeners.length; i++) {
- listeners[i].onCancelledOutgoingCall(this);
+ for (Listener listener : mListeners) {
+ listener.onCancelledOutgoingCall(this);
}
clearConnectionService();
}
diff --git a/src/com/android/telecomm/CallsManager.java b/src/com/android/telecomm/CallsManager.java
index 9cade3d..f09b751 100644
--- a/src/com/android/telecomm/CallsManager.java
+++ b/src/com/android/telecomm/CallsManager.java
@@ -27,10 +27,10 @@
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
-import java.util.HashSet;
+import java.util.Collections;
import java.util.List;
import java.util.Set;
-import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.concurrent.ConcurrentHashMap;
/**
* Singleton.
@@ -65,8 +65,13 @@
/**
* The main call repository. Keeps an instance of all live calls. New incoming and outgoing
* calls are added to the map and removed when the calls move to the disconnected state.
+ *
+ * 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<Call> mCalls = new CopyOnWriteArraySet<Call>();
+ private final Set<Call> mCalls = Collections.newSetFromMap(
+ new ConcurrentHashMap<Call, Boolean>(8, 0.9f, 1));
private final ConnectionServiceRepository mConnectionServiceRepository =
new ConnectionServiceRepository();
@@ -74,7 +79,10 @@
private final InCallController mInCallController = new InCallController();
private final CallAudioManager mCallAudioManager;
private final Ringer mRinger;
- private final Set<CallsManagerListener> mListeners = new HashSet<>();
+ // For this set initial table size to 16 because we add 13 listeners in
+ // the CallsManager constructor.
+ private final Set<CallsManagerListener> mListeners = Collections.newSetFromMap(
+ new ConcurrentHashMap<CallsManagerListener, Boolean>(16, 0.9f, 1));
private final HeadsetMediaButton mHeadsetMediaButton;
private final WiredHeadsetManager mWiredHeadsetManager;
private final TtyManager mTtyManager;
@@ -649,7 +657,7 @@
*/
void handleConnectionServiceDeath(ConnectionServiceWrapper service) {
if (service != null) {
- for (Call call : ImmutableList.copyOf(mCalls)) {
+ for (Call call : mCalls) {
if (call.getConnectionService() == service) {
markCallAsDisconnected(call, DisconnectCause.ERROR_UNSPECIFIED, null);
}
diff --git a/src/com/android/telecomm/ConnectionServiceWrapper.java b/src/com/android/telecomm/ConnectionServiceWrapper.java
index 2d5ade2..d8f2c91 100644
--- a/src/com/android/telecomm/ConnectionServiceWrapper.java
+++ b/src/com/android/telecomm/ConnectionServiceWrapper.java
@@ -48,6 +48,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
/**
* Wrapper for {@link IConnectionService}s, handles binding to {@link IConnectionService} and keeps
@@ -602,7 +603,13 @@
private final Adapter mAdapter = new Adapter();
private final CallsManager mCallsManager = CallsManager.getInstance();
- private final Set<Call> mPendingConferenceCalls = new HashSet<>();
+ /**
+ * 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<Call> mPendingConferenceCalls = Collections.newSetFromMap(
+ new ConcurrentHashMap<Call, Boolean>(8, 0.9f, 1));
private final CallIdMapper mCallIdMapper = new CallIdMapper("ConnectionService");
private final Map<String, CreateConnectionResponse> mPendingResponses = new HashMap<>();
@@ -943,7 +950,8 @@
}
// Make a list of ConnectionServices that are listed as being associated with SIM accounts
- final Set<ConnectionServiceWrapper> simServices = new HashSet<>();
+ final Set<ConnectionServiceWrapper> simServices = Collections.newSetFromMap(
+ new ConcurrentHashMap<ConnectionServiceWrapper, Boolean>(8, 0.9f, 1));
for (PhoneAccountHandle handle : registrar.getOutgoingPhoneAccounts()) {
PhoneAccount account = registrar.getPhoneAccount(handle);
if ((account.getCapabilities() & PhoneAccount.CAPABILITY_SIM_SUBSCRIPTION) != 0) {
diff --git a/src/com/android/telecomm/PhoneAccountRegistrar.java b/src/com/android/telecomm/PhoneAccountRegistrar.java
index 98dcdf3..71eb95b 100644
--- a/src/com/android/telecomm/PhoneAccountRegistrar.java
+++ b/src/com/android/telecomm/PhoneAccountRegistrar.java
@@ -288,7 +288,9 @@
}
public void removeListener(Listener l) {
- mListeners.remove(l);
+ if (l != null) {
+ mListeners.remove(l);
+ }
}
private void fireAccountsChanged() {
diff --git a/src/com/android/telecomm/ServiceBinder.java b/src/com/android/telecomm/ServiceBinder.java
index e918593..48442de 100644
--- a/src/com/android/telecomm/ServiceBinder.java
+++ b/src/com/android/telecomm/ServiceBinder.java
@@ -26,10 +26,11 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
+import java.util.Collections;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
/**
* Abstract class to perform the work of binding and unbinding to the specified service interface.
@@ -153,8 +154,12 @@
/**
* Set of currently registered listeners.
+ * 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 Set<Listener> mListeners = Sets.newHashSet();
+ private final Set<Listener> mListeners = Collections.newSetFromMap(
+ new ConcurrentHashMap<Listener, Boolean>(8, 0.9f, 1));
/**
* Persists the specified parameters and initializes the new instance.
@@ -231,7 +236,9 @@
}
final void removeListener(Listener listener) {
- mListeners.remove(listener);
+ if (listener != null) {
+ mListeners.remove(listener);
+ }
}
/**
@@ -291,9 +298,7 @@
setServiceInterface(binder);
if (binder == null) {
- // Use a copy of the listener list to allow the listeners to unregister themselves
- // as part of the unbind without causing issues.
- for (Listener l : ImmutableSet.copyOf(mListeners)) {
+ for (Listener l : mListeners) {
l.onUnbind(this);
}
}
diff --git a/src/com/android/telecomm/WiredHeadsetManager.java b/src/com/android/telecomm/WiredHeadsetManager.java
index e59f1a5..aa2bbd1 100644
--- a/src/com/android/telecomm/WiredHeadsetManager.java
+++ b/src/com/android/telecomm/WiredHeadsetManager.java
@@ -22,7 +22,9 @@
import android.content.IntentFilter;
import android.media.AudioManager;
-import java.util.HashSet;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
/** Listens for and caches headset state. */
class WiredHeadsetManager {
@@ -45,7 +47,13 @@
private final WiredHeadsetBroadcastReceiver mReceiver;
private boolean mIsPluggedIn;
- private final HashSet<Listener> mListeners = new HashSet<>();
+ /**
+ * 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));
WiredHeadsetManager(Context context) {
mReceiver = new WiredHeadsetBroadcastReceiver();
@@ -63,7 +71,9 @@
}
void removeListener(Listener listener) {
- mListeners.remove(listener);
+ if (listener != null) {
+ mListeners.remove(listener);
+ }
}
boolean isPluggedIn() {