[google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up #8972
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Will this PR still be reviewed? @stuartmorgan-g |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
| @@ -1,3 +1,9 @@ | |||
| ## 2.12.2 | |||
|
|
|||
| * Fixes memory leak by implementing mechanism for disposing stream subscriptions in `GoogleMapController`. | |||
There was a problem hiding this comment.
Just this sentence is enough for the changelog; anyone interested in the level of detail in the following sentences can always look into the PR and/or issue.
| @@ -1,3 +1,9 @@ | |||
| ## 2.12.2 | |||
|
|
|||
| * Fixes memory leak by implementing mechanism for disposing stream subscriptions in `GoogleMapController`. | |||
There was a problem hiding this comment.
Also, "by implementing mechanism for disposing" -> "by disposing".
| /// | ||
| /// This list keeps track of all event subscriptions created for the map, | ||
| /// including camera movements, marker interactions, and other map events. | ||
| /// These subscriptions are properly disposed when the controller is disposed. |
There was a problem hiding this comment.
"are properly disposed" -> "should be disposed"
Nothing about the existence of the field guarantees that the disposal actually happens, so the comment on the field shouldn't assert it.
| /// This method is used to track and manage all stream subscriptions | ||
| /// created for map events. The subscriptions are stored in | ||
| /// [_streamSubscriptionList] and will be properly disposed when the | ||
| /// controller is disposed. |
There was a problem hiding this comment.
This second sentence should be removed, as it is asserting things outside of the control of this method.
In general, it is an anti-pattern for a comment in one part of the code to make assertions about other code, because the chances of someone changing code updating all assertions about that code anywhere else in the codebase are extremely low, making them highly prone to becoming outdated and misleading.
| /// This list keeps track of all event subscriptions created for the map, | ||
| /// including camera movements, marker interactions, and other map events. | ||
| /// These subscriptions are properly disposed when the controller is disposed. | ||
| final List<StreamSubscription<dynamic>> _streamSubscriptionList = |
There was a problem hiding this comment.
Generally variable names shouldn't include their types unless they communicate some important information. This can just be called _streamSubscriptions.
| /// [_streamSubscriptionList] and will be properly disposed when the | ||
| /// controller is disposed. | ||
| void _addSubscription(StreamSubscription<dynamic> subscription) { | ||
| _streamSubscriptionList.add(subscription); |
There was a problem hiding this comment.
What's the purpose of having a wrapper method? _addSubscription doesn't seem any more readable than _streamSubscriptions.add.
|
|
||
| /// Adds a stream subscription to the tracked list for testing purposes. | ||
| /// | ||
| /// This method allows tests to add custom stream subscriptions to the |
There was a problem hiding this comment.
Adding visibleForTesting methods should only be done if there is no way to test the functionality using public APIs, and it generally indicates that some part of the actual production flow is not being tested.
In this case, this method is being used to bypass testing of all of the changes in _connectStreams. The goal of this PR is not for there to be a method that tracks subscriptions for disposal, the goal is for all the subscriptions to be cleaned up. If all of the changes to _connectStreams were reverted (or more subtly, any one of them were missing), the tests would still pass even though the issues this PR is intended to fix would not actually be fixed.
Given that the unit tests control the GoogleMapsFlutterPlatform.instance that vends all of the subscriptions, it should be possible to test—using only public API—that the platform instance's streams are listened to after init, but none of them are after dispose, thus fully verifying the desired behavior.
There was a problem hiding this comment.
You make a good point. Based on your suggestion, I was wondering if we could test this by checking the return value of (GoogleMapsFlutterPlatform.instance as FakeGoogleMapsFlutterPlatform).mapEventStreamController.hasListener. When the map is initialized, the return value would be true, indicating that the platform instance's stream is being listened to after initialization. When the map is disposed of, the return value would be false, indicating that the stream is no longer being listened to.
| final FakePlatformViewsController fakePlatformViewsController = | ||
| FakePlatformViewsController(); | ||
|
|
||
| tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( |
There was a problem hiding this comment.
What issue was this code added to solve? Tests at this layer should not be relying on the internal implementation details of a (legacy) implementation from another package. Since you're setting the fake instance above it's not clear to me what code could be triggering calls to this handler.
| fakePlatformViewsController.fakePlatformViewsMethodHandler, | ||
| ); | ||
|
|
||
| final ValueNotifier<GoogleMapController?> controllerNotifier = |
There was a problem hiding this comment.
Why is this a value notifier rather than completer? Nothing registers any listeners so it's not clear to me what this is doing.
|
|
||
| controllerNotifier.dispose(); | ||
| } else { | ||
| fail('GoogleMapController not created'); |
There was a problem hiding this comment.
This will fail racily, because nothing is waiting onMapCompleter. Using a completer with an await will fix that, and avoid the need to even have this code.
| if (controller != null) { | ||
| expect(platform.mapEventStreamController.hasListener, true); | ||
|
|
||
| await tester.pumpWidget(const Directionality( |
There was a problem hiding this comment.
An explicit comment that this is removing the map from the widget tree would help make it clear what is happening here; it's easy to miss while reading that this isn't the usual test boilerplate of putting the widget under test in the tree (and is in fact the opposite).
stuartmorgan-g
changed the title
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM
@bparrishMines Could you do the secondary review here?
stuartmorgan-g
added
the
autosubmit
…tforms due to subscriptions not getting cleaned up (flutter/packages#8972)
flutter/packages@b2ce3b0...ab44c26 2025-05-08 31216074+gentlemanxzh@users.noreply.github.com [google_maps_flutter] Fix for memory leak impacting all platforms due to subscriptions not getting cleaned up (flutter/packages#8972) 2025-05-08 engine-flutter-autoroll@skia.org Roll Flutter from cfb887c to b0f5c8c (281 revisions) (flutter/packages#9223) 2025-05-08 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Fixes support for ad tag URLs that do not contain a query (flutter/packages#9176) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…orms due to subscriptions not getting cleaned up (flutter#8972)" This reverts commit ab44c26.
…orms due to subscriptions not getting cleaned up (flutter#8972)" This reverts commit ab44c26.
… 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
… 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
… 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
3 participants
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
Fixes #115283
#92788
Pre-Review Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///).If you need help, consider asking for advice on the #hackers-new channel on [Discord].