From 31eaf452adee227f1b9f00503ed4470fb525d9b7 Mon Sep 17 00:00:00 2001 From: Jacky Kao Date: Wed, 7 Apr 2021 10:41:52 +0800 Subject: [PATCH] Improve the windows register and unregister mechanism 1. For fixing the crash due to mutl-register callback, we avoid using the display context when un-registering the callback. Instead, we remove the observer mapping if the display exists along with the observer of any embedded displays that are children of the un-registered one. 2. When an display registers its callback first, and then got its reparent window, there's an displayWindowObserver for it in the A11yWindowManager. But it didn't be used because it's an embedded display and any windows change on it will be notified through the callback of its parent display. So we add a callback to remove this un-used displayWindowObservers in the A11yWindowManager when this display is an embedded one. Bug: 143114102 Test: a11y CTS & unit tests Merged-In: I00fd1f45c0b4e362143a8d82f31264ff202f0967 (cherry picked from commit 5b8b737755b6f0d5fad0d4bf1f31254a9c71a5ab) --- .../AccessibilityWindowManager.java | 14 ++++ .../server/wm/AccessibilityController.java | 75 ++++++++++++------- .../server/wm/WindowManagerInternal.java | 10 +++ .../AccessibilityWindowManagerTest.java | 13 ++++ 4 files changed, 83 insertions(+), 29 deletions(-) diff --git a/services/accessibility/java/com/android/server/accessibility/AccessibilityWindowManager.java b/services/accessibility/java/com/android/server/accessibility/AccessibilityWindowManager.java index ff794691d2b44..8820487665bce 100644 --- a/services/accessibility/java/com/android/server/accessibility/AccessibilityWindowManager.java +++ b/services/accessibility/java/com/android/server/accessibility/AccessibilityWindowManager.java @@ -373,6 +373,20 @@ public class AccessibilityWindowManager { } } + /** + * Called when the display is reparented and becomes an embedded + * display. + * + * @param embeddedDisplayId The embedded display Id. + */ + @Override + public void onDisplayReparented(int embeddedDisplayId) { + // Removes the un-used window observer for the embedded display. + synchronized (mLock) { + mDisplayWindowsObservers.remove(embeddedDisplayId); + } + } + private boolean shouldUpdateWindowsLocked(boolean forceSend, @NonNull List windows) { if (forceSend) { diff --git a/services/core/java/com/android/server/wm/AccessibilityController.java b/services/core/java/com/android/server/wm/AccessibilityController.java index a97c0804edce6..6b8ec4cd61969 100644 --- a/services/core/java/com/android/server/wm/AccessibilityController.java +++ b/services/core/java/com/android/server/wm/AccessibilityController.java @@ -90,6 +90,7 @@ import android.view.animation.Interpolator; import com.android.internal.R; import com.android.internal.os.SomeArgs; import com.android.internal.util.TraceBuffer; +import com.android.internal.util.function.pooled.PooledLambda; import com.android.server.LocalServices; import com.android.server.policy.WindowManagerPolicy; import com.android.server.wm.WindowManagerInternal.AccessibilityControllerInternal; @@ -172,11 +173,15 @@ final class AccessibilityController { /** * Sets a callback for observing which windows are touchable for the purposes - * of accessibility on specified display. + * of accessibility on specified display. When a display is reparented and becomes + * an embedded one, the {@link WindowsForAccessibilityCallback#onDisplayReparented(int)} + * will notify the accessibility framework to remove the un-used window observer of + * this embedded display. * * @param displayId The logical display id. * @param callback The callback. - * @return {@code false} if display id is not valid or an embedded display. + * @return {@code false} if display id is not valid or an embedded display when the callback + * isn't null. */ boolean setWindowsForAccessibilityCallback(int displayId, WindowsForAccessibilityCallback callback) { @@ -185,12 +190,13 @@ final class AccessibilityController { TAG + ".setWindowsForAccessibilityCallback", "displayId=" + displayId + "; callback={" + callback + "}"); } - final DisplayContent dc = mService.mRoot.getDisplayContentOrCreate(displayId); - if (dc == null) { - return false; - } if (callback != null) { + final DisplayContent dc = mService.mRoot.getDisplayContentOrCreate(displayId); + if (dc == null) { + return false; + } + WindowsForAccessibilityObserver observer = mWindowsForAccessibilityObserver.get(displayId); if (isEmbeddedDisplay(dc)) { @@ -209,21 +215,13 @@ final class AccessibilityController { if (Build.IS_DEBUGGABLE) { throw new IllegalStateException(errorMessage); } - removeObserverOfEmbeddedDisplay(observer); + removeObserversForEmbeddedChildDisplays(observer); mWindowsForAccessibilityObserver.remove(displayId); } observer = new WindowsForAccessibilityObserver(mService, displayId, callback); mWindowsForAccessibilityObserver.put(displayId, observer); mAllObserversInitialized &= observer.mInitialized; } else { - if (isEmbeddedDisplay(dc)) { - // If this display is an embedded one, its window observer should be removed along - // with the window observer of its parent display removed because the window - // observer of the embedded display and its parent display is the same, and would - // be removed together when stopping the window tracking of its parent display. So - // here don't need to do removing window observer of the embedded display again. - return true; - } final WindowsForAccessibilityObserver windowsForA11yObserver = mWindowsForAccessibilityObserver.get(displayId); if (windowsForA11yObserver == null) { @@ -234,7 +232,7 @@ final class AccessibilityController { throw new IllegalStateException(errorMessage); } } - removeObserverOfEmbeddedDisplay(windowsForA11yObserver); + removeObserversForEmbeddedChildDisplays(windowsForA11yObserver); mWindowsForAccessibilityObserver.remove(displayId); } return true; @@ -507,22 +505,34 @@ final class AccessibilityController { if (embeddedDisplayId == Display.DEFAULT_DISPLAY || parentWindow == null) { return; } - // Finds the parent display of this embedded display - final int parentDisplayId; - WindowState candidate = parentWindow; - while (candidate != null) { - parentWindow = candidate; - candidate = parentWindow.getDisplayContent().getParentWindow(); + mService.mH.sendMessage(PooledLambda.obtainMessage( + AccessibilityController::updateWindowObserverOfEmbeddedDisplay, + this, embeddedDisplayId, parentWindow)); + } + + private void updateWindowObserverOfEmbeddedDisplay(int embeddedDisplayId, + WindowState parentWindow) { + final WindowsForAccessibilityObserver windowsForA11yObserver; + + synchronized (mService.mGlobalLock) { + // Finds the parent display of this embedded display + WindowState candidate = parentWindow; + while (candidate != null) { + parentWindow = candidate; + candidate = parentWindow.getDisplayContent().getParentWindow(); + } + final int parentDisplayId = parentWindow.getDisplayId(); + // Uses the observer of parent display + windowsForA11yObserver = mWindowsForAccessibilityObserver.get(parentDisplayId); } - parentDisplayId = parentWindow.getDisplayId(); - // Uses the observer of parent display - final WindowsForAccessibilityObserver windowsForA11yObserver = - mWindowsForAccessibilityObserver.get(parentDisplayId); if (windowsForA11yObserver != null) { + windowsForA11yObserver.notifyDisplayReparented(embeddedDisplayId); windowsForA11yObserver.addEmbeddedDisplay(embeddedDisplayId); - // Replaces the observer of embedded display to the one of parent display - mWindowsForAccessibilityObserver.put(embeddedDisplayId, windowsForA11yObserver); + synchronized (mService.mGlobalLock) { + // Replaces the observer of embedded display to the one of parent display + mWindowsForAccessibilityObserver.put(embeddedDisplayId, windowsForA11yObserver); + } } } @@ -555,7 +565,7 @@ final class AccessibilityController { + "mWindowsForAccessibilityObserver=" + mWindowsForAccessibilityObserver); } - private void removeObserverOfEmbeddedDisplay(WindowsForAccessibilityObserver + private void removeObserversForEmbeddedChildDisplays(WindowsForAccessibilityObserver observerOfParentDisplay) { final IntArray embeddedDisplayIdList = observerOfParentDisplay.getAndClearEmbeddedDisplayIdList(); @@ -1541,6 +1551,13 @@ final class AccessibilityController { mEmbeddedDisplayIdList.add(displayId); } + void notifyDisplayReparented(int embeddedDisplayId) { + // Notifies the A11y framework the display is reparented and + // becomes an embedded display for removing the un-used + // displayWindowObserver of this embedded one. + mCallback.onDisplayReparented(embeddedDisplayId); + } + /** * Check if windows have changed, and send them to the accessibility subsystem if they have. * diff --git a/services/core/java/com/android/server/wm/WindowManagerInternal.java b/services/core/java/com/android/server/wm/WindowManagerInternal.java index 47087cfbd147b..1d4882733b02f 100644 --- a/services/core/java/com/android/server/wm/WindowManagerInternal.java +++ b/services/core/java/com/android/server/wm/WindowManagerInternal.java @@ -115,6 +115,16 @@ public abstract class WindowManagerInternal { */ void onWindowsForAccessibilityChanged(boolean forceSend, int topFocusedDisplayId, IBinder topFocusedWindowToken, @NonNull List windows); + + /** + * Called when the display is reparented and becomes an embedded + * display. The {@link WindowsForAccessibilityCallback} with the given embedded + * display will be replaced by the {@link WindowsForAccessibilityCallback} + * associated with its parent display at the same time. + * + * @param embeddedDisplayId The embedded display Id. + */ + void onDisplayReparented(int embeddedDisplayId); } /** diff --git a/services/tests/servicestests/src/com/android/server/accessibility/AccessibilityWindowManagerTest.java b/services/tests/servicestests/src/com/android/server/accessibility/AccessibilityWindowManagerTest.java index e4d51e4374a78..5eaa96494ebfb 100644 --- a/services/tests/servicestests/src/com/android/server/accessibility/AccessibilityWindowManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/accessibility/AccessibilityWindowManagerTest.java @@ -834,6 +834,19 @@ public class AccessibilityWindowManagerTest { assertNull(token); } + @Test + public void onDisplayReparented_shouldRemoveObserver() throws RemoteException { + // Starts tracking window of second display. + startTrackingPerDisplay(SECONDARY_DISPLAY_ID); + assertTrue(mA11yWindowManager.isTrackingWindowsLocked(SECONDARY_DISPLAY_ID)); + // Notifies the second display is an embedded one of the default display. + final WindowsForAccessibilityCallback callbacks = + mCallbackOfWindows.get(Display.DEFAULT_DISPLAY); + callbacks.onDisplayReparented(SECONDARY_DISPLAY_ID); + // Makes sure the observer of the second display is removed. + assertFalse(mA11yWindowManager.isTrackingWindowsLocked(SECONDARY_DISPLAY_ID)); + } + private void registerLeashedTokenAndWindowId() { mA11yWindowManager.registerIdLocked(mMockHostToken, HOST_WINDOW_ID); mA11yWindowManager.registerIdLocked(mMockEmbeddedToken, EMBEDDED_WINDOW_ID);