Fixing memory leaks

[AAverin]

Resolved #389 by clearing everything possible on the MapboxMapController. PR also includes some code from #690 that is necessary for iOS to work.
The main culprit for the leak was onStyleLoadedCallback function assigned when MapboxMapController was initialised and never cleared, keeping a hold on MapboxMap instance through _widget variable.

Additionally, MapboxGLPlatform was leaking instances because they were added to the internal list and never removed.

While all these leaks would never bother if you have only single map in the app, like in the sample apps, they clearly start bringing issues when you have lots of smaller maps on different screens.

[shroff]

Hi @AAverin, thanks for your work on this issue, as well as reporting your findings in #389.

Even though this chagne probably does fix the issue, it contains a lot of unnecessary changes that are not needed for this fix, and only make the code less readable, and some others that are related to other issues. I am not comfortable accepting all of these changes.

I think the core issue here is a single reference (or two) which is kept longer than it needs to be, and my guess is that it is the method channel call handler, and maybe the platform instance kept around in MapboxGlPlatform._instances.

I have submitted a PR to address the latter in #710, so maybe you could try just applying method handler change on top of that to see if it resolves the leak?

[AAverin]

@shroff I was working on top of my other branch that had some fixes applied already.
I will fix and merge my other PR when I am back from vacation and this one should become cleaner.

[felix-ht]

I have to agree with @shroff. MapboxGlPlatform.getInstance(_id).dispose(); should be all that is needed. Flutter/dart will properly free any internal objects such as onSymbolTapped as long as the object holding it is destroyed.
In general manually disposing is only needed to get rid of things that are not directly part of the widgets state.

[AAverin]

I will try that other branch once I am back from vacation end of next week and see if it will resolve the leak.
Overall you are right, clearing the MapboxGlPlatform reference should be enough, I was just going variable by variable and clearing them to see what is actually causing the leak. The MapboxGlPlatform I found only at the very end.

Another issue that complicates debugging memory is that flutter itself seems to be leaking widget instances even in the latest version of the engine. I have created flutter/flutter#91508 already and there was another issue where something similar was tracked.

Big issue I have right now is #703, and that could be related to some memory leaks.

[AAverin]

I have merged #710 into this branch and did some more investigation and testing for leaks.
Result is almost the same as before. Even with reworked in #710 usage of MapboxGLPlatform instance, only clearing all possible callbacks in the controller and clearing the platform channel resulted in no leaks coming directly from mapbox_gl library.
@felix-ht @shroff FYI, ready for review

[shroff]

I think the other place that is keeping a reference is the method call handler, which you nulled out in dispose. Maybe try only that on top of #710

[AAverin]

@shroff you are right that this was the main leak, but doing only that was not enough, I still saw leaks in memory profiler.
Only clearing all callbacks in controller on dispose got me to the state of no leaks. There are 14 commits quashed here where I was trying step by step making a minimal possible solution that would solve the leaks.
The result is in PR, remove anything from it and I still see the leaks.

[shroff]

@AAverin thanks again for putting in the work into this. Can you tell what is holding on to this reference?

Also, could you share some minimal code that can repro this leak and how you're detecting it? I'd like to play around with it a little bit and really understand how the leak is happening.

[AAverin]

@shroff I got sick so it will take me some time.
I am using dev tools to check, the only thing that was missing in my tests was manually triggering gc before checking - this optional is hidden in settings of dev tools.

I think what's leaking is that callbacks like onMapClick and others passed into Mapbox instance are never cleared. It's common to reference MapboxContorller in those methods to interact with the map. So either this results in holding reference that prevents clearing or a known issue in flutter where outside context gets locked in the inner methods resulting in a leak.

[AAverin]

@shroff Because I couldn't reproduce leaks on a clear sample I have removed some of the code

[AAverin]

@shroff I was about to finish with PR and decided to run my sample again after merging with master.
I can see leaks when running dev tools even after triggering gc manually.

Here is the sample. Please make sure there is some mapbox key set in Android Manifest when testing on Android.

Open the sample app, click on FAB to go to map screen, then go back. Repeat a few times.
Run dev tools, go to memory tab, enable advanced memory settings, trigger gc.
Make a heap dump and check for instances of MapboxMap.

import 'dart:math';

import 'package:flutter/material.dart';
import 'package:mapbox_gl/mapbox_gl.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      initialRoute: "/",
      onGenerateRoute: (settings) {
        switch (settings.name) {
          case "/":
            return MaterialPageRoute(builder: (_) {
              return HomePage();
            });
          case "/map":
            return MaterialPageRoute(builder: (_) {
              return MapPage();
            });
          default:
            return MaterialPageRoute(
                settings: const RouteSettings(name: "error"),
                builder: (_) {
                  return Scaffold(
                    appBar: AppBar(
                      title: const Text("error"),
                    ),
                  );
                });
        }
      },
    );
  }
}

class HomePage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text("Home page"),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: () {
          Navigator.of(context).pushNamed("/map");
        },
        child: const Icon(Icons.navigation),
        backgroundColor: Colors.green,
      ),
    );
  }
}

class MapPage extends StatefulWidget {
  @override
  State<MapPage> createState() => _MapPageState();
}

class _MapPageState extends State<MapPage> {
  MapboxMapController? mapController;

  _onMapCreated(MapboxMapController controller) async {
    mapController = controller;
  }

  _onStyleLoaded() async {
    if (mapController == null) {
      return;
    }

    await mapController!.clearSymbols();
    await mapController!.clearLines();
    await mapController!.setSymbolIconAllowOverlap(true);
  }

  _onMapClick(Point<double> point, LatLng place) async {
    if (mapController == null) {
      return;
    }

    await mapController!.clearLines();
    await mapController!.addLine(const LineOptions(
        lineWidth: 5,
        lineColor: "#ffe100",
        lineOpacity: 1,
        geometry: [
          LatLng(52.5200, 13.4050),
          LatLng(52.5200, 13.4051)
        ]));
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
        appBar: AppBar(
          title: const Text("Map page"),
        ),
        body: MapboxMap(
            initialCameraPosition:
                const CameraPosition(target: LatLng(52.5200, 13.4050)),
            onMapCreated: _onMapCreated,
            onMapClick: _onMapClick,
            onStyleLoadedCallback: _onStyleLoaded));
  }
}

[felix-ht]

@AAverin can you clean up this branch so that it only contains the changes that are required to fix the memory leeks on master?

[AAverin]

@felix-ht I did, from my perspective.
I can't reliably reproduce the leak anymore when I run my app on top of this branch codebase.
Still, there is this sample that I have posted that sometimes results in a leaking MapboxMap instance. This could be related to some known leaks in Flutter and will need more investigation.

[AAverin]

I can re-merge with master because the other code for style comes from another branch that was already merged #690

[felix-ht]

might want to convert this to be abstract

[AAverin]

The class is abstract already

[felix-ht]

@AAverin if this significantly reduces the number of leaks i would be all for merging it. Or worded differently: does this fix some(most) of the leaks? (as opposed to the current master that had the id based instances removed)

[AAverin]

From my perspective no leaks should be present in code so if this fixes at least one it should already go in.
To avoid having this back and forth conversation someone is welcome to test current master for leaks, merge in this branch and check if it's better.
I already have this code merged into my local master that I use for the project and I can see improvements, although with Flutter memory analysis being so basic it's difficult to say if all of the leaks are completely resolved.

[tobrun]

Thank you @AAverin for the fixes and @felix-ht / @shroff for helping to review it. Appreciate the contributions!