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

Commit 0c674f5f authored by Devin Moore's avatar Devin Moore
Browse files

libmediatranscoding: handle death recipient cookie ownership differently 

The ownership of the death recipient cookie is now limited to the
TranscodingResourcePolicy object and the binderDied callback.
They both must be able to delete the cookie object and they both must be
aware of it already being deleted.

In all cases, the TranscodingResourcePolicy object that needs to be
unregistered will outlive the cookie and the death recipient.

Calling unlinkToDeath is unneccessary because the last strong ref to the
binder that was linked to death is removed in the unregisterSelf method
which will unlink the binder and death recipient.

Test: atest CtsMediaTranscodingTestCases MediaSampleReaderNDKTests
Bug: 319210610
Change-Id: I8e6ba40fe3da30bf8753e7a16ad5c8cd5dfda40b
parent 50a5b0b1

media/module/libmediatranscoding/TranscodingResourcePolicy.cpp

+52 −5
Original line number Original line Diff line number Diff line
@@ -21,6 +21,7 @@ 
#include <aidl/android/media/IResourceObserverService.h>

#include <aidl/android/media/IResourceObserverService.h>

#include <android/binder_manager.h>

#include <android/binder_manager.h>

#include <android/binder_process.h>

#include <android/binder_process.h>

#include <map>

#include <media/TranscodingResourcePolicy.h>

#include <media/TranscodingResourcePolicy.h>

#include <utils/Log.h>

#include <utils/Log.h>
@@ -66,11 +67,31 @@ struct TranscodingResourcePolicy::ResourceObserver : public BnResourceObserver { 
    TranscodingResourcePolicy* mOwner;

    TranscodingResourcePolicy* mOwner;

};

};





// cookie used for death recipients. The TranscodingResourcePolicy

// that this cookie is associated with must outlive this cookie. It is

// either deleted by binderDied, or in unregisterSelf which is also called

// in the destructor of TranscodingResourcePolicy

class TranscodingResourcePolicyCookie {

 public:

    TranscodingResourcePolicyCookie(TranscodingResourcePolicy* policy) : mPolicy(policy) {}

    TranscodingResourcePolicyCookie() = delete;

    TranscodingResourcePolicy* mPolicy;

};



static std::map<uintptr_t, std::unique_ptr<TranscodingResourcePolicyCookie>> sCookies;

static uintptr_t sCookieKeyCounter;

static std::mutex sCookiesMutex;



// static

// static

void TranscodingResourcePolicy::BinderDiedCallback(void* cookie) {

void TranscodingResourcePolicy::BinderDiedCallback(void* cookie) {

    TranscodingResourcePolicy* owner = reinterpret_cast<TranscodingResourcePolicy*>(cookie);

    std::lock_guard<std::mutex> guard(sCookiesMutex);

    if (owner != nullptr) {

    if (auto it = sCookies.find(reinterpret_cast<uintptr_t>(cookie)); it != sCookies.end()) {

        owner->unregisterSelf();

        ALOGI("BinderDiedCallback unregistering TranscodingResourcePolicy");

        auto policy = reinterpret_cast<TranscodingResourcePolicy*>(it->second->mPolicy);

        if (policy) {

            policy->unregisterSelf();

        }

        sCookies.erase(it);

    }

    }

    // TODO(chz): retry to connecting to IResourceObserverService after failure.

    // TODO(chz): retry to connecting to IResourceObserverService after failure.

    // Also need to have back-up logic if IResourceObserverService is offline for

    // Also need to have back-up logic if IResourceObserverService is offline for
@@ -88,6 +109,23 @@ TranscodingResourcePolicy::TranscodingResourcePolicy() 
}

}





TranscodingResourcePolicy::~TranscodingResourcePolicy() {

TranscodingResourcePolicy::~TranscodingResourcePolicy() {

    {

        std::lock_guard<std::mutex> guard(sCookiesMutex);



        // delete all of the cookies associated with this TranscodingResourcePolicy

        // instance since they are holding pointers to this object that will no

        // longer be valid.

        std::erase_if(sCookies, [this](const auto& cookieEntry) {

            auto const& [key, cookie] = cookieEntry;

            std::lock_guard guard(mCookieKeysLock);

            if (const auto& it = mCookieKeys.find(key); it != mCookieKeys.end()) {

                // No longer need to track this cookie

                mCookieKeys.erase(key);

                return true;

            }

            return false;

        });

    }

    unregisterSelf();

    unregisterSelf();

}

}
@@ -123,7 +161,17 @@ void TranscodingResourcePolicy::registerSelf() { 
        return;

        return;

    }

    }





    AIBinder_linkToDeath(binder.get(), mDeathRecipient.get(), reinterpret_cast<void*>(this));

    std::unique_ptr<TranscodingResourcePolicyCookie> cookie =

            std::make_unique<TranscodingResourcePolicyCookie>(this);

    void* cookiePtr = static_cast<void*>(cookie.get());

    uintptr_t cookieKey = sCookieKeyCounter++;

    sCookies.emplace(cookieKey, std::move(cookie));

    {

        std::lock_guard guard(mCookieKeysLock);

        mCookieKeys.insert(cookieKey);

    }



    AIBinder_linkToDeath(binder.get(), mDeathRecipient.get(), reinterpret_cast<void*>(cookieKey));





    ALOGD("@@@ registered observer");

    ALOGD("@@@ registered observer");

    mRegistered = true;

    mRegistered = true;
@@ -141,7 +189,6 @@ void TranscodingResourcePolicy::unregisterSelf() { 
    ::ndk::SpAIBinder binder = mService->asBinder();

    ::ndk::SpAIBinder binder = mService->asBinder();

    if (binder.get() != nullptr) {

    if (binder.get() != nullptr) {

        Status status = mService->unregisterObserver(mObserver);

        Status status = mService->unregisterObserver(mObserver);

        AIBinder_unlinkToDeath(binder.get(), mDeathRecipient.get(), reinterpret_cast<void*>(this));

    }

    }





    mService = nullptr;

    mService = nullptr;

media/module/libmediatranscoding/include/media/TranscodingResourcePolicy.h

+4 −0
Original line number Original line Diff line number Diff line
@@ -22,6 +22,7 @@ 
#include <utils/Condition.h>

#include <utils/Condition.h>





#include <mutex>

#include <mutex>

#include <set>

namespace aidl {

namespace aidl {

namespace android {

namespace android {

namespace media {

namespace media {
@@ -48,6 +49,8 @@ private: 
    bool mRegistered GUARDED_BY(mRegisteredLock);

    bool mRegistered GUARDED_BY(mRegisteredLock);

    std::shared_ptr<IResourceObserverService> mService GUARDED_BY(mRegisteredLock);

    std::shared_ptr<IResourceObserverService> mService GUARDED_BY(mRegisteredLock);

    std::shared_ptr<ResourceObserver> mObserver;

    std::shared_ptr<ResourceObserver> mObserver;

    mutable std::mutex mCookieKeysLock;

    std::set<uintptr_t> mCookieKeys;





    mutable std::mutex mCallbackLock;

    mutable std::mutex mCallbackLock;

    std::weak_ptr<ResourcePolicyCallbackInterface> mResourcePolicyCallback

    std::weak_ptr<ResourcePolicyCallbackInterface> mResourcePolicyCallback
@@ -59,6 +62,7 @@ private: 
    static void BinderDiedCallback(void* cookie);

    static void BinderDiedCallback(void* cookie);





    void registerSelf();

    void registerSelf();

    // must delete the associated TranscodingResourcePolicyCookie any time this is called

    void unregisterSelf();

    void unregisterSelf();

    void onResourceAvailable(pid_t pid);

    void onResourceAvailable(pid_t pid);

};  // class TranscodingUidPolicy

};  // class TranscodingUidPolicy