From d3178c4ed52358009d51518551325b5b0c84de67 Mon Sep 17 00:00:00 2001 From: Chris Thornton Date: Sat, 10 Jun 2017 17:31:57 -0700 Subject: Prevent ConcurrentModificationException in updateAllRecognitions When invoking updateAllRecognitions, if a callback binder was determined to have died, an internal function would go and remove it from mModelDataMap. However, updateAllRecognitions was iterating over this map, so it would then explode. By first making a copy of the model datas before iterating over all of them, this problem is avoided. (As part of trying to figure out what was happening, also updated all the method names that implicitly assumed they had a lock, and double checked that everything with a Locked suffix is actually locked) Bug: 62487479 Test: Use the sound trigger test app to load and start two models, force kill the app (so the dangling binders hang around), then enable power save (which triggers the call to updateAllRecognitions) Change-Id: I87b9dfc1b2af5e294050b146737916ccaad882c1 --- .../server/soundtrigger/SoundTriggerHelper.java | 32 ++++++++++++---------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java b/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java index 1aa952cd58b9..520b0e896268 100644 --- a/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java +++ b/services/voiceinteraction/java/com/android/server/soundtrigger/SoundTriggerHelper.java @@ -191,7 +191,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { // Process existing model first. if (model != null && !model.getModelId().equals(soundModel.uuid)) { // The existing model has a different UUID, should be replaced. - int status = cleanUpExistingKeyphraseModel(model); + int status = cleanUpExistingKeyphraseModelLocked(model); if (status != STATUS_OK) { return status; } @@ -210,7 +210,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } } - private int cleanUpExistingKeyphraseModel(ModelData modelData) { + private int cleanUpExistingKeyphraseModelLocked(ModelData modelData) { // Stop and clean up a previous ModelData if one exists. This usually is used when the // previous model has a different UUID for the same keyphrase ID. int status = tryStopAndUnloadLocked(modelData, true /* stop */, true /* unload */); @@ -616,7 +616,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { try { callback.onGenericSoundTriggerDetected((GenericRecognitionEvent) event); } catch (DeadObjectException e) { - forceStopAndUnloadModel(model, e); + forceStopAndUnloadModelLocked(model, e); return; } catch (RemoteException e) { Slog.w(TAG, "RemoteException in onGenericSoundTriggerDetected", e); @@ -706,7 +706,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { try { modelData.getCallback().onRecognitionPaused(); } catch (DeadObjectException e) { - forceStopAndUnloadModel(modelData, e); + forceStopAndUnloadModelLocked(modelData, e); } catch (RemoteException e) { Slog.w(TAG, "RemoteException in onRecognitionPaused", e); } @@ -717,7 +717,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { Slog.w(TAG, "Recognition failure"); MetricsLogger.count(mContext, "sth_recognition_failure_event", 1); try { - sendErrorCallbacksToAll(STATUS_ERROR); + sendErrorCallbacksToAllLocked(STATUS_ERROR); } finally { internalClearModelStateLocked(); internalClearGlobalStateLocked(); @@ -759,7 +759,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { try { modelData.getCallback().onKeyphraseDetected((KeyphraseRecognitionEvent) event); } catch (DeadObjectException e) { - forceStopAndUnloadModel(modelData, e); + forceStopAndUnloadModelLocked(modelData, e); return; } catch (RemoteException e) { Slog.w(TAG, "RemoteException in onKeyphraseDetected", e); @@ -778,7 +778,9 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { private void updateAllRecognitionsLocked(boolean notify) { boolean isAllowed = isRecognitionAllowed(); - for (ModelData modelData : mModelDataMap.values()) { + // updateRecognitionLocked can possibly update the list of models + ArrayList modelDatas = new ArrayList(mModelDataMap.values()); + for (ModelData modelData : modelDatas) { updateRecognitionLocked(modelData, isAllowed, notify); } } @@ -800,7 +802,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { private void onServiceDiedLocked() { try { MetricsLogger.count(mContext, "sth_service_died", 1); - sendErrorCallbacksToAll(SoundTrigger.STATUS_DEAD_OBJECT); + sendErrorCallbacksToAllLocked(SoundTrigger.STATUS_DEAD_OBJECT); } finally { internalClearModelStateLocked(); internalClearGlobalStateLocked(); @@ -885,21 +887,21 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { } // Sends an error callback to all models with a valid registered callback. - private void sendErrorCallbacksToAll(int errorCode) { + private void sendErrorCallbacksToAllLocked(int errorCode) { for (ModelData modelData : mModelDataMap.values()) { IRecognitionStatusCallback callback = modelData.getCallback(); if (callback != null) { try { callback.onError(errorCode); } catch (RemoteException e) { - Slog.w(TAG, "RemoteException sendErrorCallbacksToAll for model handle " + + Slog.w(TAG, "RemoteException sendErrorCallbacksToAllLocked for model handle " + modelData.getHandle(), e); } } } } - private void forceStopAndUnloadModel(ModelData modelData, Exception exception) { + private void forceStopAndUnloadModelLocked(ModelData modelData, Exception exception) { if (exception != null) { Slog.e(TAG, "forceStopAndUnloadModel", exception); } @@ -1020,7 +1022,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { try { callback.onError(status); } catch (DeadObjectException e) { - forceStopAndUnloadModel(modelData, e); + forceStopAndUnloadModelLocked(modelData, e); } catch (RemoteException e) { Slog.w(TAG, "RemoteException in onError", e); } @@ -1034,7 +1036,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { try { callback.onRecognitionResumed(); } catch (DeadObjectException e) { - forceStopAndUnloadModel(modelData, e); + forceStopAndUnloadModelLocked(modelData, e); } catch (RemoteException e) { Slog.w(TAG, "RemoteException in onRecognitionResumed", e); } @@ -1061,7 +1063,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { try { callback.onError(status); } catch (DeadObjectException e) { - forceStopAndUnloadModel(modelData, e); + forceStopAndUnloadModelLocked(modelData, e); } catch (RemoteException e) { Slog.w(TAG, "RemoteException in onError", e); } @@ -1074,7 +1076,7 @@ public class SoundTriggerHelper implements SoundTrigger.StatusListener { try { callback.onRecognitionPaused(); } catch (DeadObjectException e) { - forceStopAndUnloadModel(modelData, e); + forceStopAndUnloadModelLocked(modelData, e); } catch (RemoteException e) { Slog.w(TAG, "RemoteException in onRecognitionPaused", e); } -- cgit v1.2.3-59-g8ed1b