Fix excessive locking synchronization leading to deadlocks.
We should never have to look on the entire instance. There
are many places in the code where it there and we need to fix it.
This is a start. In this particular case what we are protecting
is the properties map of the remote device and thus we just need to lock
on it. Ofcourse, it would be better to have a state machine but this
is good for now.
Deadlock: When a new device was found it will call addProperties which will hold
a lock. addProperties calls into BluetoothService. Now Settings app makes a call
into BluetoothService which will call into this file and we have a deadlock.
Change-Id: Ieb69d5ace222bf5d1e6677af151241153303099f
diff --git a/core/java/android/server/BluetoothDeviceProperties.java b/core/java/android/server/BluetoothDeviceProperties.java
index 3dc53d7..fe3ef79 100644
--- a/core/java/android/server/BluetoothDeviceProperties.java
+++ b/core/java/android/server/BluetoothDeviceProperties.java
@@ -35,43 +35,45 @@
mService = service;
}
- synchronized Map<String, String> addProperties(String address,
- String[] properties) {
+ Map<String, String> addProperties(String address, String[] properties) {
/*
* We get a DeviceFound signal every time RSSI changes or name changes.
* Don't create a new Map object every time.
*/
- Map<String, String> propertyValues = mPropertiesMap.get(address);
- if (propertyValues == null) {
- propertyValues = new HashMap<String, String>();
- }
+ Map<String, String> propertyValues;
+ synchronized(mPropertiesMap) {
+ propertyValues = mPropertiesMap.get(address);
+ if (propertyValues == null) {
+ propertyValues = new HashMap<String, String>();
+ }
- for (int i = 0; i < properties.length; i++) {
- String name = properties[i];
- String newValue = null;
- int len;
- if (name == null) {
- Log.e(TAG, "Error: Remote Device Property at index "
+ for (int i = 0; i < properties.length; i++) {
+ String name = properties[i];
+ String newValue = null;
+ int len;
+ if (name == null) {
+ Log.e(TAG, "Error: Remote Device Property at index "
+ i + " is null");
- continue;
- }
- if (name.equals("UUIDs") || name.equals("Nodes")) {
- StringBuilder str = new StringBuilder();
- len = Integer.valueOf(properties[++i]);
- for (int j = 0; j < len; j++) {
- str.append(properties[++i]);
- str.append(",");
+ continue;
}
- if (len > 0) {
- newValue = str.toString();
+ if (name.equals("UUIDs") || name.equals("Nodes")) {
+ StringBuilder str = new StringBuilder();
+ len = Integer.valueOf(properties[++i]);
+ for (int j = 0; j < len; j++) {
+ str.append(properties[++i]);
+ str.append(",");
+ }
+ if (len > 0) {
+ newValue = str.toString();
+ }
+ } else {
+ newValue = properties[++i];
}
- } else {
- newValue = properties[++i];
- }
- propertyValues.put(name, newValue);
+ propertyValues.put(name, newValue);
+ }
+ mPropertiesMap.put(address, propertyValues);
}
- mPropertiesMap.put(address, propertyValues);
// We have added a new remote device or updated its properties.
// Also update the serviceChannel cache.
@@ -79,46 +81,56 @@
return propertyValues;
}
- synchronized void setProperty(String address, String name, String value) {
- Map <String, String> propVal = mPropertiesMap.get(address);
- if (propVal != null) {
- propVal.put(name, value);
- mPropertiesMap.put(address, propVal);
- } else {
- Log.e(TAG, "setRemoteDeviceProperty for a device not in cache:" + address);
+ void setProperty(String address, String name, String value) {
+ synchronized(mPropertiesMap) {
+ Map <String, String> propVal = mPropertiesMap.get(address);
+ if (propVal != null) {
+ propVal.put(name, value);
+ mPropertiesMap.put(address, propVal);
+ } else {
+ Log.e(TAG, "setRemoteDeviceProperty for a device not in cache:" + address);
+ }
}
}
- synchronized boolean isInCache(String address) {
- return (mPropertiesMap.get(address) != null);
+ boolean isInCache(String address) {
+ synchronized (mPropertiesMap) {
+ return (mPropertiesMap.get(address) != null);
+ }
}
- synchronized boolean isEmpty() {
- return mPropertiesMap.isEmpty();
+ boolean isEmpty() {
+ synchronized (mPropertiesMap) {
+ return mPropertiesMap.isEmpty();
+ }
}
- synchronized Set<String> keySet() {
- return mPropertiesMap.keySet();
+ Set<String> keySet() {
+ synchronized (mPropertiesMap) {
+ return mPropertiesMap.keySet();
+ }
}
- synchronized String getProperty(String address, String property) {
- Map<String, String> properties = mPropertiesMap.get(address);
- if (properties != null) {
- return properties.get(property);
- } else {
- // Query for remote device properties, again.
- // We will need to reload the cache when we switch Bluetooth on / off
- // or if we crash.
- properties = updateCache(address);
+ String getProperty(String address, String property) {
+ synchronized(mPropertiesMap) {
+ Map<String, String> properties = mPropertiesMap.get(address);
if (properties != null) {
return properties.get(property);
+ } else {
+ // Query for remote device properties, again.
+ // We will need to reload the cache when we switch Bluetooth on / off
+ // or if we crash.
+ properties = updateCache(address);
+ if (properties != null) {
+ return properties.get(property);
+ }
}
}
Log.e(TAG, "getRemoteDeviceProperty: " + property + " not present: " + address);
return null;
}
- synchronized Map<String, String> updateCache(String address) {
+ Map<String, String> updateCache(String address) {
String[] propValues = mService.getRemoteDeviceProperties(address);
if (propValues != null) {
return addProperties(address, propValues);