From 7d6d9bd6c3693563ffe14a32d5aac08df69bb04c Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Wed, 8 Sep 2021 17:58:53 -0700 Subject: [PATCH 1/2] Analyze native crash in AudioEffect via event listener Add repro code to the EffectsTest app. Steps: 1. Start playback on the "Environmental Reverb Test", memorize the session id. 2. Enter the session id on the "Bass Boost Test" or the "Visualizer" tab. 3. Press "Hammer on release()" multiple times and check if the app crashes. Currently this happens for the Bass Boost effect. Bug: 178363662 Test: see above Change-Id: I800bc352fc8bc7073d1523c837008dc6fcfb957a --- media/tests/EffectsTest/AndroidManifest.xml | 4 + .../EffectsTest/res/layout/bassboosttest.xml | 5 ++ .../EffectsTest/res/layout/visualizertest.xml | 5 ++ .../tests/EffectsTest/res/values/strings.xml | 2 + .../android/effectstest/BassBoostTest.java | 84 +++++++++++++++---- .../android/effectstest/VisualizerTest.java | 60 +++++++++++++ 6 files changed, 145 insertions(+), 15 deletions(-) diff --git a/media/tests/EffectsTest/AndroidManifest.xml b/media/tests/EffectsTest/AndroidManifest.xml index 9b59891fb2796..ad0c10ee38072 100644 --- a/media/tests/EffectsTest/AndroidManifest.xml +++ b/media/tests/EffectsTest/AndroidManifest.xml @@ -13,6 +13,10 @@ See the License for the specific language governing permissions and limitations under the License. --> + diff --git a/media/tests/EffectsTest/res/layout/bassboosttest.xml b/media/tests/EffectsTest/res/layout/bassboosttest.xml index ac912c84d107b..5f9132cb8cc77 100644 --- a/media/tests/EffectsTest/res/layout/bassboosttest.xml +++ b/media/tests/EffectsTest/res/layout/bassboosttest.xml @@ -187,6 +187,11 @@ android:layout_height="wrap_content" android:scaleType="fitXY"/> + + diff --git a/media/tests/EffectsTest/res/layout/visualizertest.xml b/media/tests/EffectsTest/res/layout/visualizertest.xml index 18d7a3648fbfe..85dabbc115f3f 100644 --- a/media/tests/EffectsTest/res/layout/visualizertest.xml +++ b/media/tests/EffectsTest/res/layout/visualizertest.xml @@ -175,6 +175,11 @@ + + Send Level Multithreaded Use + + Hammer on release() diff --git a/media/tests/EffectsTest/src/com/android/effectstest/BassBoostTest.java b/media/tests/EffectsTest/src/com/android/effectstest/BassBoostTest.java index cce2acc5869a8..a207bf1d53597 100644 --- a/media/tests/EffectsTest/src/com/android/effectstest/BassBoostTest.java +++ b/media/tests/EffectsTest/src/com/android/effectstest/BassBoostTest.java @@ -17,29 +17,24 @@ package com.android.effectstest; import android.app.Activity; -import android.content.Context; -import android.content.Intent; +import android.media.audiofx.AudioEffect; +import android.media.audiofx.BassBoost; import android.os.Bundle; import android.util.Log; import android.view.KeyEvent; -import android.view.Menu; -import android.view.View.OnClickListener; import android.view.View; -import android.view.ViewGroup; +import android.view.View.OnClickListener; import android.widget.Button; -import android.widget.TextView; -import android.widget.EditText; -import android.widget.SeekBar; -import android.widget.ToggleButton; import android.widget.CompoundButton; import android.widget.CompoundButton.OnCheckedChangeListener; -import java.nio.ByteOrder; -import java.nio.ByteBuffer; -import java.util.HashMap; -import java.util.Map; +import android.widget.EditText; +import android.widget.SeekBar; +import android.widget.TextView; +import android.widget.ToggleButton; -import android.media.audiofx.BassBoost; -import android.media.audiofx.AudioEffect; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.HashMap; public class BassBoostTest extends Activity implements OnCheckedChangeListener { @@ -78,6 +73,9 @@ public class BassBoostTest extends Activity implements OnCheckedChangeListener { mReleaseButton = (ToggleButton)findViewById(R.id.bbReleaseButton); mOnOffButton = (ToggleButton)findViewById(R.id.bassboostOnOff); + final Button hammerReleaseTest = (Button) findViewById(R.id.hammer_on_release_bug); + hammerReleaseTest.setEnabled(false); + getEffect(sSession); if (mBassBoost != null) { @@ -93,6 +91,14 @@ public class BassBoostTest extends Activity implements OnCheckedChangeListener { mStrength = new BassBoostParam(mBassBoost, 0, 1000, seekBar, textView); seekBar.setOnSeekBarChangeListener(mStrength); mStrength.setEnabled(mBassBoost.getStrengthSupported()); + + hammerReleaseTest.setEnabled(true); + hammerReleaseTest.setOnClickListener(new OnClickListener() { + @Override + public void onClick(View v) { + runHammerReleaseTest(hammerReleaseTest); + } + }); } } @@ -273,4 +279,52 @@ public class BassBoostTest extends Activity implements OnCheckedChangeListener { } } + // Stress-tests releasing of AudioEffect by doing repeated creation + // and subsequent releasing. Also forces emission of callbacks from + // the AudioFlinger by setting a control status listener. Since all + // effect instances are bound to the same session, the AF will + // notify them about the change in their status. This can reveal racy + // behavior w.r.t. releasing. + class HammerReleaseTest extends Thread { + private static final int NUM_EFFECTS = 10; + private static final int NUM_ITERATIONS = 100; + private final int mSession; + private final Runnable mOnComplete; + + HammerReleaseTest(int session, Runnable onComplete) { + mSession = session; + mOnComplete = onComplete; + } + + @Override + public void run() { + Log.w(TAG, "HammerReleaseTest started"); + BassBoost[] effects = new BassBoost[NUM_EFFECTS]; + for (int i = 0; i < NUM_ITERATIONS; i++) { + for (int j = 0; j < NUM_EFFECTS; j++) { + effects[j] = new BassBoost(0, mSession); + effects[j].setControlStatusListener(mEffectListener); + yield(); + } + for (int j = NUM_EFFECTS - 1; j >= 0; j--) { + Log.w(TAG, "HammerReleaseTest releasing effect " + (Object) effects[j]); + effects[j].release(); + effects[j] = null; + yield(); + } + } + Log.w(TAG, "HammerReleaseTest ended"); + runOnUiThread(mOnComplete); + } + } + + private void runHammerReleaseTest(Button controlButton) { + controlButton.setEnabled(false); + HammerReleaseTest thread = new HammerReleaseTest(sSession, + () -> { + controlButton.setEnabled(true); + }); + thread.start(); + } + } diff --git a/media/tests/EffectsTest/src/com/android/effectstest/VisualizerTest.java b/media/tests/EffectsTest/src/com/android/effectstest/VisualizerTest.java index 2e141c5ef7c8a..dcfe11a79ef92 100644 --- a/media/tests/EffectsTest/src/com/android/effectstest/VisualizerTest.java +++ b/media/tests/EffectsTest/src/com/android/effectstest/VisualizerTest.java @@ -17,6 +17,7 @@ package com.android.effectstest; import android.app.Activity; +import android.media.audiofx.Visualizer; import android.os.Bundle; import android.os.Handler; import android.os.Looper; @@ -24,6 +25,8 @@ import android.os.Message; import android.util.Log; import android.view.KeyEvent; import android.view.View; +import android.view.View.OnClickListener; +import android.widget.Button; import android.widget.CompoundButton; import android.widget.CompoundButton.OnCheckedChangeListener; import android.widget.EditText; @@ -74,11 +77,22 @@ public class VisualizerTest extends Activity implements OnCheckedChangeListener mCallbackOn = false; mCallbackButton.setChecked(mCallbackOn); + final Button hammerReleaseTest = (Button) findViewById(R.id.hammer_on_release_bug); + hammerReleaseTest.setEnabled(false); + mMultithreadedButton.setOnCheckedChangeListener(this); if (getEffect(sSession) != null) { mReleaseButton.setOnCheckedChangeListener(this); mOnOffButton.setOnCheckedChangeListener(this); mCallbackButton.setOnCheckedChangeListener(this); + + hammerReleaseTest.setEnabled(true); + hammerReleaseTest.setOnClickListener(new OnClickListener() { + @Override + public void onClick(View v) { + runHammerReleaseTest(hammerReleaseTest); + } + }); } } @@ -214,4 +228,50 @@ public class VisualizerTest extends Activity implements OnCheckedChangeListener } } + // Stress-tests releasing of AudioEffect by doing repeated creation + // and subsequent releasing. Unlike a similar class in BassBoostTest, + // this one doesn't sets a control status listener because Visualizer + // doesn't inherit from AudioEffect and doesn't implement this method + // by itself. + class HammerReleaseTest extends Thread { + private static final int NUM_EFFECTS = 10; + private static final int NUM_ITERATIONS = 100; + private final int mSession; + private final Runnable mOnComplete; + + HammerReleaseTest(int session, Runnable onComplete) { + mSession = session; + mOnComplete = onComplete; + } + + @Override + public void run() { + Log.w(TAG, "HammerReleaseTest started"); + Visualizer[] effects = new Visualizer[NUM_EFFECTS]; + for (int i = 0; i < NUM_ITERATIONS; i++) { + for (int j = 0; j < NUM_EFFECTS; j++) { + effects[j] = new Visualizer(mSession); + yield(); + } + for (int j = NUM_EFFECTS - 1; j >= 0; j--) { + Log.w(TAG, "HammerReleaseTest releasing effect " + (Object) effects[j]); + effects[j].release(); + effects[j] = null; + yield(); + } + } + Log.w(TAG, "HammerReleaseTest ended"); + runOnUiThread(mOnComplete); + } + } + + private void runHammerReleaseTest(Button controlButton) { + controlButton.setEnabled(false); + HammerReleaseTest thread = new HammerReleaseTest(sSession, + () -> { + controlButton.setEnabled(true); + }); + thread.start(); + } + } From 9690a02f80812f29e257cea46462916be31e6042 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Tue, 14 Sep 2021 10:58:48 -0700 Subject: [PATCH 2/2] AudioEffect: fix racy access to callback data Since the EffectCallback in android::AudioEffect can keep an instance of the latter alive for a longer time than Java AudioEffect is alive, the callback need to ensure that it's not trying to use deallocated objects and deleted JVM references. Bug: 178363662 Test: manual using EffectsTest app Test: atest CtsMediaTestCases:AudioEffectTest Test: atest CtsMediaTestCases:BassBoostTest Test: atest CtsMediaTestCases:DynamicsProcessingTest Test: atest CtsMediaTestCases:EnvReverbTest Test: atest CtsMediaTestCases:EqualizerTest Test: atest CtsMediaTestCases:LoudnessEnhancerTest Test: atest CtsMediaTestCases:PresetReverbTest Test: atest CtsMediaTestCases:VisualizerTest Change-Id: I8c36f5af4a1a2b14e81b1344570d2f8746ffa91e --- .../audioeffect/android_media_AudioEffect.cpp | 52 +++++++---- .../audioeffect/android_media_Visualizer.cpp | 92 +++++++++++++++---- 2 files changed, 110 insertions(+), 34 deletions(-) diff --git a/media/jni/audioeffect/android_media_AudioEffect.cpp b/media/jni/audioeffect/android_media_AudioEffect.cpp index 0d53ab1521299..2691983a0e3a0 100644 --- a/media/jni/audioeffect/android_media_AudioEffect.cpp +++ b/media/jni/audioeffect/android_media_AudioEffect.cpp @@ -14,8 +14,8 @@ * limitations under the License. */ - #include +#include //#define LOG_NDEBUG 0 #define LOG_TAG "AudioEffects-JNI" @@ -58,21 +58,14 @@ static fields_t fields; struct effect_callback_cookie { jclass audioEffect_class; // AudioEffect class jobject audioEffect_ref; // AudioEffect object instance - }; - -// ---------------------------------------------------------------------------- -class AudioEffectJniStorage { - public: - effect_callback_cookie mCallbackData; - - AudioEffectJniStorage() { - } - - ~AudioEffectJniStorage() { - } - + bool busy; + Condition cond; }; +// ---------------------------------------------------------------------------- +struct AudioEffectJniStorage { + effect_callback_cookie mCallbackData{}; +}; jint AudioEffectJni::translateNativeErrorToJava(int code) { switch(code) { @@ -101,6 +94,7 @@ jint AudioEffectJni::translateNativeErrorToJava(int code) { } static Mutex sLock; +static std::unordered_set sAudioEffectCallBackCookies; // ---------------------------------------------------------------------------- static void effectCallback(int event, void* user, void *info) { @@ -121,7 +115,13 @@ static void effectCallback(int event, void* user, void *info) { ALOGW("effectCallback error user %p, env %p", user, env); return; } - + { + Mutex::Autolock l(sLock); + if (sAudioEffectCallBackCookies.count(callbackInfo) == 0) { + return; + } + callbackInfo->busy = true; + } ALOGV("effectCallback: callbackInfo %p, audioEffect_ref %p audioEffect_class %p", callbackInfo, callbackInfo->audioEffect_ref, @@ -188,6 +188,11 @@ effectCallback_Exit: env->ExceptionDescribe(); env->ExceptionClear(); } + { + Mutex::Autolock l(sLock); + callbackInfo->busy = false; + callbackInfo->cond.broadcast(); + } } // ---------------------------------------------------------------------------- @@ -396,6 +401,10 @@ android_media_AudioEffect_native_setup(JNIEnv *env, jobject thiz, jobject weak_t setAudioEffect(env, thiz, lpAudioEffect); } + { + Mutex::Autolock l(sLock); + sAudioEffectCallBackCookies.insert(&lpJniStorage->mCallbackData); + } env->SetLongField(thiz, fields.fidJniData, (jlong)lpJniStorage); return (jint) AUDIOEFFECT_SUCCESS; @@ -427,6 +436,7 @@ setup_failure: // ---------------------------------------------------------------------------- +#define CALLBACK_COND_WAIT_TIMEOUT_MS 1000 static void android_media_AudioEffect_native_release(JNIEnv *env, jobject thiz) { sp lpAudioEffect = setAudioEffect(env, thiz, 0); if (lpAudioEffect == 0) { @@ -442,7 +452,17 @@ static void android_media_AudioEffect_native_release(JNIEnv *env, jobject thiz) env->SetLongField(thiz, fields.fidJniData, 0); if (lpJniStorage) { - ALOGV("deleting pJniStorage: %p\n", lpJniStorage); + Mutex::Autolock l(sLock); + effect_callback_cookie *lpCookie = &lpJniStorage->mCallbackData; + ALOGV("deleting lpJniStorage: %p\n", lpJniStorage); + sAudioEffectCallBackCookies.erase(lpCookie); + while (lpCookie->busy) { + if (lpCookie->cond.waitRelative(sLock, + milliseconds(CALLBACK_COND_WAIT_TIMEOUT_MS)) != + NO_ERROR) { + break; + } + } env->DeleteGlobalRef(lpJniStorage->mCallbackData.audioEffect_class); env->DeleteGlobalRef(lpJniStorage->mCallbackData.audioEffect_ref); delete lpJniStorage; diff --git a/media/jni/audioeffect/android_media_Visualizer.cpp b/media/jni/audioeffect/android_media_Visualizer.cpp index 4c5970a30a054..609fafee4d6ab 100644 --- a/media/jni/audioeffect/android_media_Visualizer.cpp +++ b/media/jni/audioeffect/android_media_Visualizer.cpp @@ -15,6 +15,7 @@ */ #include +#include //#define LOG_NDEBUG 0 #define LOG_TAG "visualizers-JNI" @@ -63,6 +64,12 @@ struct visualizer_callback_cookie { jclass visualizer_class; // Visualizer class jobject visualizer_ref; // Visualizer object instance + // 'busy_count' and 'cond' together with 'sLock' are used to serialize + // concurrent access to the callback cookie from 'setup'/'release' + // and the callback. + int busy_count; + Condition cond; + // Lazily allocated arrays used to hold callback data provided to java // applications. These arrays are allocated during the first callback and // reallocated when the size of the callback data changes. Allocating on @@ -70,14 +77,12 @@ struct visualizer_callback_cookie { // reference to the provided data (they need to make a copy if they want to // hold onto outside of the callback scope), but it avoids GC thrash caused // by constantly allocating and releasing arrays to hold callback data. + // 'callback_data_lock' must never be held at the same time with 'sLock'. Mutex callback_data_lock; jbyteArray waveform_data; jbyteArray fft_data; - visualizer_callback_cookie() { - waveform_data = NULL; - fft_data = NULL; - } + // Assumes use of default initialization by the client. ~visualizer_callback_cookie() { cleanupBuffers(); @@ -102,15 +107,8 @@ struct visualizer_callback_cookie { }; // ---------------------------------------------------------------------------- -class VisualizerJniStorage { - public: - visualizer_callback_cookie mCallbackData; - - VisualizerJniStorage() { - } - - ~VisualizerJniStorage() { - } +struct VisualizerJniStorage { + visualizer_callback_cookie mCallbackData{}; }; @@ -136,6 +134,7 @@ static jint translateError(int code) { } static Mutex sLock; +static std::unordered_set sVisualizerCallBackCookies; // ---------------------------------------------------------------------------- static void ensureArraySize(JNIEnv *env, jbyteArray *array, uint32_t size) { @@ -173,11 +172,19 @@ static void captureCallback(void* user, return; } + { + Mutex::Autolock l(sLock); + if (sVisualizerCallBackCookies.count(callbackInfo) == 0) { + return; + } + callbackInfo->busy_count++; + } ALOGV("captureCallback: callbackInfo %p, visualizer_ref %p visualizer_class %p", callbackInfo, callbackInfo->visualizer_ref, callbackInfo->visualizer_class); + { AutoMutex lock(&callbackInfo->callback_data_lock); if (waveformSize != 0 && waveform != NULL) { @@ -219,11 +226,17 @@ static void captureCallback(void* user, jArray); } } + } // callback_data_lock scope if (env->ExceptionCheck()) { env->ExceptionDescribe(); env->ExceptionClear(); } + { + Mutex::Autolock l(sLock); + callbackInfo->busy_count--; + callbackInfo->cond.broadcast(); + } } // ---------------------------------------------------------------------------- @@ -332,16 +345,41 @@ static void android_media_visualizer_effect_callback(int32_t event, void *info) { if ((event == AudioEffect::EVENT_ERROR) && (*((status_t*)info) == DEAD_OBJECT)) { - VisualizerJniStorage* lpJniStorage = (VisualizerJniStorage*)user; - visualizer_callback_cookie* callbackInfo = &lpJniStorage->mCallbackData; + visualizer_callback_cookie* callbackInfo = + (visualizer_callback_cookie *)user; JNIEnv *env = AndroidRuntime::getJNIEnv(); + if (!user || !env) { + ALOGW("effectCallback error user %p, env %p", user, env); + return; + } + { + Mutex::Autolock l(sLock); + if (sVisualizerCallBackCookies.count(callbackInfo) == 0) { + return; + } + callbackInfo->busy_count++; + } + ALOGV("effectCallback: callbackInfo %p, visualizer_ref %p visualizer_class %p", + callbackInfo, + callbackInfo->visualizer_ref, + callbackInfo->visualizer_class); + env->CallStaticVoidMethod( callbackInfo->visualizer_class, fields.midPostNativeEvent, callbackInfo->visualizer_ref, NATIVE_EVENT_SERVER_DIED, 0, NULL); + if (env->ExceptionCheck()) { + env->ExceptionDescribe(); + env->ExceptionClear(); + } + { + Mutex::Autolock l(sLock); + callbackInfo->busy_count--; + callbackInfo->cond.broadcast(); + } } } @@ -389,7 +427,7 @@ android_media_visualizer_native_setup(JNIEnv *env, jobject thiz, jobject weak_th } lpVisualizer->set(0, android_media_visualizer_effect_callback, - lpJniStorage, + &lpJniStorage->mCallbackData, (audio_session_t) sessionId); lStatus = translateError(lpVisualizer->initCheck()); @@ -410,6 +448,10 @@ android_media_visualizer_native_setup(JNIEnv *env, jobject thiz, jobject weak_th setVisualizer(env, thiz, lpVisualizer); + { + Mutex::Autolock l(sLock); + sVisualizerCallBackCookies.insert(&lpJniStorage->mCallbackData); + } env->SetLongField(thiz, fields.fidJniData, (jlong)lpJniStorage); return VISUALIZER_SUCCESS; @@ -432,13 +474,15 @@ setup_failure: } // ---------------------------------------------------------------------------- +#define CALLBACK_COND_WAIT_TIMEOUT_MS 1000 static void android_media_visualizer_native_release(JNIEnv *env, jobject thiz) { - { //limit scope so that lpVisualizer is deleted before JNI storage data. + { sp lpVisualizer = setVisualizer(env, thiz, 0); if (lpVisualizer == 0) { return; } lpVisualizer->release(); + // Visualizer can still can be held by AudioEffect::EffectClient } // delete the JNI data VisualizerJniStorage* lpJniStorage = @@ -449,9 +493,22 @@ static void android_media_visualizer_native_release(JNIEnv *env, jobject thiz) env->SetLongField(thiz, fields.fidJniData, 0); if (lpJniStorage) { + { + Mutex::Autolock l(sLock); + visualizer_callback_cookie *lpCookie = &lpJniStorage->mCallbackData; ALOGV("deleting pJniStorage: %p\n", lpJniStorage); + sVisualizerCallBackCookies.erase(lpCookie); + while (lpCookie->busy_count > 0) { + if (lpCookie->cond.waitRelative(sLock, + milliseconds(CALLBACK_COND_WAIT_TIMEOUT_MS)) != + NO_ERROR) { + break; + } + } + ALOG_ASSERT(lpCookie->busy_count == 0, "Unbalanced busy_count inc/dec"); env->DeleteGlobalRef(lpJniStorage->mCallbackData.visualizer_class); env->DeleteGlobalRef(lpJniStorage->mCallbackData.visualizer_ref); + } // sLock scope delete lpJniStorage; } } @@ -707,4 +764,3 @@ int register_android_media_visualizer(JNIEnv *env) { return AndroidRuntime::registerNativeMethods(env, kClassPathName, gMethods, NELEM(gMethods)); } -