Fix HFP client JNI NPE and concurrency issues in JNI and service
1.) There is a JNI NPE during shutdown of HeadsetClientService. In JNI,
{@code mCallbacksObj} is shared between {@code cleanupNative()} and
native upcalls, e.g., {@code connection_state_cb}. To protect against
this, a null check for {@code mCallbacksObj} is added to each of the
upcalls, and a mutex for {@code mCallbacksObj} is also added. For good
measure, a separate mutex was also added to guard {@code
sBluetoothHfpClientInterface}. This is the approach in CL 405014,
which solved the same problem for the phone-side/AG of HFP.
2.) The mutex in the JNI guarding {@code mCallbacksObj} (c.f. #1)
introduces a deadlock scenario in the java code. In
HeadsetClientService, {@code stop} (which calls {@code cleanupNative} in
the JNI) and {@code getStateMachine} (which is called by {@code
messageFromNative}, which is called by various native upcalls, e.g.,
{@code connection_state_cb}) are synchronized methods. This CL removes
the synchronized label from the methods and instead guard on the HashMap
of state machines. This is the approach taken on the phone-side/AG of
HFP (c.f. CL 548206). This CL also adds some additional guards around
{@code sHeadsetClientService}.
3. Guarding {@code mStateMachineMap} with itself (c.f. #2) should also
resolve a {@code ConcurrentModificationException} in the
BroadcastReceiver of HeadsetClientService.
4. {@code getStateMachine} currently creates a new state machine if one
doesn't already exist in the map (i.e., as long as a {@code device} is
non-null and the maximum number of state machines hasn't been exceeded).
However, the only times a new state machine should be created and added
is when a device connects. This CL adds checks to make it so.
5. When a state machine is allocated, this CL adds a check to verify the
service is non-null, i.e., the service has not begun stopping or has
completed startup. This protects against situations such as: {@code
stop} is called and starts removing state machines. At the same time,
something else calls {@code allocateStateMachine}, which creates and
adds one to the map -- after the service shut down and cleared out the
state machines. {@code NativeInterface} provides some protection already
(i.e., they check the service is non-null before calling {@code
messageFromNative}, which is one of the potential paths to calling
{@code allocateStateMachine}, but adding this null-check directly inside
of {@code allocateStateMachine} should make things more robust.
Tag: #stability
Bug: 177882643
Bug: 193867157
Test: atest BluetoothInstrumentationTests
Change-Id: Ia6b447764f0c11b579b82cb7ab87a083ab0ac274
2 files changed
tree: 3d443b4044b37f851dc4c087e5acbeac52947f23
- android/