MAP: Fix ANR for blocking operations on UI thread
UseCase:
Stability Testing steps followed:
1. Pair and connect with remote phone.
2. Verify successful profile connection.
3. Pair & Connect LE device.
4. Disconnect & Unpair LE device.
5. Reset BT multiple times.
6. Repeat above steps 50 times
Failure:
ANR(s) reported for long running operations perfromed on main thread.
Fix:
- Attach MAPService session handler to looper from a
worker thread to avoid long running tasks handling
for messages posted from UI thread.
- Move AppObserver Account onChange() handling
to worker thread instead of UI thread.
- Avoid running contentobserver for SMS/MMS instance on
main thread looper to fix ANR reported while handling
content changes for SMS and MMS listing. Add NPE checks
hit during stability testing.
Change-Id: Ie146cd284fd191f7c7be7f0c1e7ed4877b36ae76
diff --git a/android/app/src/com/android/bluetooth/map/BluetoothMapAppObserver.java b/android/app/src/com/android/bluetooth/map/BluetoothMapAppObserver.java
index 8c69f15..6363415 100644
--- a/android/app/src/com/android/bluetooth/map/BluetoothMapAppObserver.java
+++ b/android/app/src/com/android/bluetooth/map/BluetoothMapAppObserver.java
@@ -160,7 +160,7 @@
public void registerObserver(BluetoothMapAccountItem app) {
Uri uri = BluetoothMapContract.buildAccountUri(app.getProviderAuthority());
if (V) Log.d(TAG, "registerObserver for URI "+uri.toString()+"\n");
- ContentObserver observer = new ContentObserver(new Handler()) {
+ ContentObserver observer = new ContentObserver(null) {
@Override
public void onChange(boolean selfChange) {
onChange(selfChange, null);
@@ -179,7 +179,8 @@
}
};
mObserverMap.put(uri.toString(), observer);
- mResolver.registerContentObserver(uri, true, observer);
+ //False "notifyForDescendents" : Get notified whenever a change occurs to the exact URI.
+ mResolver.registerContentObserver(uri, false, observer);
}
public void unregisterObserver(BluetoothMapAccountItem app) {
diff --git a/android/app/src/com/android/bluetooth/map/BluetoothMapContentObserver.java b/android/app/src/com/android/bluetooth/map/BluetoothMapContentObserver.java
index 335b728..2a125e7 100644
--- a/android/app/src/com/android/bluetooth/map/BluetoothMapContentObserver.java
+++ b/android/app/src/com/android/bluetooth/map/BluetoothMapContentObserver.java
@@ -469,8 +469,7 @@
return smsType;
}
- private final ContentObserver mObserver = new ContentObserver(
- new Handler(Looper.getMainLooper())) {
+ private final ContentObserver mObserver = new ContentObserver(new Handler()) {
@Override
public void onChange(boolean selfChange) {
onChange(selfChange, null);
@@ -3401,7 +3400,9 @@
};
public void init() {
- mSmsBroadcastReceiver.register();
+ if (mSmsBroadcastReceiver != null) {
+ mSmsBroadcastReceiver.register();
+ }
registerPhoneServiceStateListener();
mInitialized = true;
}
@@ -3409,7 +3410,9 @@
public void deinit() {
mInitialized = false;
unregisterObserver();
- mSmsBroadcastReceiver.unregister();
+ if (mSmsBroadcastReceiver != null) {
+ mSmsBroadcastReceiver.unregister();
+ }
unRegisterPhoneServiceStateListener();
failPendingMessages();
removeDeletedMessages();
diff --git a/android/app/src/com/android/bluetooth/map/BluetoothMapService.java b/android/app/src/com/android/bluetooth/map/BluetoothMapService.java
index e04f959..fa04202 100644
--- a/android/app/src/com/android/bluetooth/map/BluetoothMapService.java
+++ b/android/app/src/com/android/bluetooth/map/BluetoothMapService.java
@@ -30,6 +30,8 @@
import android.content.IntentFilter;
import android.content.IntentFilter.MalformedMimeTypeException;
import android.os.Handler;
+import android.os.HandlerThread;
+import android.os.Looper;
import android.os.Message;
import android.os.ParcelUuid;
import android.os.PowerManager;
@@ -146,6 +148,7 @@
private boolean mAccountChanged = false;
private boolean mSdpSearchInitiated = false;
SdpMnsRecord mMnsRecord = null;
+ private MapServiceMessageHandler mSessionStatusHandler;
private boolean mStartError = true;
// package and class name to which we send intent to check phone book access permission
@@ -164,21 +167,18 @@
}
- private final void closeService() {
+ private synchronized void closeService() {
if (DEBUG) Log.d(TAG, "MAP Service closeService in");
if (mBluetoothMnsObexClient != null) {
mBluetoothMnsObexClient.shutdown();
mBluetoothMnsObexClient = null;
}
-
- for(int i=0, c=mMasInstances.size(); i < c; i++) {
- mMasInstances.valueAt(i).shutdown();
- }
- mMasInstances.clear();
-
- if (mSessionStatusHandler != null) {
- mSessionStatusHandler.removeCallbacksAndMessages(null);
+ if (mMasInstances.size() > 0) {
+ for (int i=0, c=mMasInstances.size(); i < c; i++) {
+ mMasInstances.valueAt(i).shutdown();
+ }
+ mMasInstances.clear();
}
mIsWaitingAuthorization = false;
@@ -190,6 +190,19 @@
if (VERBOSE) Log.v(TAG, "CloseService(): Release Wake Lock");
mWakeLock = null;
}
+ /* Only one SHUTDOWN message expected to closeService.
+ * Hence, quit looper and Handler on first SHUTDOWN message*/
+ if (mSessionStatusHandler != null) {
+ //Perform cleanup in Handler running on worker Thread
+ mSessionStatusHandler.removeCallbacksAndMessages(null);
+ Looper looper = mSessionStatusHandler.getLooper();
+ if (looper != null) {
+ looper.quit();
+ if(VERBOSE) Log.i(TAG, "Quit looper");
+ }
+ mSessionStatusHandler = null;
+ if(VERBOSE) Log.i(TAG, "Remove Handler");
+ }
mRemoteDevice = null;
if (VERBOSE) Log.v(TAG, "MAP Service closeService out");
@@ -294,7 +307,7 @@
BluetoothMapMasInstance masInst = mMasInstances.get(masId); // returns null for -1
if(masInst != null) {
masInst.restartObexServerSession();
- } else {
+ } else if(masId == -1) {
for(int i=0, c=mMasInstances.size(); i < c; i++) {
mMasInstances.valueAt(i).restartObexServerSession();
}
@@ -318,7 +331,10 @@
}
}
- private final Handler mSessionStatusHandler = new Handler() {
+ private final class MapServiceMessageHandler extends Handler {
+ private MapServiceMessageHandler(Looper looper) {
+ super(looper);
+ }
@Override
public void handleMessage(Message msg) {
if (VERBOSE) Log.v(TAG, "Handler(): got msg=" + msg.what);
@@ -496,7 +512,7 @@
public boolean disconnectMap(BluetoothDevice device) {
boolean result = false;
if (DEBUG) Log.d(TAG, "disconnectMap");
- if (getRemoteDevice().equals(device)) {
+ if (getRemoteDevice()!= null && getRemoteDevice().equals(device)) {
switch (mState) {
case BluetoothMap.STATE_CONNECTED:
/* Disconnect all connections and restart all MAS instances */
@@ -578,6 +594,11 @@
Log.w(TAG, "start received for already started, ignoring");
return false;
}
+ HandlerThread thread = new HandlerThread("BluetoothMapHandler");
+ thread.start();
+ Looper looper = thread.getLooper();
+ mSessionStatusHandler = new MapServiceMessageHandler(looper);
+
IntentFilter filter = new IntentFilter();
filter.addAction(BluetoothDevice.ACTION_CONNECTION_ACCESS_REPLY);
filter.addAction(BluetoothAdapter.ACTION_STATE_CHANGED);
@@ -791,6 +812,9 @@
} else {
if (VERBOSE) Log.d(TAG, "Service Stoping()");
}
+ if (mSessionStatusHandler != null) {
+ sendShutdownMessage();
+ }
mStartError = true;
setState(BluetoothMap.STATE_DISCONNECTED, BluetoothMap.RESULT_CANCELED);
sendShutdownMessage();
@@ -800,8 +824,9 @@
public boolean cleanup() {
if (DEBUG) Log.d(TAG, "cleanup()");
setState(BluetoothMap.STATE_DISCONNECTED, BluetoothMap.RESULT_CANCELED);
- // TODO: Change to use message? - do we need to wait for completion?
- closeService();
+ //Cleanup already handled in Stop().
+ //Move this extra check to Handler.
+ sendShutdownMessage();
return true;
}
@@ -896,13 +921,16 @@
* @param masId the MasID to start. Use -1 to start all listeners.
*/
public void sendStartListenerMessage(int masId) {
- if(mSessionStatusHandler != null) {
+ if (mSessionStatusHandler != null && ! mSessionStatusHandler.hasMessages(START_LISTENER)) {
Message msg = mSessionStatusHandler.obtainMessage(START_LISTENER, masId, 0);
/* We add a small delay here to ensure the call returns true before this message is
* handled. It seems wrong to add a delay, but the alternative is to build a lock
* system to handle synchronization, which isn't nice either... */
mSessionStatusHandler.sendMessageDelayed(msg, 20);
- } // Can only be null during shutdown
+ } else if (mSessionStatusHandler != null) {
+ if(DEBUG)
+ Log.w(TAG, "mSessionStatusHandler START_LISTENER message already in Queue");
+ }
}
private void sendConnectMessage(int masId) {
@@ -938,9 +966,23 @@
mIsWaitingAuthorization = false;
cancelUserTimeoutAlarm();
}
- mSessionStatusHandler.removeCallbacksAndMessages(null);
- // Request release of all resources
- mSessionStatusHandler.obtainMessage(SHUTDOWN).sendToTarget();
+ if (mSessionStatusHandler != null && !mSessionStatusHandler.hasMessages(SHUTDOWN)) {
+ mSessionStatusHandler.removeCallbacksAndMessages(null);
+ // Request release of all resources
+ Message msg = mSessionStatusHandler.obtainMessage(SHUTDOWN);
+ if( mSessionStatusHandler.sendMessage(msg) == false) {
+ /* most likely caused by shutdown being called from multiple sources - e.g.BT off
+ * signaled through intent and a service shutdown simultaneously.
+ * Intended behavior not documented, hence we need to be able to handle all cases*/
+ } else {
+ if(DEBUG)
+ Log.e(TAG, "mSessionStatusHandler.sendMessage() dispatched shutdown message");
+ }
+ } else if (mSessionStatusHandler != null) {
+ if(DEBUG)
+ Log.w(TAG, "mSessionStatusHandler shutdown message already in Queue");
+ }
+ if (VERBOSE) Log.d(TAG, "sendShutdownMessage() Out");
}
private MapBroadcastReceiver mMapReceiver = new MapBroadcastReceiver();
