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
diff --git a/core/java/android/view/accessibility/AccessibilityManager.java b/core/java/android/view/accessibility/AccessibilityManager.java
index 44f6fac..2dfa8cd 100644
--- a/core/java/android/view/accessibility/AccessibilityManager.java
+++ b/core/java/android/view/accessibility/AccessibilityManager.java
@@ -21,7 +21,6 @@
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 @@
/** @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 @@
private IAccessibilityManager mService;
- private EventDispatchThread mEventDispatchThread;
-
final int mUserId;
final Handler mHandler;
@@ -176,7 +170,7 @@
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 @@
* 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;
}
- }
- event.setEventTime(SystemClock.uptimeMillis());
-
- getEventDispatchThread().scheduleEvent(event);
- }
-
- private EventDispatchThread getEventDispatchThread() {
- synchronized (mLock) {
- if (mEventDispatchThread == null) {
- mEventDispatchThread = new EventDispatchThread(mService, mUserId);
- mEventDispatchThread.start();
+ 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;
+ }
}
- return mEventDispatchThread;
+ userId = mUserId;
+ }
+ 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();
+ }
}
}
@@ -611,7 +620,7 @@
}
}
- private IAccessibilityManager getServiceLocked() {
+ private IAccessibilityManager getServiceLocked() {
if (mService == null) {
tryConnectToServiceLocked(null);
}
@@ -713,99 +722,4 @@
}
}
}
-
- 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<AccessibilityEvent> mEventLists0 =
- new ArrayList<>(MAX_A11Y_EVENTS_PER_SERVICE_CALL);
- private final ArrayList<AccessibilityEvent> 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<AccessibilityEvent> listBeingDrained;
- synchronized (mEventQueueLock) {
- ArrayList<AccessibilityEvent> 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<AccessibilityEvent> getListBeingFilledLocked() {
- return (mPingPongListToggle) ? mEventLists0 : mEventLists1;
- }
-
- private void dispatchEvents(ArrayList<AccessibilityEvent> 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<AccessibilityEvent> 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 aa9cb39..7f44bac 100644
--- a/core/java/android/view/accessibility/IAccessibilityManager.aidl
+++ b/core/java/android/view/accessibility/IAccessibilityManager.aidl
@@ -21,7 +21,6 @@
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 @@
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<AccessibilityServiceInfo> 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 b1fbcde..695ea60 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 @@
}
@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 @@
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()) {
+ 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();
}
- }
-
- @Override
- public void sendAccessibilityEvents(ParceledListSlice events, int userId) {
- List<AccessibilityEvent> 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);
- }
- }
+ 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 6e3e6c6..026a2ad 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 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
