Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 5de2feab authored by Phil Weaver's avatar Phil Weaver
Browse files

Revert "Dispatch a11y events in separate thread." 

This reverts commit c3464941.

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
parent c3464941

core/java/android/view/accessibility/AccessibilityManager.java

+39 −125
Original line number Diff line number Diff line
@@ -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,7 +297,14 @@ public final class AccessibilityManager { 
     * their descendants.

     */

    public void sendAccessibilityEvent(AccessibilityEvent event) {

        if (!isEnabled()) {

        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(
@@ -317,18 +318,26 @@ public final class AccessibilityManager { 
                    return;

                }

            }

            userId = mUserId;

        }

        boolean doRecycle = false;

        try {

            event.setEventTime(SystemClock.uptimeMillis());



        getEventDispatchThread().scheduleEvent(event);

            // 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");

            }



    private EventDispatchThread getEventDispatchThread() {

        synchronized (mLock) {

            if (mEventDispatchThread == null) {

                mEventDispatchThread = new EventDispatchThread(mService, mUserId);

                mEventDispatchThread.start();

        } catch (RemoteException re) {

            Log.e(LOG_TAG, "Error during sending " + event + " ", re);

        } finally {

            if (doRecycle) {

                event.recycle();

            }

            return mEventDispatchThread;

        }

    }
@@ -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<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();

            }

        }

    }

}

core/java/android/view/accessibility/IAccessibilityManager.aidl

+1 −4
Original line number Diff line number Diff line
@@ -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<AccessibilityServiceInfo> getInstalledAccessibilityServiceList(int userId);

services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java

+15 −31
Original line number Diff line number Diff line
@@ -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 (resolvedUserId != mCurrentUserId) {

                return true; // yes, recycle the event

            }

            if (mSecurityPolicy.canDispatchAccessibilityEventLocked(event)) {

                    mSecurityPolicy.updateActiveAndAccessibilityFocusedWindowLocked(

                            event.getWindowId(), event.getSourceNodeId(),

                            event.getEventType(), event.getAction());

                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,

                mMainHandler.obtainMessage(MainHandler.MSG_SEND_ACCESSIBILITY_EVENT_TO_INPUT_FILTER,

                        AccessibilityEvent.obtain(event)).sendToTarget();

            }

            }

        }

        if (OWN_PROCESS_ID != Binder.getCallingPid()) {

            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

services/tests/servicestests/src/com/android/server/AccessibilityManagerTest.java

+8 −0
Original line number Diff line number Diff line
@@ -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