diff options
3 files changed, 167 insertions, 60 deletions
diff --git a/services/core/java/com/android/server/inputmethod/ClientController.java b/services/core/java/com/android/server/inputmethod/ClientController.java index 293464054fdc..21b952bb7760 100644 --- a/services/core/java/com/android/server/inputmethod/ClientController.java +++ b/services/core/java/com/android/server/inputmethod/ClientController.java @@ -25,9 +25,13 @@ import android.util.SparseArray; import android.view.inputmethod.InputBinding; import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.inputmethod.IInputMethodClient; import com.android.internal.inputmethod.IRemoteInputConnection; +import java.util.ArrayList; +import java.util.List; + /** * Store and manage {@link InputMethodManagerService} clients. This class was designed to be a * singleton in {@link InputMethodManagerService} since it stores information about all clients, @@ -37,9 +41,7 @@ import com.android.internal.inputmethod.IRemoteInputConnection; * As part of the re-architecture plan (described in go/imms-rearchitecture-plan), the following * fields and methods will be moved out from IMMS and placed here: * <ul> - * <li>mCurClient (ClientState)</li> * <li>mClients (ArrayMap of ClientState indexed by IBinder)</li> - * <li>mLastSwitchUserId</li> * </ul> * <p> * Nested Classes (to move from IMMS): @@ -54,7 +56,6 @@ import com.android.internal.inputmethod.IRemoteInputConnection; * <li>removeClient</li> * <li>verifyClientAndPackageMatch</li> * <li>setImeTraceEnabledForAllClients (make it reactive)</li> - * <li>unbindCurrentClient</li> * </ul> */ // TODO(b/314150112): Update the Javadoc above, by removing the re-architecture steps, once this @@ -65,18 +66,32 @@ final class ClientController { @GuardedBy("ImfLock.class") final ArrayMap<IBinder, ClientState> mClients = new ArrayMap<>(); + @GuardedBy("ImfLock.class") + private final List<ClientControllerCallback> mCallbacks = new ArrayList<>(); + private final PackageManagerInternal mPackageManagerInternal; + interface ClientControllerCallback { + + void onClientRemoved(ClientState client); + } + ClientController(PackageManagerInternal packageManagerInternal) { mPackageManagerInternal = packageManagerInternal; } @GuardedBy("ImfLock.class") - void addClient(IInputMethodClientInvoker clientInvoker, - IRemoteInputConnection inputConnection, - int selfReportedDisplayId, IBinder.DeathRecipient deathRecipient, int callerUid, + ClientState addClient(IInputMethodClientInvoker clientInvoker, + IRemoteInputConnection inputConnection, int selfReportedDisplayId, int callerUid, int callerPid) { - // TODO: Optimize this linear search. + final IBinder.DeathRecipient deathRecipient = () -> { + // Exceptionally holding ImfLock here since this is a internal lambda expression. + synchronized (ImfLock.class) { + removeClientAsBinder(clientInvoker.asBinder()); + } + }; + + // TODO(b/319457906): Optimize this linear search. final int numClients = mClients.size(); for (int i = 0; i < numClients; ++i) { final ClientState state = mClients.valueAt(i); @@ -101,14 +116,40 @@ final class ClientController { // have the client crash. Thus we do not verify the display ID at all here. Instead we // later check the display ID every time the client needs to interact with the specified // display. - mClients.put(clientInvoker.asBinder(), new ClientState(clientInvoker, inputConnection, - callerUid, callerPid, selfReportedDisplayId, deathRecipient)); + final ClientState cs = new ClientState(clientInvoker, inputConnection, + callerUid, callerPid, selfReportedDisplayId, deathRecipient); + mClients.put(clientInvoker.asBinder(), cs); + return cs; + } + + @VisibleForTesting + @GuardedBy("ImfLock.class") + boolean removeClient(IInputMethodClient client) { + return removeClientAsBinder(client.asBinder()); + } + + @GuardedBy("ImfLock.class") + private boolean removeClientAsBinder(IBinder binder) { + final ClientState cs = mClients.remove(binder); + if (cs == null) { + return false; + } + binder.unlinkToDeath(cs.mClientDeathRecipient, 0 /* flags */); + for (int i = 0; i < mCallbacks.size(); i++) { + mCallbacks.get(i).onClientRemoved(cs); + } + return true; + } + + @GuardedBy("ImfLock.class") + void addClientControllerCallback(ClientControllerCallback callback) { + mCallbacks.add(callback); } @GuardedBy("ImfLock.class") boolean verifyClientAndPackageMatch( @NonNull IInputMethodClient client, @NonNull String packageName) { - ClientState cs = mClients.get(client.asBinder()); + final ClientState cs = mClients.get(client.asBinder()); if (cs == null) { throw new IllegalArgumentException("unknown client " + client.asBinder()); } diff --git a/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java b/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java index 6fe1885ce57b..b18c8d981da7 100644 --- a/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java +++ b/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java @@ -477,7 +477,6 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub /** * The client that is currently bound to an input method. */ - // TODO(b/314150112): Move this to ClientController. @Nullable private ClientState mCurClient; @@ -1674,7 +1673,11 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub mVisibilityStateComputer = new ImeVisibilityStateComputer(this); mVisibilityApplier = new DefaultImeVisibilityApplier(this); + mClientController = new ClientController(mPackageManagerInternal); + synchronized (ImfLock.class) { + mClientController.addClientControllerCallback(c -> onClientRemoved(c)); + } mPreventImeStartupUnlessTextEditor = mRes.getBoolean( com.android.internal.R.bool.config_preventImeStartupUnlessTextEditor); @@ -2171,47 +2174,41 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub // actually running. final int callerUid = Binder.getCallingUid(); final int callerPid = Binder.getCallingPid(); - - // TODO(b/314150112): Move the death recipient logic to ClientController when moving - // removeClient method. - final IBinder.DeathRecipient deathRecipient = () -> removeClient(client); final IInputMethodClientInvoker clientInvoker = IInputMethodClientInvoker.create(client, mHandler); synchronized (ImfLock.class) { mClientController.addClient(clientInvoker, inputConnection, selfReportedDisplayId, - deathRecipient, callerUid, callerPid); + callerUid, callerPid); } } - // TODO(b/314150112): Move this to ClientController. - void removeClient(IInputMethodClient client) { + // TODO(b/314150112): Move this method to InputMethodBindingController + /** + * Hide the IME if the removed user is the current user. + */ + private void onClientRemoved(ClientController.ClientState client) { synchronized (ImfLock.class) { - ClientState cs = mClientController.mClients.remove(client.asBinder()); - if (cs != null) { - client.asBinder().unlinkToDeath(cs.mClientDeathRecipient, 0 /* flags */); - clearClientSessionLocked(cs); - clearClientSessionForAccessibilityLocked(cs); - - if (mCurClient == cs) { - hideCurrentInputLocked(mCurFocusedWindow, null /* statsToken */, 0 /* flags */, - null /* resultReceiver */, SoftInputShowHideReason.HIDE_REMOVE_CLIENT); - if (mBoundToMethod) { - mBoundToMethod = false; - IInputMethodInvoker curMethod = getCurMethodLocked(); - if (curMethod != null) { - // When we unbind input, we are unbinding the client, so we always - // unbind ime and a11y together. - curMethod.unbindInput(); - AccessibilityManagerInternal.get().unbindInput(); - } + clearClientSessionLocked(client); + clearClientSessionForAccessibilityLocked(client); + if (mCurClient == client) { + hideCurrentInputLocked(mCurFocusedWindow, null /* statsToken */, 0 /* flags */, + null /* resultReceiver */, SoftInputShowHideReason.HIDE_REMOVE_CLIENT); + if (mBoundToMethod) { + mBoundToMethod = false; + IInputMethodInvoker curMethod = getCurMethodLocked(); + if (curMethod != null) { + // When we unbind input, we are unbinding the client, so we always + // unbind ime and a11y together. + curMethod.unbindInput(); + AccessibilityManagerInternal.get().unbindInput(); } - mBoundToAccessibility = false; - mCurClient = null; - } - if (mCurFocusedWindowClient == cs) { - mCurFocusedWindowClient = null; - mCurFocusedWindowEditorInfo = null; } + mBoundToAccessibility = false; + mCurClient = null; + } + if (mCurFocusedWindowClient == client) { + mCurFocusedWindowClient = null; + mCurFocusedWindowEditorInfo = null; } } } @@ -2221,8 +2218,7 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub void unbindCurrentClientLocked(@UnbindReason int unbindClientReason) { if (mCurClient != null) { if (DEBUG) { - Slog.v(TAG, "unbindCurrentInputLocked: client=" - + mCurClient.mClient.asBinder()); + Slog.v(TAG, "unbindCurrentInputLocked: client=" + mCurClient.mClient.asBinder()); } if (mBoundToMethod) { mBoundToMethod = false; @@ -2315,7 +2311,8 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub final StartInputInfo info = new StartInputInfo(mSettings.getCurrentUserId(), getCurTokenLocked(), mCurTokenDisplayId, getCurIdLocked(), startInputReason, restarting, - UserHandle.getUserId(mCurClient.mUid), mCurClient.mSelfReportedDisplayId, + UserHandle.getUserId(mCurClient.mUid), + mCurClient.mSelfReportedDisplayId, mCurFocusedWindow, mCurEditorInfo, mCurFocusedWindowSoftInputMode, getSequenceNumberLocked()); mImeTargetWindowMap.put(startInputToken, mCurFocusedWindow); @@ -2326,14 +2323,14 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub // same-user scenarios. // That said ignoring cross-user scenario will never affect IMEs that do not have // INTERACT_ACROSS_USERS(_FULL) permissions, which is actually almost always the case. - if (mSettings.getCurrentUserId() == UserHandle.getUserId(mCurClient.mUid)) { + if (mSettings.getCurrentUserId() == UserHandle.getUserId( + mCurClient.mUid)) { mPackageManagerInternal.grantImplicitAccess(mSettings.getCurrentUserId(), null /* intent */, UserHandle.getAppId(getCurMethodUidLocked()), mCurClient.mUid, true /* direct */); } - @InputMethodNavButtonFlags - final int navButtonFlags = getInputMethodNavButtonFlagsLocked(); + @InputMethodNavButtonFlags final int navButtonFlags = getInputMethodNavButtonFlagsLocked(); final SessionState session = mCurClient.mCurSession; setEnabledSessionLocked(session); session.mMethod.startInput(startInputToken, mCurInputConnection, mCurEditorInfo, restarting, @@ -2753,8 +2750,8 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub && curMethod.asBinder() == method.asBinder()) { if (mCurClient != null) { clearClientSessionLocked(mCurClient); - mCurClient.mCurSession = new SessionState(mCurClient, - method, session, channel); + mCurClient.mCurSession = new SessionState( + mCurClient, method, session, channel); InputBindResult res = attachNewInputLocked( StartInputReason.SESSION_CREATED_BY_IME, true); attachNewAccessibilityLocked(StartInputReason.SESSION_CREATED_BY_IME, true); @@ -5768,8 +5765,10 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub // TODO(b/305829876): Implement user ID verification if (mCurClient != null) { clearClientSessionForAccessibilityLocked(mCurClient, accessibilityConnectionId); - mCurClient.mAccessibilitySessions.put(accessibilityConnectionId, - new AccessibilitySessionState(mCurClient, accessibilityConnectionId, + mCurClient.mAccessibilitySessions.put( + accessibilityConnectionId, + new AccessibilitySessionState(mCurClient, + accessibilityConnectionId, session)); attachNewAccessibilityLocked(StartInputReason.SESSION_CREATED_BY_ACCESSIBILITY, @@ -5803,7 +5802,8 @@ public final class InputMethodManagerService extends IInputMethodManager.Stub } // A11yManagerService unbinds the disabled accessibility service. We don't need // to do it here. - mCurClient.mClient.onUnbindAccessibilityService(getSequenceNumberLocked(), + mCurClient.mClient.onUnbindAccessibilityService( + getSequenceNumberLocked(), accessibilityConnectionId); } // We only have sessions when we bound to an input method. Remove this session diff --git a/services/tests/InputMethodSystemServerTests/src/com/android/server/inputmethod/ClientControllerTest.java b/services/tests/InputMethodSystemServerTests/src/com/android/server/inputmethod/ClientControllerTest.java index 3c8f5c9578d3..30afa72e0f03 100644 --- a/services/tests/InputMethodSystemServerTests/src/com/android/server/inputmethod/ClientControllerTest.java +++ b/services/tests/InputMethodSystemServerTests/src/com/android/server/inputmethod/ClientControllerTest.java @@ -15,9 +15,16 @@ */ package com.android.server.inputmethod; +import static com.android.server.inputmethod.ClientController.ClientControllerCallback; +import static com.android.server.inputmethod.ClientController.ClientState; + import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import android.content.pm.PackageManagerInternal; @@ -38,6 +45,8 @@ import org.junit.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; // This test is designed to run on both device and host (Ravenwood) side. public final class ClientControllerTest { @@ -58,9 +67,6 @@ public final class ClientControllerTest { @Mock private IRemoteInputConnection mConnection; - @Mock - private IBinder.DeathRecipient mDeathRecipient; - private Handler mHandler; private ClientController mController; @@ -68,9 +74,10 @@ public final class ClientControllerTest { @Before public void setUp() { MockitoAnnotations.initMocks(this); + when(mClient.asBinder()).thenReturn((IBinder) mClient); + mHandler = new Handler(Looper.getMainLooper()); mController = new ClientController(mMockPackageManagerInternal); - when(mClient.asBinder()).thenReturn((IBinder) mClient); } @Test @@ -80,18 +87,77 @@ public final class ClientControllerTest { var invoker = IInputMethodClientInvoker.create(mClient, mHandler); synchronized (ImfLock.class) { - mController.addClient(invoker, mConnection, ANY_DISPLAY_ID, mDeathRecipient, - ANY_CALLER_UID, ANY_CALLER_PID); + mController.addClient(invoker, mConnection, ANY_DISPLAY_ID, ANY_CALLER_UID, + ANY_CALLER_PID); SecurityException thrown = assertThrows(SecurityException.class, () -> { synchronized (ImfLock.class) { mController.addClient(invoker, mConnection, ANY_DISPLAY_ID, - mDeathRecipient, ANY_CALLER_UID, ANY_CALLER_PID); + ANY_CALLER_UID, ANY_CALLER_PID); } }); assertThat(thrown.getMessage()).isEqualTo( "uid=1/pid=1/displayId=0 is already registered"); } } + + @Test + // TODO(b/314150112): Enable host side mode for this test once b/315544364 is fixed. + @IgnoreUnderRavenwood(blockedBy = {InputBinding.class, IInputMethodClientInvoker.class}) + public void testAddClient() throws Exception { + synchronized (ImfLock.class) { + var invoker = IInputMethodClientInvoker.create(mClient, mHandler); + var added = mController.addClient(invoker, mConnection, ANY_DISPLAY_ID, ANY_CALLER_UID, + ANY_CALLER_PID); + + verify(invoker.asBinder()).linkToDeath(any(IBinder.DeathRecipient.class), eq(0)); + assertThat(mController.mClients).containsEntry(invoker.asBinder(), added); + } + } + + @Test + // TODO(b/314150112): Enable host side mode for this test once b/315544364 is fixed. + @IgnoreUnderRavenwood(blockedBy = {InputBinding.class, IInputMethodClientInvoker.class}) + public void testRemoveClient() { + var callback = new TestClientControllerCallback(); + ClientState added; + synchronized (ImfLock.class) { + mController.addClientControllerCallback(callback); + + var invoker = IInputMethodClientInvoker.create(mClient, mHandler); + added = mController.addClient(invoker, mConnection, ANY_DISPLAY_ID, ANY_CALLER_UID, + ANY_CALLER_PID); + assertThat(mController.mClients).containsEntry(invoker.asBinder(), added); + assertThat(mController.removeClient(mClient)).isTrue(); + } + + // Test callback + var removed = callback.waitForRemovedClient(5, TimeUnit.SECONDS); + assertThat(removed).isSameInstanceAs(added); + } + + private static class TestClientControllerCallback implements ClientControllerCallback { + + private final CountDownLatch mLatch = new CountDownLatch(1); + + private ClientState mRemoved; + + @Override + public void onClientRemoved(ClientState removed) { + mRemoved = removed; + mLatch.countDown(); + } + + ClientState waitForRemovedClient(long timeout, TimeUnit unit) { + try { + assertWithMessage("ClientController callback wasn't called on user removed").that( + mLatch.await(timeout, unit)).isTrue(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Unexpected thread interruption", e); + } + return mRemoved; + } + } } |