[ios, macos] Call snapshotter (Obj-C) completion block on dealloc/can…

…cel if snapshot hasn't finished. (#12355)

platform/darwin/src/MGLMapSnapshotter.mm

@@ -109,7 +109,10 @@ - (CLLocationCoordinate2D)coordinateForPoint:(NSPoint)point
 @end
 
 @interface MGLMapSnapshotter()
-@property (nonatomic) BOOL loading;
+@property (nonatomic) BOOL cancelled;
+@property (nonatomic) dispatch_queue_t resultQueue;
+@property (nonatomic, copy) MGLMapSnapshotCompletionHandler completion;
++ (void)completeWithErrorCode:(MGLErrorCode)errorCode description:(nonnull NSString*)description onQueue:(dispatch_queue_t)queue completion:(MGLMapSnapshotCompletionHandler)completion;
 @end
 
 @implementation MGLMapSnapshotter {
@@ -118,13 +121,22 @@ @implementation MGLMapSnapshotter {
     std::unique_ptr<mbgl::Actor<mbgl::MapSnapshotter::Callback>> _snapshotCallback;
 }
 
+- (void)dealloc {
+    if (_completion) {
+        NSAssert(_snapshotCallback, @"Snapshot in progress - there should be a valid callback");
+
+        [MGLMapSnapshotter completeWithErrorCode:MGLErrorCodeSnapshotFailed
+                                     description:@"MGLMapSnapshotter deallocated prior to snapshot completion."
+                                         onQueue:_resultQueue
+                                      completion:_completion];
+    }
+}
+
 - (instancetype)initWithOptions:(MGLMapSnapshotOptions *)options
 {
     self = [super init];
     if (self) {
         [self setOptions:options];
-        _loading = false;
-
     }
     return self;
 }
@@ -141,13 +153,16 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot
                     format:@"startWithQueue:completionHandler: must be called from a thread with an active run loop."];
     }
 
-    if ([self isLoading]) {
+    if (self.completion) {
+        // Consider replacing this exception with an error passed to the completion block.
         [NSException raise:NSInternalInconsistencyException
                     format:@"Already started this snapshotter."];
     }
 
-    self.loading = true;
-
+    self.completion = completion;
+    self.resultQueue = queue;
+    self.cancelled = NO;
+
     __weak __typeof__(self) weakSelf = self;
     // mbgl::Scheduler::GetCurrent() scheduler means "run callback on current (ie UI/main) thread"
     // capture weakSelf to avoid retain cycle if callback is never called (ie snapshot cancelled)
@@ -160,16 +175,18 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot
         // If self had died, _snapshotCallback would have been destroyed and this block would not be executed
         NSCAssert(strongSelf, @"Snapshot callback executed after being destroyed.");
 
-        strongSelf.loading = false;
+        if (!strongSelf.completion)
+            return;
 
         if (mbglError) {
             NSString *description = @(mbgl::util::toString(mbglError).c_str());
             NSDictionary *userInfo = @{NSLocalizedDescriptionKey: description};
             NSError *error = [NSError errorWithDomain:MGLErrorDomain code:MGLErrorCodeSnapshotFailed userInfo:userInfo];
 
-            // Dispatch result to origin queue
+            // Dispatch to result queue
             dispatch_async(queue, ^{
-                completion(nil, error);
+                strongSelf.completion(nil, error);
+                strongSelf.completion = nil;
             });
         } else {
 #if TARGET_OS_IPHONE
@@ -179,9 +196,10 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot
             mglImage.size = NSMakeSize(mglImage.size.width / strongSelf.options.scale,
                                        mglImage.size.height / strongSelf.options.scale);
 #endif
-            [strongSelf drawAttributedSnapshot:attributions snapshotImage:mglImage pointForFn:pointForFn latLngForFn:latLngForFn queue:queue completionHandler:completion];
+            [strongSelf drawAttributedSnapshot:attributions snapshotImage:mglImage pointForFn:pointForFn latLngForFn:latLngForFn];
         }
         strongSelf->_snapshotCallback = NULL;
+
     });
 
     // Launches snapshot on background Thread owned by mbglMapSnapshotter
@@ -190,10 +208,10 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot
     _mbglMapSnapshotter->snapshot(_snapshotCallback->self());
 }
 
-+ (void)drawAttributedSnapshotWorker:(mbgl::MapSnapshotter::Attributions)attributions snapshotImage:(MGLImage *)mglImage pointForFn:(mbgl::MapSnapshotter::PointForFn)pointForFn latLngForFn:(mbgl::MapSnapshotter::LatLngForFn)latLngForFn queue:(dispatch_queue_t)queue scale:(CGFloat)scale size:(CGSize)size completionHandler:(MGLMapSnapshotCompletionHandler)completion {
-    
++ (MGLImage*)drawAttributedSnapshotWorker:(mbgl::MapSnapshotter::Attributions)attributions snapshotImage:(MGLImage *)mglImage pointForFn:(mbgl::MapSnapshotter::PointForFn)pointForFn latLngForFn:(mbgl::MapSnapshotter::LatLngForFn)latLngForFn scale:(CGFloat)scale size:(CGSize)size {
+
     NSArray<MGLAttributionInfo *>* attributionInfo = [MGLMapSnapshotter generateAttributionInfos:attributions];
-    
+
 #if TARGET_OS_IPHONE
     MGLAttributionInfoStyle attributionInfoStyle = MGLAttributionInfoStyleLong;
     for (NSUInteger styleValue = MGLAttributionInfoStyleLong; styleValue >= MGLAttributionInfoStyleShort; styleValue--) {
@@ -251,7 +269,11 @@ + (void)drawAttributedSnapshotWorker:(mbgl::MapSnapshotter::Attributions)attribu
     UIImage *compositedImage = UIGraphicsGetImageFromCurrentImageContext();
 
     UIGraphicsEndImageContext();
+
+    return compositedImage;
+
 #else
+
     NSSize targetSize = NSMakeSize(size.width, size.height);
     NSRect targetFrame = NSMakeRect(0, 0, targetSize.width, targetSize.height);
 
@@ -310,29 +332,41 @@ + (void)drawAttributedSnapshotWorker:(mbgl::MapSnapshotter::Attributions)attribu
     [MGLMapSnapshotter drawAttributionTextWithStyle:attributionInfoStyle origin:attributionTextPosition attributionInfo:attributionInfo];
 
     [compositedImage unlockFocus];
-
+
+    return compositedImage;
 #endif
-    // Dispatch result to origin queue
-    dispatch_async(queue, ^{
-        MGLMapSnapshot* snapshot = [[MGLMapSnapshot alloc] initWithImage:compositedImage
-                                                                   scale:scale
-                                                              pointForFn:pointForFn
-                                                             latLngForFn:latLngForFn];
-        completion(snapshot, nil);
-    });
 }
 
-- (void)drawAttributedSnapshot:(mbgl::MapSnapshotter::Attributions)attributions snapshotImage:(MGLImage *)mglImage pointForFn:(mbgl::MapSnapshotter::PointForFn)pointForFn latLngForFn:(mbgl::MapSnapshotter::LatLngForFn)latLngForFn queue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshotCompletionHandler)completion {
+- (void)drawAttributedSnapshot:(mbgl::MapSnapshotter::Attributions)attributions snapshotImage:(MGLImage *)mglImage pointForFn:(mbgl::MapSnapshotter::PointForFn)pointForFn latLngForFn:(mbgl::MapSnapshotter::LatLngForFn)latLngForFn {
 
     // Process image watermark in a work queue
     dispatch_queue_t workQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
+    dispatch_queue_t resultQueue = self.resultQueue;
+
     // Capture scale and size by value to avoid accessing self from another thread
     CGFloat scale = self.options.scale;
     CGSize size = self.options.size;
+
     // pointForFn is a copyable std::function that captures state by value: see MapSnapshotter::Impl::snapshot
+    __weak __typeof__(self) weakself = self;
+
     dispatch_async(workQueue, ^{
         // Call a class method to ensure we're not accidentally capturing self
-        [MGLMapSnapshotter drawAttributedSnapshotWorker:attributions snapshotImage:mglImage pointForFn:pointForFn latLngForFn:latLngForFn queue:queue scale:scale size:size completionHandler:completion];
+        MGLImage *compositedImage = [MGLMapSnapshotter drawAttributedSnapshotWorker:attributions snapshotImage:mglImage pointForFn:pointForFn latLngForFn:latLngForFn scale:scale size:size];
+
+        // Dispatch result to origin queue
+        dispatch_async(resultQueue, ^{
+            __typeof__(self) strongself = weakself;
+
+            if (strongself.completion) {
+                MGLMapSnapshot* snapshot = [[MGLMapSnapshot alloc] initWithImage:compositedImage
+                                                                           scale:scale
+                                                                      pointForFn:pointForFn
+                                                                     latLngForFn:latLngForFn];
+                strongself.completion(snapshot, nil);
+                strongself.completion = nil;
+            }
+        });
     });
 }
 
@@ -466,10 +500,33 @@ + (CGSize)attributionTextSizeWithStyle:(MGLAttributionInfoStyle)attributionStyle
 
 - (void)cancel
 {
+    self.cancelled = YES;
+
+    if (_snapshotCallback) {
+        [MGLMapSnapshotter completeWithErrorCode:MGLErrorCodeSnapshotFailed
+                                     description:[NSString stringWithFormat:@"MGLMapSnapshotter cancelled from %s", __PRETTY_FUNCTION__]
+                                         onQueue:self.resultQueue
+                                      completion:self.completion];
+        self.completion = nil;
+    }
+
     _snapshotCallback.reset();
     _mbglMapSnapshotter.reset();
 }
 
++ (void)completeWithErrorCode:(MGLErrorCode)errorCode description:(nonnull NSString*)description onQueue:(dispatch_queue_t)queue completion:(MGLMapSnapshotCompletionHandler)completion {
+    // The snapshot hasn't completed, so we should alert the caller
+    if (completion && queue) {
+        dispatch_async(queue, ^{
+            NSDictionary *userInfo = @{NSLocalizedDescriptionKey: description};
+            NSError *error = [NSError errorWithDomain:MGLErrorDomain
+                                                 code:errorCode
+                                             userInfo:userInfo];
+            completion(NULL, error);
+        });
+    }
+}
+
 - (void)setOptions:(MGLMapSnapshotOptions *)options
 {
     _options = options;

platform/darwin/src/MGLTypes.h

@@ -49,7 +49,7 @@ typedef NS_ENUM(NSInteger, MGLErrorCode) {
     /** An attempt to load the style failed. */
     MGLErrorCodeLoadStyleFailed = 5,
     /** An error occurred while snapshotting the map. */
-    MGLErrorCodeSnapshotFailed = 6,
+    MGLErrorCodeSnapshotFailed = 6
 };
 
 /** Options for enabling debugging features in an `MGLMapView` instance. */

platform/ios/CHANGELOG.md

@@ -2,6 +2,12 @@
 
 Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.
 
+## master
+
+### Other changes
+
+* Fixed bug where completion block passed to `-[MGLMapSnapshotter startWithQueue:completionHandler:]` was not being called in all code paths. ([#12355](https://github.com/mapbox/mapbox-gl-native/pull/12355))
+
 ## 4.4.0
 
 ### Styles and rendering

platform/ios/Integration Tests/MGLMapViewIntegrationTest.h

@@ -28,6 +28,7 @@
 @property (nonatomic) void (^regionDidChange)(MGLMapView *mapView, MGLCameraChangeReason reason, BOOL animated);
 
 // Utility methods
+- (NSString*)validAccessToken;
 - (void)waitForMapViewToFinishLoadingStyleWithTimeout:(NSTimeInterval)timeout;
 - (void)waitForMapViewToBeRenderedWithTimeout:(NSTimeInterval)timeout;
 @end

platform/ios/Integration Tests/MGLMapViewIntegrationTest.m

@@ -6,6 +6,17 @@ - (void)updateFromDisplayLink;
 
 @implementation MGLMapViewIntegrationTest
 
+- (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;
+}
+
 - (void)setUp {
     [super setUp];
 

platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterSwiftTests.swift

@@ -0,0 +1,62 @@
+import XCTest
+
+class MGLMapSnapshotterSwiftTests: MGLMapViewIntegrationTest {
+
+    // Create snapshot options
+    private class func snapshotterOptions(size: CGSize) -> MGLMapSnapshotOptions {
+        let camera = MGLMapCamera()
+
+        let options = MGLMapSnapshotOptions(styleURL: MGLStyle.satelliteStreetsStyleURL, camera: camera, size: size)
+
+        let sw = CLLocationCoordinate2D(latitude: 52.3, longitude: 13.0)
+        let ne = CLLocationCoordinate2D(latitude: 52.5, longitude: 13.2)
+        options.coordinateBounds = MGLCoordinateBounds(sw:sw, ne:ne)
+
+        return options
+    }
+
+    func testCapturingSnapshotterInSnapshotCompletion() {
+        // See the Obj-C testDeallocatingSnapshotterDuringSnapshot
+        // This Swift test, is essentially the same except for capturing the snapshotter
+        guard validAccessToken() != nil else {
+            return
+        }
+
+        let timeout: TimeInterval = 5.0
+        let expectation = self.expectation(description: "snapshot")
+
+        let options = MGLMapSnapshotterSwiftTests.snapshotterOptions(size: mapView.bounds.size)
+
+        let backgroundQueue = DispatchQueue.main
+
+        backgroundQueue.async {
+            let dg = DispatchGroup()
+            dg.enter()
+
+            DispatchQueue.main.async {
+
+                let snapshotter = MGLMapSnapshotter(options: options)
+
+                snapshotter.start(completionHandler: { (snapshot, error) in
+
+//                    // Without capturing snapshotter:
+//                    XCTAssertNil(snapshot)
+//                    XCTAssertNotNil(error)
+
+                    // Capture snapshotter
+                    dump(snapshotter)
+                    XCTAssertNotNil(snapshot)
+                    XCTAssertNil(error)
+
+                    dg.leave()
+                })
+            }
+
+            dg.notify(queue: .main) {
+                expectation.fulfill()
+            }
+        }
+
+        wait(for: [expectation], timeout: timeout)
+    }
+}