From 5de2feab261831e2d6fbe28eab53b884860cd6ea Mon Sep 17 00:00:00 2001 From: Phil Weaver Date: Thu, 25 Aug 2016 16:31:25 +0000 Subject: [PATCH] Revert "Dispatch a11y events in separate thread." This reverts commit c34649411d053185b3572c4cd924e6f14295d8cd. Dispatching accessibility events in their own thread is causing Chrome and gmail to crash. We've identified two issues: Chrome is allocating strings natively using references that aren't valid outside of their thread, and the text is being set to values that are changed in the UI thread. I'm going to resolve these issues on master by making deep copies of the strings, but that change will have its own performance implications. Since we were bit almost immediately by an unexpected result of this change, and I need to erode its benefit by making deep copies, I think it's a bad bet to push it into MR1. Bug: 31042124 Change-Id: I6f5c225a9197036db43fd0ac6008447b22617525 --- .../view/accessibility/AccessibilityManager.java | 164 +++++---------------- .../view/accessibility/IAccessibilityManager.aidl | 5 +- .../accessibility/AccessibilityManagerService.java | 46 ++---- .../android/server/AccessibilityManagerTest.java | 8 + 4 files changed, 63 insertions(+), 160 deletions(-) diff --git a/core/java/android/view/accessibility/AccessibilityManager.java b/core/java/android/view/accessibility/AccessibilityManager.java index 44f6facd88f5..2dfa8cdd3db9 100644 --- a/core/java/android/view/accessibility/AccessibilityManager.java +++ b/core/java/android/view/accessibility/AccessibilityManager.java @@ -21,7 +21,6 @@ import android.accessibilityservice.AccessibilityServiceInfo; import android.annotation.NonNull; import android.content.Context; import android.content.pm.PackageManager; -import android.content.pm.ParceledListSlice; import android.content.pm.ServiceInfo; import android.os.Binder; import android.os.Handler; @@ -92,9 +91,6 @@ public final class AccessibilityManager { /** @hide */ public static final int AUTOCLICK_DELAY_DEFAULT = 600; - /** @hide */ - public static final int MAX_A11Y_EVENTS_PER_SERVICE_CALL = 20; - static final Object sInstanceSync = new Object(); private static AccessibilityManager sInstance; @@ -103,8 +99,6 @@ public final class AccessibilityManager { private IAccessibilityManager mService; - private EventDispatchThread mEventDispatchThread; - final int mUserId; final Handler mHandler; @@ -176,7 +170,7 @@ public final class AccessibilityManager { private final IAccessibilityManagerClient.Stub mClient = new IAccessibilityManagerClient.Stub() { public void setState(int state) { - // We do not want to change this immediately as the application may + // We do not want to change this immediately as the applicatoin may // have already checked that accessibility is on and fired an event, // that is now propagating up the view tree, Hence, if accessibility // is now off an exception will be thrown. We want to have the exception @@ -303,32 +297,47 @@ public final class AccessibilityManager { * their descendants. */ public void sendAccessibilityEvent(AccessibilityEvent event) { - if (!isEnabled()) { - Looper myLooper = Looper.myLooper(); - if (myLooper == Looper.getMainLooper()) { - throw new IllegalStateException( - "Accessibility off. Did you forget to check that?"); - } else { - // If we're not running on the thread with the main looper, it's possible for - // the state of accessibility to change between checking isEnabled and - // calling this method. So just log the error rather than throwing the - // exception. - Log.e(LOG_TAG, "AccessibilityEvent sent with accessibility disabled"); + final IAccessibilityManager service; + final int userId; + synchronized (mLock) { + service = getServiceLocked(); + if (service == null) { return; } + if (!mIsEnabled) { + Looper myLooper = Looper.myLooper(); + if (myLooper == Looper.getMainLooper()) { + throw new IllegalStateException( + "Accessibility off. Did you forget to check that?"); + } else { + // If we're not running on the thread with the main looper, it's possible for + // the state of accessibility to change between checking isEnabled and + // calling this method. So just log the error rather than throwing the + // exception. + Log.e(LOG_TAG, "AccessibilityEvent sent with accessibility disabled"); + return; + } + } + userId = mUserId; } - event.setEventTime(SystemClock.uptimeMillis()); - - getEventDispatchThread().scheduleEvent(event); - } - - private EventDispatchThread getEventDispatchThread() { - synchronized (mLock) { - if (mEventDispatchThread == null) { - mEventDispatchThread = new EventDispatchThread(mService, mUserId); - mEventDispatchThread.start(); + boolean doRecycle = false; + try { + event.setEventTime(SystemClock.uptimeMillis()); + // it is possible that this manager is in the same process as the service but + // client using it is called through Binder from another process. Example: MMS + // app adds a SMS notification and the NotificationManagerService calls this method + long identityToken = Binder.clearCallingIdentity(); + doRecycle = service.sendAccessibilityEvent(event, userId); + Binder.restoreCallingIdentity(identityToken); + if (DEBUG) { + Log.i(LOG_TAG, event + " sent"); + } + } catch (RemoteException re) { + Log.e(LOG_TAG, "Error during sending " + event + " ", re); + } finally { + if (doRecycle) { + event.recycle(); } - return mEventDispatchThread; } } @@ -611,7 +620,7 @@ public final class AccessibilityManager { } } - private IAccessibilityManager getServiceLocked() { + private IAccessibilityManager getServiceLocked() { if (mService == null) { tryConnectToServiceLocked(null); } @@ -713,99 +722,4 @@ public final class AccessibilityManager { } } } - - private static class EventDispatchThread extends Thread { - // Second lock used to keep UI thread performant. Never try to grab mLock when holding - // this one, or the UI thread will block in send AccessibilityEvent. - private final Object mEventQueueLock = new Object(); - - // Two lists to hold events. The app thread fills one while we empty the other. - private final ArrayList mEventLists0 = - new ArrayList<>(MAX_A11Y_EVENTS_PER_SERVICE_CALL); - private final ArrayList mEventLists1 = - new ArrayList<>(MAX_A11Y_EVENTS_PER_SERVICE_CALL); - - private boolean mPingPongListToggle; - - private final IAccessibilityManager mService; - - private final int mUserId; - - EventDispatchThread(IAccessibilityManager service, int userId) { - mService = service; - mUserId = userId; - } - - @Override - public void run() { - while (true) { - ArrayList listBeingDrained; - synchronized (mEventQueueLock) { - ArrayList listBeingFilled = getListBeingFilledLocked(); - if (listBeingFilled.isEmpty()) { - try { - mEventQueueLock.wait(); - } catch (InterruptedException e) { - // Treat as a notify - } - } - // Swap buffers - mPingPongListToggle = !mPingPongListToggle; - listBeingDrained = listBeingFilled; - } - dispatchEvents(listBeingDrained); - } - } - - public void scheduleEvent(AccessibilityEvent event) { - synchronized (mEventQueueLock) { - getListBeingFilledLocked().add(event); - mEventQueueLock.notifyAll(); - } - } - - private ArrayList getListBeingFilledLocked() { - return (mPingPongListToggle) ? mEventLists0 : mEventLists1; - } - - private void dispatchEvents(ArrayList events) { - int eventListCapacityLowerBound = events.size(); - while (events.size() > 0) { - // We don't want to consume extra memory if an app sends a lot of events in a - // one-off event. Cap the list length at double the max events per call. - // We'll end up with extra GC for apps that send huge numbers of events, but - // sending that many events will lead to bad performance in any case. - if ((eventListCapacityLowerBound > 2 * MAX_A11Y_EVENTS_PER_SERVICE_CALL) - && (events.size() <= 2 * MAX_A11Y_EVENTS_PER_SERVICE_CALL)) { - events.trimToSize(); - eventListCapacityLowerBound = events.size(); - } - // We only expect this loop to run once, as the app shouldn't be sending - // huge numbers of events. - // The clear in the called method will remove the sent events - dispatchOneBatchOfEvents(events.subList(0, - Math.min(events.size(), MAX_A11Y_EVENTS_PER_SERVICE_CALL))); - } - } - - private void dispatchOneBatchOfEvents(List events) { - if (events.isEmpty()) { - return; - } - long identityToken = Binder.clearCallingIdentity(); - try { - mService.sendAccessibilityEvents(new ParceledListSlice<>(events), - mUserId); - } catch (RemoteException re) { - Log.e(LOG_TAG, "Error sending multiple events"); - } - Binder.restoreCallingIdentity(identityToken); - if (DEBUG) { - Log.i(LOG_TAG, events.size() + " events sent"); - } - for (int i = events.size() - 1; i >= 0; i--) { - events.remove(i).recycle(); - } - } - } } diff --git a/core/java/android/view/accessibility/IAccessibilityManager.aidl b/core/java/android/view/accessibility/IAccessibilityManager.aidl index aa9cb39062f9..7f44bac8bc6f 100644 --- a/core/java/android/view/accessibility/IAccessibilityManager.aidl +++ b/core/java/android/view/accessibility/IAccessibilityManager.aidl @@ -21,7 +21,6 @@ import android.accessibilityservice.AccessibilityServiceInfo; import android.accessibilityservice.IAccessibilityServiceConnection; import android.accessibilityservice.IAccessibilityServiceClient; import android.content.ComponentName; -import android.content.pm.ParceledListSlice; import android.view.accessibility.AccessibilityEvent; import android.view.accessibility.AccessibilityNodeInfo; import android.view.accessibility.IAccessibilityInteractionConnection; @@ -38,9 +37,7 @@ interface IAccessibilityManager { int addClient(IAccessibilityManagerClient client, int userId); - void sendAccessibilityEvent(in AccessibilityEvent uiEvent, int userId); - - void sendAccessibilityEvents(in ParceledListSlice events, int userId); + boolean sendAccessibilityEvent(in AccessibilityEvent uiEvent, int userId); List getInstalledAccessibilityServiceList(int userId); diff --git a/services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java b/services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java index b1fbcde6c727..695ea606a90b 100644 --- a/services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java +++ b/services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java @@ -451,7 +451,7 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub { } @Override - public void sendAccessibilityEvent(AccessibilityEvent event, int userId) { + public boolean sendAccessibilityEvent(AccessibilityEvent event, int userId) { synchronized (mLock) { // We treat calls from a profile as if made by its parent as profiles // share the accessibility state of the parent. The call below @@ -459,39 +459,23 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub { final int resolvedUserId = mSecurityPolicy .resolveCallingUserIdEnforcingPermissionsLocked(userId); // This method does nothing for a background user. - if (resolvedUserId == mCurrentUserId) { - if (mSecurityPolicy.canDispatchAccessibilityEventLocked(event)) { - mSecurityPolicy.updateActiveAndAccessibilityFocusedWindowLocked( - event.getWindowId(), event.getSourceNodeId(), - event.getEventType(), event.getAction()); - mSecurityPolicy.updateEventSourceLocked(event); - notifyAccessibilityServicesDelayedLocked(event, false); - notifyAccessibilityServicesDelayedLocked(event, true); - } - if (mHasInputFilter && mInputFilter != null) { - mMainHandler.obtainMessage( - MainHandler.MSG_SEND_ACCESSIBILITY_EVENT_TO_INPUT_FILTER, - AccessibilityEvent.obtain(event)).sendToTarget(); - } + if (resolvedUserId != mCurrentUserId) { + return true; // yes, recycle the event } - } - if (OWN_PROCESS_ID != Binder.getCallingPid()) { - event.recycle(); - } - } - - @Override - public void sendAccessibilityEvents(ParceledListSlice events, int userId) { - List a11yEvents = events.getList(); - // Grab the lock once for the entire batch - synchronized (mLock) { - int numEventsToProcess = Math.min(a11yEvents.size(), - AccessibilityManager.MAX_A11Y_EVENTS_PER_SERVICE_CALL); - for (int i = 0; i < numEventsToProcess; i++) { - AccessibilityEvent event = a11yEvents.get(i); - sendAccessibilityEvent(event, userId); + if (mSecurityPolicy.canDispatchAccessibilityEventLocked(event)) { + mSecurityPolicy.updateActiveAndAccessibilityFocusedWindowLocked(event.getWindowId(), + event.getSourceNodeId(), event.getEventType(), event.getAction()); + mSecurityPolicy.updateEventSourceLocked(event); + notifyAccessibilityServicesDelayedLocked(event, false); + notifyAccessibilityServicesDelayedLocked(event, true); + } + if (mHasInputFilter && mInputFilter != null) { + mMainHandler.obtainMessage(MainHandler.MSG_SEND_ACCESSIBILITY_EVENT_TO_INPUT_FILTER, + AccessibilityEvent.obtain(event)).sendToTarget(); } + event.recycle(); } + return (OWN_PROCESS_ID != Binder.getCallingPid()); } @Override diff --git a/services/tests/servicestests/src/com/android/server/AccessibilityManagerTest.java b/services/tests/servicestests/src/com/android/server/AccessibilityManagerTest.java index 6e3e6c6278a9..026a2adc1f96 100644 --- a/services/tests/servicestests/src/com/android/server/AccessibilityManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/AccessibilityManagerTest.java @@ -131,10 +131,18 @@ public class AccessibilityManagerTest extends AndroidTestCase { public void testSendAccessibilityEvent_AccessibilityEnabled() throws Exception { AccessibilityEvent sentEvent = AccessibilityEvent.obtain(); + when(mMockService.sendAccessibilityEvent(eq(sentEvent), anyInt())) + .thenReturn(true /* should recycle event object */) + .thenReturn(false /* should not recycle event object */); + AccessibilityManager manager = createManager(true); manager.sendAccessibilityEvent(sentEvent); assertSame("The event should be recycled.", sentEvent, AccessibilityEvent.obtain()); + + manager.sendAccessibilityEvent(sentEvent); + + assertNotSame("The event should not be recycled.", sentEvent, AccessibilityEvent.obtain()); } @MediumTest -- 2.11.0