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

Commit 15ea8e72 authored by William Escande's avatar William Escande
Browse files

Adapter: prevent race condition around sServiceLock 

Since get_profile_use_lock, the onServiceConnected callback is running
on the binder thread instead of bloating the main thread (use to block
main app thread for ~200 ms which is not great).

But because it doesn't post the event, it tries to complete its
execution while holding some other lock, including sServiceLock during
the onBluetoothOn.

This lock is also use when getting the adapterService from
BluetoothDevice.
Prior to this change, both call are mutually exclusive, but both are
only trying to access to the service value in read mode and they
shouldn't be blocked.

In an optimal world, no lock would be necessary, and BluetoothDevice
wouldn't need to fetch a static adapterService binder. But this is a too
big refactor, using a read/write lock is simple enough for this issue
and doesn't change the behavior that much

Bug: 373533897
Bug: 370815283
Flag: com.android.bluetooth.flags.get_profile_use_lock
Test: m Bluetooth -- There is no race condition test written for Bt
Change-Id: Id9bb4aea1a9f1ee4f282cff9fde703646e886585
parent 9415ddaa

framework/java/android/bluetooth/BluetoothAdapter.java

+109 −3
Original line number Diff line number Diff line
@@ -871,6 +871,7 @@ public final class BluetoothAdapter { 
    @GuardedBy("mServiceLock")

    private IBluetooth mService;



    private static final ReentrantReadWriteLock sServiceLock = new ReentrantReadWriteLock();

    private final ReentrantReadWriteLock mServiceLock = new ReentrantReadWriteLock();



    @GuardedBy("sServiceLock")
@@ -879,8 +880,6 @@ public final class BluetoothAdapter { 
    @GuardedBy("sServiceLock")

    private static IBluetooth sService;



    private static final Object sServiceLock = new Object();



    private final Object mLock = new Object();

    private final Map<LeScanCallback, ScanCallback> mLeScanClients = new HashMap<>();

    private final Map<BluetoothDevice, List<Pair<OnMetadataChangedListener, Executor>>>
@@ -3698,12 +3697,32 @@ public final class BluetoothAdapter { 


    private static final IBluetoothManagerCallback sManagerCallback =

            new IBluetoothManagerCallback.Stub() {

                private void onBluetoothServiceUpFlagged(IBinder bluetoothService) {

                    sServiceLock.writeLock().lock();

                    try {

                        sService = IBluetooth.Stub.asInterface(bluetoothService);

                        for (IBluetoothManagerCallback cb : sProxyServiceStateCallbacks.keySet()) {

                            try {

                                cb.onBluetoothServiceUp(bluetoothService);

                            } catch (RemoteException e) {

                                Log.e(TAG, "", e);

                            }

                        }

                    } finally {

                        sServiceLock.writeLock().unlock();

                    }

                }



                @RequiresNoPermission

                public void onBluetoothServiceUp(IBinder bluetoothService) {

                    if (DBG) {

                        Log.d(TAG, "onBluetoothServiceUp: " + bluetoothService);

                    }



                    if (Flags.getProfileUseLock()) {

                        onBluetoothServiceUpFlagged(bluetoothService);

                        return;

                    }

                    synchronized (sServiceLock) {

                        sService = IBluetooth.Stub.asInterface(bluetoothService);

                        for (IBluetoothManagerCallback cb : sProxyServiceStateCallbacks.keySet()) {
@@ -3720,12 +3739,33 @@ public final class BluetoothAdapter { 
                    }

                }



                private void onBluetoothServiceDownFlagged() {

                    sServiceLock.writeLock().lock();

                    try {

                        sService = null;

                        for (IBluetoothManagerCallback cb : sProxyServiceStateCallbacks.keySet()) {

                            try {

                                cb.onBluetoothServiceDown();

                            } catch (RemoteException e) {

                                Log.e(TAG, "", e);

                            }

                        }

                    } finally {

                        sServiceLock.writeLock().unlock();

                    }

                }



                @RequiresNoPermission

                public void onBluetoothServiceDown() {

                    if (DBG) {

                        Log.d(TAG, "onBluetoothServiceDown");

                    }



                    if (Flags.getProfileUseLock()) {

                        onBluetoothServiceDownFlagged();

                        return;

                    }



                    synchronized (sServiceLock) {

                        sService = null;

                        for (IBluetoothManagerCallback cb : sProxyServiceStateCallbacks.keySet()) {
@@ -3742,12 +3782,31 @@ public final class BluetoothAdapter { 
                    }

                }



                private void onBluetoothOnFlagged() {

                    sServiceLock.readLock().lock();

                    try {

                        for (IBluetoothManagerCallback cb : sProxyServiceStateCallbacks.keySet()) {

                            try {

                                cb.onBluetoothOn();

                            } catch (RemoteException e) {

                                Log.e(TAG, "", e);

                            }

                        }

                    } finally {

                        sServiceLock.readLock().unlock();

                    }

                }



                @RequiresNoPermission

                public void onBluetoothOn() {

                    if (DBG) {

                        Log.d(TAG, "onBluetoothOn");

                    }



                    if (Flags.getProfileUseLock()) {

                        onBluetoothOnFlagged();

                        return;

                    }

                    synchronized (sServiceLock) {

                        for (IBluetoothManagerCallback cb : sProxyServiceStateCallbacks.keySet()) {

                            try {
@@ -3763,12 +3822,31 @@ public final class BluetoothAdapter { 
                    }

                }



                private void onBluetoothOffFlagged() {

                    sServiceLock.readLock().lock();

                    try {

                        for (IBluetoothManagerCallback cb : sProxyServiceStateCallbacks.keySet()) {

                            try {

                                cb.onBluetoothOff();

                            } catch (RemoteException e) {

                                Log.e(TAG, "", e);

                            }

                        }

                    } finally {

                        sServiceLock.readLock().unlock();

                    }

                }



                @RequiresNoPermission

                public void onBluetoothOff() {

                    if (DBG) {

                        Log.d(TAG, "onBluetoothOff");

                    }



                    if (Flags.getProfileUseLock()) {

                        onBluetoothOffFlagged();

                        return;

                    }

                    synchronized (sServiceLock) {

                        for (IBluetoothManagerCallback cb : sProxyServiceStateCallbacks.keySet()) {

                            try {
@@ -4120,6 +4198,14 @@ public final class BluetoothAdapter { 
            new WeakHashMap<>();



    /*package*/ IBluetooth getBluetoothService() {

        if (Flags.getProfileUseLock()) {

            sServiceLock.readLock().lock();

            try {

                return sService;

            } finally {

                sServiceLock.readLock().unlock();

            }

        }

        synchronized (sServiceLock) {

            return sService;

        }
@@ -4135,6 +4221,16 @@ public final class BluetoothAdapter { 
     */

    IBluetooth getBluetoothService(IBluetoothManagerCallback cb) {

        requireNonNull(cb);

        if (Flags.getProfileUseLock()) {

            sServiceLock.writeLock().lock();

            try {

                sProxyServiceStateCallbacks.put(cb, null);

                registerOrUnregisterAdapterLocked();

                return sService;

            } finally {

                sServiceLock.writeLock().unlock();

            }

        }

        synchronized (sServiceLock) {

            sProxyServiceStateCallbacks.put(cb, null);

            registerOrUnregisterAdapterLocked();
@@ -4200,6 +4296,16 @@ public final class BluetoothAdapter { 


    /*package*/ void removeServiceStateCallback(IBluetoothManagerCallback cb) {

        requireNonNull(cb);

        if (Flags.getProfileUseLock()) {

            sServiceLock.writeLock().lock();

            try {

                sProxyServiceStateCallbacks.remove(cb);

                registerOrUnregisterAdapterLocked();

            } finally {

                sServiceLock.writeLock().unlock();

            }

            return;

        }

        synchronized (sServiceLock) {

            sProxyServiceStateCallbacks.remove(cb);

            registerOrUnregisterAdapterLocked();
@@ -4210,7 +4316,7 @@ public final class BluetoothAdapter { 
     * Handle registering (or unregistering) a single process-wide {@link IBluetoothManagerCallback}

     * based on the presence of local {@link #sProxyServiceStateCallbacks} clients.

     */

    @GuardedBy("sServiceLock")

    @GuardedBy("sServiceLock") // with write lock

    private void registerOrUnregisterAdapterLocked() {

        final boolean isRegistered = sServiceRegistered;

        final boolean wantRegistered = !sProxyServiceStateCallbacks.isEmpty();