[ios, macos] Fix memory leaks in MGLMapSnapshotter. #11133

fabian-guerra · 2018-02-06T23:09:28Z

Fixes #11117
WIP

fabian-guerra

fabian-guerra · 2018-02-09T21:55:35Z

platform/darwin/src/MGLAttributionInfo.mm

@@ -70,7 +70,7 @@ @implementation MGLAttributionInfo
                                                                               documentAttributes:nil];
 #endif

-    NSMutableArray *infos = [NSMutableArray array];
+    __block NSMutableArray *infos = [NSMutableArray array];


No particular reason besides the fact to highlight it's use in the following block.

__block is only needed if the block modifies what infos points to – that is, it modifies not only the array but which array it points to. You’d see a compiler warning if __block is missing and necessary.

fabian-guerra · 2018-02-09T21:57:11Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -81,7 +81,7 @@ - (CGPoint)pointForCoordinate:(CLLocationCoordinate2D)coordinate
 @end

 @interface MGLMapSnapshotter()
-
+@property (nonatomic) BOOL loading;


loading is called inside the lambda that's why I exposed it as property to call weakSelf.loading

fabian-guerra · 2018-02-09T21:59:02Z

platform/darwin/src/MGLMapSnapshotter.mm

-    dispatch_async(queue, ^{
-        _snapshotCallback = std::make_unique<mbgl::Actor<mbgl::MapSnapshotter::Callback>>(*mbgl::Scheduler::GetCurrent(), [=](std::exception_ptr mbglError, mbgl::PremultipliedImage image, mbgl::MapSnapshotter::Attributions attributions, mbgl::MapSnapshotter::PointForFn pointForFn) {
-            _loading = false;
+    __weak __typeof__(self) weakSelf = self;


We were capturing strong references before due to calling self.

fabian-guerra · 2018-02-09T22:02:06Z

platform/darwin/src/MGLMapSnapshotter.mm

+            });
+        } else {
+            // This code is in place to test the _snapshotCallback release cycle.
+            dispatch_async(queue, ^{


I use this code snippet to try to isolate GL's snapshotter generation. This way I detected that _snapshotterCall was not getting released. My hypothesis is that an object that is called before the lambda contains the code that is retaining the remaining memory.

fabian-guerra · 2018-02-09T22:05:44Z

platform/darwin/src/MGLMapSnapshotter.mm

-            }
-
-            _attributionInfo = infos;
+//            [weakSelf drawAttributedSnapshot:attributions snapshotImage:mglImage pointForFn:pointForFn queue:queue completionHandler:completion];


The code for snapshotter used to be part of this method. I move it to it's own method to isolate each memory leak that I detected.

fabian-guerra · 2018-02-09T22:07:29Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -442,4 +461,9 @@ - (void)setOptions:(MGLMapSnapshotOptions *)options
    _mbglMapSnapshotter = std::make_unique<mbgl::MapSnapshotter>(*mbglFileSource, *_mbglThreadPool, styleURL, size, pixelRatio, cameraOptions, coordinateBounds);
 }

+- (void)dealloc {


This is for debug purposed only. I will remove it.

fabian-guerra · 2018-02-09T22:08:02Z

platform/ios/app/MBXSnapshotsViewController.m

@@ -33,12 +33,19 @@ - (void)viewDidLoad {

    // Start snapshotters
    topLeftSnapshotter = [self startSnapshotterForImageView:_snapshotImageViewTL coordinates:CLLocationCoordinate2DMake(37.7184, -122.4365)];
-    topCenterSnapshotter = [self startSnapshotterForImageView:_snapshotImageViewTM coordinates:CLLocationCoordinate2DMake(38.8936, -77.0146)];
-    topRightSnapshotter = [self startSnapshotterForImageView:_snapshotImageViewTR coordinates:CLLocationCoordinate2DMake(-13.1356, -74.2442)];
+    // This code was commented to isolate the creating of a single snapshot.


I commented this code to use a single snapshotter instance to tes.

fabian-guerra · 2018-02-09T22:09:04Z

platform/ios/app/MBXSnapshotsViewController.m

@@ -50,12 +57,13 @@ - (MGLMapSnapshotter*) startSnapshotterForImageView:(UIImageView*) imageView coo
    options.zoomLevel = 10;

    // Create and start the snapshotter
+    __weak UIImageView *weakImageView = imageView;


There was a leak here because we were retaining self when assigning the image to the imageView.

fabian-guerra · 2018-02-09T22:09:37Z

platform/ios/app/MBXSnapshotsViewController.m

-    bottomLeftSnapshotter = [self startSnapshotterForImageView:_snapshotImageViewBL coordinates:CLLocationCoordinate2DMake(52.5072, 13.4247)];
-    bottomCenterSnapshotter = [self startSnapshotterForImageView:_snapshotImageViewBM coordinates:CLLocationCoordinate2DMake(60.2118, 24.6754)];
-    bottomRightSnapshotter = [self startSnapshotterForImageView:_snapshotImageViewBR coordinates:CLLocationCoordinate2DMake(31.2780, 121.4286)];
+- (void)viewWillDisappear:(BOOL)animated


This code is for testing.

fabian-guerra · 2018-02-10T03:14:15Z

platform/darwin/src/MGLMapSnapshotter.mm

+        // Dispatch result to origin queue
+        dispatch_async(queue, ^{
+            MGLMapSnapshot* snapshot = [[MGLMapSnapshot alloc] initWithImage:compositedImage scale:self.options.scale pointForFn:pointForFn];
+            _snapshotCallback = NULL;


This line is forcing callback release. I tried first calling _snapshotCallback.reset() but didn't work. Setting the unique pointer to NULL seems more effective.

Is this statement necessary? _snapshotCallback is already nilled out above, in the completion handler passed into -drawAttributedSnapshot:snapshotImage:pointForFn:queue:completionHandler:. Preferably, this method would be free of side effects related to self.

ivovandongen · 2018-02-12T17:30:43Z

@fabian-guerra Checking the memory usage on a device, it seems pretty much stable when adding a single release to master:

--- a/platform/darwin/src/MGLMapSnapshotter.mm
+++ b/platform/darwin/src/MGLMapSnapshotter.mm
@@ -284,6 +284,7 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64;
                     // Dispatch result to origin queue
                     dispatch_async(queue, ^{
                         MGLMapSnapshot* snapshot = [[MGLMapSnapshot alloc] initWithImage:compositedImage scale:self.options.scale pointForFn:pointForFn];
+                        _snapshotCallback.reset();
                         completion(snapshot, nil);
                     });
                 });

However, the simulator seems to give different results. Is that also what you see?

fabian-guerra · 2018-02-12T18:00:06Z

@ivovandongen I have seen that behavior. Using instruments I can actually see how much memory keeps around. I'm using my iPhone 6+ to test.

1ec5 · 2018-02-12T20:08:14Z

platform/darwin/src/MGLAttributionInfo.mm

@@ -70,7 +70,7 @@ @implementation MGLAttributionInfo
                                                                               documentAttributes:nil];
 #endif

-    NSMutableArray *infos = [NSMutableArray array];
+    __block NSMutableArray *infos = [NSMutableArray array];


__block is only needed if the block modifies what infos points to – that is, it modifies not only the array but which array it points to. You’d see a compiler warning if __block is missing and necessary.

1ec5 · 2018-02-12T20:09:06Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -117,180 +118,190 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot

    _loading = true;


For consistency, get and set loading via the property getter and setter instead of digging into the underlying ivar.

1ec5 · 2018-02-12T20:13:12Z

platform/darwin/src/MGLMapSnapshotter.mm

-            _loading = false;
+    __weak __typeof__(self) weakSelf = self;
+    _snapshotCallback = std::make_unique<mbgl::Actor<mbgl::MapSnapshotter::Callback>>(*mbgl::Scheduler::GetCurrent(), [=](std::exception_ptr mbglError, mbgl::PremultipliedImage image, mbgl::MapSnapshotter::Attributions attributions, mbgl::MapSnapshotter::PointForFn pointForFn) {
+        weakSelf.loading = false;


This lambda uses weakSelf multiple times, with interspersed logic. This introduces a race condition: self could be freed at any time during the lambda’s execution, leading to undefined behavior. weakSelf should generally be accompanied by a strongSelf at the top of the lambda or block that strongly (but temporarily) captures the object. The only exception is for trivial blocks where weakSelf is used only once and the result of the statement is discarded.

1ec5 · 2018-02-12T20:16:36Z

platform/darwin/src/MGLMapSnapshotter.mm

+        // Dispatch result to origin queue
+        dispatch_async(queue, ^{
+            MGLMapSnapshot* snapshot = [[MGLMapSnapshot alloc] initWithImage:compositedImage scale:self.options.scale pointForFn:pointForFn];
+            _snapshotCallback = NULL;


Is this statement necessary? _snapshotCallback is already nilled out above, in the completion handler passed into -drawAttributedSnapshot:snapshotImage:pointForFn:queue:completionHandler:. Preferably, this method would be free of side effects related to self.

1ec5 · 2018-02-12T21:20:07Z

platform/darwin/src/MGLMapSnapshotter.mm

-            _loading = false;
+    __weak __typeof__(self) weakSelf = self;
+    _snapshotCallback = std::make_unique<mbgl::Actor<mbgl::MapSnapshotter::Callback>>(*mbgl::Scheduler::GetCurrent(), [=](std::exception_ptr mbglError, mbgl::PremultipliedImage image, mbgl::MapSnapshotter::Attributions attributions, mbgl::MapSnapshotter::PointForFn pointForFn) {
+        __typeof__(self) snapshotter = weakSelf;


FYI, the usual pattern is to call this variable strongSelf, which sounds similar to weakSelf.

1ec5 · 2018-02-12T21:21:09Z

platform/darwin/src/MGLMapSnapshotter.mm

 #if TARGET_OS_IPHONE
-            CGFloat fontSize = [UIFont smallSystemFontSize];
-            UIColor *attributeFontColor = [UIColor blackColor];
+            MGLImage *mglImage = [[MGLImage alloc] initWithMGLPremultipliedImage:std::move(image) scale:weakSelf.options.scale];


Use snapshotter (or strongSelf) instead of weakSelf throughout this lambda. Otherwise, snapshotter only strongly captures the snapshotter object for as long as that variable is needed within this lambda.

1ec5

🚰🚯

fabian-guerra self-assigned this Feb 6, 2018

fabian-guerra force-pushed the fabian-snapshotter-leaks-11117 branch 5 times, most recently from ffcadd9 to 8ebc67b Compare February 9, 2018 19:38

fabian-guerra requested review from ivovandongen and 1ec5 February 9, 2018 21:26

fabian-guerra commented Feb 9, 2018

View reviewed changes

fabian-guerra commented Feb 10, 2018

View reviewed changes

fabian-guerra force-pushed the fabian-snapshotter-leaks-11117 branch from 8ebc67b to b1a8731 Compare February 12, 2018 18:56

1ec5 reviewed Feb 12, 2018

View reviewed changes

friedbunny added bug iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage labels Feb 12, 2018

fabian-guerra force-pushed the fabian-snapshotter-leaks-11117 branch 2 times, most recently from df95622 to 8ba30fb Compare February 12, 2018 21:12

1ec5 reviewed Feb 12, 2018

View reviewed changes

[ios, macos] Fix memory leaks in MGLMapSnapshotter.

ea7082a

fabian-guerra force-pushed the fabian-snapshotter-leaks-11117 branch from 8ba30fb to ea7082a Compare February 12, 2018 21:29

1ec5 approved these changes Feb 12, 2018

View reviewed changes

fabian-guerra merged commit 024e75b into release-agua Feb 12, 2018

fabian-guerra deleted the fabian-snapshotter-leaks-11117 branch February 12, 2018 21:47

fabian-guerra mentioned this pull request Feb 13, 2018

[ios, macos] Fix a memory leak when creating an EAGLContext in MGLMaSnapshotter #11193

Merged

lilykaiser added this to the ios-v3.7.4 milestone Feb 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios, macos] Fix memory leaks in MGLMapSnapshotter. #11133

[ios, macos] Fix memory leaks in MGLMapSnapshotter. #11133

fabian-guerra commented Feb 6, 2018 •

edited

Loading

fabian-guerra left a comment

fabian-guerra Feb 9, 2018

1ec5 Feb 12, 2018

fabian-guerra Feb 9, 2018

fabian-guerra Feb 9, 2018

fabian-guerra Feb 9, 2018

fabian-guerra Feb 9, 2018

fabian-guerra Feb 9, 2018

fabian-guerra Feb 9, 2018

fabian-guerra Feb 9, 2018

fabian-guerra Feb 9, 2018

fabian-guerra Feb 10, 2018

1ec5 Feb 12, 2018

ivovandongen commented Feb 12, 2018

fabian-guerra commented Feb 12, 2018

1ec5 Feb 12, 2018

1ec5 Feb 12, 2018

1ec5 Feb 12, 2018

1ec5 Feb 12, 2018

1ec5 Feb 12, 2018

1ec5 Feb 12, 2018

1ec5 left a comment

		@@ -117,180 +118,190 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot

		_loading = true;

[ios, macos] Fix memory leaks in MGLMapSnapshotter. #11133

[ios, macos] Fix memory leaks in MGLMapSnapshotter. #11133

Conversation

fabian-guerra commented Feb 6, 2018 • edited Loading

fabian-guerra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivovandongen commented Feb 12, 2018

fabian-guerra commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

fabian-guerra commented Feb 6, 2018 •

edited

Loading