Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 92f705ce authored by Devin Moore's avatar Devin Moore
Browse files

Use onUnlinked in health HAL

It's possible to get an onBinderDied callback after a call to
AIBinder_unlinkToDeath() so we can't delete the objects in callbacks_
until we are done using the void* cookie.
Handling the cleanup in onBinderUnlinked will handle the case where we
manually unlink it as well as the case where it's unlinked due to death.

Test: atest VtsHalHealthTargetTest
Bug: 319210610
Change-Id: Iee4783217cc88134af6de0fe66128684ca984dba
parent 2f666675
Loading
Loading
Loading
Loading
+26 −8
Original line number Original line Diff line number Diff line
@@ -36,6 +36,11 @@ void OnCallbackDiedWrapped(void* cookie) {
    LinkedCallback* linked = reinterpret_cast<LinkedCallback*>(cookie);
    LinkedCallback* linked = reinterpret_cast<LinkedCallback*>(cookie);
    linked->OnCallbackDied();
    linked->OnCallbackDied();
}
}
// Delete the owned cookie.
void onCallbackUnlinked(void* cookie) {
    LinkedCallback* linked = reinterpret_cast<LinkedCallback*>(cookie);
    delete linked;
}
}  // namespace
}  // namespace


/*
/*
@@ -57,6 +62,7 @@ Health::Health(std::string_view instance_name, std::unique_ptr<struct healthd_co
    : instance_name_(instance_name),
    : instance_name_(instance_name),
      healthd_config_(std::move(config)),
      healthd_config_(std::move(config)),
      death_recipient_(AIBinder_DeathRecipient_new(&OnCallbackDiedWrapped)) {
      death_recipient_(AIBinder_DeathRecipient_new(&OnCallbackDiedWrapped)) {
    AIBinder_DeathRecipient_setOnUnlinked(death_recipient_.get(), onCallbackUnlinked);
    battery_monitor_.init(healthd_config_.get());
    battery_monitor_.init(healthd_config_.get());
}
}


@@ -286,7 +292,7 @@ ndk::ScopedAStatus Health::registerCallback(const std::shared_ptr<IHealthInfoCal
        if (!linked_callback_result.ok()) {
        if (!linked_callback_result.ok()) {
            return ndk::ScopedAStatus::fromStatus(-linked_callback_result.error().code());
            return ndk::ScopedAStatus::fromStatus(-linked_callback_result.error().code());
        }
        }
        callbacks_.emplace_back(std::move(*linked_callback_result));
        callbacks_[*linked_callback_result] = callback;
        // unlock
        // unlock
    }
    }


@@ -314,12 +320,24 @@ ndk::ScopedAStatus Health::unregisterCallback(


    std::lock_guard<decltype(callbacks_lock_)> lock(callbacks_lock_);
    std::lock_guard<decltype(callbacks_lock_)> lock(callbacks_lock_);


    auto matches = [callback](const auto& linked) {
    auto matches = [callback](const auto& cb) {
        return linked->callback()->asBinder() == callback->asBinder();  // compares binder object
        return cb->asBinder() == callback->asBinder();  // compares binder object
    };
    };
    auto it = std::remove_if(callbacks_.begin(), callbacks_.end(), matches);
    bool removed = false;
    bool removed = (it != callbacks_.end());
    for (auto it = callbacks_.begin(); it != callbacks_.end();) {
    callbacks_.erase(it, callbacks_.end());  // calls unlinkToDeath on deleted callbacks.
        if (it->second->asBinder() == callback->asBinder()) {
            auto status = AIBinder_unlinkToDeath(callback->asBinder().get(), death_recipient_.get(),
                                                 reinterpret_cast<void*>(it->first));
            if (status != STATUS_OK && status != STATUS_DEAD_OBJECT) {
                LOG(WARNING) << __func__
                             << "Cannot unlink to death: " << ::android::statusToString(status);
            }
            it = callbacks_.erase(it);
            removed = true;
        } else {
            it++;
        }
    }
    return removed ? ndk::ScopedAStatus::ok()
    return removed ? ndk::ScopedAStatus::ok()
                   : ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT);
                   : ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT);
}
}
@@ -347,8 +365,8 @@ ndk::ScopedAStatus Health::update() {
void Health::OnHealthInfoChanged(const HealthInfo& health_info) {
void Health::OnHealthInfoChanged(const HealthInfo& health_info) {
    // Notify all callbacks
    // Notify all callbacks
    std::unique_lock<decltype(callbacks_lock_)> lock(callbacks_lock_);
    std::unique_lock<decltype(callbacks_lock_)> lock(callbacks_lock_);
    for (const auto& linked : callbacks_) {
    for (const auto& [_, callback] : callbacks_) {
        auto res = linked->callback()->healthInfoChanged(health_info);
        auto res = callback->healthInfoChanged(health_info);
        if (!res.isOk()) {
        if (!res.isOk()) {
            LOG(DEBUG) << "Cannot call healthInfoChanged:" << res.getDescription()
            LOG(DEBUG) << "Cannot call healthInfoChanged:" << res.getDescription()
                       << ". Do nothing here if callback is dead as it will be cleaned up later.";
                       << ". Do nothing here if callback is dead as it will be cleaned up later.";
+9 −17
Original line number Original line Diff line number Diff line
@@ -24,35 +24,24 @@


namespace aidl::android::hardware::health {
namespace aidl::android::hardware::health {


::android::base::Result<std::unique_ptr<LinkedCallback>> LinkedCallback::Make(
::android::base::Result<LinkedCallback*> LinkedCallback::Make(
        std::shared_ptr<Health> service, std::shared_ptr<IHealthInfoCallback> callback) {
        std::shared_ptr<Health> service, std::shared_ptr<IHealthInfoCallback> callback) {
    std::unique_ptr<LinkedCallback> ret(new LinkedCallback());
    LinkedCallback* ret(new LinkedCallback());
    // pass ownership of this object to the death recipient
    binder_status_t linkRet =
    binder_status_t linkRet =
            AIBinder_linkToDeath(callback->asBinder().get(), service->death_recipient_.get(),
            AIBinder_linkToDeath(callback->asBinder().get(), service->death_recipient_.get(),
                                 reinterpret_cast<void*>(ret.get()));
                                 reinterpret_cast<void*>(ret));
    if (linkRet != ::STATUS_OK) {
    if (linkRet != ::STATUS_OK) {
        LOG(WARNING) << __func__ << "Cannot link to death: " << linkRet;
        LOG(WARNING) << __func__ << "Cannot link to death: " << linkRet;
        return ::android::base::Error(-linkRet);
        return ::android::base::Error(-linkRet);
    }
    }
    ret->service_ = service;
    ret->service_ = service;
    ret->callback_ = std::move(callback);
    ret->callback_ = callback;
    return ret;
    return ret;
}
}


LinkedCallback::LinkedCallback() = default;
LinkedCallback::LinkedCallback() = default;


LinkedCallback::~LinkedCallback() {
    if (callback_ == nullptr) {
        return;
    }
    auto status =
            AIBinder_unlinkToDeath(callback_->asBinder().get(), service()->death_recipient_.get(),
                                   reinterpret_cast<void*>(this));
    if (status != STATUS_OK && status != STATUS_DEAD_OBJECT) {
        LOG(WARNING) << __func__ << "Cannot unlink to death: " << ::android::statusToString(status);
    }
}

std::shared_ptr<Health> LinkedCallback::service() {
std::shared_ptr<Health> LinkedCallback::service() {
    auto service_sp = service_.lock();
    auto service_sp = service_.lock();
    CHECK_NE(nullptr, service_sp);
    CHECK_NE(nullptr, service_sp);
@@ -60,7 +49,10 @@ std::shared_ptr<Health> LinkedCallback::service() {
}
}


void LinkedCallback::OnCallbackDied() {
void LinkedCallback::OnCallbackDied() {
    service()->unregisterCallback(callback_);
    auto sCb = callback_.lock();
    if (sCb) {
        service()->unregisterCallback(sCb);
    }
}
}


}  // namespace aidl::android::hardware::health
}  // namespace aidl::android::hardware::health
+4 −13
Original line number Original line Diff line number Diff line
@@ -32,19 +32,10 @@ namespace aidl::android::hardware::health {
class LinkedCallback {
class LinkedCallback {
  public:
  public:
    // Automatically linkToDeath upon construction with the returned object as the cookie.
    // Automatically linkToDeath upon construction with the returned object as the cookie.
    // service->death_reciepient() should be from CreateDeathRecipient().
    // The deathRecipient owns the LinkedCallback object and will delete it with
    // Not using a strong reference to |service| to avoid circular reference. The lifetime
    // cookie when it's unlinked.
    // of |service| must be longer than this LinkedCallback object.
    static ::android::base::Result<LinkedCallback*> Make(
    static ::android::base::Result<std::unique_ptr<LinkedCallback>> Make(
            std::shared_ptr<Health> service, std::shared_ptr<IHealthInfoCallback> callback);
            std::shared_ptr<Health> service, std::shared_ptr<IHealthInfoCallback> callback);

    // Automatically unlinkToDeath upon destruction. So, it is always safe to reinterpret_cast
    // the cookie back to the LinkedCallback object.
    ~LinkedCallback();

    // The wrapped IHealthInfoCallback object.
    const std::shared_ptr<IHealthInfoCallback>& callback() const { return callback_; }

    // On callback died, unreigster it from the service.
    // On callback died, unreigster it from the service.
    void OnCallbackDied();
    void OnCallbackDied();


@@ -55,7 +46,7 @@ class LinkedCallback {
    std::shared_ptr<Health> service();
    std::shared_ptr<Health> service();


    std::weak_ptr<Health> service_;
    std::weak_ptr<Health> service_;
    std::shared_ptr<IHealthInfoCallback> callback_;
    std::weak_ptr<IHealthInfoCallback> callback_;
};
};


}  // namespace aidl::android::hardware::health
}  // namespace aidl::android::hardware::health
+2 −1
Original line number Original line Diff line number Diff line
@@ -16,6 +16,7 @@


#pragma once
#pragma once


#include <map>
#include <memory>
#include <memory>
#include <optional>
#include <optional>


@@ -112,7 +113,7 @@ class Health : public BnHealth, public HalHealthLoopCallback {
    ndk::ScopedAIBinder_DeathRecipient death_recipient_;
    ndk::ScopedAIBinder_DeathRecipient death_recipient_;
    int binder_fd_ = -1;
    int binder_fd_ = -1;
    std::mutex callbacks_lock_;
    std::mutex callbacks_lock_;
    std::vector<std::unique_ptr<LinkedCallback>> callbacks_;
    std::map<LinkedCallback*, std::shared_ptr<IHealthInfoCallback>> callbacks_;
};
};


}  // namespace aidl::android::hardware::health
}  // namespace aidl::android::hardware::health