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
diff --git a/media/libstagefright/include/OMXNodeInstance.h b/media/libstagefright/include/OMXNodeInstance.h
index ca24c2f..dae7380 100644
--- a/media/libstagefright/include/OMXNodeInstance.h
+++ b/media/libstagefright/include/OMXNodeInstance.h
@@ -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 @@
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;
diff --git a/media/libstagefright/omx/OMX.cpp b/media/libstagefright/omx/OMX.cpp
index 7907c62..80c125c 100644
--- a/media/libstagefright/omx/OMX.cpp
+++ b/media/libstagefright/omx/OMX.cpp
@@ -139,18 +139,20 @@
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;
+
+ // 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);
}
- mLiveNodes.removeItemsAt(index);
}
- IInterface::asBinder(instance->observer())->unlinkToDeath(this);
-
- OMX_ERRORTYPE err = OMX_ErrorNone;
- if (instance->handle() != NULL) {
- err = mMaster->destroyComponentInstance(
- static_cast<OMX_COMPONENTTYPE *>(instance->handle()));
- }
+ 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);
}
diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp
index ea86a37..bbfa48f 100644
--- a/media/libstagefright/omx/OMXNodeInstance.cpp
+++ b/media/libstagefright/omx/OMXNodeInstance.cpp
@@ -398,15 +398,9 @@
}
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 @@
// 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);
diff --git a/services/mediaresourcemanager/ResourceManagerService.cpp b/services/mediaresourcemanager/ResourceManagerService.cpp
index 7346f51..78bb587 100644
--- a/services/mediaresourcemanager/ResourceManagerService.cpp
+++ b/services/mediaresourcemanager/ResourceManagerService.cpp
@@ -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 @@
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 @@
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;
diff --git a/services/mediaresourcemanager/ResourceManagerService.h b/services/mediaresourcemanager/ResourceManagerService.h
index 2a4a6b2..9e97ac0 100644
--- a/services/mediaresourcemanager/ResourceManagerService.h
+++ b/services/mediaresourcemanager/ResourceManagerService.h
@@ -36,6 +36,7 @@
struct ResourceInfo {
int64_t clientId;
sp<IResourceManagerClient> client;
+ sp<IBinder::DeathRecipient> deathNotifier;
Vector<MediaResource> resources;
};
@@ -70,6 +71,8 @@
// 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();