Memory Leak on Android

[ened]

Used version audio_session 0.0.9.

┬───
│ GC Root: Local variable in native code
│
├─ android.net.ConnectivityThread instance
│    Leaking: NO (PathClassLoader↓ is not leaking)
│    Thread name: 'ConnectivityThread'
│    ↓ ConnectivityThread.contextClassLoader
├─ dalvik.system.PathClassLoader instance
│    Leaking: NO (AndroidAudioManager↓ is not leaking and A ClassLoader is
│    never leaking)
│    ↓ PathClassLoader.runtimeInternalObjects
├─ java.lang.Object[] array
│    Leaking: NO (AndroidAudioManager↓ is not leaking)
│    ↓ Object[].[2988]
├─ com.ryanheise.audio_session.AndroidAudioManager class
│    Leaking: NO (a class is never leaking)
│    ↓ static AndroidAudioManager.noisyReceiver
│                                 ~~~~~~~~~~~~~
├─ com.ryanheise.audio_session.AndroidAudioManager$1 instance
│    Leaking: UNKNOWN
│    Retaining 49041 bytes in 865 objects
│    Anonymous subclass of android.content.BroadcastReceiver
│    ↓ AndroidAudioManager$1.this$0
│                            ~~~~~~
├─ com.ryanheise.audio_session.AndroidAudioManager instance
│    Leaking: UNKNOWN
│    Retaining 49021 bytes in 864 objects
│    applicationContext instance of com.company.app.MainApplication
│    ↓ AndroidAudioManager.messenger
│                          ~~~~~~~~~
├─ io.flutter.embedding.engine.dart.DartExecutor instance
│    Leaking: UNKNOWN
│    Retaining 48977 bytes in 862 objects
│    ↓ DartExecutor.flutterJNI
│                   ~~~~~~~~~~
├─ io.flutter.embedding.engine.FlutterJNI instance
│    Leaking: UNKNOWN
│    Retaining 128 bytes in 10 objects
│    ↓ FlutterJNI.localizationPlugin
│                 ~~~~~~~~~~~~~~~~~~
├─ io.flutter.plugin.localization.LocalizationPlugin instance
│    Leaking: UNKNOWN
│    Retaining 16 bytes in 1 objects
│    context instance of com.company.app.MainActivity with mDestroyed =
│    true
│    ↓ LocalizationPlugin.context
│                         ~~~~~~~
╰→ com.company.app.MainActivity instance
​     Leaking: YES (ObjectWatcher was watching this because com.mynextbase.
​     connect.MainActivity received Activity#onDestroy() callback and
​     Activity#mDestroyed is true)
​     Retaining 41732 bytes in 612 objects
​     key = c9ca838e-bc2f-4f78-a78e-a77abef2e000
​     watchDurationMillis = 20643
​     retainedDurationMillis = 15642
​     mApplication instance of com.company.app.MainApplication
​     mBase instance of android.app.ContextImpl, not wrapping known Android
​     context

METADATA

Build.VERSION.SDK_INT: 29
Build.MANUFACTURER: samsung
LeakCanary version: 2.5
App process name: com.company.app
Stats: LruCache[maxSize=3000,hits=2458,misses=79998,hitRate=2%]
RandomAccess[bytes=3743193,reads=79998,travel=30190866019,range=22454797,size=28
937691]
Analysis duration: 4149 ms

Instead of registering the receiver any time a plugin is instantiated, the native side of the plugin should expose a single event channel per use case and the dart side exposes the broadcast stream to the application.

The native side should only register the receive when it itself has stream subscriptions and cancel them when they cancel.

@ryanheise do you have time to work on this any time soon?

[ryanheise]

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.

[ened]

Not sure what you mean exactly. :-)

[ryanheise]

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?

[ened]

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.

[ryanheise]

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?

[ryanheise]

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

[ryanheise]

The latest commit now does a bit more null setting.

[ryanheise]

Hi @ened is the latest commit working for you?

[ened]

Will test today, sorry for the delay.

[ened]

@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

[ryanheise]

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.

[ryanheise]

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.

[ryanheise]

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.

[ryanheise]

The above changes are now published in release 0.0.10.

[ened]

@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.