SoundPool: Fix lifetime of SoundPool native object
Avoids race between release and Binder volume adjustment.
Test: atest SoundPoolAacTest
Test: atest SoundPoolHapticTest
Test: atest SoundPoolMidiTest
Test: atest SoundPoolOggTest
Test: atest AudioManagerTest#testSoundEffects
Bug: 223815163
Change-Id: Iac6ad878050ffc0cd1faa70abe11d3441ea34d8e
diff --git a/media/jni/soundpool/android_media_SoundPool.cpp b/media/jni/soundpool/android_media_SoundPool.cpp
index a66d99f..66870d3 100644
--- a/media/jni/soundpool/android_media_SoundPool.cpp
+++ b/media/jni/soundpool/android_media_SoundPool.cpp
@@ -33,10 +33,156 @@
jmethodID mPostEvent;
jclass mSoundPoolClass;
} fields;
-static inline SoundPool* MusterSoundPool(JNIEnv *env, jobject thiz) {
- // NOLINTNEXTLINE(performance-no-int-to-ptr)
- return reinterpret_cast<SoundPool*>(env->GetLongField(thiz, fields.mNativeContext));
+
+namespace {
+
+/**
+ * ObjectManager creates a native "object" on the heap and stores
+ * its pointer in a long field in a Java object.
+ *
+ * The type T must have 3 properties in the current implementation.
+ * 1) A T{} default constructor which represents a nullValue.
+ * 2) T::operator bool() const efficient detection of such a nullValue.
+ * 3) T must be copyable.
+ *
+ * Some examples of such a type T are std::shared_ptr<>, android::sp<>,
+ * std::optional, std::function<>, etc.
+ *
+ * Using set() with a nullValue T results in destroying the underlying native
+ * "object" if it exists. A nullValue T is returned by get() if there is
+ * no underlying native Object.
+ *
+ * This class is thread safe for multiple access.
+ *
+ * Design notes:
+ * 1) For objects of type T that do not naturally have an "nullValue",
+ * wrapping with
+ * a) TOpt, where TOpt = std::optional<T>
+ * b) TShared, where TShared = std::shared_ptr<T>
+ *
+ * 2) An overload for an explicit equality comparable nullValue such as
+ * get(..., const T& nullValue) or set(..., const T& nullValue)
+ * is omitted. An alternative is to pass a fixed nullValue in the constructor.
+ */
+template <typename T>
+class ObjectManager
+{
+// Can a jlong hold a pointer?
+static_assert(sizeof(jlong) >= sizeof(void*));
+
+public:
+ // fieldId is associated with a Java long member variable in the object.
+ // ObjectManager will store the native pointer in that field.
+ //
+ // If a native object is set() in that field, it
+ explicit ObjectManager(jfieldID fieldId) : mFieldId(fieldId) {}
+ ~ObjectManager() {
+ ALOGE_IF(mObjectCount != 0, "%s: mObjectCount: %d should be zero on destruction",
+ __func__, mObjectCount.load());
+ // Design note: it would be possible to keep a map of the outstanding allocated
+ // objects and force a delete on them on ObjectManager destruction.
+ // The consequences of that is probably worse than keeping them alive.
+ }
+
+ // Retrieves the associated object, returns nullValue T if not available.
+ T get(JNIEnv *env, jobject thiz) {
+ std::lock_guard lg(mLock);
+ // NOLINTNEXTLINE(performance-no-int-to-ptr)
+ auto ptr = reinterpret_cast<T*>(env->GetLongField(thiz, mFieldId));
+ if (ptr != nullptr) {
+ return *ptr;
+ }
+ return {};
+ }
+
+ // Sets the object and returns the old one.
+ //
+ // If the old object doesn't exist, then nullValue T is returned.
+ // If the new object is false by operator bool(), the internal object is destroyed.
+ // Note: The old object is returned so if T is a smart pointer, it can be held
+ // by the caller to be deleted outside of any external lock.
+ //
+ // Remember to call set(env, thiz, {}) to destroy the object in the Java
+ // object finalize to avoid orphaned objects on the heap.
+ T set(JNIEnv *env, jobject thiz, const T& newObject) {
+ std::lock_guard lg(mLock);
+ // NOLINTNEXTLINE(performance-no-int-to-ptr)
+ auto ptr = reinterpret_cast<T*>(env->GetLongField(thiz, mFieldId));
+ if (ptr != nullptr) {
+ T old = std::move(*ptr); // *ptr will be replaced or deleted.
+ if (newObject) {
+ env->SetLongField(thiz, mFieldId, (jlong)0);
+ delete ptr;
+ --mObjectCount;
+ } else {
+ *ptr = newObject;
+ }
+ return old;
+ } else {
+ if (newObject) {
+ env->SetLongField(thiz, mFieldId, (jlong)new T(newObject));
+ ++mObjectCount;
+ }
+ return {};
+ }
+ }
+
+ // Returns the number of outstanding objects.
+ //
+ // This is purely for debugging purposes and tracks the number of active Java
+ // objects that have native T objects; hence represents the number of
+ // T heap allocations we have made.
+ //
+ // When all those Java objects have been finalized we expect this to go to 0.
+ int32_t getObjectCount() const {
+ return mObjectCount;
+ }
+
+private:
+ // NOLINTNEXTLINE(misc-misplaced-const)
+ const jfieldID mFieldId; // '_jfieldID *const'
+
+ // mObjectCount is the number of outstanding native T heap allocations we have
+ // made (and thus the number of active Java objects which are associated with them).
+ std::atomic_int32_t mObjectCount{};
+
+ mutable std::mutex mLock;
+};
+
+// We use SoundPoolManager to associate a native std::shared_ptr<SoundPool>
+// object with a field in the Java object.
+//
+// We can then retrieve the std::shared_ptr<SoundPool> from the object.
+//
+// Design notes:
+// 1) This is based on ObjectManager class.
+// 2) An alternative that does not require a field in the Java object
+// is to create an associative map using as a key a NewWeakGlobalRef
+// to the Java object.
+// The problem of this method is that lookup is O(N) because comparison
+// between the WeakGlobalRef to a JNI jobject LocalRef must be done
+// through the JNI IsSameObject() call, hence iterative through the map.
+// One advantage of this method is that manual garbage collection
+// is possible by checking if the WeakGlobalRef is null equivalent.
+
+auto& getSoundPoolManager() {
+ static ObjectManager<std::shared_ptr<SoundPool>> soundPoolManager(fields.mNativeContext);
+ return soundPoolManager;
}
+
+inline auto getSoundPool(JNIEnv *env, jobject thiz) {
+ return getSoundPoolManager().get(env, thiz);
+}
+
+// Note: one must call setSoundPool(env, thiz, nullptr) to release any native resources
+// somewhere in the Java object finalize().
+inline auto setSoundPool(
+ JNIEnv *env, jobject thiz, const std::shared_ptr<SoundPool>& soundPool) {
+ return getSoundPoolManager().set(env, thiz, soundPool);
+}
+
+} // namespace
+
static const char* const kAudioAttributesClassPathName = "android/media/AudioAttributes";
struct audio_attributes_fields_t {
jfieldID fieldUsage; // AudioAttributes.mUsage
@@ -53,18 +199,18 @@
jlong offset, jlong length, jint priority)
{
ALOGV("android_media_SoundPool_load_FD");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return 0;
- return (jint) ap->load(jniGetFDFromFileDescriptor(env, fileDescriptor),
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return 0;
+ return (jint) soundPool->load(jniGetFDFromFileDescriptor(env, fileDescriptor),
int64_t(offset), int64_t(length), int(priority));
}
static jboolean
android_media_SoundPool_unload(JNIEnv *env, jobject thiz, jint sampleID) {
ALOGV("android_media_SoundPool_unload\n");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return JNI_FALSE;
- return ap->unload(sampleID) ? JNI_TRUE : JNI_FALSE;
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return JNI_FALSE;
+ return soundPool->unload(sampleID) ? JNI_TRUE : JNI_FALSE;
}
static jint
@@ -73,54 +219,54 @@
jfloat rate)
{
ALOGV("android_media_SoundPool_play\n");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return 0;
- return (jint) ap->play(sampleID, leftVolume, rightVolume, priority, loop, rate);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return 0;
+ return (jint) soundPool->play(sampleID, leftVolume, rightVolume, priority, loop, rate);
}
static void
android_media_SoundPool_pause(JNIEnv *env, jobject thiz, jint channelID)
{
ALOGV("android_media_SoundPool_pause");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->pause(channelID);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->pause(channelID);
}
static void
android_media_SoundPool_resume(JNIEnv *env, jobject thiz, jint channelID)
{
ALOGV("android_media_SoundPool_resume");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->resume(channelID);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->resume(channelID);
}
static void
android_media_SoundPool_autoPause(JNIEnv *env, jobject thiz)
{
ALOGV("android_media_SoundPool_autoPause");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->autoPause();
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->autoPause();
}
static void
android_media_SoundPool_autoResume(JNIEnv *env, jobject thiz)
{
ALOGV("android_media_SoundPool_autoResume");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->autoResume();
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->autoResume();
}
static void
android_media_SoundPool_stop(JNIEnv *env, jobject thiz, jint channelID)
{
ALOGV("android_media_SoundPool_stop");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->stop(channelID);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->stop(channelID);
}
static void
@@ -128,18 +274,18 @@
jfloat leftVolume, jfloat rightVolume)
{
ALOGV("android_media_SoundPool_setVolume");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->setVolume(channelID, (float) leftVolume, (float) rightVolume);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->setVolume(channelID, (float) leftVolume, (float) rightVolume);
}
static void
android_media_SoundPool_mute(JNIEnv *env, jobject thiz, jboolean muting)
{
ALOGV("android_media_SoundPool_mute(%d)", muting);
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->mute(muting == JNI_TRUE);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->mute(muting == JNI_TRUE);
}
static void
@@ -147,9 +293,9 @@
jint priority)
{
ALOGV("android_media_SoundPool_setPriority");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->setPriority(channelID, (int) priority);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->setPriority(channelID, (int) priority);
}
static void
@@ -157,9 +303,9 @@
int loop)
{
ALOGV("android_media_SoundPool_setLoop");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->setLoop(channelID, loop);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->setLoop(channelID, loop);
}
static void
@@ -167,9 +313,9 @@
jfloat rate)
{
ALOGV("android_media_SoundPool_setRate");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap == nullptr) return;
- ap->setRate(channelID, (float) rate);
+ auto soundPool = getSoundPool(env, thiz);
+ if (soundPool == nullptr) return;
+ soundPool->setRate(channelID, (float) rate);
}
static void android_media_callback(SoundPoolEvent event, SoundPool* soundPool, void* user)
@@ -206,17 +352,16 @@
ALOGV("android_media_SoundPool_native_setup");
ScopedUtfChars opPackageNameStr(env, opPackageName);
- auto *ap = new SoundPool(maxChannels, paa, opPackageNameStr.c_str());
- if (ap == nullptr) {
- return -1;
- }
-
- // save pointer to SoundPool C++ object in opaque field in Java object
- env->SetLongField(thiz, fields.mNativeContext, (jlong) ap);
+ auto soundPool = std::make_shared<SoundPool>(maxChannels, paa, opPackageNameStr.c_str());
// set callback with weak reference
jobject globalWeakRef = env->NewGlobalRef(weakRef);
- ap->setCallback(android_media_callback, globalWeakRef);
+ soundPool->setCallback(android_media_callback, globalWeakRef);
+
+ // register with SoundPoolManager.
+ auto oldSoundPool = setSoundPool(env, thiz, soundPool);
+ ALOGW_IF(oldSoundPool != nullptr, "%s: Aliased SoundPool object %p",
+ __func__, oldSoundPool.get());
// audio attributes were copied in SoundPool creation
free(paa);
@@ -228,20 +373,23 @@
android_media_SoundPool_release(JNIEnv *env, jobject thiz)
{
ALOGV("android_media_SoundPool_release");
- SoundPool *ap = MusterSoundPool(env, thiz);
- if (ap != nullptr) {
+ // Remove us from SoundPoolManager.
+ auto oldSoundPool = setSoundPool(env, thiz, nullptr);
+
+ // Caution: Deleting the weakRef is not race free from invoking
+ // the Java callback because we may not have the last remaining
+ // reference to the SoundPool object - another method could still
+ // be in progress.
+ if (oldSoundPool != nullptr) {
// release weak reference and clear callback
- auto weakRef = (jobject) ap->getUserData();
- ap->setCallback(nullptr /* callback */, nullptr /* user */);
+ auto weakRef = (jobject) oldSoundPool->getUserData();
+ oldSoundPool->setCallback(nullptr /* callback */, nullptr /* user */);
if (weakRef != nullptr) {
env->DeleteGlobalRef(weakRef);
}
-
- // clear native context
- env->SetLongField(thiz, fields.mNativeContext, 0);
- delete ap;
}
+ // destructor to oldSoundPool should occur at exit.
}
// ----------------------------------------------------------------------------