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

Commit 3e378967 authored by Wonsik Kim's avatar Wonsik Kim
Browse files

stagefright: fix unreleased OMX handle 

- Ensure OMX handle is freed even if binder death notification comes
first.
- Add DeathRecipient in ResourceManagerService so that it could
handle dead clients properly.

Fix: 28824626
Fix: 34252788
Test: adb shell am instrument -e size small -w 'android.media.cts/android.support.test.runner.AndroidJUnitRunner'
Change-Id: Ifc441a2771b5674749ff65a4520177dda115b292
parent dffb51f4

media/libstagefright/include/OMXNodeInstance.h

+3 −1
Original line number Diff line number Diff line
@@ -18,6 +18,8 @@ 


#define OMX_NODE_INSTANCE_H_



#include <atomic>



#include <media/IOMX.h>

#include <utils/RefBase.h>

#include <utils/threads.h>
@@ -111,7 +113,7 @@ private: 
    OMX_HANDLETYPE mHandle;

    sp<IOMXObserver> mObserver;

    sp<CallbackDispatcher> mDispatcher;

    bool mDying;

    std::atomic_bool mDying;

    bool mSailed;  // configuration is set (no more meta-mode changes)

    bool mQueriedProhibitedExtensions;

    SortedVector<OMX_INDEXTYPE> mProhibitedExtensions;

media/libstagefright/omx/OMX.cpp

+11 −9
Original line number Diff line number Diff line
@@ -139,18 +139,20 @@ status_t OMX::freeNode(const sp<OMXNodeInstance> &instance) { 
        if (index < 0) {

            // This could conceivably happen if the observer dies at roughly the

            // same time that a client attempts to free the node explicitly.

            return OK;

        }

        mLiveNodes.removeItemsAt(index);

    }



            // NOTE: it's guaranteed that this method is called at most once per

            //       instance.

            ALOGV("freeNode: instance already removed from book-keeping.");

        } else {

            mLiveNodes.removeItemsAt(index);

            IInterface::asBinder(instance->observer())->unlinkToDeath(this);

        }

    }



    OMX_ERRORTYPE err = OMX_ErrorNone;

    if (instance->handle() != NULL) {

        err = mMaster->destroyComponentInstance(

    CHECK(instance->handle() != NULL);

    OMX_ERRORTYPE err = mMaster->destroyComponentInstance(

            static_cast<OMX_COMPONENTTYPE *>(instance->handle()));

    }

    ALOGV("freeNode: handle destroyed: %p", instance->handle());



    return StatusFromOMXError(err);

}

media/libstagefright/omx/OMXNodeInstance.cpp

+7 −7
Original line number Diff line number Diff line
@@ -398,15 +398,9 @@ sp<IOMXObserver> OMXNodeInstance::observer() { 
}



status_t OMXNodeInstance::freeNode() {



    CLOG_LIFE(freeNode, "handle=%p", mHandle);

    static int32_t kMaxNumIterations = 10;



    // exit if we have already freed the node

    if (mHandle == NULL) {

        return mOwner->freeNode(this);

    }



    // Transition the node from its current state all the way down

    // to "Loaded".

    // This ensures that all active buffers are properly freed even
@@ -416,7 +410,13 @@ status_t OMXNodeInstance::freeNode() { 
    // The code below may trigger some more events to be dispatched

    // by the OMX component - we want to ignore them as our client

    // does not expect them.

    mDying = true;

    bool expected = false;

    if (!mDying.compare_exchange_strong(expected, true)) {

        // exit if we have already freed the node or doing so right now.

        // NOTE: this ensures that the block below executes at most once.

        ALOGV("Already dying");

        return OK;

    }



    OMX_STATETYPE state;

    CHECK_EQ(OMX_GetState(mHandle, &state), OMX_ErrorNone);

services/mediaresourcemanager/ResourceManagerService.cpp

+35 −1
Original line number Diff line number Diff line
@@ -34,6 +34,31 @@ 


namespace android {



namespace {



class DeathNotifier : public IBinder::DeathRecipient {

public:

    DeathNotifier(const wp<ResourceManagerService> &service, int pid, int64_t clientId)

        : mService(service), mPid(pid), mClientId(clientId) {}



    virtual void binderDied(const wp<IBinder> & /* who */) override {

        // Don't check for pid validity since we know it's already dead.

        sp<ResourceManagerService> service = mService.promote();

        if (service == nullptr) {

            ALOGW("ResourceManagerService is dead as well.");

            return;

        }

        service->removeResource(mPid, mClientId, false);

    }



private:

    wp<ResourceManagerService> mService;

    int mPid;

    int64_t mClientId;

};



}  // namespace



template <typename T>

static String8 getString(const Vector<T> &items) {

    String8 itemsStr;
@@ -214,17 +239,25 @@ void ResourceManagerService::addResource( 
    ResourceInfo& info = getResourceInfoForEdit(clientId, client, infos);

    // TODO: do the merge instead of append.

    info.resources.appendVector(resources);

    if (info.deathNotifier == nullptr) {

        info.deathNotifier = new DeathNotifier(this, pid, clientId);

        IInterface::asBinder(client)->linkToDeath(info.deathNotifier);

    }

    notifyResourceGranted(pid, resources);

}



void ResourceManagerService::removeResource(int pid, int64_t clientId) {

    removeResource(pid, clientId, true);

}



void ResourceManagerService::removeResource(int pid, int64_t clientId, bool checkValid) {

    String8 log = String8::format(

            "removeResource(pid %d, clientId %lld)",

            pid, (long long) clientId);

    mServiceLog->add(log);



    Mutex::Autolock lock(mLock);

    if (!mProcessInfo->isValidPid(pid)) {

    if (checkValid && !mProcessInfo->isValidPid(pid)) {

        ALOGE("Rejected removeResource call with invalid pid.");

        return;

    }
@@ -237,6 +270,7 @@ void ResourceManagerService::removeResource(int pid, int64_t clientId) { 
    ResourceInfos &infos = mMap.editValueAt(index);

    for (size_t j = 0; j < infos.size(); ++j) {

        if (infos[j].clientId == clientId) {

            IInterface::asBinder(infos[j].client)->unlinkToDeath(infos[j].deathNotifier);

            j = infos.removeAt(j);

            found = true;

            break;

services/mediaresourcemanager/ResourceManagerService.h

+3 −0
Original line number Diff line number Diff line
@@ -36,6 +36,7 @@ struct ProcessInfoInterface; 
struct ResourceInfo {

    int64_t clientId;

    sp<IResourceManagerClient> client;

    sp<IBinder::DeathRecipient> deathNotifier;

    Vector<MediaResource> resources;

};
@@ -70,6 +71,8 @@ public: 
    // Returns true if any resource has been reclaimed, otherwise returns false.

    virtual bool reclaimResource(int callingPid, const Vector<MediaResource> &resources);



    void removeResource(int pid, int64_t clientId, bool checkValid);



protected:

    virtual ~ResourceManagerService();