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

Commit 0503b823 authored by Atneya Nair's avatar Atneya Nair
Browse files

[audio] Fix AudioRecordingMonitorImpl leak

Ensure that we don't leak resources if all callbacks aren't unregistered
prior to release.

Remove handler thread to align with avoiding handler trampolines when
consuming executors, which also avoids the additional thread cost and
leak risk.

Some minor refactoring/cleanup as part of these changes.

Bug: 432245274
Flag: EXEMPT bugfix
Test: atest AudioRecordTest
Change-Id: I6a6a6964bcf78f9acc47a4eaebe45f39918011f2
parent 6ebbf65b
Loading
Loading
Loading
Loading
+1 −0
Original line number Diff line number Diff line
@@ -1219,6 +1219,7 @@ public class AudioRecord implements AudioRouting, MicrophoneDirection,
            AudioManager.unregisterAudioPolicyAsyncStatic(mAudioCapturePolicy);
            mAudioCapturePolicy = null;
        }
        mRecordingInfoImpl.endRecordingCallbackHandling();
        native_release();
        mState = STATE_UNINITIALIZED;
    }
+53 −112
Original line number Diff line number Diff line
@@ -20,6 +20,7 @@ import android.annotation.CallbackExecutor;
import android.annotation.NonNull;
import android.annotation.Nullable;
import android.content.Context;
import android.media.AudioManager.AudioRecordingCallback;
import android.os.Binder;
import android.os.Handler;
import android.os.HandlerThread;
@@ -46,9 +47,27 @@ public class AudioRecordingMonitorImpl implements AudioRecordingMonitor {
    private static final String TAG = "android.media.AudioRecordingMonitor";

    private static IAudioService sService; //lazy initialization, use getService()

    private final AudioRecordingMonitorClient mClient;

    private final Object mRecordCallbackLock = new Object();

    @GuardedBy("mRecordCallbackLock")
    private List<AudioRecordingCallbackInfo> mRecordCallbackList =
            new ArrayList<AudioRecordingCallbackInfo>();

    private final IRecordingConfigDispatcher mRecordingCallback =
            new IRecordingConfigDispatcher.Stub() {
                @Override
                public void dispatchRecordingConfigChange(
                        List<AudioRecordingConfiguration> configs) {
                    AudioRecordingConfiguration config = getMyConfig(configs);
                    if (config != null) {
                        configs.removeIf(c -> c != config);
                        handleCallback(configs);
                    }
                }
            };

    AudioRecordingMonitorImpl(@NonNull AudioRecordingMonitorClient client) {
        mClient = client;
    }
@@ -62,7 +81,7 @@ public class AudioRecordingMonitorImpl implements AudioRecordingMonitor {
     * @param cb non-null callback to register
     */
    public void registerAudioRecordingCallback(@NonNull @CallbackExecutor Executor executor,
            @NonNull AudioManager.AudioRecordingCallback cb) {
            @NonNull AudioRecordingCallback cb) {
        if (cb == null) {
            throw new IllegalArgumentException("Illegal null AudioRecordingCallback");
        }
@@ -72,13 +91,19 @@ public class AudioRecordingMonitorImpl implements AudioRecordingMonitor {
        synchronized (mRecordCallbackLock) {
            // check if eventCallback already in list
            for (AudioRecordingCallbackInfo arci : mRecordCallbackList) {
                if (arci.mCb == cb) {
                if (arci.cb == cb) {
                    throw new IllegalArgumentException(
                            "AudioRecordingCallback already registered");
                }
            }
            beginRecordingCallbackHandling();
            mRecordCallbackList.add(new AudioRecordingCallbackInfo(executor, cb));
            if (mRecordCallbackList.isEmpty()) {
                try {
                    getService().registerRecordingCallback(mRecordingCallback);
                } catch (RemoteException e) {
                    throw e.rethrowFromSystemServer();
                }
            }
            mRecordCallbackList.add(new AudioRecordingCallbackInfo(cb, executor));
        }
    }

@@ -87,25 +112,20 @@ public class AudioRecordingMonitorImpl implements AudioRecordingMonitor {
     * {@link #registerAudioRecordingCallback(Executor, AudioManager.AudioRecordingCallback)}.
     * @param cb non-null callback to unregister
     */
    public void unregisterAudioRecordingCallback(@NonNull AudioManager.AudioRecordingCallback cb) {
    public void unregisterAudioRecordingCallback(@NonNull AudioRecordingCallback cb) {
        if (cb == null) {
            throw new IllegalArgumentException("Illegal null AudioRecordingCallback argument");
        }

        synchronized (mRecordCallbackLock) {
            for (AudioRecordingCallbackInfo arci : mRecordCallbackList) {
                if (arci.mCb == cb) {
                    // ok to remove while iterating over list as we exit iteration
                    mRecordCallbackList.remove(arci);
            if (mRecordCallbackList.removeIf((arci) -> arci.cb == cb)) {
                if (mRecordCallbackList.size() == 0) {
                    endRecordingCallbackHandling();
                }
                    return;
                }
            }
            } else {
                throw new IllegalArgumentException("AudioRecordingCallback was not registered");
            }
        }
    }

    /**
     * Returns the current active audio recording for this audio recorder.
@@ -123,110 +143,31 @@ public class AudioRecordingMonitorImpl implements AudioRecordingMonitor {
        }
    }

    private static class AudioRecordingCallbackInfo {
        final AudioManager.AudioRecordingCallback mCb;
        final Executor mExecutor;
        AudioRecordingCallbackInfo(Executor e, AudioManager.AudioRecordingCallback cb) {
            mExecutor = e;
            mCb = cb;
    private static record AudioRecordingCallbackInfo(AudioRecordingCallback cb, Executor executor) {
        void dispatch(List<AudioRecordingConfiguration> configs) {
            executor.execute(() -> cb.onRecordingConfigChanged(configs));
        }
    }

    private static final int MSG_RECORDING_CONFIG_CHANGE = 1;

    private final Object mRecordCallbackLock = new Object();
    @GuardedBy("mRecordCallbackLock")
    @NonNull private LinkedList<AudioRecordingCallbackInfo> mRecordCallbackList =
            new LinkedList<AudioRecordingCallbackInfo>();
    @GuardedBy("mRecordCallbackLock")
    private @Nullable HandlerThread mRecordingCallbackHandlerThread;
    @GuardedBy("mRecordCallbackLock")
    private @Nullable volatile Handler mRecordingCallbackHandler;

    @GuardedBy("mRecordCallbackLock")
    private final IRecordingConfigDispatcher mRecordingCallback =
            new IRecordingConfigDispatcher.Stub() {
        @Override
        public void dispatchRecordingConfigChange(List<AudioRecordingConfiguration> configs) {
            AudioRecordingConfiguration config = getMyConfig(configs);
            if (config != null) {
    private void handleCallback(List<AudioRecordingConfiguration> configs) {
        List<AudioRecordingCallbackInfo> cbInfoList;
        synchronized (mRecordCallbackLock) {
                    if (mRecordingCallbackHandler != null) {
                        final Message m = mRecordingCallbackHandler.obtainMessage(
                                              MSG_RECORDING_CONFIG_CHANGE/*what*/, config /*obj*/);
                        mRecordingCallbackHandler.sendMessage(m);
                    }
                }
            }
        }
    };

    @GuardedBy("mRecordCallbackLock")
    private void beginRecordingCallbackHandling() {
        if (mRecordingCallbackHandlerThread == null) {
            mRecordingCallbackHandlerThread = new HandlerThread(TAG + ".RecordingCallback");
            mRecordingCallbackHandlerThread.start();
            final Looper looper = mRecordingCallbackHandlerThread.getLooper();
            if (looper != null) {
                mRecordingCallbackHandler = new Handler(looper) {
                    @Override
                    public void handleMessage(Message msg) {
                        switch (msg.what) {
                            case MSG_RECORDING_CONFIG_CHANGE: {
                                if (msg.obj == null) {
            if (mRecordCallbackList.isEmpty()) {
                return;
            }
                                ArrayList<AudioRecordingConfiguration> configs =
                                        new ArrayList<AudioRecordingConfiguration>();
                                configs.add((AudioRecordingConfiguration) msg.obj);

                                final LinkedList<AudioRecordingCallbackInfo> cbInfoList;
                                synchronized (mRecordCallbackLock) {
                                    if (mRecordCallbackList.size() == 0) {
                                        return;
            cbInfoList = new ArrayList<AudioRecordingCallbackInfo>(mRecordCallbackList);
        }
                                    cbInfoList = new LinkedList<AudioRecordingCallbackInfo>(
                                        mRecordCallbackList);
        Binder.withCleanCallingIdentity(() -> cbInfoList.forEach(cbi -> cbi.dispatch(configs)));
    }

                                final long identity = Binder.clearCallingIdentity();
    // Package private for cleanup from AudioRecord#release
    // AudioService tolerates double unregister calls
    void endRecordingCallbackHandling() {
        try {
                                    for (AudioRecordingCallbackInfo cbi : cbInfoList) {
                                        cbi.mExecutor.execute(() ->
                                                cbi.mCb.onRecordingConfigChanged(configs));
                                    }
                                } finally {
                                    Binder.restoreCallingIdentity(identity);
                                }
                            } break;
                            default:
                                Log.e(TAG, "Unknown event " + msg.what);
                                break;
                        }
                    }
                };
                final IAudioService service = getService();
                try {
                    service.registerRecordingCallback(mRecordingCallback);
                } catch (RemoteException e) {
                    throw e.rethrowFromSystemServer();
                }
            }
        }
    }

    @GuardedBy("mRecordCallbackLock")
    private void endRecordingCallbackHandling() {
        if (mRecordingCallbackHandlerThread != null) {
            final IAudioService service = getService();
            try {
                service.unregisterRecordingCallback(mRecordingCallback);
            getService().unregisterRecordingCallback(mRecordingCallback);
        } catch (RemoteException e) {
            throw e.rethrowFromSystemServer();
        }
            mRecordingCallbackHandlerThread.quit();
            mRecordingCallbackHandlerThread = null;
        }
    }

    AudioRecordingConfiguration getMyConfig(List<AudioRecordingConfiguration> configs) {
+6 −1
Original line number Diff line number Diff line
@@ -1975,7 +1975,12 @@ public class MediaRecorder implements AudioRouting,
     * may be expected when unnecessary multiple instances are used
     * at the same time.
     */
    public native void release();
    public void release() {
        native_release();
        mRecordingInfoImpl.endRecordingCallbackHandling();
    }

    private native void native_release();

    @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 115609023)
    private static native final void native_init();
+3 −3
Original line number Diff line number Diff line
@@ -574,7 +574,7 @@ android_media_MediaRecorder_native_reset(JNIEnv *env, jobject thiz)
}

static void
android_media_MediaRecorder_release(JNIEnv *env, jobject thiz)
android_media_MediaRecorder_native_release(JNIEnv *env, jobject thiz)
{
    ALOGV("release");
    sp<MediaRecorder> mr = setMediaRecorder(env, thiz, 0);
@@ -669,7 +669,7 @@ static void
android_media_MediaRecorder_native_finalize(JNIEnv *env, jobject thiz)
{
    ALOGV("finalize");
    android_media_MediaRecorder_release(env, thiz);
    android_media_MediaRecorder_native_release(env, thiz);
}

void android_media_MediaRecorder_setInputSurface(
@@ -891,7 +891,7 @@ static const JNINativeMethod gMethods[] = {
    {"pause",                "()V",                             (void *)android_media_MediaRecorder_pause},
    {"resume",               "()V",                             (void *)android_media_MediaRecorder_resume},
    {"native_reset",         "()V",                             (void *)android_media_MediaRecorder_native_reset},
    {"release",              "()V",                             (void *)android_media_MediaRecorder_release},
    {"native_release",       "()V",                             (void *)android_media_MediaRecorder_native_release},
    {"native_init",          "()V",                             (void *)android_media_MediaRecorder_native_init},
    {"native_setup",         "(Ljava/lang/Object;Ljava/lang/String;Landroid/os/Parcel;)V",
                                                                (void *)android_media_MediaRecorder_native_setup},