From 501107a4451f111f093350c3ceed692b7a87bf24 Mon Sep 17 00:00:00 2001 From: Ming-Shin Lu Date: Wed, 17 Nov 2021 02:08:51 +0800 Subject: [PATCH] Reduce potential lock contention during start input IMMS#startInputOrWindowGainedFocus invokes several WM internal methods to check if the IME client and its windowToken has IME focus: - getDisplayIdForWindow(windowToken) - Checked at beginning of startInputOrWindowGainedFocus to know if the displayID of the client window is same as the client reported displayID. - isInputMethodClientFocus(uid, pid, displayId) - Checked next to getDisplayIdForWindow invoked, it's to know if the client is focused on the display. - isUidAllowedOnDisplay(displayId, uid) - Checked in startInputUncheckedLocked, used to see if the client's uid is allow to access on the display. As calling these methods requres holding WM locks, if the lock contention takes longer time, it may impact the UI performance since startInputOrWindowGainedFocus is a sync IPC call that requires the remote client to execute on the UI thread. To Remove/reduce unnecessary internal module calls that requires holding lock by followed ways: 1) Remove isUidAllowedOnDisplay & getDisplayIdForWindow validation check & combine these checks into isInputMethodClientFocus with adding windowToken parameter, to reduce the timing of being blocked by WM lock. 2) Make startInputUncheckLocked as private method and add documentation - as all call points of startInputUncheckedLocked is inside of startInputOrWindowGainedFocus, even though 1) removed calling isUidAllowedOnDisplay, it still be checked in the combined version of isInputMethodClientFocus without any visible change, and adding the documentation for invoking startInputUncheckedLocked requires the focus check must done before. 2) Refine WindowManagerInternal#shouldRestoreImeVisibility to smaller the synchronized block since WMS#getTaskSnapshot should not be called when holding WM lock. Bug: 197848765 Test: atest CtsInputMethodTestCases Change-Id: Ie850a67c116466c46bd62d44f4fbdd5584eda5de --- .../InputMethodManagerService.java | 76 ++++++++++--------- .../server/wm/WindowManagerInternal.java | 22 +++++- .../server/wm/WindowManagerService.java | 37 ++++++--- 3 files changed, 88 insertions(+), 47 deletions(-) diff --git a/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java b/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java index 5d2a90023d8e8..dd021286887a4 100644 --- a/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java +++ b/services/core/java/com/android/server/inputmethod/InputMethodManagerService.java @@ -2347,11 +2347,20 @@ public class InputMethodManagerService extends IInputMethodManager.Stub curId, getSequenceNumberLocked(), suppressesSpellChecker); } + /** + * Called by {@link #startInputOrWindowGainedFocusInternalLocked} to bind/unbind/attach the + * selected InputMethod to the given focused IME client. + * + * Note that this should be called after validating if the IME client has IME focus. + * + * @see WindowManagerInternal#hasInputMethodClientFocus(IBinder, int, int, int) + */ @GuardedBy("ImfLock.class") @NonNull - InputBindResult startInputUncheckedLocked(@NonNull ClientState cs, IInputContext inputContext, - @NonNull EditorInfo attribute, @StartInputFlags int startInputFlags, - @StartInputReason int startInputReason, int unverifiedTargetSdkVersion) { + private InputBindResult startInputUncheckedLocked(@NonNull ClientState cs, + IInputContext inputContext, @NonNull EditorInfo attribute, + @StartInputFlags int startInputFlags, @StartInputReason int startInputReason, + int unverifiedTargetSdkVersion) { // If no method is currently selected, do nothing. String selectedMethodId = getSelectedMethodIdLocked(); if (selectedMethodId == null) { @@ -2373,10 +2382,6 @@ public class InputMethodManagerService extends IInputMethodManager.Stub return InputBindResult.INVALID_PACKAGE_NAME; } - if (!mWindowManagerInternal.isUidAllowedOnDisplay(cs.selfReportedDisplayId, cs.uid)) { - // Wait, the client no longer has access to the display. - return InputBindResult.INVALID_DISPLAY_ID; - } // Compute the final shown display ID with validated cs.selfReportedDisplayId for this // session & other conditions. mDisplayIdToShowIme = computeImeDisplayIdForTarget(cs.selfReportedDisplayId, @@ -3200,14 +3205,11 @@ public class InputMethodManagerService extends IInputMethodManager.Stub // be made before input is started in it. final ClientState cs = mClients.get(client.asBinder()); if (cs == null) { - throw new IllegalArgumentException( - "unknown client " + client.asBinder()); + throw new IllegalArgumentException("unknown client " + client.asBinder()); } - if (!mWindowManagerInternal.isInputMethodClientFocus(cs.uid, cs.pid, - cs.selfReportedDisplayId)) { + if (!isImeClientFocused(windowToken, cs)) { if (DEBUG) { - Slog.w(TAG, - "Ignoring hideSoftInput of uid " + uid + ": " + client); + Slog.w(TAG, "Ignoring hideSoftInput of uid " + uid + ": " + client); } return false; } @@ -3277,6 +3279,12 @@ public class InputMethodManagerService extends IInputMethodManager.Stub return res; } + private boolean isImeClientFocused(IBinder windowToken, ClientState cs) { + final int imeClientFocus = mWindowManagerInternal.hasInputMethodClientFocus( + windowToken, cs.uid, cs.pid, cs.selfReportedDisplayId); + return imeClientFocus == WindowManagerInternal.ImeClientFocusResult.HAS_IME_FOCUS; + } + @NonNull @Override public InputBindResult startInputOrWindowGainedFocus( @@ -3370,31 +3378,30 @@ public class InputMethodManagerService extends IInputMethodManager.Stub + " unverifiedTargetSdkVersion=" + unverifiedTargetSdkVersion); } - final int windowDisplayId = mWindowManagerInternal.getDisplayIdForWindow(windowToken); - final ClientState cs = mClients.get(client.asBinder()); if (cs == null) { throw new IllegalArgumentException("unknown client " + client.asBinder()); } - if (cs.selfReportedDisplayId != windowDisplayId) { - Slog.e(TAG, "startInputOrWindowGainedFocusInternal: display ID mismatch." - + " from client:" + cs.selfReportedDisplayId - + " from window:" + windowDisplayId); - return InputBindResult.DISPLAY_ID_MISMATCH; - } - if (!mWindowManagerInternal.isInputMethodClientFocus(cs.uid, cs.pid, - cs.selfReportedDisplayId)) { - // Check with the window manager to make sure this client actually - // has a window with focus. If not, reject. This is thread safe - // because if the focus changes some time before or after, the - // next client receiving focus that has any interest in input will - // be calling through here after that change happens. - if (DEBUG) { - Slog.w(TAG, "Focus gain on non-focused client " + cs.client - + " (uid=" + cs.uid + " pid=" + cs.pid + ")"); - } - return InputBindResult.NOT_IME_TARGET_WINDOW; + final int imeClientFocus = mWindowManagerInternal.hasInputMethodClientFocus( + windowToken, cs.uid, cs.pid, cs.selfReportedDisplayId); + switch (imeClientFocus) { + case WindowManagerInternal.ImeClientFocusResult.DISPLAY_ID_MISMATCH: + Slog.e(TAG, "startInputOrWindowGainedFocusInternal: display ID mismatch."); + return InputBindResult.DISPLAY_ID_MISMATCH; + case WindowManagerInternal.ImeClientFocusResult.NOT_IME_TARGET_WINDOW: + // Check with the window manager to make sure this client actually + // has a window with focus. If not, reject. This is thread safe + // because if the focus changes some time before or after, the + // next client receiving focus that has any interest in input will + // be calling through here after that change happens. + if (DEBUG) { + Slog.w(TAG, "Focus gain on non-focused client " + cs.client + + " (uid=" + cs.uid + " pid=" + cs.pid + ")"); + } + return InputBindResult.NOT_IME_TARGET_WINDOW; + case WindowManagerInternal.ImeClientFocusResult.INVALID_DISPLAY_ID: + return InputBindResult.INVALID_DISPLAY_ID; } if (mUserSwitchHandlerTask != null) { @@ -3622,8 +3629,7 @@ public class InputMethodManagerService extends IInputMethodManager.Stub if (cs == null) { throw new IllegalArgumentException("unknown client " + client.asBinder()); } - if (!mWindowManagerInternal.isInputMethodClientFocus(cs.uid, cs.pid, - cs.selfReportedDisplayId)) { + if (!isImeClientFocused(mCurFocusedWindow, cs)) { Slog.w(TAG, String.format("Ignoring %s of uid %d : %s", methodName, uid, client)); return false; } diff --git a/services/core/java/com/android/server/wm/WindowManagerInternal.java b/services/core/java/com/android/server/wm/WindowManagerInternal.java index 1ab191bb66502..b9fa29733aa61 100644 --- a/services/core/java/com/android/server/wm/WindowManagerInternal.java +++ b/services/core/java/com/android/server/wm/WindowManagerInternal.java @@ -16,6 +16,9 @@ package com.android.server.wm; +import static java.lang.annotation.RetentionPolicy.SOURCE; + +import android.annotation.IntDef; import android.annotation.NonNull; import android.annotation.Nullable; import android.content.ClipData; @@ -40,6 +43,7 @@ import com.android.internal.policy.KeyInterceptionInfo; import com.android.server.input.InputManagerService; import com.android.server.policy.WindowManagerPolicy; +import java.lang.annotation.Retention; import java.util.List; import java.util.Set; @@ -609,6 +613,7 @@ public abstract class WindowManagerInternal { /** * Checks whether the specified IME client has IME focus or not. * + * @param windowToken The window token of the input method client * @param uid UID of the process to be queried * @param pid PID of the process to be queried * @param displayId Display ID reported from the client. Note that this method also verifies @@ -616,7 +621,22 @@ public abstract class WindowManagerInternal { * @return {@code true} if the IME client specified with {@code uid}, {@code pid}, and * {@code displayId} has IME focus */ - public abstract boolean isInputMethodClientFocus(int uid, int pid, int displayId); + public abstract @ImeClientFocusResult int hasInputMethodClientFocus(IBinder windowToken, + int uid, int pid, int displayId); + + @Retention(SOURCE) + @IntDef({ + ImeClientFocusResult.HAS_IME_FOCUS, + ImeClientFocusResult.NOT_IME_TARGET_WINDOW, + ImeClientFocusResult.DISPLAY_ID_MISMATCH, + ImeClientFocusResult.INVALID_DISPLAY_ID + }) + public @interface ImeClientFocusResult { + int HAS_IME_FOCUS = 0; + int NOT_IME_TARGET_WINDOW = -1; + int DISPLAY_ID_MISMATCH = -2; + int INVALID_DISPLAY_ID = -3; + } /** * Checks whether the given {@code uid} is allowed to use the given {@code displayId} or not. diff --git a/services/core/java/com/android/server/wm/WindowManagerService.java b/services/core/java/com/android/server/wm/WindowManagerService.java index 3515926e1f51a..03c70e1010ccf 100644 --- a/services/core/java/com/android/server/wm/WindowManagerService.java +++ b/services/core/java/com/android/server/wm/WindowManagerService.java @@ -7736,19 +7736,32 @@ public class WindowManagerService extends IWindowManager.Stub } @Override - public boolean isInputMethodClientFocus(int uid, int pid, int displayId) { + public @ImeClientFocusResult int hasInputMethodClientFocus(IBinder windowToken, + int uid, int pid, int displayId) { if (displayId == Display.INVALID_DISPLAY) { - return false; + return ImeClientFocusResult.INVALID_DISPLAY_ID; } synchronized (mGlobalLock) { final DisplayContent displayContent = mRoot.getTopFocusedDisplayContent(); + final WindowState window = mWindowMap.get(windowToken); + if (window == null) { + return ImeClientFocusResult.NOT_IME_TARGET_WINDOW; + } + final int tokenDisplayId = window.getDisplayContent().getDisplayId(); + if (tokenDisplayId != displayId) { + Slog.e(TAG, "isInputMethodClientFocus: display ID mismatch." + + " from client: " + displayId + + " from window: " + tokenDisplayId); + return ImeClientFocusResult.DISPLAY_ID_MISMATCH; + } if (displayContent == null || displayContent.getDisplayId() != displayId || !displayContent.hasAccess(uid)) { - return false; + return ImeClientFocusResult.INVALID_DISPLAY_ID; } + if (displayContent.isInputMethodClientFocus(uid, pid)) { - return true; + return ImeClientFocusResult.HAS_IME_FOCUS; } // Okay, how about this... what is the current focus? // It seems in some cases we may not have moved the IM @@ -7761,10 +7774,11 @@ public class WindowManagerService extends IWindowManager.Stub final WindowState currentFocus = displayContent.mCurrentFocus; if (currentFocus != null && currentFocus.mSession.mUid == uid && currentFocus.mSession.mPid == pid) { - return currentFocus.canBeImeTarget(); + return currentFocus.canBeImeTarget() ? ImeClientFocusResult.HAS_IME_FOCUS + : ImeClientFocusResult.NOT_IME_TARGET_WINDOW; } } - return false; + return ImeClientFocusResult.NOT_IME_TARGET_WINDOW; } @Override @@ -8760,20 +8774,21 @@ public class WindowManagerService extends IWindowManager.Stub } boolean shouldRestoreImeVisibility(IBinder imeTargetWindowToken) { + final Task imeTargetWindowTask; synchronized (mGlobalLock) { final WindowState imeTargetWindow = mWindowMap.get(imeTargetWindowToken); if (imeTargetWindow == null) { return false; } - final Task imeTargetWindowTask = imeTargetWindow.getTask(); + imeTargetWindowTask = imeTargetWindow.getTask(); if (imeTargetWindowTask == null) { return false; } - final TaskSnapshot snapshot = getTaskSnapshot(imeTargetWindowTask.mTaskId, - imeTargetWindowTask.mUserId, false /* isLowResolution */, - false /* restoreFromDisk */); - return snapshot != null && snapshot.hasImeSurface(); } + final TaskSnapshot snapshot = getTaskSnapshot(imeTargetWindowTask.mTaskId, + imeTargetWindowTask.mUserId, false /* isLowResolution */, + false /* restoreFromDisk */); + return snapshot != null && snapshot.hasImeSurface(); } @Override