Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7dc9009
fix: Clean up subscriptions when disposing, instead of firing and for…
wolfie82 Jun 23, 2023
565def2
doc: update CHANGELOG.md for google_maps_flutter
wolfie82 Jun 23, 2023
d3a4f84
fix: don't initialize if widget isn't mounted
wolfie82 Jun 23, 2023
e38f413
chore: version bump
wolfie82 Jun 23, 2023
dec6f73
Merge branch 'main' into bug/google-maps-memory-leak
wolfie82 Jun 23, 2023
4e082cb
Merge branch 'main' into bug/google-maps-memory-leak
wolfie82 Jun 27, 2023
0aab33e
test: add unit test to assert that StreamSubscription are cancelled
wolfie82 Jun 27, 2023
27256d1
Merge branch 'main' into bug/google-maps-memory-leak
wolfie82 Jun 27, 2023
971a143
doc: add license to new test file
wolfie82 Jun 27, 2023
4065b90
chore: correct linting/analyze issues
wolfie82 Jun 27, 2023
509fe6c
doc: add doc string for _addSubscription
wolfie82 Jun 27, 2023
b778349
doc: fixup doc string for _addSubscription
wolfie82 Jun 27, 2023
a21690e
doc: fixup CHANGELOG
wolfie82 Jun 27, 2023
61c293e
chore: limit expose to streamSubscriptionsState through getter
wolfie82 Jun 27, 2023
6a399e0
doc: fixup doc strings
wolfie82 Jun 27, 2023
2a1a5e9
doc: fill in more docs and fixup doc strings
wolfie82 Jun 27, 2023
ccb562c
doc: fixup doc strings
wolfie82 Jun 28, 2023
df4c835
chore: cleanup whitespace
wolfie82 Jun 28, 2023
fd940d6
doc: cleanup CHANGELOG.md
wolfie82 Jun 28, 2023
19bc07b
doc: add myself to authors
wolfie82 Jun 28, 2023
5c51d1b
Merge branch 'main' into bug/google-maps-memory-leak
wolfie82 Jun 28, 2023
bc650c8
Merge branch 'main' into bug/google-maps-memory-leak
wolfie82 Jun 28, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/google_maps_flutter/google_maps_flutter/AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,4 @@ Anton Borries <mail@antonborri.es>
Alex Li <google@alexv525.com>
Rahul Raj <64.rahulraj@gmail.com>
Taha Tesser <tesser@gmail.com>
Patrick Wolf <patricksbaker@gmail.com>
6 changes: 6 additions & 0 deletions packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 2.3.2

* Fixes memory leak by implementing mechanism for disposing stream subscriptions in `GoogleMapController`.
Previously, subscriptions were not canceled upon disposal of the controller, which could lead to memory leaks or
unexpected behavior. The controller now tracks all stream subscriptions and cancels them when `dispose()` is called.

## 2.3.1

* Fixes a regression from 2.2.8 that could cause incorrect handling of a
Expand Down
169 changes: 133 additions & 36 deletions packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

///
/// 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`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// This map allows for efficient management of subscriptions, facilitating operations
/// like mass cancellation during controller disposal.
///
/// This field is private to prevent external mutation. Access is provided through
/// the [streamSubscriptionsState] getter, which returns an unmodifiable view of this map.
final Map<StreamSubscription<dynamic>, bool> _streamSubscriptionsState =
<StreamSubscription<dynamic>, bool>{};

/// Returns an unmodifiable view of the map that holds the active StreamSubscriptions
/// and their corresponding states.
///
/// This getter provides a way to inspect the current state of active StreamSubscriptions
/// in the [GoogleMapController] class without allowing any outside mutations.
///
/// The map contains [StreamSubscription] instances as keys, and [bool] as values.
/// The value indicates if the subscription has been canceled `true` or not `false`.
///
/// This is useful in situations where the need arises to monitor or debug the
/// lifecycle of various StreamSubscriptions handled within this class.
Map<StreamSubscription<dynamic>, bool> get streamSubscriptionsState =>
Map<StreamSubscription<dynamic>, bool>.unmodifiable(
_streamSubscriptionsState);

/// Initialize control of a [GoogleMap] with [id].
///
/// Mainly for internal use when instantiating a [GoogleMapController] passed
Expand All @@ -34,51 +63,112 @@ class GoogleMapController {
);
}

/// The state object of the [GoogleMap] widget.
///
/// This instance holds important properties and methods that are essential for
/// the behavior and state of the [GoogleMap] widget.
final _GoogleMapState _googleMapState;

/// Subscribes to the various event streams from the [GoogleMapsFlutterPlatform] for a specific map.
///
/// This method takes the `mapId` as an argument and uses it to subscribe to the various streams available from the [GoogleMapsFlutterPlatform].
/// It only subscribes to the streams if the corresponding callback from the widget is not null. This includes streams for camera movements,
/// marker taps, marker drag events, info window taps, polyline taps, polygon taps, circle taps, map taps, and map long presses.
///
/// Each subscription is added to [_streamSubscriptionsState] for lifecycle management.
///
/// - [mapId] is the identifier of the map for which to initiate the streams.
void _connectStreams(int mapId) {
if (_googleMapState.widget.onCameraMoveStarted != null) {
GoogleMapsFlutterPlatform.instance
.onCameraMoveStarted(mapId: mapId)
.listen((_) => _googleMapState.widget.onCameraMoveStarted!());
_addSubscription(
GoogleMapsFlutterPlatform.instance
.onCameraMoveStarted(mapId: mapId)
.listen((_) => _googleMapState.widget.onCameraMoveStarted!()),
);
}
if (_googleMapState.widget.onCameraMove != null) {
GoogleMapsFlutterPlatform.instance.onCameraMove(mapId: mapId).listen(
(CameraMoveEvent e) => _googleMapState.widget.onCameraMove!(e.value));
_addSubscription(
GoogleMapsFlutterPlatform.instance.onCameraMove(mapId: mapId).listen(
(CameraMoveEvent e) =>
_googleMapState.widget.onCameraMove!(e.value),
),
);
}
if (_googleMapState.widget.onCameraIdle != null) {
GoogleMapsFlutterPlatform.instance
.onCameraIdle(mapId: mapId)
.listen((_) => _googleMapState.widget.onCameraIdle!());
_addSubscription(
GoogleMapsFlutterPlatform.instance
.onCameraIdle(mapId: mapId)
.listen((_) => _googleMapState.widget.onCameraIdle!()),
);
}
GoogleMapsFlutterPlatform.instance
.onMarkerTap(mapId: mapId)
.listen((MarkerTapEvent e) => _googleMapState.onMarkerTap(e.value));
GoogleMapsFlutterPlatform.instance.onMarkerDragStart(mapId: mapId).listen(
(MarkerDragStartEvent e) =>
_googleMapState.onMarkerDragStart(e.value, e.position));
GoogleMapsFlutterPlatform.instance.onMarkerDrag(mapId: mapId).listen(
(MarkerDragEvent e) =>
_googleMapState.onMarkerDrag(e.value, e.position));
GoogleMapsFlutterPlatform.instance.onMarkerDragEnd(mapId: mapId).listen(
(MarkerDragEndEvent e) =>
_googleMapState.onMarkerDragEnd(e.value, e.position));
GoogleMapsFlutterPlatform.instance.onInfoWindowTap(mapId: mapId).listen(
(InfoWindowTapEvent e) => _googleMapState.onInfoWindowTap(e.value));
GoogleMapsFlutterPlatform.instance
.onPolylineTap(mapId: mapId)
.listen((PolylineTapEvent e) => _googleMapState.onPolylineTap(e.value));
GoogleMapsFlutterPlatform.instance
.onPolygonTap(mapId: mapId)
.listen((PolygonTapEvent e) => _googleMapState.onPolygonTap(e.value));
GoogleMapsFlutterPlatform.instance
.onCircleTap(mapId: mapId)
.listen((CircleTapEvent e) => _googleMapState.onCircleTap(e.value));
GoogleMapsFlutterPlatform.instance
.onTap(mapId: mapId)
.listen((MapTapEvent e) => _googleMapState.onTap(e.position));
GoogleMapsFlutterPlatform.instance.onLongPress(mapId: mapId).listen(
(MapLongPressEvent e) => _googleMapState.onLongPress(e.position));
_addSubscription(
GoogleMapsFlutterPlatform.instance
.onMarkerTap(mapId: mapId)
.listen((MarkerTapEvent e) => _googleMapState.onMarkerTap(e.value)),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance.onMarkerDragStart(mapId: mapId).listen(
(MarkerDragStartEvent e) =>
_googleMapState.onMarkerDragStart(e.value, e.position),
),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance.onMarkerDrag(mapId: mapId).listen(
(MarkerDragEvent e) =>
_googleMapState.onMarkerDrag(e.value, e.position),
),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance.onMarkerDragEnd(mapId: mapId).listen(
(MarkerDragEndEvent e) =>
_googleMapState.onMarkerDragEnd(e.value, e.position),
),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance.onInfoWindowTap(mapId: mapId).listen(
(InfoWindowTapEvent e) => _googleMapState.onInfoWindowTap(e.value),
),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance.onPolylineTap(mapId: mapId).listen(
(PolylineTapEvent e) => _googleMapState.onPolylineTap(e.value),
),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance.onPolygonTap(mapId: mapId).listen(
(PolygonTapEvent e) => _googleMapState.onPolygonTap(e.value),
),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance
.onCircleTap(mapId: mapId)
.listen((CircleTapEvent e) => _googleMapState.onCircleTap(e.value)),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance
.onTap(mapId: mapId)
.listen((MapTapEvent e) => _googleMapState.onTap(e.position)),
);
_addSubscription(
GoogleMapsFlutterPlatform.instance.onLongPress(mapId: mapId).listen(
(MapLongPressEvent e) => _googleMapState.onLongPress(e.position),
),
);
}

/// Adds a new stream subscription to the [_streamSubscriptionsState] map.
///
/// The method takes a [StreamSubscription] and sets its status to `false`
/// in the [_streamSubscriptionsState] map. The `false` status indicates that the subscription
/// is currently active and has not been canceled yet.
///
/// The [_streamSubscriptionsState] map manages all active stream subscriptions
/// associated with this instance, which facilitates their lifecycle management
/// and ensures that they are properly canceled and disposed when necessary.
///
/// [subscription]: A [StreamSubscription] that will be managed by the [_streamSubscriptionsState] map.
void _addSubscription(StreamSubscription<dynamic> subscription) {
_streamSubscriptionsState[subscription] = false;
}

/// Updates configuration options of the map user interface.
Expand Down Expand Up @@ -270,6 +360,13 @@ class GoogleMapController {

/// Disposes of the platform resources
void dispose() {
for (final StreamSubscription<dynamic> subscription
in _streamSubscriptionsState.keys) {
subscription.cancel().then((void value) {
_streamSubscriptionsState[subscription] = true;
});
}
_streamSubscriptionsState.clear();
GoogleMapsFlutterPlatform.instance.dispose(mapId: mapId);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,9 @@ class _GoogleMapState extends State<GoogleMap> {
}

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this related to the PR?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}
final GoogleMapController controller = await GoogleMapController.init(
id,
widget.initialCameraPosition,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: google_maps_flutter
description: A Flutter plugin for integrating Google Maps in iOS and Android applications.
repository: https://github.com/flutter/packages/tree/main/packages/google_maps_flutter/google_maps_flutter
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+maps%22
version: 2.3.1
version: 2.3.2

environment:
sdk: ">=3.0.0 <4.0.0"
Expand Down

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/services.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:google_maps_flutter/google_maps_flutter.dart';
import 'package:google_maps_flutter_platform_interface/google_maps_flutter_platform_interface.dart';

import 'fake_google_maps_flutter_platform.dart';

void main() {
TestWidgetsFlutterBinding.ensureInitialized();

group('GoogleMap widget', () {
testWidgets('Subscriptions are canceled on dispose',
(WidgetTester tester) async {
GoogleMapsFlutterPlatform.instance = FakeGoogleMapsFlutterPlatform();

final FakePlatformViewsController fakePlatformViewsController =
FakePlatformViewsController();

tester.binding.defaultBinaryMessenger.setMockMethodCallHandler(
SystemChannels.platform_views,
fakePlatformViewsController.fakePlatformViewsMethodHandler,
);

final ValueNotifier<GoogleMapController?> controllerNotifier =
ValueNotifier<GoogleMapController?>(null);

final GoogleMap googleMap = GoogleMap(
onMapCreated: (GoogleMapController controller) {
controllerNotifier.value = controller;
},
initialCameraPosition: const CameraPosition(
target: LatLng(0, 0),
),
);

await tester.pumpWidget(Directionality(
textDirection: TextDirection.ltr,
child: googleMap,
));

await tester.pump();

final GoogleMapController? controller = controllerNotifier.value;

if (controller != null) {
expect(controller.streamSubscriptionsState, isNotEmpty);

controller.streamSubscriptionsState.forEach((_, bool isCanceled) {
expect(isCanceled, isFalse);
});

await tester.pumpWidget(const Directionality(
textDirection: TextDirection.ltr,
child: SizedBox(),
));

await tester.binding.runAsync(() async {
await tester.pump();
});

controller.streamSubscriptionsState.forEach((_, bool isCanceled) {
expect(isCanceled, isTrue);
});
} else {
fail('GoogleMapController not created');
}
});
});
}

class FakePlatformViewsController {
Future<dynamic> fakePlatformViewsMethodHandler(MethodCall call) {
switch (call.method) {
case 'create':
return Future<int>.value(1);
default:
return Future<dynamic>.value();
}
}
}