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
Loading
Please register or sign in to comment