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

Commit 782ba846 authored by Sal Savage's avatar Sal Savage
Browse files

Always quit state machines once removing them 

When removing state machines from our maps and dropping their
references, there's a chance we leak their underlying threads if the
state machines are not quit.

This change adds doQuit() or quitNow() calls in places where we remove
state machines and drop their references.

Tag: #stability
Bug: 293920022
Test: atest BluetoothInstrumentationTests
Change-Id: I1ff69590a204eb7e92619d0ed6584ba2c27b1b3d
parent b9f33066

android/app/src/com/android/bluetooth/a2dpsink/A2dpSinkService.java

+12 −2
Original line number Diff line number Diff line
@@ -434,7 +434,11 @@ public class A2dpSinkService extends ProfileService { 
     */

    @VisibleForTesting

    public void removeStateMachine(A2dpSinkStateMachine stateMachine) {

        if (stateMachine == null) {

            return;

        }

        mDeviceStateMap.remove(stateMachine.getDevice());

        stateMachine.quitNow();

    }



    public List<BluetoothDevice> getConnectedDevices() {
@@ -447,10 +451,16 @@ public class A2dpSinkService extends ProfileService { 
        A2dpSinkStateMachine existingStateMachine =

                mDeviceStateMap.putIfAbsent(device, newStateMachine);

        // Given null is not a valid value in our map, ConcurrentHashMap will return null if the

        // key was absent and our new value was added. We should then start and return it.

        // key was absent and our new value was added. We should then start and return it. Else

        // we quit the new one so we don't leak a thread

        if (existingStateMachine == null) {

            newStateMachine.start();

            return newStateMachine;

        } else {

            // If you try to quit a StateMachine that hasn't been constructed yet, the StateMachine

            // spits out an NPE trying to read a state stack array that only gets made on start().

            // We can just quit the thread made explicitly

            newStateMachine.getHandler().getLooper().quit();

        }

        return existingStateMachine;

    }
@@ -621,7 +631,7 @@ public class A2dpSinkService extends ProfileService { 
        if (device == null) {

            return;

        }

        A2dpSinkStateMachine stateMachine = getOrCreateStateMachine(device);

        A2dpSinkStateMachine stateMachine = getStateMachineForDevice(device);

        stateMachine.sendMessage(A2dpSinkStateMachine.STACK_EVENT, event);

    }

}

android/app/src/com/android/bluetooth/a2dpsink/A2dpSinkStateMachine.java

+1 −1
Original line number Diff line number Diff line
@@ -36,7 +36,7 @@ import com.android.internal.util.StateMachine; 




public class A2dpSinkStateMachine extends StateMachine {

    static final String TAG = "A2DPSinkStateMachine";

    static final String TAG = "A2dpSinkStateMachine";

    static final boolean DBG = Log.isLoggable(TAG, Log.DEBUG);



    //0->99 Events from Outside

android/app/src/com/android/bluetooth/avrcpcontroller/AvrcpControllerService.java

+20 −10
Original line number Diff line number Diff line
@@ -275,10 +275,6 @@ public class AvrcpControllerService extends ProfileService { 
        return playbackState;

    }



    protected AvrcpControllerStateMachine newStateMachine(BluetoothDevice device) {

        return new AvrcpControllerStateMachine(device, this);

    }



    protected void getCurrentMetadataIfNoCoverArt(BluetoothDevice device) {

        if (device == null) return;

        AvrcpControllerStateMachine stateMachine = getStateMachine(device);
@@ -943,11 +939,15 @@ public class AvrcpControllerService extends ProfileService { 
     * Remove state machine from device map once it is no longer needed.

     */

    public void removeStateMachine(AvrcpControllerStateMachine stateMachine) {

        if (stateMachine == null) {

            return;

        }

        BluetoothDevice device = stateMachine.getDevice();

        if (device.equals(getActiveDevice())) {

            setActiveDevice(null);

        }

        mDeviceStateMap.remove(stateMachine.getDevice());

        stateMachine.quitNow();

    }



    public List<BluetoothDevice> getConnectedDevices() {
@@ -962,13 +962,23 @@ public class AvrcpControllerService extends ProfileService { 
    }



    protected AvrcpControllerStateMachine getOrCreateStateMachine(BluetoothDevice device) {

        AvrcpControllerStateMachine stateMachine = mDeviceStateMap.get(device);

        if (stateMachine == null) {

            stateMachine = newStateMachine(device);

            mDeviceStateMap.put(device, stateMachine);

            stateMachine.start();

        AvrcpControllerStateMachine newStateMachine =

                new AvrcpControllerStateMachine(device, this);

        AvrcpControllerStateMachine existingStateMachine =

                mDeviceStateMap.putIfAbsent(device, newStateMachine);

        // Given null is not a valid value in our map, ConcurrentHashMap will return null if the

        // key was absent and our new value was added. We should then start and return it. Else

        // we quit the new one so we don't leak a thread

        if (existingStateMachine == null) {

            newStateMachine.start();

            return newStateMachine;

        } else {

            // If you try to quit a StateMachine that hasn't been constructed yet, the StateMachine

            // spits out an NPE trying to read a state stack array that only gets made on start().

            // We can just quit the thread made explicitly

            newStateMachine.getHandler().getLooper().quit();

        }

        return stateMachine;

        return existingStateMachine;

    }



    protected AvrcpCoverArtManager getCoverArtManager() {

android/app/src/com/android/bluetooth/mapclient/MapClientService.java

+3 −0
Original line number Diff line number Diff line
@@ -155,6 +155,8 @@ public class MapClientService extends ProfileService { 
            Log.d(TAG, "Statemachine exists for a device in unexpected state: " + state);

        }

        mMapInstanceMap.remove(device);

        mapStateMachine.doQuit();



        addDeviceToMapAndConnect(device);

        if (DBG) {

            StringBuilder sb = new StringBuilder();
@@ -389,6 +391,7 @@ public class MapClientService extends ProfileService { 
            MceStateMachine stateMachine = mMapInstanceMap.get(device);

            if (stateMachine != null) {

                mMapInstanceMap.remove(device);

                stateMachine.doQuit();

            }

        }

        if (DBG) {

android/app/src/com/android/bluetooth/pbapclient/PbapClientService.java

+1 −0
Original line number Diff line number Diff line
@@ -179,6 +179,7 @@ public class PbapClientService extends ProfileService { 
            PbapClientStateMachine pbapClientStateMachine = mPbapClientStateMachineMap.get(device);

            if (pbapClientStateMachine != null) {

                mPbapClientStateMachineMap.remove(device);

                pbapClientStateMachine.doQuit();

            }

        }

    }