diff --git a/platform/darwin/src/MGLMapSnapshotter.h b/platform/darwin/src/MGLMapSnapshotter.h index e30830ebc..2a30a9d41 100644 --- a/platform/darwin/src/MGLMapSnapshotter.h +++ b/platform/darwin/src/MGLMapSnapshotter.h @@ -185,11 +185,6 @@ typedef void (^MGLMapSnapshotCompletionHandler)(MGLMapSnapshot* _Nullable snapsh snapshot at a time. If you need to generate multiple snapshots concurrently, create multiple snapshotter objects. - An `MGLMapSnapshotter` object may be deallocated before the completion handler - is called. To prevent this object from being deallocated prematurely, maintain - a strong reference to it, for example by storing it in an instance variable of - the class where you initialize and start the snapshotter. - For an interactive map, use the `MGLMapView` class. Both `MGLMapSnapshotter` and `MGLMapView` are compatible with offline packs managed by the `MGLOfflineStorage` class. @@ -208,8 +203,7 @@ typedef void (^MGLMapSnapshotCompletionHandler)(MGLMapSnapshot* _Nullable snapsh let options = MGLMapSnapshotOptions(styleURL: MGLStyle.satelliteStreetsStyleURL, camera: camera, size: CGSize(width: 320, height: 480)) options.zoomLevel = 10 - // The containing class should hold a strong reference to this object. - snapshotter = MGLMapSnapshotter(options: options) + let snapshotter = MGLMapSnapshotter(options: options) snapshotter.start { (snapshot, error) in if let error = error { fatalError(error.localizedDescription) @@ -242,11 +236,6 @@ MGL_EXPORT /** Starts the snapshot creation and executes the specified block with the result. - The snapshotter may be deallocated before the completion handler is called. To - prevent the snapshotter from being deallocated prematurely, maintain a strong - reference to it, for example by storing it in an instance variable of the class - where you call this method. - @param completionHandler The block to call with a finished snapshot. The block is executed on the main queue. */ @@ -256,11 +245,6 @@ MGL_EXPORT Starts the snapshot creation and executes the specified block with the result on the specified queue. - The snapshotter may be deallocated before the completion handler is called. To - prevent the snapshotter from being deallocated prematurely, maintain a strong - reference to it, for example by storing it in an instance variable of the class - where you call this method. - @param queue The queue on which to call the block specified in the `completionHandler` parameter. @param completionHandler The block to call with a finished snapshot. The block @@ -273,11 +257,6 @@ MGL_EXPORT on the specified queue. Use this option if you want to add custom drawing on top of the resulting `MGLMapSnapshot`. - The snapshotter may be deallocated before the completion handler is called. To - prevent the snapshotter from being deallocated prematurely, maintain a strong - reference to it, for example by storing it in an instance variable of the class - where you call this method. - @param overlayHandler The block to call after the base map finishes drawing but before certain built-in overlays draw. The block can use Core Graphics to draw custom content directly over the base map. The block is executed on a diff --git a/platform/darwin/src/MGLMapSnapshotter.mm b/platform/darwin/src/MGLMapSnapshotter.mm index 908181ef7..b556349d1 100644 --- a/platform/darwin/src/MGLMapSnapshotter.mm +++ b/platform/darwin/src/MGLMapSnapshotter.mm @@ -261,6 +261,9 @@ NSArray *MGLAttributionInfosFromAttributions(mbgl::MapSnap - (void)startWithQueue:(dispatch_queue_t)queue overlayHandler:(MGLMapSnapshotOverlayHandler)overlayHandler completionHandler:(MGLMapSnapshotCompletionHandler)completion { + if (!completion) { + return; + } if (!mbgl::Scheduler::GetCurrent()) { [NSException raise:NSInvalidArgumentException format:@"startWithQueue:completionHandler: must be called from a thread with an active run loop."]; @@ -282,9 +285,11 @@ NSArray *MGLAttributionInfosFromAttributions(mbgl::MapSnap [self configureWithOptions:options]; MGLLogDebug(@"Starting with options: %@", self.options); - // Capture weakSelf to avoid retain cycle if callback is never called (i.e., snapshot canceled). - __weak __typeof__(self) weakSelf = self; - _mbglMapSnapshotter->snapshot([=](std::exception_ptr mbglError, mbgl::PremultipliedImage image, mbgl::MapSnapshotter::Attributions attributions, mbgl::MapSnapshotter::PointForFn pointForFn, mbgl::MapSnapshotter::LatLngForFn latLngForFn) { + // Temporarily capture the snapshotter until the completion handler finishes executing, to keep standalone local usage of the snapshotter from becoming a no-op. + // POSTCONDITION: Only refer to this variable in the final result queue. + // POSTCONDITION: It is important to nil out this variable at some point in the future to avoid a leak. In cases where the completion handler gets called, the variable should be nilled out explicitly. If -cancel is called, mbgl releases the snapshot block below, causing the only remaining references to the snapshotter to go out of scope. + __block MGLMapSnapshotter *strongSelf = self; + _mbglMapSnapshotter->snapshot(^(std::exception_ptr mbglError, mbgl::PremultipliedImage image, mbgl::MapSnapshotter::Attributions attributions, mbgl::MapSnapshotter::PointForFn pointForFn, mbgl::MapSnapshotter::LatLngForFn latLngForFn) { if (mbglError) { NSString *description = @(mbgl::util::toString(mbglError).c_str()); NSDictionary *userInfo = @{NSLocalizedDescriptionKey: description}; @@ -294,8 +299,9 @@ NSArray *MGLAttributionInfosFromAttributions(mbgl::MapSnap #endif // Dispatch to result queue dispatch_async(queue, ^{ - weakSelf.loading = NO; + strongSelf.loading = NO; completion(nil, error); + strongSelf = nil; }); } else { #if TARGET_OS_IPHONE @@ -315,11 +321,9 @@ NSArray *MGLAttributionInfosFromAttributions(mbgl::MapSnap // Dispatch result to result queue dispatch_async(queue, ^{ - __typeof__(self) strongSelf = weakSelf; strongSelf.loading = NO; - if (completion) { - completion(snapshot, error); - } + completion(snapshot, error); + strongSelf = nil; }); }); } diff --git a/platform/darwin/test/MGLDocumentationExampleTests.swift b/platform/darwin/test/MGLDocumentationExampleTests.swift index 0c1256111..7d6bdbed5 100644 --- a/platform/darwin/test/MGLDocumentationExampleTests.swift +++ b/platform/darwin/test/MGLDocumentationExampleTests.swift @@ -463,15 +463,13 @@ class MGLDocumentationExampleTests: XCTestCase, MGLMapViewDelegate { } } - let snapshotter: MGLMapSnapshotter //#-example-code let camera = MGLMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 37.7184, longitude: -122.4365), altitude: 100, pitch: 20, heading: 0) let options = MGLMapSnapshotOptions(styleURL: MGLStyle.satelliteStreetsStyleURL, camera: camera, size: CGSize(width: 320, height: 480)) options.zoomLevel = 10 - // The containing class should hold a strong reference to this object. - snapshotter = MGLMapSnapshotter(options: options) + let snapshotter = MGLMapSnapshotter(options: options) snapshotter.start { (snapshot, error) in if let error = error { fatalError(error.localizedDescription) diff --git a/platform/ios/CHANGELOG.md b/platform/ios/CHANGELOG.md index 4a315a2a4..0ccab01f7 100644 --- a/platform/ios/CHANGELOG.md +++ b/platform/ios/CHANGELOG.md @@ -16,7 +16,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT ### Snapshots -* `MGLMapSnapshotter` no longer keeps itself from being deallocated before its completion handler is called. To ensure that the completion handler passed into `-[MGLMapSnapshotter startWithCompletionHandler:]` is called, maintain a strong reference to the snapshotter from a longer-lived class, such as the class where you call `-[MGLMapSnapshotter startWithCompletionHandler:]`. ([#210](https://github.com/mapbox/mapbox-gl-native-ios/pull/210)) +* You no longer need to explicitly capture the `MGLMapSnapshotter` object in the completion handler that you specify in `-[MGLMapSnapshotter startWithCompletionHandler:]`. Even if you declare the snapshotter locally without holding a strong reference to it, the snapshotter is only deallocated after the completion handler finishes, and the completion handler generally receives a valid snapshot. ([#210](https://github.com/mapbox/mapbox-gl-native-ios/pull/210)) * The `-[MGLMapSnapshotter cancel]` method no longer calls the completion handler passed into `-[MGLMapSnapshotter startWithCompletionHandler:]`. ([#210](https://github.com/mapbox/mapbox-gl-native-ios/pull/210)) * Fixed an issue where the `MGLMapSnapshotter.loading` property always returned `NO`, even while loading a snapshot. ([#210](https://github.com/mapbox/mapbox-gl-native-ios/pull/210)) diff --git a/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m b/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m index e016651de..b09f4fae4 100644 --- a/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m +++ b/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m @@ -73,8 +73,7 @@ MGLMapSnapshotter* snapshotterWithBounds(MGLCoordinateBounds bounds, CGSize size } - (void)testSnapshotterWithoutStrongReference🔒 { - XCTestExpectation *expectation = [self expectationWithDescription:@"Completion handler shouldn’t be called if there’s no strong reference to the snapshotter and the snapshotter goes away."]; - expectation.inverted = YES; + XCTestExpectation *expectation = [self expectationWithDescription:@"Completion handler should be called even if there’s no strong reference to the snapshotter."]; CGSize size = self.mapView.bounds.size; CLLocationCoordinate2D coordinates = CLLocationCoordinate2DMake(30.0, 30.0); @@ -82,18 +81,21 @@ MGLMapSnapshotter* snapshotterWithBounds(MGLCoordinateBounds bounds, CGSize size @autoreleasepool { MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coordinates, size); weakSnapshotter = snapshotter; + __weak __typeof__(self) weakSelf = self; [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) { - // This completion block should not be called. + MGLTestAssertNotNil(weakSelf, weakSnapshotter, @"Snapshotter should not go away until the completion handler finishes executing."); + // This completion block should be called. [expectation fulfill]; }]; MGLTestAssertNotNil(self, weakSnapshotter, @"Locally scoped snapshotter should not go away while in scope."); } - MGLTestAssertNil(self, weakSnapshotter, @"Snapshotter should go away on its own if there’s no strong reference to it."); + MGLTestAssertNotNil(self, weakSnapshotter, @"Snapshotter should remain until it finishes even there’s no strong reference to it."); - [self waitForExpectations:@[expectation] timeout:5]; + [self waitForExpectations:@[expectation] timeout:10.0]; + MGLTestAssertNil(self, weakSnapshotter, @"Completion handler should not leak snapshotter."); } -- (void)testDeallocatingSnapshotterDuringSnapshot🔒 { +- (void)testSnapshotterInBackgroundWithoutStrongReference🔒 { // See also https://github.com/mapbox/mapbox-gl-native/issues/12336 NSTimeInterval timeout = 10.0; @@ -106,6 +108,7 @@ MGLMapSnapshotter* snapshotterWithBounds(MGLCoordinateBounds bounds, CGSize size // dispatch_queue_t backgroundQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); __weak __typeof__(self) weakself = self; + __block __weak MGLMapSnapshotter *weakSnapshotter; dispatch_async(backgroundQueue, ^{ @@ -113,13 +116,21 @@ MGLMapSnapshotter* snapshotterWithBounds(MGLCoordinateBounds bounds, CGSize size dispatch_group_enter(dg); dispatch_async(dispatch_get_main_queue(), ^{ - MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size); - [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) { - // This completion block should not be called. - __typeof__(self) strongself = weakself; - MGLTestFail(strongself, @"Completion handler should not be called after snapshotter is deallocated."); - }]; - dispatch_group_leave(dg); + @autoreleasepool { + MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size); + weakSnapshotter = snapshotter; + + [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) { + // We expect this completion block to be called with an error + __typeof__(self) strongself = weakself; + + MGLTestAssertNotNil(strongself, snapshot); + MGLTestAssertNil(strongself, error, @"Snapshotter should have completed"); + MGLTestAssertNotNil(strongself, weakSnapshotter, @"Snapshotter should not have been deallocated yet."); + + dispatch_group_leave(dg); + }]; + } }); dispatch_group_notify(dg, dispatch_get_main_queue(), ^{ @@ -128,6 +139,7 @@ MGLMapSnapshotter* snapshotterWithBounds(MGLCoordinateBounds bounds, CGSize size }); [self waitForExpectations:@[expectation] timeout:timeout]; + MGLTestAssertNil(self, weakSnapshotter, @"Snapshotter should not leak."); } - (void)testSnapshotterUsingNestedDispatchQueues🔒 { @@ -157,7 +169,7 @@ MGLMapSnapshotter* snapshotterWithBounds(MGLCoordinateBounds bounds, CGSize size __block MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size); [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) { - // We expect this completion block to be called with an error + // We expect this completion block to be called with no error __typeof__(self) strongself = weakself; MGLTestAssertNotNil(strongself, snapshot); MGLTestAssertNil(strongself, error, @"Snapshotter should have completed"); @@ -178,16 +190,20 @@ MGLMapSnapshotter* snapshotterWithBounds(MGLCoordinateBounds bounds, CGSize size CGSize size = self.mapView.bounds.size; CLLocationCoordinate2D coord = CLLocationCoordinate2DMake(30.0, 30.0); - MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size); + __block __weak MGLMapSnapshotter *weakSnapshotter; + @autoreleasepool { + MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size); + weakSnapshotter = snapshotter; + __weak __typeof__(self) weakself = self; + [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) { + __typeof__(self) strongself = weakself; + MGLTestFail(strongself, @"Completion handler should not be called when canceling."); + }]; + } - __weak __typeof__(self) weakself = self; - - [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) { - __typeof__(self) strongself = weakself; - MGLTestFail(strongself, @"Completion handler should not be called when canceling."); - }]; - - [snapshotter cancel]; + MGLTestAssertNotNil(self, weakSnapshotter); + [weakSnapshotter cancel]; + MGLTestAssertNil(self, weakSnapshotter, @"Snapshotter should not leak after being canceled."); } - (void)testAllocatingSnapshotOnBackgroundQueue🔒 { @@ -290,39 +306,27 @@ MGLMapSnapshotter* snapshotterWithBounds(MGLCoordinateBounds bounds, CGSize size 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); + MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size); + [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]; + + [expectation fulfill]; + }]; } - - [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]; diff --git a/platform/macos/CHANGELOG.md b/platform/macos/CHANGELOG.md index 61acbcb42..18b5a7c42 100644 --- a/platform/macos/CHANGELOG.md +++ b/platform/macos/CHANGELOG.md @@ -47,7 +47,7 @@ ### Snapshots * Added an `-[MGLMapSnapshotter startWithOverlayHandler:completionHandler:]` method to provide the snapshot's current `CGContext` in order to perform custom drawing on `MGLMapSnapshot` objects. ([#15530](https://github.com/mapbox/mapbox-gl-native/pull/15530)) -* `MGLMapSnapshotter` no longer keeps itself from being deallocated before its completion handler is called. To ensure that the completion handler passed into `-[MGLMapSnapshotter startWithCompletionHandler:]` is called, maintain a strong reference to the snapshotter from a longer-lived class, such as the class where you call `-[MGLMapSnapshotter startWithCompletionHandler:]`. ([#210](https://github.com/mapbox/mapbox-gl-native-ios/pull/210)) +* You no longer need to explicitly capture the `MGLMapSnapshotter` object in the completion handler that you specify in `-[MGLMapSnapshotter startWithCompletionHandler:]`. Even if you declare the snapshotter locally without holding a strong reference to it, the snapshotter is only deallocated after the completion handler finishes, and the completion handler generally receives a valid snapshot. ([#210](https://github.com/mapbox/mapbox-gl-native-ios/pull/210)) * The `-[MGLMapSnapshotter cancel]` method no longer calls the completion handler passed into `-[MGLMapSnapshotter startWithCompletionHandler:]`. ([#210](https://github.com/mapbox/mapbox-gl-native-ios/pull/210)) * Fixed an issue where the `MGLMapSnapshotter.loading` property always returned `NO`, even while loading a snapshot. ([#210](https://github.com/mapbox/mapbox-gl-native-ios/pull/210))