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

Commit 724d2146 authored by Sally's avatar Sally Committed by Daniel Norman
Browse files

Fix race condition between VDM and ProxyManager when registering a proxy 

Exo may register a proxy before VDM begins tracking the streamed,
but after this app's AccessibilityManagerClient is created in AMS.
This race conditions means the app is not notified that
it is proxied, since VDM#getDevicesForUid will not return the proxy
device id when called.

Add a AppsOnVirtualDeviceListener to the ProxyManager. If the running
apps are changed on any virtual device, force notifying the apps if
any of these belong to a proxied device. We add `forceUpdate` since the
last and current client state may be the same (no proxy connections
have changed) in this case.

Also move any apps that are no longer proxied back to the default device
if needed when updating the client device ids, since Exo will use the
same device and launch activities on the same display. (We were
previously resetting clients only when a device id
had no registered proxies, but Exo should be able to keep the proxy
registered)

Bug: 286587811
Test: manual with Exo, atest ProxyManagerTest, atest
AccessibilityDisplayProxyTest

Change-Id: I1a1b600c9e71d7baf1f9647475ac63580f7f01c2
parent 40db2be3

services/accessibility/Android.bp

+16 −0
Original line number Diff line number Diff line
@@ -27,4 +27,20 @@ java_library_static { 
        "services.core",

        "androidx.annotation_annotation",

    ],

    static_libs: [

        "com_android_server_accessibility_flags_lib",

    ],

}



aconfig_declarations {

    name: "com_android_server_accessibility_flags",

    package: "com.android.server.accessibility",

    srcs: [

        "accessibility.aconfig",

    ],

}



java_aconfig_library {

    name: "com_android_server_accessibility_flags_lib",

    aconfig_declarations: "com_android_server_accessibility_flags",

}

services/accessibility/accessibility.aconfig

0 → 100644
+7 −0
Original line number Diff line number Diff line 
package: "com.android.server.accessibility"

flag {

    name: "proxy_use_apps_on_virtual_device_listener"

    namespace: "accessibility"

    description: "Fixes race condition described in b/286587811"

    bug: "286587811"

}

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

+104 −12
Original line number Diff line number Diff line
@@ -33,6 +33,7 @@ import android.os.Handler; 
import android.os.IBinder;

import android.os.RemoteCallbackList;

import android.os.RemoteException;

import android.util.ArraySet;

import android.util.IntArray;

import android.util.Slog;

import android.util.SparseArray;
@@ -42,6 +43,7 @@ import android.view.accessibility.AccessibilityEvent; 
import android.view.accessibility.AccessibilityManager;

import android.view.accessibility.IAccessibilityManagerClient;



import com.android.internal.annotations.VisibleForTesting;

import com.android.internal.util.IntPair;

import com.android.server.LocalServices;

import com.android.server.companion.virtual.VirtualDeviceManagerInternal;
@@ -96,6 +98,9 @@ public class ProxyManager { 


    private final SystemSupport mSystemSupport;



    private VirtualDeviceManagerInternal.AppsOnVirtualDeviceListener

            mAppsOnVirtualDeviceListener;



    /**

     * Callbacks into AccessibilityManagerService.

     */
@@ -174,6 +179,16 @@ public class ProxyManager { 


        synchronized (mLock) {

            mProxyA11yServiceConnections.put(displayId, connection);

            if (Flags.proxyUseAppsOnVirtualDeviceListener()) {

                if (mAppsOnVirtualDeviceListener == null) {

                    mAppsOnVirtualDeviceListener = allRunningUids ->

                            notifyProxyOfRunningAppsChange(allRunningUids);

                    final VirtualDeviceManagerInternal localVdm = getLocalVdm();

                    if (localVdm != null) {

                        localVdm.registerAppsOnVirtualDeviceListener(mAppsOnVirtualDeviceListener);

                    }

                }

            }

        }



        // If the client dies, make sure to remove the connection.
@@ -276,11 +291,21 @@ public class ProxyManager { 
                        }

                    }

                });

        // If there isn't an existing proxy for the device id, reset clients. Resetting

        // If there isn't an existing proxy for the device id, reset app clients. Resetting

        // will usually happen, since in most cases there will only be one proxy for a

        // device.

        if (!isProxyedDeviceId(deviceId)) {

            synchronized (mLock) {

                if (Flags.proxyUseAppsOnVirtualDeviceListener()) {

                    if (mProxyA11yServiceConnections.size() == 0) {

                        final VirtualDeviceManagerInternal localVdm = getLocalVdm();

                        if (localVdm != null && mAppsOnVirtualDeviceListener != null) {

                            localVdm.unregisterAppsOnVirtualDeviceListener(

                                    mAppsOnVirtualDeviceListener);

                            mAppsOnVirtualDeviceListener = null;

                        }

                    }

                }

                mSystemSupport.removeDeviceIdLocked(deviceId);

                mLastStates.delete(deviceId);

            }
@@ -307,7 +332,7 @@ public class ProxyManager { 
     * Returns {@code true} if {@code deviceId} is being proxy-ed.

     */

    public boolean isProxyedDeviceId(int deviceId) {

        if (deviceId == DEVICE_ID_DEFAULT && deviceId == DEVICE_ID_INVALID) {

        if (deviceId == DEVICE_ID_DEFAULT || deviceId == DEVICE_ID_INVALID) {

            return false;

        }

        boolean isTrackingDeviceId;
@@ -566,7 +591,7 @@ public class ProxyManager { 
     * This is similar to onUserStateChangeLocked and onClientChangeLocked, but does not require an

     * A11yUserState and only checks proxy-relevant settings.

     */

    public void onProxyChanged(int deviceId) {

    private void onProxyChanged(int deviceId, boolean forceUpdate) {

        if (DEBUG) {

            Slog.v(LOG_TAG, "onProxyChanged called for deviceId: " + deviceId);

        }
@@ -584,7 +609,7 @@ public class ProxyManager { 
            // Calls A11yManager#setRelevantEventTypes (test these)

            updateRelevantEventTypesLocked(deviceId);

            // Calls A11yManager#setState

            scheduleUpdateProxyClientsIfNeededLocked(deviceId);

            scheduleUpdateProxyClientsIfNeededLocked(deviceId, forceUpdate);

            //Calls A11yManager#notifyServicesStateChanged(timeout)

            scheduleNotifyProxyClientsOfServicesStateChangeLocked(deviceId);

            // Calls A11yManager#setFocusAppearance
@@ -593,17 +618,26 @@ public class ProxyManager { 
        }

    }



    /**

     * Handles proxy changes, but does not force an update of app clients.

     */

    public void onProxyChanged(int deviceId) {

        onProxyChanged(deviceId, false);

    }



    /**

     * Updates the states of the app AccessibilityManagers.

     */

    private void scheduleUpdateProxyClientsIfNeededLocked(int deviceId) {

    private void scheduleUpdateProxyClientsIfNeededLocked(int deviceId, boolean forceUpdate) {

        final int proxyState = getStateLocked(deviceId);

        if (DEBUG) {

            Slog.v(LOG_TAG, "State for device id " + deviceId + " is " + proxyState);

            Slog.v(LOG_TAG, "Last state for device id " + deviceId + " is "

                    + getLastSentStateLocked(deviceId));

            Slog.v(LOG_TAG, "force update: " + forceUpdate);

        }

        if ((getLastSentStateLocked(deviceId)) != proxyState) {

        if ((getLastSentStateLocked(deviceId)) != proxyState

                || (Flags.proxyUseAppsOnVirtualDeviceListener() && forceUpdate)) {

            setLastStateLocked(deviceId, proxyState);

            mMainHandler.post(() -> {

                synchronized (mLock) {
@@ -792,7 +826,7 @@ public class ProxyManager { 
    }



    /**

     * Updates the device ids of IAccessibilityManagerClients if needed.

     * Updates the device ids of IAccessibilityManagerClients if needed after a proxy change.

     */

    private void updateDeviceIdsIfNeededLocked(int deviceId,

            @NonNull RemoteCallbackList<IAccessibilityManagerClient> clients) {
@@ -804,6 +838,26 @@ public class ProxyManager { 
        for (int i = 0; i < clients.getRegisteredCallbackCount(); i++) {

            final AccessibilityManagerService.Client client =

                    ((AccessibilityManagerService.Client) clients.getRegisteredCallbackCookie(i));

            if (Flags.proxyUseAppsOnVirtualDeviceListener()) {

                if (deviceId == DEVICE_ID_DEFAULT || deviceId == DEVICE_ID_INVALID) {

                    continue;

                }

                boolean uidBelongsToDevice =

                        localVdm.getDeviceIdsForUid(client.mUid).contains(deviceId);

                if (client.mDeviceId != deviceId && uidBelongsToDevice) {

                    if (DEBUG) {

                        Slog.v(LOG_TAG, "Packages moved to device id " + deviceId + " are "

                                + Arrays.toString(client.mPackageNames));

                    }

                    client.mDeviceId = deviceId;

                } else if (client.mDeviceId == deviceId && !uidBelongsToDevice) {

                    client.mDeviceId = DEVICE_ID_DEFAULT;

                    if (DEBUG) {

                        Slog.v(LOG_TAG, "Packages moved to the default device from device id "

                                + deviceId + " are " + Arrays.toString(client.mPackageNames));

                    }

                }

            } else {

                if (deviceId != DEVICE_ID_DEFAULT && deviceId != DEVICE_ID_INVALID

                    && localVdm.getDeviceIdsForUid(client.mUid).contains(deviceId)) {

                    if (DEBUG) {
@@ -814,6 +868,39 @@ public class ProxyManager { 
                }

            }

        }

    }



    @VisibleForTesting

    void notifyProxyOfRunningAppsChange(Set<Integer> allRunningUids) {

        if (DEBUG) {

            Slog.v(LOG_TAG, "notifyProxyOfRunningAppsChange: " + allRunningUids);

        }

        synchronized (mLock) {

            if (mProxyA11yServiceConnections.size() == 0) {

                return;

            }

            final VirtualDeviceManagerInternal localVdm = getLocalVdm();

            if  (localVdm == null) {

                return;

            }

            final ArraySet<Integer> deviceIdsToUpdate = new ArraySet<>();

            for (int i = 0; i < mProxyA11yServiceConnections.size(); i++) {

                final ProxyAccessibilityServiceConnection proxy =

                        mProxyA11yServiceConnections.valueAt(i);

                if (proxy != null) {

                    final int proxyDeviceId = proxy.getDeviceId();

                    for (Integer uid : allRunningUids) {

                        if (localVdm.getDeviceIdsForUid(uid).contains(proxyDeviceId)) {

                            deviceIdsToUpdate.add(proxyDeviceId);

                        }

                    }

                }

            }

            for (Integer proxyDeviceId : deviceIdsToUpdate) {

                onProxyChanged(proxyDeviceId, true);

            }

        }

    }



    /**

     * Clears all proxy caches.
@@ -843,6 +930,11 @@ public class ProxyManager { 
        return mLocalVdm;

    }



    @VisibleForTesting

    void setLocalVirtualDeviceManager(VirtualDeviceManagerInternal localVdm) {

        mLocalVdm = localVdm;

    }



    /**

     * Prints information belonging to each display that is controlled by an

     * AccessibilityDisplayProxy.

services/tests/servicestests/src/com/android/server/accessibility/ProxyManagerTest.java

+51 −10
Original line number Diff line number Diff line
@@ -23,7 +23,14 @@ import static com.android.server.accessibility.ProxyAccessibilityServiceConnecti 
import static com.android.server.accessibility.ProxyManager.PROXY_COMPONENT_CLASS_NAME;

import static com.android.server.accessibility.ProxyManager.PROXY_COMPONENT_PACKAGE_NAME;



import static com.google.common.truth.Truth.assertThat;



import static org.junit.Assert.fail;

import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyInt;

import static org.mockito.Mockito.never;

import static org.mockito.Mockito.verify;

import static org.mockito.Mockito.when;



import android.accessibilityservice.AccessibilityGestureEvent;

import android.accessibilityservice.AccessibilityServiceInfo;
@@ -40,6 +47,7 @@ import android.graphics.Region; 
import android.os.IBinder;

import android.os.RemoteCallbackList;

import android.os.RemoteException;

import android.platform.test.flag.junit.SetFlagsRule;

import android.util.ArraySet;

import android.view.KeyEvent;

import android.view.MotionEvent;
@@ -48,6 +56,8 @@ import android.view.accessibility.AccessibilityManager; 
import android.view.accessibility.IAccessibilityManagerClient;

import android.view.inputmethod.EditorInfo;



import androidx.test.InstrumentationRegistry;



import com.android.internal.R;

import com.android.internal.inputmethod.IAccessibilityInputMethodSession;

import com.android.internal.inputmethod.IAccessibilityInputMethodSessionCallback;
@@ -58,20 +68,12 @@ import com.android.server.accessibility.test.MessageCapturingHandler; 
import com.android.server.companion.virtual.VirtualDeviceManagerInternal;

import com.android.server.wm.WindowManagerInternal;



import static com.google.common.truth.Truth.assertThat;



import androidx.test.InstrumentationRegistry;



import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyInt;

import static org.mockito.Mockito.never;

import static org.mockito.Mockito.verify;

import static org.mockito.Mockito.when;



import org.junit.After;

import org.junit.Before;

import org.junit.Rule;

import org.junit.Test;

import org.mockito.Mock;

import org.mockito.Mockito;

import org.mockito.MockitoAnnotations;



import java.util.ArrayList;
@@ -87,6 +89,10 @@ public class ProxyManagerTest { 
    private static final int DEVICE_ID = 10;

    private static final int STREAMED_CALLING_UID = 9876;



    @Rule

    public final SetFlagsRule mSetFlagsRule = new SetFlagsRule();





    @Mock private Context mMockContext;

    @Mock private AccessibilitySecurityPolicy mMockSecurityPolicy;

    @Mock private AccessibilityWindowManager mMockA11yWindowManager;
@@ -110,6 +116,8 @@ public class ProxyManagerTest { 
        MockitoAnnotations.initMocks(this);

        final Resources resources = InstrumentationRegistry.getContext().getResources();



        mSetFlagsRule.enableFlags(Flags.FLAG_PROXY_USE_APPS_ON_VIRTUAL_DEVICE_LISTENER);



        mFocusStrokeWidthDefaultValue =

                resources.getDimensionPixelSize(R.dimen.accessibility_focus_highlight_stroke_width);

        mFocusColorDefaultValue = resources.getColor(R.color.accessibility_focus_highlight_color);
@@ -217,6 +225,39 @@ public class ProxyManagerTest { 
        verify(mMockProxySystemSupport).notifyClearAccessibilityCacheLocked();

    }



    /**

     * Tests that the manager's AppsOnVirtualDeviceListener implementation propagates the running

     * app changes to the proxy device.

     */

    @Test

    public void testUpdateProxyOfRunningAppsChange_changedUidIsStreamedApp_propagatesChange() {

        final VirtualDeviceManagerInternal localVdm =

                Mockito.mock(VirtualDeviceManagerInternal.class);

        when(localVdm.getDeviceIdsForUid(anyInt())).thenReturn(new ArraySet(Set.of(DEVICE_ID)));



        mProxyManager.setLocalVirtualDeviceManager(localVdm);

        registerProxy(DISPLAY_ID);

        verify(localVdm).registerAppsOnVirtualDeviceListener(any());



        final ArraySet<Integer> runningUids = new ArraySet(Set.of(STREAMED_CALLING_UID));



        // Flush any existing messages. The messages after this come from onProxyChanged.

        mMessageCapturingHandler.sendAllMessages();



        // The virtual device has been updated with the streamed app's UID, so the proxy is

        // updated.

        mProxyManager.notifyProxyOfRunningAppsChange(runningUids);



        verify(localVdm).getDeviceIdsForUid(STREAMED_CALLING_UID);

        verify(mMockProxySystemSupport).getCurrentUserClientsLocked();

        verify(mMockProxySystemSupport).getGlobalClientsLocked();

        // Messages to notify IAccessibilityManagerClients should be posted.

        assertThat(mMessageCapturingHandler.hasMessages()).isTrue();



        mProxyManager.unregisterProxy(DISPLAY_ID);

        verify(localVdm).unregisterAppsOnVirtualDeviceListener(any());

    }



    /**

     * Tests that getting the first device id for an app uid, such as when an app queries for

     * device-specific state, returns the right device id.