From 8e8bc5bf8c8f609f31818d278d6ac8b8e16a9345 Mon Sep 17 00:00:00 2001 From: Keun young Park Date: Tue, 27 Jul 2021 16:36:23 -0700 Subject: [PATCH] unite all locks under BroadcastRadio - Each layer having separate a lock and making calls to other layers while holding lock leads into deadlock. - Change all of them to use a single lock from the service to avoid deadlock. - Performance impact should be minimal as there are not that many players for radio (one app, HAL, and system server) and the amount of calls are not that frequent. Bug: 194818704 Test: atest BroadcastRadioTests Change-Id: I5f51c0e0a04ae10428f721be508d5579dc1dd837 (cherry picked from commit 2cdb566acb733815063e3c2f4651112cadfc4189) Merged-In: I5f51c0e0a04ae10428f721be508d5579dc1dd837 --- .../hal2/StartProgramListUpdatesFanoutTest.java | 3 ++- .../broadcastradio/BroadcastRadioService.java | 6 +++--- .../hal1/BroadcastRadioService.java | 17 +++++++++-------- .../hal2/AnnouncementAggregator.java | 5 +++-- .../hal2/BroadcastRadioService.java | 9 +++++---- .../server/broadcastradio/hal2/RadioModule.java | 17 ++++++++++------- .../broadcastradio/hal2/TunerSession.java | 6 ++++-- 7 files changed, 36 insertions(+), 27 deletions(-) diff --git a/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/hal2/StartProgramListUpdatesFanoutTest.java b/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/hal2/StartProgramListUpdatesFanoutTest.java index 7c6271cbdf61e..c194989b2752b 100644 --- a/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/hal2/StartProgramListUpdatesFanoutTest.java +++ b/core/tests/BroadcastRadioTests/src/com/android/server/broadcastradio/hal2/StartProgramListUpdatesFanoutTest.java @@ -65,6 +65,7 @@ public class StartProgramListUpdatesFanoutTest { @Mock ITunerSession mHalTunerSessionMock; private android.hardware.radio.ITunerCallback[] mAidlTunerCallbackMocks; + private final Object mLock = new Object(); // RadioModule under test private RadioModule mRadioModule; @@ -96,7 +97,7 @@ public class StartProgramListUpdatesFanoutTest { mRadioModule = new RadioModule(mBroadcastRadioMock, new RadioManager.ModuleProperties(0, "", 0, "", "", "", "", 0, 0, false, false, null, false, new int[] {}, new int[] {}, - null, null)); + null, null), mLock); doAnswer((Answer) invocation -> { mHalTunerCallback = (ITunerCallback) invocation.getArguments()[0]; diff --git a/services/core/java/com/android/server/broadcastradio/BroadcastRadioService.java b/services/core/java/com/android/server/broadcastradio/BroadcastRadioService.java index 54a4ad42fd99e..23f0ffbbf0b85 100644 --- a/services/core/java/com/android/server/broadcastradio/BroadcastRadioService.java +++ b/services/core/java/com/android/server/broadcastradio/BroadcastRadioService.java @@ -52,11 +52,11 @@ public class BroadcastRadioService extends SystemService { public BroadcastRadioService(Context context) { super(context); - mHal1 = new com.android.server.broadcastradio.hal1.BroadcastRadioService(); + mHal1 = new com.android.server.broadcastradio.hal1.BroadcastRadioService(mLock); mV1Modules = mHal1.loadModules(); OptionalInt max = mV1Modules.stream().mapToInt(RadioManager.ModuleProperties::getId).max(); mHal2 = new com.android.server.broadcastradio.hal2.BroadcastRadioService( - max.isPresent() ? max.getAsInt() + 1 : 0); + max.isPresent() ? max.getAsInt() + 1 : 0, mLock); } @Override @@ -111,7 +111,7 @@ public class BroadcastRadioService extends SystemService { synchronized (mLock) { if (!mHal2.hasAnyModules()) { Slog.i(TAG, "There are no HAL 2.x modules registered"); - return new AnnouncementAggregator(listener); + return new AnnouncementAggregator(listener, mLock); } return mHal2.addAnnouncementListener(enabledTypes, listener); diff --git a/services/core/java/com/android/server/broadcastradio/hal1/BroadcastRadioService.java b/services/core/java/com/android/server/broadcastradio/hal1/BroadcastRadioService.java index e8ac5477469b7..5da60328cd709 100644 --- a/services/core/java/com/android/server/broadcastradio/hal1/BroadcastRadioService.java +++ b/services/core/java/com/android/server/broadcastradio/hal1/BroadcastRadioService.java @@ -17,16 +17,9 @@ package com.android.server.broadcastradio.hal1; import android.annotation.NonNull; -import android.Manifest; -import android.content.Context; -import android.content.pm.PackageManager; -import android.hardware.radio.IRadioService; import android.hardware.radio.ITuner; import android.hardware.radio.ITunerCallback; import android.hardware.radio.RadioManager; -import android.os.ParcelableException; - -import com.android.server.SystemService; import java.util.List; import java.util.Objects; @@ -37,7 +30,7 @@ public class BroadcastRadioService { */ private final long mNativeContext = nativeInit(); - private final Object mLock = new Object(); + private final Object mLock; @Override protected void finalize() throws Throwable { @@ -51,6 +44,14 @@ public class BroadcastRadioService { private native Tuner nativeOpenTuner(long nativeContext, int moduleId, RadioManager.BandConfig config, boolean withAudio, ITunerCallback callback); + /** + * Constructor. should pass + * {@code com.android.server.broadcastradio.BroadcastRadioService#mLock} for lock. + */ + public BroadcastRadioService(@NonNull Object lock) { + mLock = lock; + } + public @NonNull List loadModules() { synchronized (mLock) { return Objects.requireNonNull(nativeLoadModules(mNativeContext)); diff --git a/services/core/java/com/android/server/broadcastradio/hal2/AnnouncementAggregator.java b/services/core/java/com/android/server/broadcastradio/hal2/AnnouncementAggregator.java index 53076975849b6..42e296f3e4ec7 100644 --- a/services/core/java/com/android/server/broadcastradio/hal2/AnnouncementAggregator.java +++ b/services/core/java/com/android/server/broadcastradio/hal2/AnnouncementAggregator.java @@ -35,7 +35,7 @@ import java.util.Objects; public class AnnouncementAggregator extends ICloseHandle.Stub { private static final String TAG = "BcRadio2Srv.AnnAggr"; - private final Object mLock = new Object(); + private final Object mLock; @NonNull private final IAnnouncementListener mListener; private final IBinder.DeathRecipient mDeathRecipient = new DeathRecipient(); @@ -45,8 +45,9 @@ public class AnnouncementAggregator extends ICloseHandle.Stub { @GuardedBy("mLock") private boolean mIsClosed = false; - public AnnouncementAggregator(@NonNull IAnnouncementListener listener) { + public AnnouncementAggregator(@NonNull IAnnouncementListener listener, @NonNull Object lock) { mListener = Objects.requireNonNull(listener); + mLock = Objects.requireNonNull(lock); try { listener.asBinder().linkToDeath(mDeathRecipient, 0); } catch (RemoteException ex) { diff --git a/services/core/java/com/android/server/broadcastradio/hal2/BroadcastRadioService.java b/services/core/java/com/android/server/broadcastradio/hal2/BroadcastRadioService.java index 5e79c5943d7b1..5c07f76e5011e 100644 --- a/services/core/java/com/android/server/broadcastradio/hal2/BroadcastRadioService.java +++ b/services/core/java/com/android/server/broadcastradio/hal2/BroadcastRadioService.java @@ -42,7 +42,7 @@ import java.util.stream.Collectors; public class BroadcastRadioService { private static final String TAG = "BcRadio2Srv"; - private final Object mLock = new Object(); + private final Object mLock; @GuardedBy("mLock") private int mNextModuleId = 0; @@ -68,7 +68,7 @@ public class BroadcastRadioService { moduleId = mNextModuleId; } - RadioModule module = RadioModule.tryLoadingModule(moduleId, serviceName); + RadioModule module = RadioModule.tryLoadingModule(moduleId, serviceName, mLock); if (module == null) { return; } @@ -116,8 +116,9 @@ public class BroadcastRadioService { } }; - public BroadcastRadioService(int nextModuleId) { + public BroadcastRadioService(int nextModuleId, Object lock) { mNextModuleId = nextModuleId; + mLock = lock; try { IServiceManager manager = IServiceManager.getService(); if (manager == null) { @@ -174,7 +175,7 @@ public class BroadcastRadioService { public ICloseHandle addAnnouncementListener(@NonNull int[] enabledTypes, @NonNull IAnnouncementListener listener) { - AnnouncementAggregator aggregator = new AnnouncementAggregator(listener); + AnnouncementAggregator aggregator = new AnnouncementAggregator(listener, mLock); boolean anySupported = false; synchronized (mLock) { for (RadioModule module : mModules.values()) { diff --git a/services/core/java/com/android/server/broadcastradio/hal2/RadioModule.java b/services/core/java/com/android/server/broadcastradio/hal2/RadioModule.java index b7e188c73eab2..ef7f4c9fc919c 100644 --- a/services/core/java/com/android/server/broadcastradio/hal2/RadioModule.java +++ b/services/core/java/com/android/server/broadcastradio/hal2/RadioModule.java @@ -58,7 +58,7 @@ class RadioModule { @NonNull private final IBroadcastRadio mService; @NonNull public final RadioManager.ModuleProperties mProperties; - private final Object mLock = new Object(); + private final Object mLock; @NonNull private final Handler mHandler; @GuardedBy("mLock") @@ -132,13 +132,15 @@ class RadioModule { @VisibleForTesting RadioModule(@NonNull IBroadcastRadio service, - @NonNull RadioManager.ModuleProperties properties) { + @NonNull RadioManager.ModuleProperties properties, @NonNull Object lock) { mProperties = Objects.requireNonNull(properties); mService = Objects.requireNonNull(service); + mLock = Objects.requireNonNull(lock); mHandler = new Handler(Looper.getMainLooper()); } - public static @Nullable RadioModule tryLoadingModule(int idx, @NonNull String fqName) { + public static @Nullable RadioModule tryLoadingModule(int idx, @NonNull String fqName, + Object lock) { try { IBroadcastRadio service = IBroadcastRadio.getService(fqName); if (service == null) return null; @@ -156,7 +158,7 @@ class RadioModule { RadioManager.ModuleProperties prop = Convert.propertiesFromHal(idx, fqName, service.getProperties(), amfmConfig.value, dabConfig.value); - return new RadioModule(service, prop); + return new RadioModule(service, prop, lock); } catch (RemoteException ex) { Slog.e(TAG, "failed to load module " + fqName, ex); return null; @@ -178,7 +180,8 @@ class RadioModule { }); mHalTunerSession = Objects.requireNonNull(hwSession.value); } - TunerSession tunerSession = new TunerSession(this, mHalTunerSession, userCb); + TunerSession tunerSession = new TunerSession(this, mHalTunerSession, userCb, + mLock); mAidlTunerSessions.add(tunerSession); // Propagate state to new client. Note: These callbacks are invoked while holding mLock @@ -377,7 +380,7 @@ class RadioModule { } }; - synchronized (mService) { + synchronized (mLock) { mService.registerAnnouncementListener(enabledList, hwListener, (result, closeHnd) -> { halResult.value = result; hwCloseHandle.value = closeHnd; @@ -401,7 +404,7 @@ class RadioModule { if (id == 0) throw new IllegalArgumentException("Image ID is missing"); byte[] rawImage; - synchronized (mService) { + synchronized (mLock) { List rawList = Utils.maybeRethrow(() -> mService.getImage(id)); rawImage = new byte[rawList.size()]; for (int i = 0; i < rawList.size(); i++) { diff --git a/services/core/java/com/android/server/broadcastradio/hal2/TunerSession.java b/services/core/java/com/android/server/broadcastradio/hal2/TunerSession.java index 7ab3bdd859e46..200af2fb1da72 100644 --- a/services/core/java/com/android/server/broadcastradio/hal2/TunerSession.java +++ b/services/core/java/com/android/server/broadcastradio/hal2/TunerSession.java @@ -40,7 +40,7 @@ class TunerSession extends ITuner.Stub { private static final String TAG = "BcRadio2Srv.session"; private static final String kAudioDeviceName = "Radio tuner source"; - private final Object mLock = new Object(); + private final Object mLock; private final RadioModule mModule; private final ITunerSession mHwSession; @@ -53,10 +53,12 @@ class TunerSession extends ITuner.Stub { private RadioManager.BandConfig mDummyConfig = null; TunerSession(@NonNull RadioModule module, @NonNull ITunerSession hwSession, - @NonNull android.hardware.radio.ITunerCallback callback) { + @NonNull android.hardware.radio.ITunerCallback callback, + @NonNull Object lock) { mModule = Objects.requireNonNull(module); mHwSession = Objects.requireNonNull(hwSession); mCallback = Objects.requireNonNull(callback); + mLock = Objects.requireNonNull(lock); } @Override