Fix SoundTriggerMiddleware deadlock issues

This change enforces strict lock ordering between the various layers
of the service and the audio policy lock. Previously, we were exposed
to various deadlock scenarios due to nested locking done without
consistent lock ordering, specifically in cases involving callbacks
from SoundTriggerModule to SoundTriggerMiddlewareValidation.



Change-Id: I28382d10c7bc7faaa8f9b33dda3aa9c94a68061e
Bug: 159151235
Test: Manually disable concurrent capture, verify basic soundtrigger
      functionality and do some stress testing by enabling/disabling
      recording quickly and check for deadlocks.

3 files changed, 163 insertions, 86 deletions

diff --git a/services/core/java/com/android/server/soundtrigger_middleware/README.md b/services/core/java/com/android/server/soundtrigger_middleware/README.md
new file mode 100644
index 000000000000..416548d9bc5e
--- /dev/null
+++ b/services/core/java/com/android/server/soundtrigger_middleware/README.md
@@ -0,0 +1,19 @@
+# Sound Trigger Middleware
+TODO: Add component description.
+
+## Notes about thread synchronization
+This component has some tricky thread synchronization considerations due to its layered design and
+due to the fact that it is involved in both in-bound and out-bound calls from / to
+external components. To avoid potential deadlocks, a strict locking order must be ensured whenever
+nesting locks. The order is:
+- `SoundTriggerMiddlewareValidation` lock.
+- Audio policy service lock. This one is external - it should be assumed to be held whenever we're
+  inside the `ExternalCaptureStateTracker.setCaptureState()` call stack *AND* to be acquired from
+  within our calls into `AudioSessionProvider.acquireSession()`.
+- `SoundTriggerModule` lock.
+
+This dictates careful consideration of callbacks going from `SoundTriggerModule` to
+`SoundTriggerMiddlewareValidation` and especially those coming from the `setCaptureState()` path.
+We always invoke those calls outside of the `SoundTriggerModule` lock, so we can lock
+`SoundTriggerMiddlewareValidation`. However, in the `setCaptureState()` case, we have to use atomics
+in `SoundTriggerMiddlewareValidation` and avoid the lock.
diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java
index f4c77a0b88ca..5d25d2cb554d 100644
--- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java
+++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java
@@ -47,6 +47,9 @@ import java.util.HashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
 /**
@@ -328,7 +331,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
         }
 
         /** Activity state. */
-        Activity activityState = Activity.LOADED;
+        private AtomicInteger mActivityState = new AtomicInteger(Activity.LOADED.ordinal());
 
         /** Human-readable description of the model. */
         final String description;
@@ -383,6 +386,14 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
         void updateParameterSupport(int modelParam, @Nullable ModelParameterRange range) {
             parameterSupport.put(modelParam, range);
         }
+
+        Activity getActivityState() {
+            return Activity.values()[mActivityState.get()];
+        }
+
+        void setActivityState(Activity activity) {
+            mActivityState.set(activity.ordinal());
+        }
     }
 
     /**
@@ -393,7 +404,13 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
             IBinder.DeathRecipient {
         private final ISoundTriggerCallback mCallback;
         private ISoundTriggerModule mDelegate;
-        private @NonNull Map<Integer, ModelState> mLoadedModels = new HashMap<>();
+        // While generally all the fields of this class must be changed under a lock, an exception
+        // is made for the specific case of changing a model state from ACTIVE to LOADED, which
+        // may happen as result of a recognition callback. This would happen atomically and is
+        // necessary in order to avoid deadlocks associated with locking from within callbacks
+        // possibly originating from the audio server.
+        private @NonNull
+        ConcurrentMap<Integer, ModelState> mLoadedModels = new ConcurrentHashMap<>();
         private final int mHandle;
         private ModuleStatus mState = ModuleStatus.ALIVE;
 
@@ -476,10 +493,9 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
                 if (modelState == null) {
                     throw new IllegalStateException("Invalid handle: " + modelHandle);
                 }
-                if (modelState.activityState
-                        != ModelState.Activity.LOADED) {
+                if (modelState.getActivityState() != ModelState.Activity.LOADED) {
                     throw new IllegalStateException("Model with handle: " + modelHandle
-                            + " has invalid state for unloading: " + modelState.activityState);
+                            + " has invalid state for unloading: " + modelState.getActivityState());
                 }
 
                 // From here on, every exception isn't client's fault.
@@ -509,19 +525,21 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
                 if (modelState == null) {
                     throw new IllegalStateException("Invalid handle: " + modelHandle);
                 }
-                if (modelState.activityState
-                        != ModelState.Activity.LOADED) {
+                if (modelState.getActivityState() != ModelState.Activity.LOADED) {
                     throw new IllegalStateException("Model with handle: " + modelHandle
                             + " has invalid state for starting recognition: "
-                            + modelState.activityState);
+                            + modelState.getActivityState());
                 }
 
                 // From here on, every exception isn't client's fault.
                 try {
+                    // Normally, we would set the state after the operation succeeds. However, since
+                    // the activity state may be reset outside of the lock, we set it here first,
+                    // and reset it in case of exception.
+                    modelState.setActivityState(ModelState.Activity.ACTIVE);
                     mDelegate.startRecognition(modelHandle, config);
-                    modelState.activityState =
-                            ModelState.Activity.ACTIVE;
                 } catch (Exception e) {
+                    modelState.setActivityState(ModelState.Activity.LOADED);
                     throw handleException(e);
                 }
             }
@@ -548,8 +566,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
                 // From here on, every exception isn't client's fault.
                 try {
                     mDelegate.stopRecognition(modelHandle);
-                    modelState.activityState =
-                            ModelState.Activity.LOADED;
+                    modelState.setActivityState(ModelState.Activity.LOADED);
                 } catch (Exception e) {
                     throw handleException(e);
                 }
@@ -719,7 +736,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
                 for (Map.Entry<Integer, ModelState> entry : mLoadedModels.entrySet()) {
                     pw.print(entry.getKey());
                     pw.print('\t');
-                    pw.print(entry.getValue().activityState.name());
+                    pw.print(entry.getValue().getActivityState().name());
                     pw.print('\t');
                     pw.print(entry.getValue().description);
                     pw.println();
@@ -735,48 +752,61 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
 
         @Override
         public void onRecognition(int modelHandle, @NonNull RecognitionEvent event) {
-            synchronized (SoundTriggerMiddlewareValidation.this) {
-                if (event.status != RecognitionStatus.FORCED) {
-                    mLoadedModels.get(modelHandle).activityState =
-                            ModelState.Activity.LOADED;
-                }
-                try {
-                    mCallback.onRecognition(modelHandle, event);
-                } catch (RemoteException e) {
-                    // Dead client will be handled by binderDied() - no need to handle here.
-                    // In any case, client callbacks are considered best effort.
-                    Log.e(TAG, "Client callback exception.", e);
+            // We cannot obtain a lock on SoundTriggerMiddlewareValidation.this, since this call
+            // might be coming from the audio server (via setCaptureState()) while it is holding
+            // a lock that is also acquired while loading / unloading models. Thus, we require a
+            // strict locking order here, where obtaining our lock must always come first.
+            // To avoid this problem, we use an atomic model activity state. There is a risk of the
+            // model not being in the mLoadedModels map here, since it might have been stopped /
+            // unloaded while the event was in flight.
+            if (event.status != RecognitionStatus.FORCED) {
+                ModelState modelState = mLoadedModels.get(modelHandle);
+                if (modelState != null) {
+                    modelState.setActivityState(ModelState.Activity.LOADED);
                 }
             }
+            try {
+                mCallback.onRecognition(modelHandle, event);
+            } catch (RemoteException e) {
+                // Dead client will be handled by binderDied() - no need to handle here.
+                // In any case, client callbacks are considered best effort.
+                Log.e(TAG, "Client callback exception.", e);
+            }
         }
 
         @Override
         public void onPhraseRecognition(int modelHandle, @NonNull PhraseRecognitionEvent event) {
-            synchronized (SoundTriggerMiddlewareValidation.this) {
-                if (event.common.status != RecognitionStatus.FORCED) {
-                    mLoadedModels.get(modelHandle).activityState =
-                            ModelState.Activity.LOADED;
-                }
-                try {
-                    mCallback.onPhraseRecognition(modelHandle, event);
-                } catch (RemoteException e) {
-                    // Dead client will be handled by binderDied() - no need to handle here.
-                    // In any case, client callbacks are considered best effort.
-                    Log.e(TAG, "Client callback exception.", e);
+            // We cannot obtain a lock on SoundTriggerMiddlewareValidation.this, since this call
+            // might be coming from the audio server (via setCaptureState()) while it is holding
+            // a lock that is also acquired while loading / unloading models. Thus, we require a
+            // strict locking order here, where obtaining our lock must always come first.
+            // To avoid this problem, we use an atomic model activity state. There is a risk of the
+            // model not being in the mLoadedModels map here, since it might have been stopped /
+            // unloaded while the event was in flight.
+            if (event.common.status != RecognitionStatus.FORCED) {
+                ModelState modelState = mLoadedModels.get(modelHandle);
+                if (modelState != null) {
+                    modelState.setActivityState(ModelState.Activity.LOADED);
                 }
             }
+            try {
+                mCallback.onPhraseRecognition(modelHandle, event);
+            } catch (RemoteException e) {
+                // Dead client will be handled by binderDied() - no need to handle here.
+                // In any case, client callbacks are considered best effort.
+                Log.e(TAG, "Client callback exception.", e);
+            }
         }
 
         @Override
         public void onRecognitionAvailabilityChange(boolean available) {
-            synchronized (SoundTriggerMiddlewareValidation.this) {
-                try {
-                    mCallback.onRecognitionAvailabilityChange(available);
-                } catch (RemoteException e) {
-                    // Dead client will be handled by binderDied() - no need to handle here.
-                    // In any case, client callbacks are considered best effort.
-                    Log.e(TAG, "Client callback exception.", e);
-                }
+            // Not locking to avoid deadlocks (not affecting any state).
+            try {
+                mCallback.onRecognitionAvailabilityChange(available);
+            } catch (RemoteException e) {
+                // Dead client will be handled by binderDied() - no need to handle here.
+                // In any case, client callbacks are considered best effort.
+                Log.e(TAG, "Client callback exception.", e);
             }
         }
 
@@ -804,10 +834,9 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
                     // Gracefully stop all active recognitions and unload the models.
                     for (Map.Entry<Integer, ModelState> entry :
                             mLoadedModels.entrySet()) {
-                        if (entry.getValue().activityState
-                                == ModelState.Activity.ACTIVE) {
-                            mDelegate.stopRecognition(entry.getKey());
-                        }
+                        // Idempotent call, no harm in calling even for models that are already
+                        // stopped.
+                        mDelegate.stopRecognition(entry.getKey());
                         mDelegate.unloadModel(entry.getKey());
                     }
                     // Detach.
diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java
index 522e5e189232..f809ed4a4a2b 100644
--- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java
+++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java
@@ -42,6 +42,7 @@ import android.util.Log;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -153,19 +154,29 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient {
      *
      * @param active true iff external capture is active.
      */
-    synchronized void setExternalCaptureState(boolean active) {
-        if (mProperties.concurrentCapture) {
-            // If we support concurrent capture, we don't care about any of this.
-            return;
-        }
-        mRecognitionAvailable = !active;
-        if (!mRecognitionAvailable) {
-            // Our module does not support recognition while a capture is active -
-            // need to abort all active recognitions.
-            for (Session session : mActiveSessions) {
-                session.abortActiveRecognitions();
+    void setExternalCaptureState(boolean active) {
+        // We should never invoke callbacks while holding the lock, since this may deadlock with
+        // forward calls. Thus, we first gather all the callbacks we need to invoke while holding
+        // the lock, but invoke them after releasing it.
+        List<Runnable> callbacks = new LinkedList<>();
+
+        synchronized (this) {
+            if (mProperties.concurrentCapture) {
+                // If we support concurrent capture, we don't care about any of this.
+                return;
+            }
+            mRecognitionAvailable = !active;
+            if (!mRecognitionAvailable) {
+                // Our module does not support recognition while a capture is active -
+                // need to abort all active recognitions.
+                for (Session session : mActiveSessions) {
+                    session.abortActiveRecognitions(callbacks);
+                }
             }
         }
+        for (Runnable callback : callbacks) {
+            callback.run();
+        }
         for (Session session : mActiveSessions) {
             session.notifyRecognitionAvailability();
         }
@@ -329,9 +340,18 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient {
 
         @Override
         public void startRecognition(int modelHandle, @NonNull RecognitionConfig config) {
+            // We should never invoke callbacks while holding the lock, since this may deadlock with
+            // forward calls. Thus, we first gather all the callbacks we need to invoke while holding
+            // the lock, but invoke them after releasing it.
+            List<Runnable> callbacks = new LinkedList<>();
+
             synchronized (SoundTriggerModule.this) {
                 checkValid();
-                mLoadedModels.get(modelHandle).startRecognition(config);
+                mLoadedModels.get(modelHandle).startRecognition(config, callbacks);
+            }
+
+            for (Runnable callback : callbacks) {
+                callback.run();
             }
         }
 
@@ -377,10 +397,12 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient {
 
         /**
          * Abort all currently active recognitions.
+         * @param callbacks Will be appended with a list of callbacks that need to be invoked
+         *                  after this method returns, without holding the module lock.
          */
-        private void abortActiveRecognitions() {
+        private void abortActiveRecognitions(@NonNull List<Runnable> callbacks) {
             for (Model model : mLoadedModels.values()) {
-                model.abortActiveRecognition();
+                model.abortActiveRecognition(callbacks);
             }
         }
 
@@ -475,10 +497,11 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient {
                 return mSession.mSessionHandle;
             }
 
-            private void startRecognition(@NonNull RecognitionConfig config) {
+            private void startRecognition(@NonNull RecognitionConfig config,
+                    @NonNull List<Runnable> callbacks) {
                 if (!mRecognitionAvailable) {
                     // Recognition is unavailable - send an abort event immediately.
-                    notifyAbort();
+                    callbacks.add(this::notifyAbort);
                     return;
                 }
                 android.hardware.soundtrigger.V2_3.RecognitionConfig hidlConfig =
@@ -525,8 +548,12 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient {
                                 ConversionUtil.aidl2hidlModelParameter(modelParam)));
             }
 
-            /** Abort the recognition, if active. */
-            private void abortActiveRecognition() {
+            /**
+             * Abort the recognition, if active.
+             * @param callbacks Will be appended with a list of callbacks that need to be invoked
+             *                  after this method returns, without holding the module lock.
+             */
+            private void abortActiveRecognition(List<Runnable> callbacks) {
                 // If we're inactive, do nothing.
                 if (getState() != ModelState.ACTIVE) {
                     return;
@@ -535,7 +562,7 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient {
                 stopRecognition();
 
                 // Notify the client that recognition has been aborted.
-                notifyAbort();
+                callbacks.add(this::notifyAbort);
             }
 
             /** Notify the client that recognition has been aborted. */
@@ -577,42 +604,44 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient {
             public void recognitionCallback(
                     @NonNull ISoundTriggerHwCallback.RecognitionEvent recognitionEvent,
                     int cookie) {
+                RecognitionEvent aidlEvent =
+                        ConversionUtil.hidl2aidlRecognitionEvent(recognitionEvent);
+                aidlEvent.captureSession = mSession.mSessionHandle;
                 synchronized (SoundTriggerModule.this) {
-                    RecognitionEvent aidlEvent =
-                            ConversionUtil.hidl2aidlRecognitionEvent(recognitionEvent);
-                    aidlEvent.captureSession = mSession.mSessionHandle;
-                    try {
-                        mCallback.onRecognition(mHandle, aidlEvent);
-                    } catch (RemoteException e) {
-                        // Dead client will be handled by binderDied() - no need to handle here.
-                        // In any case, client callbacks are considered best effort.
-                        Log.e(TAG, "Client callback execption.", e);
-                    }
                     if (aidlEvent.status != RecognitionStatus.FORCED) {
                         setState(ModelState.LOADED);
                     }
                 }
+                // The callback must be invoked outside of the lock.
+                try {
+                    mCallback.onRecognition(mHandle, aidlEvent);
+                } catch (RemoteException e) {
+                    // We're not expecting any exceptions here.
+                    throw e.rethrowAsRuntimeException();
+                }
             }
 
             @Override
             public void phraseRecognitionCallback(
                     @NonNull ISoundTriggerHwCallback.PhraseRecognitionEvent phraseRecognitionEvent,
                     int cookie) {
+                PhraseRecognitionEvent aidlEvent =
+                        ConversionUtil.hidl2aidlPhraseRecognitionEvent(phraseRecognitionEvent);
+                aidlEvent.common.captureSession = mSession.mSessionHandle;
+
                 synchronized (SoundTriggerModule.this) {
-                    PhraseRecognitionEvent aidlEvent =
-                            ConversionUtil.hidl2aidlPhraseRecognitionEvent(phraseRecognitionEvent);
-                    aidlEvent.common.captureSession = mSession.mSessionHandle;
-                    try {
-                        mCallback.onPhraseRecognition(mHandle, aidlEvent);
-                    } catch (RemoteException e) {
-                        // Dead client will be handled by binderDied() - no need to handle here.
-                        // In any case, client callbacks are considered best effort.
-                        Log.e(TAG, "Client callback execption.", e);
-                    }
                     if (aidlEvent.common.status != RecognitionStatus.FORCED) {
                         setState(ModelState.LOADED);
                     }
                 }
+
+                // The callback must be invoked outside of the lock.
+                try {
+                    mCallback.onPhraseRecognition(mHandle, aidlEvent);
+                } catch (RemoteException e) {
+                    // We're not expecting any exceptions here.
+                    throw e.rethrowAsRuntimeException();
+                }
             }
         }
     }