Refactor locks in radio client for AIDL HAL

Seperate locks were used for radio service implementation, radio
module, and tuner session for broadcast radio AIDL HAL client to
minimize performance issue. Potential dead lock issue in
TunerSession#close was also fixed.

Bug: 258688572
Test: atest com.android.server.broadcastradio.aidl
Test: atest android.broadcastradio.cts
Change-Id: I8de3afa2935e077488e7575bb9b64495c2113292

6 files changed, 39 insertions, 50 deletions

diff --git a/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/BroadcastRadioServiceImplTest.java b/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/BroadcastRadioServiceImplTest.java
index 36aa915a67ec..1cc0a98512bf 100644
--- a/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/BroadcastRadioServiceImplTest.java
+++ b/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/BroadcastRadioServiceImplTest.java
@@ -210,9 +210,9 @@ public final class BroadcastRadioServiceImplTest extends ExtendedRadioMockitoTes
                 any(IServiceCallback.class)));
 
         doReturn(mFmRadioModuleMock).when(() -> RadioModule.tryLoadingModule(
-                eq(FM_RADIO_MODULE_ID), anyString(), any(IBinder.class), any(Object.class)));
+                eq(FM_RADIO_MODULE_ID), anyString(), any(IBinder.class)));
         doReturn(mDabRadioModuleMock).when(() -> RadioModule.tryLoadingModule(
-                eq(DAB_RADIO_MODULE_ID), anyString(), any(IBinder.class), any(Object.class)));
+                eq(DAB_RADIO_MODULE_ID), anyString(), any(IBinder.class)));
 
         when(mFmRadioModuleMock.getProperties()).thenReturn(mFmModuleMock);
         when(mDabRadioModuleMock.getProperties()).thenReturn(mDabModuleMock);
diff --git a/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/RadioModuleTest.java b/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/RadioModuleTest.java
index 7a8475fe4d8f..a0346538ddc5 100644
--- a/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/RadioModuleTest.java
+++ b/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/RadioModuleTest.java
@@ -58,14 +58,13 @@ public final class RadioModuleTest {
     @Mock
     private android.hardware.broadcastradio.ICloseHandle mHalCloseHandleMock;
 
-    private final Object mLock = new Object();
     // RadioModule under test
     private RadioModule mRadioModule;
     private android.hardware.broadcastradio.IAnnouncementListener mHalListener;
 
     @Before
     public void setup() throws RemoteException {
-        mRadioModule = new RadioModule(mBroadcastRadioMock, TEST_MODULE_PROPERTIES, mLock);
+        mRadioModule = new RadioModule(mBroadcastRadioMock, TEST_MODULE_PROPERTIES);
 
         // TODO(b/241118988): test non-null image for getImage method
         when(mBroadcastRadioMock.getImage(anyInt())).thenReturn(null);
diff --git a/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/TunerSessionTest.java b/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/TunerSessionTest.java
index 87d0ea473665..993ca7728374 100644
--- a/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/TunerSessionTest.java
+++ b/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/aidl/TunerSessionTest.java
@@ -83,7 +83,6 @@ public final class TunerSessionTest extends ExtendedRadioMockitoTestCase {
     @Mock private IBroadcastRadio mBroadcastRadioMock;
     private android.hardware.radio.ITunerCallback[] mAidlTunerCallbackMocks;
 
-    private final Object mLock = new Object();
     // RadioModule under test
     private RadioModule mRadioModule;
 
@@ -104,7 +103,7 @@ public final class TunerSessionTest extends ExtendedRadioMockitoTestCase {
         doReturn(true).when(() -> RadioServiceUserController.isCurrentOrSystemUser());
 
         mRadioModule = new RadioModule(mBroadcastRadioMock,
-                AidlTestUtils.makeDefaultModuleProperties(), mLock);
+                AidlTestUtils.makeDefaultModuleProperties());
 
         doAnswer(invocation -> {
             mHalTunerCallback = (ITunerCallback) invocation.getArguments()[0];
diff --git a/services/core/java/com/android/server/broadcastradio/aidl/BroadcastRadioServiceImpl.java b/services/core/java/com/android/server/broadcastradio/aidl/BroadcastRadioServiceImpl.java
index 1d7112133b48..03acf72725e7 100644
--- a/services/core/java/com/android/server/broadcastradio/aidl/BroadcastRadioServiceImpl.java
+++ b/services/core/java/com/android/server/broadcastradio/aidl/BroadcastRadioServiceImpl.java
@@ -77,7 +77,7 @@ public final class BroadcastRadioServiceImpl {
                 }
 
                 RadioModule radioModule =
-                        RadioModule.tryLoadingModule(moduleId, name, newBinder, mLock);
+                        RadioModule.tryLoadingModule(moduleId, name, newBinder);
                 if (radioModule == null) {
                     Slogf.w(TAG, "No module %s with id %d (HAL AIDL)", name, moduleId);
                     return;
diff --git a/services/core/java/com/android/server/broadcastradio/aidl/RadioModule.java b/services/core/java/com/android/server/broadcastradio/aidl/RadioModule.java
index eb9dafbe5281..e956a9c2038c 100644
--- a/services/core/java/com/android/server/broadcastradio/aidl/RadioModule.java
+++ b/services/core/java/com/android/server/broadcastradio/aidl/RadioModule.java
@@ -55,7 +55,7 @@ final class RadioModule {
 
     private final IBroadcastRadio mService;
 
-    private final Object mLock;
+    private final Object mLock = new Object();
     private final Handler mHandler;
     private final RadioLogger mLogger;
     private final RadioManager.ModuleProperties mProperties;
@@ -165,18 +165,15 @@ final class RadioModule {
     };
 
     @VisibleForTesting
-    RadioModule(IBroadcastRadio service,
-            RadioManager.ModuleProperties properties, Object lock) {
+    RadioModule(IBroadcastRadio service, RadioManager.ModuleProperties properties) {
         mProperties = Objects.requireNonNull(properties, "properties cannot be null");
         mService = Objects.requireNonNull(service, "service cannot be null");
-        mLock = Objects.requireNonNull(lock, "lock cannot be null");
         mHandler = new Handler(Looper.getMainLooper());
         mLogger = new RadioLogger(TAG, RADIO_EVENT_LOGGER_QUEUE_SIZE);
     }
 
     @Nullable
-    static RadioModule tryLoadingModule(int moduleId, String moduleName,
-            IBinder serviceBinder, Object lock) {
+    static RadioModule tryLoadingModule(int moduleId, String moduleName, IBinder serviceBinder) {
         try {
             Slogf.i(TAG, "Try loading module for module id = %d, module name = %s",
                     moduleId, moduleName);
@@ -206,7 +203,7 @@ final class RadioModule {
             RadioManager.ModuleProperties prop = ConversionUtils.propertiesFromHalProperties(
                     moduleId, moduleName, service.getProperties(), amfmConfig, dabConfig);
 
-            return new RadioModule(service, prop, lock);
+            return new RadioModule(service, prop);
         } catch (RemoteException ex) {
             Slogf.e(TAG, ex, "Failed to load module %s", moduleName);
             return null;
@@ -222,9 +219,7 @@ final class RadioModule {
     }
 
     void setInternalHalCallback() throws RemoteException {
-        synchronized (mLock) {
-            mService.setTunerCallback(mHalTunerCallback);
-        }
+        mService.setTunerCallback(mHalTunerCallback);
     }
 
     TunerSession openSession(android.hardware.radio.ITunerCallback userCb)
@@ -234,7 +229,7 @@ final class RadioModule {
         Boolean antennaConnected;
         RadioManager.ProgramInfo currentProgramInfo;
         synchronized (mLock) {
-            tunerSession = new TunerSession(this, mService, userCb, mLock);
+            tunerSession = new TunerSession(this, mService, userCb);
             mAidlTunerSessions.add(tunerSession);
             antennaConnected = mAntennaConnected;
             currentProgramInfo = mCurrentProgramInfo;
@@ -356,14 +351,14 @@ final class RadioModule {
             // Otherwise, update the HAL's filter, and AIDL clients will be updated when
             // mHalTunerCallback.onProgramListUpdated() is called.
             mUnionOfAidlProgramFilters = newFilter;
-            try {
-                mService.startProgramListUpdates(
-                        ConversionUtils.filterToHalProgramFilter(newFilter));
-            } catch (RuntimeException ex) {
-                throw ConversionUtils.throwOnError(ex, /* action= */ "Start Program ListUpdates");
-            } catch (RemoteException ex) {
-                Slogf.e(TAG, ex, "mHalTunerSession.startProgramListUpdates() failed");
-            }
+        }
+        try {
+            mService.startProgramListUpdates(
+                    ConversionUtils.filterToHalProgramFilter(newFilter));
+        } catch (RuntimeException ex) {
+            throw ConversionUtils.throwOnError(ex, /* action= */ "Start Program ListUpdates");
+        } catch (RemoteException ex) {
+            Slogf.e(TAG, ex, "mHalTunerSession.startProgramListUpdates() failed");
         }
     }
 
@@ -453,12 +448,10 @@ final class RadioModule {
             }
         };
 
-        synchronized (mLock) {
-            try {
-                hwCloseHandle[0] = mService.registerAnnouncementListener(hwListener, enabledList);
-            } catch (RuntimeException ex) {
-                throw ConversionUtils.throwOnError(ex, /* action= */ "AnnouncementListener");
-            }
+        try {
+            hwCloseHandle[0] = mService.registerAnnouncementListener(hwListener, enabledList);
+        } catch (RuntimeException ex) {
+            throw ConversionUtils.throwOnError(ex, /* action= */ "AnnouncementListener");
         }
 
         return new android.hardware.radio.ICloseHandle.Stub() {
@@ -478,12 +471,10 @@ final class RadioModule {
         if (id == 0) throw new IllegalArgumentException("Image ID is missing");
 
         byte[] rawImage;
-        synchronized (mLock) {
-            try {
-                rawImage = mService.getImage(id);
-            } catch (RemoteException ex) {
-                throw ex.rethrowFromSystemServer();
-            }
+        try {
+            rawImage = mService.getImage(id);
+        } catch (RemoteException ex) {
+            throw ex.rethrowFromSystemServer();
         }
 
         if (rawImage == null || rawImage.length == 0) return null;
diff --git a/services/core/java/com/android/server/broadcastradio/aidl/TunerSession.java b/services/core/java/com/android/server/broadcastradio/aidl/TunerSession.java
index d33633c435b1..1ce4044d4c51 100644
--- a/services/core/java/com/android/server/broadcastradio/aidl/TunerSession.java
+++ b/services/core/java/com/android/server/broadcastradio/aidl/TunerSession.java
@@ -42,7 +42,7 @@ final class TunerSession extends ITuner.Stub {
     private static final String TAG = "BcRadioAidlSrv.session";
     private static final int TUNER_EVENT_LOGGER_QUEUE_SIZE = 25;
 
-    private final Object mLock;
+    private final Object mLock = new Object();
 
     private final RadioLogger mLogger;
     private final RadioModule mModule;
@@ -61,12 +61,10 @@ final class TunerSession extends ITuner.Stub {
     private RadioManager.BandConfig mPlaceHolderConfig;
 
     TunerSession(RadioModule radioModule, IBroadcastRadio service,
-            android.hardware.radio.ITunerCallback callback,
-            Object lock) {
+            android.hardware.radio.ITunerCallback callback) {
         mModule = Objects.requireNonNull(radioModule, "radioModule cannot be null");
         mService = Objects.requireNonNull(service, "service cannot be null");
         mCallback = Objects.requireNonNull(callback, "callback cannot be null");
-        mLock = Objects.requireNonNull(lock, "lock cannot be null");
         mLogger = new RadioLogger(TAG, TUNER_EVENT_LOGGER_QUEUE_SIZE);
     }
 
@@ -91,17 +89,19 @@ final class TunerSession extends ITuner.Stub {
             mLogger.logRadioEvent("Close tuner session on error %d", error);
         }
         synchronized (mLock) {
-            if (mIsClosed) return;
-            if (error != null) {
-                try {
-                    mCallback.onError(error);
-                } catch (RemoteException ex) {
-                    Slogf.w(TAG, ex, "mCallback.onError(%s) failed", error);
-                }
+            if (mIsClosed) {
+                return;
             }
             mIsClosed = true;
-            mModule.onTunerSessionClosed(this);
         }
+        if (error != null) {
+            try {
+                mCallback.onError(error);
+            } catch (RemoteException ex) {
+                Slogf.w(TAG, ex, "mCallback.onError(%s) failed", error);
+            }
+        }
+        mModule.onTunerSessionClosed(this);
     }
 
     @Override