audio: Retain IBinder for instances with MinSchedulerPolicy

The binder passed to AIBinder_setMinSchedulerPolicy must also be
returned to the client, otherwise setting the policy for it does
not make any sense. However, server side interface instance
classes only hold a weak binder reference. It's the caller of the
'asBinder' method who must retain a strong reference. This
reference must be retained past exiting from the method which
returns the instance to the client.

To solve this issue, add storing of binders along with server
object references. These binders get released after the client
calls a 'close'/'destroy'-type method to release instance
resources.

Bug: 205884982
Test: run `atest VtsHalAudioCoreTargetTest` and effect VTS,
      and grep logcat for
     'destroyed after setMinSchedulerPolicy before being parceled'
Change-Id: I8b905b85cb8263c85edae8839a126ffe4e4d1e69
This commit is contained in:
Mikhail Naganov 2022-12-15 00:11:14 +00:00 committed by Shunkai Yao
parent 2100e6323c
commit df5feba141
6 changed files with 45 additions and 33 deletions

View file

@ -40,12 +40,13 @@ Factory::Factory(const std::string& file) : mConfig(EffectConfig(file)) {
}
Factory::~Factory() {
if (auto count = mEffectUuidMap.size()) {
if (auto count = mEffectMap.size()) {
LOG(ERROR) << __func__ << " remaining " << count
<< " effect instances not destroyed indicating resource leak!";
for (const auto& it : mEffectUuidMap) {
for (const auto& it : mEffectMap) {
if (auto spEffect = it.first.lock()) {
LOG(ERROR) << __func__ << " erase remaining instance UUID " << it.second.toString();
LOG(ERROR) << __func__ << " erase remaining instance UUID "
<< it.second.first.toString();
destroyEffectImpl(spEffect);
}
}
@ -109,9 +110,10 @@ ndk::ScopedAStatus Factory::createEffect(const AudioUuid& in_impl_uuid,
return ndk::ScopedAStatus::fromExceptionCode(EX_TRANSACTION_FAILED);
}
*_aidl_return = effectSp;
AIBinder_setMinSchedulerPolicy(effectSp->asBinder().get(), SCHED_NORMAL,
ANDROID_PRIORITY_AUDIO);
mEffectUuidMap[std::weak_ptr<IEffect>(effectSp)] = in_impl_uuid;
ndk::SpAIBinder effectBinder = effectSp->asBinder();
AIBinder_setMinSchedulerPolicy(effectBinder.get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO);
mEffectMap[std::weak_ptr<IEffect>(effectSp)] =
std::make_pair(in_impl_uuid, std::move(effectBinder));
LOG(DEBUG) << __func__ << ": instance " << effectSp.get() << " created successfully";
return ndk::ScopedAStatus::ok();
} else {
@ -123,9 +125,9 @@ ndk::ScopedAStatus Factory::createEffect(const AudioUuid& in_impl_uuid,
ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr<IEffect>& in_handle) {
std::weak_ptr<IEffect> wpHandle(in_handle);
// find UUID with key (std::weak_ptr<IEffect>)
if (auto uuidIt = mEffectUuidMap.find(wpHandle); uuidIt != mEffectUuidMap.end()) {
auto& uuid = uuidIt->second;
// find the effect entry with key (std::weak_ptr<IEffect>)
if (auto effectIt = mEffectMap.find(wpHandle); effectIt != mEffectMap.end()) {
auto& uuid = effectIt->second.first;
// find implementation library with UUID
if (auto libIt = mEffectLibMap.find(uuid); libIt != mEffectLibMap.end()) {
auto& interface = std::get<kMapEntryInterfaceIndex>(libIt->second);
@ -136,7 +138,7 @@ ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr<IEffect>& in
LOG(ERROR) << __func__ << ": UUID " << uuid.toString() << " does not exist in libMap!";
return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT);
}
mEffectUuidMap.erase(uuidIt);
mEffectMap.erase(effectIt);
return ndk::ScopedAStatus::ok();
} else {
LOG(ERROR) << __func__ << ": instance " << in_handle << " does not exist!";
@ -146,9 +148,9 @@ ndk::ScopedAStatus Factory::destroyEffectImpl(const std::shared_ptr<IEffect>& in
// go over the map and cleanup all expired weak_ptrs.
void Factory::cleanupEffectMap() {
for (auto it = mEffectUuidMap.begin(); it != mEffectUuidMap.end();) {
for (auto it = mEffectMap.begin(); it != mEffectMap.end();) {
if (nullptr == it->first.lock()) {
it = mEffectUuidMap.erase(it);
it = mEffectMap.erase(it);
} else {
++it;
}

View file

@ -316,7 +316,8 @@ ndk::ScopedAStatus Module::setModuleDebug(
ndk::ScopedAStatus Module::getTelephony(std::shared_ptr<ITelephony>* _aidl_return) {
if (mTelephony == nullptr) {
mTelephony = ndk::SharedRefBase::make<Telephony>();
AIBinder_setMinSchedulerPolicy(mTelephony->asBinder().get(), SCHED_NORMAL,
mTelephonyBinder = mTelephony->asBinder();
AIBinder_setMinSchedulerPolicy(mTelephonyBinder.get(), SCHED_NORMAL,
ANDROID_PRIORITY_AUDIO);
}
*_aidl_return = mTelephony;
@ -536,8 +537,9 @@ ndk::ScopedAStatus Module::openInputStream(const OpenInputStreamArguments& in_ar
if (auto status = stream->init(); !status.isOk()) {
return status;
}
AIBinder_setMinSchedulerPolicy(stream->asBinder().get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO);
StreamWrapper streamWrapper(stream);
AIBinder_setMinSchedulerPolicy(streamWrapper.getBinder().get(), SCHED_NORMAL,
ANDROID_PRIORITY_AUDIO);
auto patchIt = mPatches.find(in_args.portConfigId);
if (patchIt != mPatches.end()) {
streamWrapper.setStreamIsConnected(findConnectedDevices(in_args.portConfigId));
@ -587,8 +589,9 @@ ndk::ScopedAStatus Module::openOutputStream(const OpenOutputStreamArguments& in_
if (auto status = stream->init(); !status.isOk()) {
return status;
}
AIBinder_setMinSchedulerPolicy(stream->asBinder().get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO);
StreamWrapper streamWrapper(stream);
AIBinder_setMinSchedulerPolicy(streamWrapper.getBinder().get(), SCHED_NORMAL,
ANDROID_PRIORITY_AUDIO);
auto patchIt = mPatches.find(in_args.portConfigId);
if (patchIt != mPatches.end()) {
streamWrapper.setStreamIsConnected(findConnectedDevices(in_args.portConfigId));
@ -935,6 +938,9 @@ ndk::ScopedAStatus Module::updateScreenState(bool in_isTurnedOn) {
ndk::ScopedAStatus Module::getSoundDose(std::shared_ptr<ISoundDose>* _aidl_return) {
if (mSoundDose == nullptr) {
mSoundDose = ndk::SharedRefBase::make<SoundDose>();
mSoundDoseBinder = mSoundDose->asBinder();
AIBinder_setMinSchedulerPolicy(mSoundDoseBinder.get(), SCHED_NORMAL,
ANDROID_PRIORITY_AUDIO);
}
*_aidl_return = mSoundDose;
LOG(DEBUG) << __func__ << ": returning instance of ISoundDose: " << _aidl_return->get();

View file

@ -115,6 +115,7 @@ class Module : public BnModule {
// Since it is required to return the same instance of the ITelephony, even
// if the client has released it on its side, we need to hold it via a strong pointer.
std::shared_ptr<ITelephony> mTelephony;
ndk::SpAIBinder mTelephonyBinder;
// ids of ports created at runtime via 'connectExternalDevice'.
std::set<int32_t> mConnectedDevicePorts;
Streams mStreams;
@ -125,6 +126,7 @@ class Module : public BnModule {
float mMasterVolume = 1.0f;
bool mMicMute = false;
std::shared_ptr<ISoundDose> mSoundDose;
ndk::SpAIBinder mSoundDoseBinder;
};
} // namespace aidl::android::hardware::audio::core

View file

@ -279,8 +279,11 @@ class StreamOut : public StreamCommon<::aidl::android::hardware::audio::common::
class StreamWrapper {
public:
explicit StreamWrapper(std::shared_ptr<StreamIn> streamIn) : mStream(streamIn) {}
explicit StreamWrapper(std::shared_ptr<StreamOut> streamOut) : mStream(streamOut) {}
explicit StreamWrapper(const std::shared_ptr<StreamIn>& streamIn)
: mStream(streamIn), mStreamBinder(streamIn->asBinder()) {}
explicit StreamWrapper(const std::shared_ptr<StreamOut>& streamOut)
: mStream(streamOut), mStreamBinder(streamOut->asBinder()) {}
ndk::SpAIBinder getBinder() const { return mStreamBinder; }
bool isStreamOpen() const {
return std::visit(
[](auto&& ws) -> bool {
@ -301,6 +304,7 @@ class StreamWrapper {
private:
std::variant<std::weak_ptr<StreamIn>, std::weak_ptr<StreamOut>> mStream;
ndk::SpAIBinder mStreamBinder;
};
class Streams {

View file

@ -96,9 +96,8 @@ class Factory : public BnFactory {
std::map<aidl::android::media::audio::common::AudioUuid /* implUUID */, DlEntry> mEffectLibMap;
std::map<std::weak_ptr<IEffect>, aidl::android::media::audio::common::AudioUuid,
std::owner_less<>>
mEffectUuidMap;
typedef std::pair<aidl::android::media::audio::common::AudioUuid, ndk::SpAIBinder> EffectEntry;
std::map<std::weak_ptr<IEffect>, EffectEntry, std::owner_less<>> mEffectMap;
ndk::ScopedAStatus destroyEffectImpl(const std::shared_ptr<IEffect>& in_handle);
void cleanupEffectMap();

View file

@ -16,6 +16,7 @@
#include <cstdlib>
#include <ctime>
#include <utility>
#include <android-base/logging.h>
#include <android/binder_ibinder_platform.h>
@ -44,19 +45,17 @@ int main() {
CHECK_EQ(STATUS_OK, status);
// Make modules
auto moduleDefault = ndk::SharedRefBase::make<Module>(Module::Type::DEFAULT);
const std::string moduleDefaultName = std::string() + Module::descriptor + "/default";
AIBinder_setMinSchedulerPolicy(moduleDefault->asBinder().get(), SCHED_NORMAL,
ANDROID_PRIORITY_AUDIO);
status = AServiceManager_addService(moduleDefault->asBinder().get(), moduleDefaultName.c_str());
CHECK_EQ(STATUS_OK, status);
auto moduleRSubmix = ndk::SharedRefBase::make<Module>(Module::Type::R_SUBMIX);
const std::string moduleRSubmixName = std::string() + Module::descriptor + "/r_submix";
AIBinder_setMinSchedulerPolicy(moduleRSubmix->asBinder().get(), SCHED_NORMAL,
ANDROID_PRIORITY_AUDIO);
status = AServiceManager_addService(moduleRSubmix->asBinder().get(), moduleRSubmixName.c_str());
CHECK_EQ(STATUS_OK, status);
auto createModule = [](Module::Type type, const std::string& instance) {
auto module = ndk::SharedRefBase::make<Module>(type);
ndk::SpAIBinder moduleBinder = module->asBinder();
const std::string moduleName = std::string(Module::descriptor).append("/").append(instance);
AIBinder_setMinSchedulerPolicy(moduleBinder.get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO);
binder_status_t status = AServiceManager_addService(moduleBinder.get(), moduleName.c_str());
CHECK_EQ(STATUS_OK, status);
return std::make_pair(module, moduleBinder);
};
auto modules = {createModule(Module::Type::DEFAULT, "default"),
createModule(Module::Type::R_SUBMIX, "r_submix")};
ABinderProcess_joinThreadPool();
return EXIT_FAILURE; // should not reach