From d116d7c78a9c53f30a73bf273bd7618312cf3847 Mon Sep 17 00:00:00 2001 From: Svetoslav Ganov Date: Mon, 21 Nov 2011 18:41:59 -0800 Subject: Fixing memory leaks in the accessiiblity layer. 1. AccessibilityInteractionConnections were removed from the AccessiiblityManagerService but their DeathRecipents were not unregistered, thus every removed interaction connection was essentially leaking. Such connection is registered in the system for every ViewRootImpl when accessiiblity is enabled and inregistered when disabled. 2. Every AccessibilityEvent and AccessiilbityEventInfo obtained from a widnow content querying accessibility service had a handle to a binder proxy over which to make queries. Hoewever, holding a proxy to a remote binder prevents the latter from being garbage collected. Therefore, now the events and infos have a connection id insteand and the hindden singleton AccessiiblityInteaction client via which queries are made has a registry with the connections. This class looks up the connection given its id before making an IPC. Now the connection is stored in one place and when an accessibility service is disconnected the system sets the connection to null so the binder object in the system process can be GCed. Note that before this change a bad implemented accessibility service could cache events or infos causing a leak in the system process. This should never happen. 3. SparseArray was not clearing the reference to the last moved element while garbage collecting thus causing a leak. bug:5664337 Change-Id: Id397f614b026d43bd7b57bb7f8186bca5cdfcff9 --- .../accessibility/AccessibilityManagerService.java | 112 ++++++++++++++++----- 1 file changed, 85 insertions(+), 27 deletions(-) (limited to 'services') diff --git a/services/java/com/android/server/accessibility/AccessibilityManagerService.java b/services/java/com/android/server/accessibility/AccessibilityManagerService.java index fd528cc..b70ed96 100644 --- a/services/java/com/android/server/accessibility/AccessibilityManagerService.java +++ b/services/java/com/android/server/accessibility/AccessibilityManagerService.java @@ -115,8 +115,8 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub private final Set mEnabledServices = new HashSet(); - private final SparseArray mWindowIdToInteractionConnectionMap = - new SparseArray(); + private final SparseArray mWindowIdToInteractionConnectionWrapperMap = + new SparseArray(); private final SparseArray mWindowIdToWindowTokenMap = new SparseArray(); @@ -439,16 +439,11 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub final IWindow addedWindowToken = windowToken; final IAccessibilityInteractionConnection addedConnection = connection; final int windowId = sNextWindowId++; - addedConnection.asBinder().linkToDeath(new DeathRecipient() { - public void binderDied() { - synchronized (mLock) { - addedConnection.asBinder().unlinkToDeath(this, 0); - removeAccessibilityInteractionConnection(addedWindowToken); - } - } - }, 0); + AccessibilityConnectionWrapper wrapper = new AccessibilityConnectionWrapper(windowId, + connection); + wrapper.linkToDeath(); mWindowIdToWindowTokenMap.put(windowId, addedWindowToken.asBinder()); - mWindowIdToInteractionConnectionMap.put(windowId, connection); + mWindowIdToInteractionConnectionWrapperMap.put(windowId, wrapper); if (DEBUG) { Slog.i(LOG_TAG, "Adding interaction connection to windowId: " + windowId); } @@ -462,18 +457,17 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub for (int i = 0; i < count; i++) { if (mWindowIdToWindowTokenMap.valueAt(i) == windowToken.asBinder()) { final int windowId = mWindowIdToWindowTokenMap.keyAt(i); - mWindowIdToWindowTokenMap.remove(windowId); - mWindowIdToInteractionConnectionMap.remove(windowId); - if (DEBUG) { - Slog.i(LOG_TAG, "Removing interaction connection to windowId: " + windowId); - } + AccessibilityConnectionWrapper wrapper = + mWindowIdToInteractionConnectionWrapperMap.get(windowId); + wrapper.unlinkToDeath(); + removeAccessibilityInteractionConnectionLocked(windowId); return; } } } } - public IAccessibilityServiceConnection registerEventListener(IEventListener listener) { + public void registerEventListener(IEventListener listener) { mSecurityPolicy.enforceCallingPermission(Manifest.permission.RETRIEVE_WINDOW_CONTENT, FUNCTION_REGISTER_EVENT_LISTENER); ComponentName componentName = new ComponentName("foo.bar", @@ -501,7 +495,19 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub accessibilityServiceInfo.feedbackType = AccessibilityServiceInfo.FEEDBACK_GENERIC; Service service = new Service(componentName, accessibilityServiceInfo, true); service.onServiceConnected(componentName, listener.asBinder()); - return service; + } + + /** + * Removes an AccessibilityInteractionConnection. + * + * @param windowId The id of the window to which the connection is targeted. + */ + private void removeAccessibilityInteractionConnectionLocked(int windowId) { + mWindowIdToWindowTokenMap.remove(windowId); + mWindowIdToInteractionConnectionWrapperMap.remove(windowId); + if (DEBUG) { + Slog.i(LOG_TAG, "Removing interaction connection to windowId: " + windowId); + } } /** @@ -594,6 +600,13 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub */ private void notifyEventListenerLocked(Service service, int eventType) { IEventListener listener = service.mServiceInterface; + + // If the service died/was disabled while the message for dispatching + // the accessibility event was propagating the listener may be null. + if (listener == null) { + return; + } + AccessibilityEvent event = service.mPendingEvents.get(eventType); // Check for null here because there is a concurrent scenario in which this @@ -618,7 +631,7 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub service.mPendingEvents.remove(eventType); try { if (mSecurityPolicy.canRetrieveWindowContent(service)) { - event.setConnection(service); + event.setConnectionId(service.mId); } else { event.setSource(null); } @@ -666,6 +679,7 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub mComponentNameToServiceMap.remove(service.mComponentName); mHandler.removeMessages(service.mId); service.unlinkToOwnDeath(); + service.dispose(); updateInputFilterLocked(); return removed; } @@ -895,6 +909,33 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub sendStateToClientsLocked(); } + private class AccessibilityConnectionWrapper implements DeathRecipient { + private final int mWindowId; + private final IAccessibilityInteractionConnection mConnection; + + public AccessibilityConnectionWrapper(int windowId, + IAccessibilityInteractionConnection connection) { + mWindowId = windowId; + mConnection = connection; + } + + public void linkToDeath() throws RemoteException { + mConnection.asBinder().linkToDeath(this, 0); + } + + public void unlinkToDeath() { + mConnection.asBinder().unlinkToDeath(this, 0); + } + + @Override + public void binderDied() { + unlinkToDeath(); + synchronized (mLock) { + removeAccessibilityInteractionConnectionLocked(mWindowId); + } + } + } + /** * This class represents an accessibility service. It stores all per service * data required for the service management, provides API for starting/stopping the @@ -997,7 +1038,6 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub if (!mIsAutomation) { mContext.unbindService(this); } - mService = null; return true; } return false; @@ -1021,7 +1061,7 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub mService = service; mServiceInterface = IEventListener.Stub.asInterface(service); try { - mServiceInterface.setConnection(this); + mServiceInterface.setConnection(this, mId); synchronized (mLock) { tryAddServiceLocked(this); } @@ -1123,14 +1163,16 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub if (!permissionGranted) { return 0; } else { - connection = mWindowIdToInteractionConnectionMap.get(accessibilityWindowId); - if (connection == null) { + AccessibilityConnectionWrapper wrapper = + mWindowIdToInteractionConnectionWrapperMap.get(accessibilityWindowId); + if (wrapper == null) { if (DEBUG) { Slog.e(LOG_TAG, "No interaction connection to window: " + accessibilityWindowId); } return 0; } + connection = wrapper.mConnection; } } final int interrogatingPid = Binder.getCallingPid(); @@ -1159,14 +1201,16 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub if (!permissionGranted) { return false; } else { - connection = mWindowIdToInteractionConnectionMap.get(accessibilityWindowId); - if (connection == null) { + AccessibilityConnectionWrapper wrapper = + mWindowIdToInteractionConnectionWrapperMap.get(accessibilityWindowId); + if (wrapper == null) { if (DEBUG) { Slog.e(LOG_TAG, "No interaction connection to window: " + accessibilityWindowId); } return false; } + connection = wrapper.mConnection; } } final int interrogatingPid = Binder.getCallingPid(); @@ -1197,9 +1241,21 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub mService.unlinkToDeath(this, 0); } + public void dispose() { + try { + // Clear the proxy in the other process so this + // IAccessibilityServiceConnection can be garbage collected. + mServiceInterface.setConnection(null, mId); + } catch (RemoteException re) { + /* ignore */ + } + mService = null; + mServiceInterface = null; + } + public void binderDied() { synchronized (mLock) { - mService.unlinkToDeath(this, 0); + unlinkToOwnDeath(); tryRemoveServiceLocked(this); // We no longer have an automation service, so restore // the state based on values in the settings database. @@ -1214,7 +1270,9 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub if (DEBUG) { Slog.i(LOG_TAG, "Trying to get interaction connection to windowId: " + windowId); } - return mWindowIdToInteractionConnectionMap.get(windowId); + AccessibilityConnectionWrapper wrapper = + mWindowIdToInteractionConnectionWrapperMap.get(windowId); + return (wrapper != null) ? wrapper.mConnection : null; } private float getCompatibilityScale(int windowId) { -- cgit v1.1