Fix unregisterListener bug in CarPropertyService.
There is a bug in CarPropertyService that might cause property
unsubscribed from PropertyHalService when unregisterListener
is called even when there are other listeners for the property.
This bug is due to using upateMaxRate == 0 to check whether there
are other clients. However, sampleRate might be 0 for on_change
event so even if updateMaxRate is 0, it doesn't mean the client
array is empty.
Test: Add unit test in CarPropertyServiceTest
Bug: 229327788
Change-Id: Iff5d18a2f5d05ea9be9b8af28908026306a7a99e
diff --git a/service/src/com/android/car/CarPropertyService.java b/service/src/com/android/car/CarPropertyService.java
index 90e404c..df11ba9 100644
--- a/service/src/com/android/car/CarPropertyService.java
+++ b/service/src/com/android/car/CarPropertyService.java
@@ -54,7 +54,7 @@
*/
public class CarPropertyService extends ICarProperty.Stub
implements CarServiceBase, PropertyHalService.PropertyHalListener {
- private static final boolean DBG = true;
+ private static final boolean DBG = false;
private static final String TAG = CarLog.tagFor(CarPropertyService.class);
private final Context mContext;
private final PropertyHalService mHal;
@@ -349,36 +349,35 @@
if ((client == null) || (propertyClients == null)) {
Slogf.e(TAG, "unregisterListenerBinderLocked: Listener was not previously "
+ "registered.");
- } else {
- if (propertyClients.remove(client)) {
- int propLeft = client.removeProperty(propId);
- if (propLeft == 0) {
- mClientMap.remove(listenerBinder);
- }
- clearSetOperationRecorderLocked(propId, client);
-
- } else {
- Slogf.e(TAG, "unregisterListenerBinderLocked: Listener was not registered for "
- + "propId=0x" + toHexString(propId));
- }
-
- if (propertyClients.isEmpty()) {
- // Last listener for this property unsubscribed. Clean up
- mPropIdClientMap.remove(propId);
- mSetOperationClientMap.remove(propId);
- } else {
- // Other listeners are still subscribed. Calculate the new rate
- for (int i = 0; i < propertyClients.size(); i++) {
- Client c = propertyClients.get(i);
- float rate = c.getRate(propId);
- updateMaxRate = Math.max(rate, updateMaxRate);
- }
- }
+ return;
}
- if (Float.compare(updateMaxRate, 0f) == 0) {
- // Unsubscribe property if we did not find any other client register to this property
+ if (propertyClients.remove(client)) {
+ int propLeft = client.removeProperty(propId);
+ if (propLeft == 0) {
+ mClientMap.remove(listenerBinder);
+ }
+ clearSetOperationRecorderLocked(propId, client);
+
+ } else {
+ Slogf.e(TAG, "unregisterListenerBinderLocked: Listener was not registered for "
+ + "propId=0x" + toHexString(propId));
+ return;
+ }
+
+ if (propertyClients.isEmpty()) {
+ // Last listener for this property unsubscribed. Clean up
+ mPropIdClientMap.remove(propId);
+ mSetOperationClientMap.remove(propId);
mHal.unsubscribeProperty(propId);
- } else if (Float.compare(updateMaxRate, mHal.getSampleRate(propId)) != 0) {
+ return;
+ }
+ // Other listeners are still subscribed. Calculate the new rate
+ for (int i = 0; i < propertyClients.size(); i++) {
+ Client c = propertyClients.get(i);
+ float rate = c.getRate(propId);
+ updateMaxRate = Math.max(rate, updateMaxRate);
+ }
+ if (Float.compare(updateMaxRate, mHal.getSampleRate(propId)) != 0) {
try {
// Only reset the sample rate if needed
mHal.subscribeProperty(propId, updateMaxRate);
diff --git a/tests/carservice_unit_test/src/com/android/car/CarPropertyServiceUnitTest.java b/tests/carservice_unit_test/src/com/android/car/CarPropertyServiceUnitTest.java
new file mode 100644
index 0000000..a60201f
--- /dev/null
+++ b/tests/carservice_unit_test/src/com/android/car/CarPropertyServiceUnitTest.java
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.car;
+
+import static android.car.hardware.property.CarPropertyManager.SENSOR_RATE_ONCHANGE;
+
+import static org.junit.Assert.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyFloat;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.Mockito.clearInvocations;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.timeout;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import android.car.VehicleAreaType;
+import android.car.VehiclePropertyIds;
+import android.car.hardware.CarPropertyConfig;
+import android.car.hardware.CarPropertyValue;
+import android.car.hardware.property.ICarPropertyEventListener;
+import android.content.Context;
+import android.content.pm.PackageManager;
+import android.os.IBinder;
+import android.util.SparseArray;
+
+import com.android.car.hal.PropertyHalService;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public final class CarPropertyServiceUnitTest {
+
+ @Mock
+ private Context mContext;
+ @Mock
+ private PropertyHalService mHalService;
+
+ private String mReadPermission;
+ private CarPropertyService mService;
+
+ private static final int SPEED_ID = VehiclePropertyIds.PERF_VEHICLE_SPEED;
+ private static final int HVAC_TEMP = VehiclePropertyIds.HVAC_TEMPERATURE_SET;
+
+ @Before
+ public void setUp() {
+ mReadPermission = "READ_PERMISSION";
+
+ when(mContext.checkCallingOrSelfPermission(mReadPermission)).thenReturn(
+ PackageManager.PERMISSION_GRANTED);
+
+ SparseArray<CarPropertyConfig<?>> configs = new SparseArray<>();
+ configs.put(SPEED_ID, CarPropertyConfig.newBuilder(
+ Float.class, SPEED_ID,
+ VehicleAreaType.VEHICLE_AREA_TYPE_GLOBAL).build());
+ when(mHalService.getReadPermission(SPEED_ID)).thenReturn(mReadPermission);
+ // HVAC_TEMP is actually not a global property, but for simplicity, make it global here.
+ configs.put(HVAC_TEMP, CarPropertyConfig.newBuilder(
+ Float.class, HVAC_TEMP,
+ VehicleAreaType.VEHICLE_AREA_TYPE_GLOBAL).build());
+ when(mHalService.getReadPermission(HVAC_TEMP)).thenReturn(mReadPermission);
+ when(mHalService.getPropertyList()).thenReturn(configs);
+
+ mService = new CarPropertyService(mContext, mHalService);
+ mService.init();
+ }
+
+ @Test
+ public void testRegisterListenerNull() {
+ assertThrows(IllegalArgumentException.class,
+ () -> mService.registerListener(SPEED_ID, /* rate=*/ 10, /* listener= */ null));
+ }
+
+ @Test
+ public void testRegisterUnregisterForContinuousProperty() throws Exception {
+ ICarPropertyEventListener mMockHandler1 = mock(ICarPropertyEventListener.class);
+ ICarPropertyEventListener mMockHandler2 = mock(ICarPropertyEventListener.class);
+ // Must use two different binders because listener is uniquely identified by binder.
+ IBinder mBinder1 = mock(IBinder.class);
+ IBinder mBinder2 = mock(IBinder.class);
+ when(mMockHandler1.asBinder()).thenReturn(mBinder1);
+ when(mMockHandler2.asBinder()).thenReturn(mBinder2);
+ // Initially SPEED_ID is not subscribed, so should return -1.
+ when(mHalService.getSampleRate(SPEED_ID)).thenReturn(-1f);
+ CarPropertyValue mValue = mock(CarPropertyValue.class);
+ when(mHalService.getPropertySafe(SPEED_ID, 0)).thenReturn(mValue);
+
+ // Register the first listener.
+ mService.registerListener(SPEED_ID, /* rate= */ 10, mMockHandler1);
+
+ // Wait until we get the on property change event for the initial value.
+ verify(mMockHandler1, timeout(5000)).onEvent(any());
+
+ verify(mHalService).subscribeProperty(SPEED_ID, 10f);
+ verify(mHalService).getPropertySafe(SPEED_ID, 0);
+
+ // Clean up invocation state.
+ clearInvocations(mHalService);
+ when(mHalService.getSampleRate(SPEED_ID)).thenReturn(10f);
+
+ // Register the second listener.
+ mService.registerListener(SPEED_ID, /* rate= */ 20, mMockHandler2);
+
+ // Wait until we get the on property change event for the initial value.
+ verify(mMockHandler2, timeout(5000)).onEvent(any());
+
+ verify(mHalService).subscribeProperty(SPEED_ID, 20f);
+ verify(mHalService).getPropertySafe(SPEED_ID, 0);
+
+ // Clean up invocation state.
+ clearInvocations(mHalService);
+ when(mHalService.getSampleRate(SPEED_ID)).thenReturn(20f);
+
+ // Unregister the second listener, the first listener must still be registered.
+ mService.unregisterListener(SPEED_ID, mMockHandler2);
+
+ // The property must not be unsubscribed.
+ verify(mHalService, never()).unsubscribeProperty(anyInt());
+ // The subscription rate must be updated.
+ verify(mHalService).subscribeProperty(SPEED_ID, 10f);
+ when(mHalService.getSampleRate(SPEED_ID)).thenReturn(10f);
+
+ // Unregister the first listener. We have no more listeners, must cause unsubscription.
+ mService.unregisterListener(SPEED_ID, mMockHandler1);
+
+ verify(mHalService).unsubscribeProperty(SPEED_ID);
+ }
+
+ @Test
+ public void testRegisterUnregisterForOnChangeProperty() throws Exception {
+ ICarPropertyEventListener mMockHandler1 = mock(ICarPropertyEventListener.class);
+ ICarPropertyEventListener mMockHandler2 = mock(ICarPropertyEventListener.class);
+ // Must use two different binders because listener is uniquely identified by binder.
+ IBinder mBinder1 = mock(IBinder.class);
+ IBinder mBinder2 = mock(IBinder.class);
+ when(mMockHandler1.asBinder()).thenReturn(mBinder1);
+ when(mMockHandler2.asBinder()).thenReturn(mBinder2);
+ // Initially HVAC_TEMP is not subscribed, so should return -1.
+ when(mHalService.getSampleRate(HVAC_TEMP)).thenReturn(-1f);
+ CarPropertyValue mValue = mock(CarPropertyValue.class);
+ when(mHalService.getPropertySafe(HVAC_TEMP, 0)).thenReturn(mValue);
+
+ // Register the first listener.
+ mService.registerListener(HVAC_TEMP, /* rate= */ SENSOR_RATE_ONCHANGE, mMockHandler1);
+
+ // Wait until we get the on property change event for the initial value.
+ verify(mMockHandler1, timeout(5000)).onEvent(any());
+
+ verify(mHalService).subscribeProperty(HVAC_TEMP, 0f);
+ verify(mHalService).getPropertySafe(HVAC_TEMP, 0);
+
+ // Clean up invocation state.
+ clearInvocations(mHalService);
+ when(mHalService.getSampleRate(HVAC_TEMP)).thenReturn(0f);
+
+ // Register the second listener.
+ mService.registerListener(HVAC_TEMP, /* rate= */ SENSOR_RATE_ONCHANGE, mMockHandler2);
+
+ // Wait until we get the on property change event for the initial value.
+ verify(mMockHandler2, timeout(5000)).onEvent(any());
+
+ // Must not subscribe again.
+ verify(mHalService, never()).subscribeProperty(anyInt(), anyFloat());
+ verify(mHalService).getPropertySafe(HVAC_TEMP, 0);
+
+ // Clean up invocation state.
+ clearInvocations(mHalService);
+
+ // Unregister the second listener, the first listener must still be registered.
+ mService.unregisterListener(HVAC_TEMP, mMockHandler2);
+
+ // The property must not be unsubscribed.
+ verify(mHalService, never()).unsubscribeProperty(anyInt());
+
+ // Unregister the first listener. We have no more listeners, must cause unsubscription.
+ mService.unregisterListener(HVAC_TEMP, mMockHandler1);
+
+ verify(mHalService).unsubscribeProperty(HVAC_TEMP);
+ }
+}
