From 07848e302b03d65ed7c340a28980eb3a5a85bdcb Mon Sep 17 00:00:00 2001
From: Felipe Leme <felipeal@google.com>
Date: Thu, 10 Jun 2021 14:10:39 -0700
Subject: [PATCH] Improved implementation of
 VoiceInteractionManagerService.setDisabled()

This method is called by CarService when the system goes into suspend
state and the initial implementation completely disabled the service
(and unbound system server from it), which had 2 drawbacks:
- It adds extra work when the system resumes as it has to rebind.
- It can cause memory leaks if the service implementation doesn't
  handle it well.

This CL changes how the API is implemented: rather than disabling the
service, it just ignores new sessions.

To test it:

1. Disable

$ adb shell cmd voiceinteraction disable true

2. Start a session

$ adb shell cmd voiceinteraction show

3. Check logcat

I VoiceInteractionManager: showSessionForActiveService(): ignored while temporarily disabled

Bug: 190051449
Test: manual verification (see above)

Change-Id: Idbc4881b92d9254fb07ea5af1370ea600fc10b80
---
 .../VoiceInteractionManagerService.java       | 57 +++++++++++--------
 ...InteractionManagerServiceShellCommand.java |  1 +
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java b/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java
index a14912e5593db..bc812c2ae4a7a 100644
--- a/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java
+++ b/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerService.java
@@ -562,11 +562,10 @@ public class VoiceInteractionManagerService extends SystemService {
         }
 
         void switchImplementationIfNeededLocked(boolean force) {
-            if (!mCurUserSupported || mTemporarilyDisabled) {
+            if (!mCurUserSupported) {
                 if (DEBUG_USER) {
                     Slog.d(TAG, "switchImplementationIfNeeded(): skipping: force= " + force
-                            + "mCurUserSupported=" + mCurUserSupported
-                            + "mTemporarilyDisabled=" + mTemporarilyDisabled);
+                            + "mCurUserSupported=" + mCurUserSupported);
                 }
                 if (mImpl != null) {
                     mImpl.shutdownLocked();
@@ -1048,13 +1047,16 @@ public class VoiceInteractionManagerService extends SystemService {
                     if (DEBUG) Slog.d(TAG, "setDisabled(): already " + disabled);
                     return;
                 }
-                Slog.i(TAG, "setDisabled(): changing to " + disabled);
-                final long caller = Binder.clearCallingIdentity();
-                try {
-                    mTemporarilyDisabled = disabled;
-                    switchImplementationIfNeeded(/* force= */ false);
-                } finally {
-                    Binder.restoreCallingIdentity(caller);
+                mTemporarilyDisabled = disabled;
+                if (mTemporarilyDisabled) {
+                    Slog.i(TAG, "setDisabled(): temporarily disabling and hiding current session");
+                    try {
+                        hideCurrentSession();
+                    } catch (RemoteException e) {
+                        Log.w(TAG, "Failed to call hideCurrentSession", e);
+                    }
+                } else {
+                    Slog.i(TAG, "setDisabled(): re-enabling");
                 }
             }
         }
@@ -1508,12 +1510,20 @@ public class VoiceInteractionManagerService extends SystemService {
         public boolean showSessionForActiveService(Bundle args, int sourceFlags,
                 IVoiceInteractionSessionShowCallback showCallback, IBinder activityToken) {
             enforceCallingPermission(Manifest.permission.ACCESS_VOICE_INTERACTION_SERVICE);
+            if (DEBUG_USER) Slog.d(TAG, "showSessionForActiveService()");
+
             synchronized (this) {
                 if (mImpl == null) {
                     Slog.w(TAG, "showSessionForActiveService without running voice interaction"
                             + "service");
                     return false;
                 }
+                if (mTemporarilyDisabled) {
+                    Slog.i(TAG, "showSessionForActiveService(): ignored while temporarily "
+                            + "disabled");
+                    return false;
+                }
+
                 final long caller = Binder.clearCallingIdentity();
                 try {
                     return mImpl.showSessionLocked(args,
@@ -1530,22 +1540,21 @@ public class VoiceInteractionManagerService extends SystemService {
         @Override
         public void hideCurrentSession() throws RemoteException {
             enforceCallingPermission(Manifest.permission.ACCESS_VOICE_INTERACTION_SERVICE);
-            synchronized (this) {
-                if (mImpl == null) {
-                    return;
-                }
-                final long caller = Binder.clearCallingIdentity();
-                try {
-                    if (mImpl.mActiveSession != null && mImpl.mActiveSession.mSession != null) {
-                        try {
-                            mImpl.mActiveSession.mSession.closeSystemDialogs();
-                        } catch (RemoteException e) {
-                            Log.w(TAG, "Failed to call closeSystemDialogs", e);
-                        }
+
+            if (mImpl == null) {
+                return;
+            }
+            final long caller = Binder.clearCallingIdentity();
+            try {
+                if (mImpl.mActiveSession != null && mImpl.mActiveSession.mSession != null) {
+                    try {
+                        mImpl.mActiveSession.mSession.closeSystemDialogs();
+                    } catch (RemoteException e) {
+                        Log.w(TAG, "Failed to call closeSystemDialogs", e);
                     }
-                } finally {
-                    Binder.restoreCallingIdentity(caller);
                 }
+            } finally {
+                Binder.restoreCallingIdentity(caller);
             }
         }
 
diff --git a/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerServiceShellCommand.java b/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerServiceShellCommand.java
index 6c355a3b4b303..2e3ca0157a3b6 100644
--- a/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerServiceShellCommand.java
+++ b/services/voiceinteraction/java/com/android/server/voiceinteraction/VoiceInteractionManagerServiceShellCommand.java
@@ -71,6 +71,7 @@ final class VoiceInteractionManagerServiceShellCommand extends ShellCommand {
             pw.println("");
             pw.println("  hide");
             pw.println("    Hides the current session");
+            pw.println("");
             pw.println("  disable [true|false]");
             pw.println("    Temporarily disable (when true) service");
             pw.println("");