[google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up #4281

wolfie82 · 2023-06-23T01:54:53Z

Prior to this change, GoogleMaps would create a condition where its state wasn't cleaned up. Any widget that was leveraging GoogleMap wasn't cleaned up if it relied on a subscription, such as onCameraIdle, onCameraMoved, etc. This was creating crashes for our team specifically.

List which issues are fixed by this PR. You must list at least one issue.

#129089
#115283
#92788

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…getting

wolfie82 · 2023-06-23T04:51:55Z

I'm having a bit of trouble writing a test for this given the public API surface of the internals that were fixed. @stuartmorgan I hate to open a PR without meeting all the criteria, however, I'm happy to move this forward with a bit of guidance. I think this PR is extremely important as it fixes a severe memory leak. In our app, if we navigate away from the GoogleMap and then back again * 3 on an iPhone SE, it will crash with an out of memory exception due to these subscriptions not getting closed and therefore retaining state from the calling widget (see flutter/flutter#129089).

stuartmorgan-g · 2023-06-23T14:46:58Z

Since the goal is to ensure that the subscriptions are cancelled, it looks like you could test it by vending mock/fake subscriptions in a unit test, and validating that they all have cancel called on them.

wolfie82 · 2023-06-24T03:22:06Z

Since the goal is to ensure that the subscriptions are cancelled, it looks like you could test it by vending mock/fake subscriptions in a unit test, and validating that they all have cancel called on them.

Appreciate that. This one is a case, at least for me, where the test is harder than the fix. I'll give it a fresh pair of eyes on Monday.

I was aiming for a test that would leverage the new controller update, but the API surface that's public to the test is what makes this complicated. I could use the fake platform test implementation already existing, just figuring out how to wire everything up.

wolfie82 · 2023-06-27T21:14:32Z

Hi @stuartmorgan. I added some unit tests and it looks like all the build checks have passed except for submit-queue, which I think is outside my control. Thanks again!

stuartmorgan-g · 2023-06-29T12:48:55Z

packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart

This file should be called controller_test.dart, since we generally want unit test files to correspond directly to the files they test.

stuartmorgan-g · 2023-06-29T12:50:19Z

packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart

@@ -18,6 +18,35 @@ class GoogleMapController {
  /// The mapId for this controller
  final int mapId;

+  /// State map of the stream subscriptions for this controller, used internally.


Internal implementation details should not be public properties; this would make it impossible for us to ever change without a major version bump.

stuartmorgan-g · 2023-06-29T12:53:58Z

packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart

+  ///
+  /// The map stores a [StreamSubscription] as key and a [bool] as value.
+  /// Each [StreamSubscription] represents a subscription to an event from the native side.
+  /// The corresponding [bool] indicates whether the subscription has been canceled `true` or not `false`.


It's unclear to me why the boolean exists, since once it's been canceled there is no reason to track it any more; just being in a set is already a boolean state.

Is this only for the unit test? If so, the unit test should be testing that the subscription is actually canceled, via fake subscription that keep track of cancel calls, rather than testing an unrelated property that we just hope corresponds to that. As currently written, it would be easy to break the actual functionality without breaking the tests.

stuartmorgan-g · 2023-06-29T12:54:41Z

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

@@ -398,6 +398,9 @@ class _GoogleMapState extends State<GoogleMap> {
  }

  Future<void> onPlatformViewCreated(int id) async {
+    if (!mounted) {


How is this related to the PR?

See flutter/flutter#115283 - I can add a test for this.

stuartmorgan-g · 2023-07-20T18:57:41Z

@wolfie82 Are you still planning on updating this to address the latest feedback?

MattyBoy4444 · 2023-07-28T18:32:02Z

Any update on this?

stuartmorgan-g · 2023-08-17T18:48:51Z

Marking as a draft for now just to get this off of our review queue. Please mark it as ready for review once you've had a chance to update it!

Hixie · 2023-11-07T23:52:11Z

@wolfie82 Thanks for your contribution. I'm going to close this PR for now, but I'll leave a pointer on the issues you listed in case someone wants to take it over. Thanks again!

… to subscriptions not getting cleaned up (#8972) This PR is #4281 follow-up,Fix map memory leak *List which issues are fixed by this PR. You must list at least one issue.* [#129089](flutter/flutter#129089) Fixes [#115283](flutter/flutter#115283) [#92788](flutter/flutter#92788) ## Pre-Review Checklist

… to subscriptions not getting cleaned up (flutter#8972) This PR is flutter#4281 follow-up,Fix map memory leak *List which issues are fixed by this PR. You must list at least one issue.* [#129089](flutter/flutter#129089) Fixes [#115283](flutter/flutter#115283) [#92788](flutter/flutter#92788) ## Pre-Review Checklist

fix: Clean up subscriptions when disposing, instead of firing and for…

7dc9009

…getting

github-actions bot added the p: google_maps_flutter label Jun 23, 2023

wolfie82 changed the title ~~fix: Clean up subscriptions when disposing, instead of firing and for…~~ [google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up Jun 23, 2023

doc: update CHANGELOG.md for google_maps_flutter

565def2

wolfie82 marked this pull request as ready for review June 23, 2023 04:47

wolfie82 requested a review from stuartmorgan-g as a code owner June 23, 2023 04:47

wolfie82 added 3 commits June 23, 2023 09:26

fix: don't initialize if widget isn't mounted

d3a4f84

chore: version bump

e38f413

Merge branch 'main' into bug/google-maps-memory-leak

dec6f73

wolfie82 added 5 commits June 27, 2023 09:20

Merge branch 'main' into bug/google-maps-memory-leak

4e082cb

test: add unit test to assert that StreamSubscription are cancelled

0aab33e

Merge branch 'main' into bug/google-maps-memory-leak

27256d1

doc: add license to new test file

971a143

chore: correct linting/analyze issues

4065b90

wolfie82 added 12 commits June 27, 2023 14:25

doc: add doc string for _addSubscription

509fe6c

doc: fixup doc string for _addSubscription

b778349

doc: fixup CHANGELOG

a21690e

chore: limit expose to streamSubscriptionsState through getter

61c293e

doc: fixup doc strings

6a399e0

doc: fill in more docs and fixup doc strings

2a1a5e9

doc: fixup doc strings

ccb562c

chore: cleanup whitespace

df4c835

doc: cleanup CHANGELOG.md

fd940d6

doc: add myself to authors

19bc07b

Merge branch 'main' into bug/google-maps-memory-leak

5c51d1b

Merge branch 'main' into bug/google-maps-memory-leak

bc650c8

stuartmorgan-g requested changes Jun 29, 2023

View reviewed changes

stuartmorgan-g marked this pull request as draft August 17, 2023 18:48

Hixie closed this Nov 7, 2023

ChristianEdwardPadilla mentioned this pull request Jan 31, 2025

[google_maps_flutter] improvements to google maps state handling #8431

Closed

11 tasks

gentlemanxzh mentioned this pull request Apr 1, 2025

[google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up #8972

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up #4281

[google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up #4281

Uh oh!

wolfie82 commented Jun 23, 2023 •

edited

Loading

Uh oh!

wolfie82 commented Jun 23, 2023

Uh oh!

stuartmorgan-g commented Jun 23, 2023

Uh oh!

wolfie82 commented Jun 24, 2023

Uh oh!

wolfie82 commented Jun 27, 2023

Uh oh!

stuartmorgan-g Jun 29, 2023

Uh oh!

stuartmorgan-g Jun 29, 2023

Uh oh!

stuartmorgan-g Jun 29, 2023

Uh oh!

stuartmorgan-g Jun 29, 2023

Uh oh!

wolfie82 Jun 30, 2023

Uh oh!

stuartmorgan-g commented Jul 20, 2023

Uh oh!

MattyBoy4444 commented Jul 28, 2023

Uh oh!

stuartmorgan-g commented Aug 17, 2023

Uh oh!

Hixie commented Nov 7, 2023

Uh oh!

Uh oh!

[google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up #4281

[google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up #4281

Uh oh!

Conversation

wolfie82 commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

wolfie82 commented Jun 23, 2023

Uh oh!

stuartmorgan-g commented Jun 23, 2023

Uh oh!

wolfie82 commented Jun 24, 2023

Uh oh!

wolfie82 commented Jun 27, 2023

Uh oh!

stuartmorgan-g Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

wolfie82 Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jul 20, 2023

Uh oh!

MattyBoy4444 commented Jul 28, 2023

Uh oh!

stuartmorgan-g commented Aug 17, 2023

Uh oh!

Hixie commented Nov 7, 2023

Uh oh!

Uh oh!

wolfie82 commented Jun 23, 2023 •

edited

Loading