Fix MGLMapSnapshotter concurrency bugs (issue #11827).

platform/ios/CHANGELOG.md

@@ -20,6 +20,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
 * Fixed an issue preventing nested key path expressions get parsed accordingly to the spec. ([#11959](https://github.com/mapbox/mapbox-gl-native/pull/11959))
 * Added custom `-hitTest:withEvent:` to `MGLSMCalloutView` to avoid registering taps in transparent areas of the standard annotation callout. ([#11939](https://github.com/mapbox/mapbox-gl-native/pull/11939))
 * Improved performance and memory impact of `MGLScaleBar`. ([#11921](https://github.com/mapbox/mapbox-gl-native/pull/11921))
+* Fixed race conditions that could cause crashes when re-using `MGLMapSnapshotter` or using multiple snapshotters at the same time. ([#11831](https://github.com/mapbox/mapbox-gl-native/pull/11831))
 
 ## 4.0.2 - May 29, 2018
 

platform/ios/Integration Tests/MGLMapViewIntegrationTest.h

@@ -10,6 +10,12 @@
 
 #define MGLTestAssertEqualWithAccuracy(myself, expression1, expression2, accuracy, ...) \
     _XCTPrimitiveAssertEqualWithAccuracy(myself, expression1, @#expression1, expression2, @#expression2, accuracy, @#accuracy, __VA_ARGS__)
+#define MGLTestAssertNil(myself, expression, ...) \
+    _XCTPrimitiveAssertNil(myself, expression, @#expression, __VA_ARGS__)
+
+#define MGLTestAssertNotNil(myself, expression, ...) \
+    _XCTPrimitiveAssertNotNil(myself, expression, @#expression, __VA_ARGS__)
+
 
 @interface MGLMapViewIntegrationTest : XCTestCase <MGLMapViewDelegate>
 @property (nonatomic) MGLMapView *mapView;

platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m

@@ -0,0 +1,237 @@
+#import "MGLMapViewIntegrationTest.h"
+
+@interface MGLMapSnapshotterTest : MGLMapViewIntegrationTest
+@end
+
+// Convenience func to create snapshotter
+MGLMapSnapshotter* snapshotterWithCoordinates(CLLocationCoordinate2D coordinates, CGSize size) {
+    // Create snapshot options
+    MGLMapCamera* mapCamera    = [[MGLMapCamera alloc] init];
+    mapCamera.pitch            = 20;
+    mapCamera.centerCoordinate = coordinates;
+    MGLMapSnapshotOptions* options = [[MGLMapSnapshotOptions alloc] initWithStyleURL:[MGLStyle satelliteStreetsStyleURL]
+                                                                              camera:mapCamera
+                                                                                size:size];
+    options.zoomLevel = 10;
+
+    // Create and start the snapshotter
+    MGLMapSnapshotter* snapshotter = [[MGLMapSnapshotter alloc] initWithOptions:options];
+    return snapshotter;
+}
+
+NSString* validAccessToken() {
+    NSString *accessToken = [[NSProcessInfo processInfo] environment][@"MAPBOX_ACCESS_TOKEN"];
+    if (!accessToken) {
+        printf("warning: MAPBOX_ACCESS_TOKEN env var is required for this test - skipping.\n");
+        return nil;
+    }
+
+    [MGLAccountManager setAccessToken:accessToken];
+    return accessToken;
+}
+
+@implementation MGLMapSnapshotterTest
+
+- (void)testMultipleSnapshotsWithASingleSnapshotter {
+    if (!validAccessToken()) {
+        return;
+    }
+
+    CGSize size = self.mapView.bounds.size;
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"snapshots"];
+    expectation.expectedFulfillmentCount = 2;
+    expectation.assertForOverFulfill = YES;
+
+    CLLocationCoordinate2D coord = CLLocationCoordinate2DMake(30.0, 30.0);
+
+    MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size);
+    XCTAssertNotNil(snapshotter);
+
+    [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) {
+        [expectation fulfill];
+    }];
+
+    @try {
+        [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) {
+            XCTFail(@"Should not be called - but should it?");
+        }];
+        XCTFail(@"Should not be called");
+    }
+    @catch (NSException *exception) {
+        XCTAssert(exception.name == NSInternalInconsistencyException);
+        [expectation fulfill];
+    }
+
+    [self waitForExpectations:@[expectation] timeout:10.0];
+}
+
+- (void)testAllocatingSnapshotOnBackgroundQueue {
+    if (!validAccessToken()) {
+        return;
+    }
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"snapshots"];
+
+    CGSize size                  = self.mapView.bounds.size;
+    CLLocationCoordinate2D coord = CLLocationCoordinate2DMake(30.0, 30.0);
+
+    dispatch_queue_attr_t attr = dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_SERIAL, QOS_CLASS_USER_INITIATED, QOS_MIN_RELATIVE_PRIORITY);
+    dispatch_queue_t backgroundQueue = dispatch_queue_create(__PRETTY_FUNCTION__, attr);
+
+    // This crashes maybe 1 in 10 times.
+    dispatch_async(backgroundQueue, ^{
+
+        // Create the snapshotter - DO NOT START.
+        MGLMapSnapshotter* snapshotter = snapshotterWithCoordinates(coord, size);
+
+        dispatch_group_t group = dispatch_group_create();
+        dispatch_group_enter(group);
+
+        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
+            dispatch_group_leave(group);
+        });
+
+        dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
+
+        snapshotter = nil;
+
+        dispatch_sync(dispatch_get_main_queue(), ^{
+            [expectation fulfill];
+        });
+    });
+
+    [self waitForExpectations:@[expectation] timeout:2.0];
+}
+
+- (void)testSnapshotterFromBackgroundQueue {
+    if (!validAccessToken()) {
+        return;
+    }
+
+    CGSize size = self.mapView.bounds.size;
+    CLLocationCoordinate2D coord = CLLocationCoordinate2DMake(30.0, 30.0);
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"snapshots"];
+    expectation.expectedFulfillmentCount = 1;
+    expectation.assertForOverFulfill = YES;
+
+    __weak __typeof__(self) weakself = self;
+
+    dispatch_queue_attr_t attr = dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_SERIAL, QOS_CLASS_USER_INITIATED, QOS_MIN_RELATIVE_PRIORITY); // also for concurrent
+    dispatch_queue_t backgroundQueue = dispatch_queue_create(__PRETTY_FUNCTION__, attr);
+
+
+    // Use dispatch_group to keep the backgroundQueue block around (and
+    // so also the MGLMapSnapshotter
+    dispatch_group_t group = dispatch_group_create();
+    dispatch_group_enter(group);
+
+
+    dispatch_async(backgroundQueue, ^{
+
+        MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size);
+        XCTAssertNotNil(snapshotter);
+
+        MGLMapSnapshotCompletionHandler completion = ^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) {
+
+            // This should be the main queue
+            __typeof__(self) strongself = weakself;
+
+            MGLTestAssertNotNil(strongself, strongself);
+
+            MGLTestAssertNotNil(strongself, snapshot);
+            MGLTestAssertNotNil(strongself, snapshot.image);
+            MGLTestAssertNil(strongself, error, @"Snapshot should not error with: %@", error);
+
+            // Change this to XCTAttachmentLifetimeKeepAlways to be able to look at the snapshots after running
+            XCTAttachment *attachment = [XCTAttachment attachmentWithImage:snapshot.image];
+            attachment.lifetime = XCTAttachmentLifetimeDeleteOnSuccess;
+            [strongself addAttachment:attachment];
+
+            dispatch_group_leave(group);
+        };
+
+        // untested
+        @try {
+            [snapshotter startWithCompletionHandler:completion];
+            MGLTestFail(weakself);
+        }
+        @catch (NSException *exception) {
+            MGLTestAssert(weakself, exception.name == NSInvalidArgumentException);
+            dispatch_group_leave(group);
+        }
+
+        // Wait for the snapshot to complete
+        dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
+
+        snapshotter = nil;
+
+        dispatch_sync(dispatch_get_main_queue(), ^{
+            [expectation fulfill];
+        });
+    });
+
+    [self waitForExpectations:@[expectation] timeout:60.0];
+}
+
+- (void)testMultipleSnapshottersPENDING {
+    MGL_CHECK_IF_PENDING_TEST_SHOULD_RUN();
+    if (!validAccessToken()) {
+        return;
+    }
+
+    NSUInteger numSnapshots = 8;
+    CGSize size = self.mapView.bounds.size;
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"snapshots"];
+    expectation.expectedFulfillmentCount = numSnapshots;
+    expectation.assertForOverFulfill = YES;
+
+    __weak __typeof__(self) weakself = self;
+
+    for (size_t run = 0; run < numSnapshots; run++) {
+
+        float ratio = (float)run/(float)numSnapshots;
+        float lon = (ratio*120.0) + ((1.0-ratio)*54.0);
+        CLLocationCoordinate2D coord = CLLocationCoordinate2DMake(57.0, lon);
+
+        __block MGLMapSnapshotter *snapshotter;
+
+        // Allocate from an autorelease pool here, to avoid having
+        // snapshotter retained for longer than we'd like to test.
+        @autoreleasepool {
+            snapshotter = snapshotterWithCoordinates(coord, size);
+            XCTAssertNotNil(snapshotter);
+        }
+
+        [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) {
+
+            // This should be the main queue
+            __typeof__(self) strongself = weakself;
+
+            MGLTestAssertNotNil(strongself, strongself);
+
+            MGLTestAssertNotNil(strongself, snapshot);
+            MGLTestAssertNotNil(strongself, snapshot.image);
+            MGLTestAssertNil(strongself, error, @"Snapshot should not error with: %@", error);
+
+            // Change this to XCTAttachmentLifetimeKeepAlways to be able to look at the snapshots after running
+            XCTAttachment *attachment = [XCTAttachment attachmentWithImage:snapshot.image];
+            attachment.lifetime = XCTAttachmentLifetimeDeleteOnSuccess;
+            [strongself addAttachment:attachment];
+
+            // Dealloc the snapshotter (by having this line in the block, we
+            // also retained the snapshotter. Setting to nil should release, as
+            // this block should be the only thing retaining it (since it was
+            // allocated from the above autorelease pool)
+            snapshotter = nil;
+
+            [expectation fulfill];
+        }];
+    } // end for loop
+
+    [self waitForExpectations:@[expectation] timeout:60.0];
+}
+
+@end

platform/ios/ios.xcodeproj/project.pbxproj

@@ -365,6 +365,7 @@
 		AC518E04201BB56100EBC820 /* MGLTelemetryConfig.m in Sources */ = {isa = PBXBuildFile; fileRef = AC518DFE201BB55A00EBC820 /* MGLTelemetryConfig.m */; };
 		CA0C27922076C804001CE5B7 /* MGLShapeSourceTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CA0C27912076C804001CE5B7 /* MGLShapeSourceTests.m */; };
 		CA0C27942076CA19001CE5B7 /* MGLMapViewIntegrationTest.m in Sources */ = {isa = PBXBuildFile; fileRef = CA0C27932076CA19001CE5B7 /* MGLMapViewIntegrationTest.m */; };
+		CA1B4A512099FB2200EDD491 /* MGLMapSnapshotterTest.m in Sources */ = {isa = PBXBuildFile; fileRef = CA1B4A502099FB2200EDD491 /* MGLMapSnapshotterTest.m */; };
 		CA4EB8C720863487006AB465 /* MGLStyleLayerIntegrationTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CA4EB8C620863487006AB465 /* MGLStyleLayerIntegrationTests.m */; };
 		CA34C9C3207FD272005C1A06 /* MGLCameraTransitionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = CA34C9C2207FD272005C1A06 /* MGLCameraTransitionTests.mm */; };
 		CA55CD41202C16AA00CE7095 /* MGLCameraChangeReason.h in Headers */ = {isa = PBXBuildFile; fileRef = CA55CD3E202C16AA00CE7095 /* MGLCameraChangeReason.h */; settings = {ATTRIBUTES = (Public, ); }; };
@@ -1001,6 +1002,7 @@
 		CA0C27912076C804001CE5B7 /* MGLShapeSourceTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = MGLShapeSourceTests.m; sourceTree = "<group>"; };
 		CA0C27932076CA19001CE5B7 /* MGLMapViewIntegrationTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = MGLMapViewIntegrationTest.m; sourceTree = "<group>"; };
 		CA0C27952076CA50001CE5B7 /* MGLMapViewIntegrationTest.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MGLMapViewIntegrationTest.h; sourceTree = "<group>"; };
+		CA1B4A502099FB2200EDD491 /* MGLMapSnapshotterTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = MGLMapSnapshotterTest.m; sourceTree = "<group>"; };
 		CA4EB8C620863487006AB465 /* MGLStyleLayerIntegrationTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = MGLStyleLayerIntegrationTests.m; sourceTree = "<group>"; };
 		CA34C9C2207FD272005C1A06 /* MGLCameraTransitionTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MGLCameraTransitionTests.mm; sourceTree = "<group>"; };
 		CA55CD3E202C16AA00CE7095 /* MGLCameraChangeReason.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLCameraChangeReason.h; sourceTree = "<group>"; };
@@ -1356,6 +1358,7 @@
 		16376B081FFD9DAF0000563E /* Integration Tests */ = {
 			isa = PBXGroup;
 			children = (
+				CA1B4A4F2099FA2800EDD491 /* Snapshotter Tests */,
 				16376B091FFD9DAF0000563E /* MBGLIntegrationTests.m */,
 				16376B0B1FFD9DAF0000563E /* Info.plist */,
 				CA34C9C2207FD272005C1A06 /* MGLCameraTransitionTests.mm */,
@@ -1708,6 +1711,14 @@
 			name = Fixtures;
 			sourceTree = "<group>";
 		};
+		CA1B4A4F2099FA2800EDD491 /* Snapshotter Tests */ = {
+			isa = PBXGroup;
+			children = (
+				CA1B4A502099FB2200EDD491 /* MGLMapSnapshotterTest.m */,
+			);
+			path = "Snapshotter Tests";
+			sourceTree = "<group>";
+		};
 		DA1DC9411CB6C1C2006E619F = {
 			isa = PBXGroup;
 			children = (
@@ -2821,6 +2832,7 @@
 				16376B0A1FFD9DAF0000563E /* MBGLIntegrationTests.m in Sources */,
 				CA0C27942076CA19001CE5B7 /* MGLMapViewIntegrationTest.m in Sources */,
 				CA0C27922076C804001CE5B7 /* MGLShapeSourceTests.m in Sources */,
+				CA1B4A512099FB2200EDD491 /* MGLMapSnapshotterTest.m in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};

platform/macos/CHANGELOG.md

@@ -15,6 +15,7 @@
 * Unknown tokens in URLs are now preserved, rather than replaced with an empty string. ([#11787](https://github.com/mapbox/mapbox-gl-native/issues/11787))
 * Adjusted when and how the camera transition update and finish callbacks are called, fixing recursion bugs. ([#11614](https://github.com/mapbox/mapbox-gl-native/pull/11614))
 * Fixed an issue preventing nested key path expressions get parsed accordingly to the spec. ([#11959](https://github.com/mapbox/mapbox-gl-native/pull/11959))
+* Fixed race conditions that could cause crashes when re-using `MGLMapSnapshotter` or using multiple snapshotters at the same time. ([#11831](https://github.com/mapbox/mapbox-gl-native/pull/11831))
 
 ## 0.7.1