Memory Leak on Android about audio_session HOT 15 CLOSED

@ryanheise thank you for releasing 0.0.10. I have integrated it and can confirm the leak is gone.

I will look into the issues mentioned above (thank you) regarding the cloud_firestore PR.
My PR fixed the issues I've seen in our production App, so I have to revisit the comment.

from audio_session.

I will take a look soon. In order to maintain unification between iOS and Android, my feeling is that reference counting can be done without having to track multiple requests.

from audio_session.

Not sure what you mean exactly. :-)

from audio_session.

I was thinking of implementing an approach on the Android side that corresponds to how broadcast streams work in Dart. Broadcast streams do a sort of reference counting so that onListen and onCancel don't get repeatedly called each time a client subscribes to or cancels a subscription. Rather, onListen happens only for the first subscriber, and onCancel happens only for the last canceller. So instead, we would do this reference counting on the platform side by binding it to the lifecycle of the Android context. I want to also point out that audio_session doesn't actually register the noisy receiver when the plugin is created, it creates it only when audio focus is requested.

But anyway, now that I've had a chance to investigate your issue, I don't think we need to go with the above plan (at least for now).

First, I noticed that the abandonAudioFocus method is supposed to unregister the noisy receiver and it doesn't. That's something that I should fix. So even if you call setActive(false), the noisy receiver is still registered.

But even if that fails, it is intended that the plugin should automatically unregister the noisy receiver when the android context is destroyed if it hasn't been unregistered already. There is code to handle this already which should "normally" work, because when the activity is destroyed, I would expect the FlutterEngine to detach from the plugin and there is code in the plugin that then kicks in and it unregisteres the noisy receiver at this point.

The fact that this is not happening in your app suggests that your FlutterEngine is not being destroyed when the activity is destroyed. Are you able to share any information about the way you're using FlutterEngines in your app? Are you using any other plugins that do anything special with FlutterEngines?

from audio_session.

I was thinking of implementing an approach on the Android side that corresponds to how broadcast streams work in Dart. Broadcast streams do a sort of reference counting so that onListen and onCancel don't get repeatedly called each time a client subscribes to or cancels a subscription. Rather, onListen happens only for the first subscriber, and onCancel happens only for the last canceller. So instead, we would do this reference counting on the platform side by binding it to the lifecycle of the Android context. I want to also point out that audio_session doesn't actually register the noisy receiver when the plugin is created, it creates it only when audio focus is requested.

This is exactly what should happen!

The engine we are running is just regular (Flutter 1.20.4) and besides the fact that we run multiple isolates it should not matter.
The leak also points towards the messenger variable, which could point to the fact that the methodchannel is never nulled (in onDetachFromEngine).
Also is the MethodCallHandler set back to null in onDetachFromEngine?

There's this:

	@Override
	public void onDetachedFromEngine(@NonNull FlutterPluginBinding binding) {
		channel.setMethodCallHandler(null);
		androidAudioManager.dispose();
		instances.remove(this);
	}

Maybe add a channel = null & androidAudioManager = null for quicker GC.

The message handling (invokeMethod) currently also requires this line:

private static List<AudioSessionPlugin> instances = new ArrayList<>();

Which really should be removed as it could burn.

Letting the native side implement a EventChannel is much easier for this case.

Also,

    public AndroidAudioManager(Context applicationContext, BinaryMessenger messenger) {
	this.applicationContext = applicationContext;
	this.messenger = messenger;
	channel = new MethodChannel(messenger, "com.ryanheise.android_audio_manager");
	channel.setMethodCallHandler(this);
	audioManager = (AudioManager)applicationContext.getSystemService(Context.AUDIO_SERVICE);
	instances.add(this);
	registerNoisyReceiver();
    }

Whenever AndroidAudioManager is instantiated, it registers a broadcast receiver; but our App does not need to read this information at that point in time.

from audio_session.

Oh, yes you're right that it registers the broadcast receiver in the constructor. That's certainly a mistake.

I've just committed changes to fix the broadcast receiver registration and unregistration as well as set the channel to null.

However, I'd be interested to understand why your FlutterEngine isn't triggering the onDetachedFromEngine method. Setting the channel to null won't help to fix this because that is happening after onDetachedFromEngine is entered which is not happening in the first place in your app. It does happen correctly in the example app, however.

You mentioned that you're using multiple isolates, but are any of those isolates backed by FlutterEngines? Are you using any plugins that create FlutterEngine instances such as flutter_isolate, android_alarm_manager, audio_service, etc? Are you able to possibly provide a minimal reproduction project?

from audio_session.

I've just noticed some more things I can set to null, so I'll push a new commit in a moment.

from audio_session.

The latest commit now does a bit more null setting.

from audio_session.

Hi @ened is the latest commit working for you?

from audio_session.

Will test today, sorry for the delay.

from audio_session.

@ryanheise unfortunately the GIT version requires rxdart 0.25.0 now, while our App is still pinned to 0.24.0.. so will need a bit more time to evaluate

from audio_session.

Sorry about that, I have just updated the dependency to ">= 0.24.1 < 0.26.0" which I believe should allow your app to resolve the dependency.

from audio_session.

Any updates on your end?

I suspect the above issue should be fixed. Although audio_session was designed with multiple FlutterEngine instances in mind, the broadcast receiver itself probably should be a singleton registered with the application context, and using that reference counting approach I alluded to earlier (we don't really want multiple instances of the noisy broadcast receiver in the same app). But that being said, this should not affect you unless you are actually requesting audio focus simultaneously from different FlutterEngine instances (and there would probably be preferable ways to design the app architecture in that case). So that is probably a low priority improvement.

At the very least, I would expect with the latest release that the memory leak is resolved, although I don't have your test case to confirm. Once you confirm, I can push out the next release, and roll these changes into the nnbd branch.

from audio_session.

Speaking of the multiple FlutterEngine instances issue, I noticed you are working on a PR for cloud_firestore's issue of the same variety. I'm not sure if you are aware of it, but there was a similar issue reported before yours here: firebase/flutterfire#3671 . And here was my suggested solution to whoever wanted to implement it.

I have no need for cloud_firestore personally, but users have reported that your PR didn't fix that issue (see firebase/flutterfire#3671 (comment)). I thought you might like to take a look at it and see if there is any way you might be able to modify your solution in a way that solves both problems in a more general way.

from audio_session.

The above changes are now published in release 0.0.10.

from audio_session.