[ios] Potential fix for over retain/release issues with adding layers to styles. Integration tests run on device.

4 files changed, 58 insertions, 15 deletions

diff --git a/platform/darwin/src/MGLOpenGLStyleLayer.mm b/platform/darwin/src/MGLOpenGLStyleLayer.mm
index 8933a77382..9f957b7082 100644
--- a/platform/darwin/src/MGLOpenGLStyleLayer.mm
+++ b/platform/darwin/src/MGLOpenGLStyleLayer.mm
@@ -15,7 +15,11 @@
     when creating an OpenGL style layer.
  */
 void MGLPrepareCustomStyleLayer(void *context) {
-    MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer *)context;
+
+    // Pair retain/release during rendering (see MGLFinishCustomStyleLayer)
+    id retaineee = (__bridge id)context;
+    MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer*)CFBridgingRetain(retaineee);
+
     [layer didMoveToMapView:layer.style.mapView];
 }
 
@@ -47,7 +51,8 @@ void MGLDrawCustomStyleLayer(void *context, const mbgl::style::CustomLayerRender
     when creating an OpenGL style layer.
  */
 void MGLFinishCustomStyleLayer(void *context) {
-    MGLOpenGLStyleLayer *layer = (__bridge_transfer MGLOpenGLStyleLayer *)context;
+    // Release the layer (since we retained it in the initialization)
+    MGLOpenGLStyleLayer *layer = CFBridgingRelease(context);
     [layer willMoveFromMapView:layer.style.mapView];
 }
 
@@ -97,11 +102,12 @@ void MGLFinishCustomStyleLayer(void *context) {
  @return An initialized OpenGL style layer.
  */
 - (instancetype)initWithIdentifier:(NSString *)identifier {
+    // Note, do not retain self here, otherwise MGLOpenGLStyleLayer will never be dealloc'd
     auto layer = std::make_unique<mbgl::style::CustomLayer>(identifier.UTF8String,
                                                             MGLPrepareCustomStyleLayer,
                                                             MGLDrawCustomStyleLayer,
                                                             MGLFinishCustomStyleLayer,
-                                                            (__bridge_retained void *)self);
+                                                            (__bridge void*)self);
     return self = [super initWithPendingLayer:std::move(layer)];
 }
 
diff --git a/platform/darwin/src/MGLStyleLayer.mm b/platform/darwin/src/MGLStyleLayer.mm
index 6400b8fcbf..2a70cab563 100644
--- a/platform/darwin/src/MGLStyleLayer.mm
+++ b/platform/darwin/src/MGLStyleLayer.mm
@@ -38,6 +38,10 @@
                     "to the style more than once is invalid.", self, style];
     }
 
+    // Since we're adding self to a C++ collection, we need to retain ourselves, so that we don't
+    // end up with a dangling pointer
+    CFBridgingRetain(self);
+
     if (otherLayer) {
         const mbgl::optional<std::string> belowLayerId{otherLayer.identifier.UTF8String};
         style.rawStyle->addLayer(std::move(_pendingLayer), belowLayerId);
@@ -51,6 +55,10 @@
     if (self.rawLayer == style.rawStyle->getLayer(self.identifier.UTF8String)) {
         _pendingLayer = style.rawStyle->removeLayer(self.identifier.UTF8String);
     }
+
+    // Pair the retain above, and release self, since we're now removed from the collection
+    CFTypeRef toRelease = (__bridge CFTypeRef)self;
+    CFBridgingRelease(toRelease);
 }
 
 - (void)setVisible:(BOOL)visible
diff --git a/platform/ios/Integration Tests/MBGLIntegrationTests.m b/platform/ios/Integration Tests/MBGLIntegrationTests.m
index deb362cd64..941e73621f 100644
--- a/platform/ios/Integration Tests/MBGLIntegrationTests.m
+++ b/platform/ios/Integration Tests/MBGLIntegrationTests.m
@@ -53,16 +53,33 @@
 - (void)testAddingRemovingOpenGLLayer {
     XCTAssertNotNil(self.style);
 
-    void(^addRemoveGLLayer)(void) = ^{
-        MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
-        [self.style insertLayer:layer atIndex:0];
-        layer = nil;
+    // Test fails with 0.1 (presumably because it's < one frame, ie. 1/60)
+    NSTimeInterval waitInterval = 0.02;
 
-        [[NSRunLoop currentRunLoop] runUntilDate:[NSDate date]];
+    void(^addRemoveGLLayer)(void) = ^{
 
-        id retrievedLayer = [self.style layerWithIdentifier:@"gl-layer"];
-        XCTAssertNotNil(retrievedLayer);
-        [self.style removeLayer:retrievedLayer];
+        MGLOpenGLStyleLayer *layer = nil;
+        __weak id retrievedLayer = nil;
+        
+        @autoreleasepool {
+            layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+            [self.style insertLayer:layer atIndex:0];
+            layer = nil;
+            
+            [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
+            
+            retrievedLayer = [self.style layerWithIdentifier:@"gl-layer"];
+            XCTAssertNotNil(retrievedLayer);
+            [self.style removeLayer:retrievedLayer];
+
+            // We need to run the runloop for a little while so that the following assert will be correct
+            // this is because although the layer has been removed from the style, there's still a pending
+            // render (deinitialize) call, that will needs to be handled, which will finally release the
+            // layer (and then the layer will be dealloc'd when the autorelease pool drains)
+            [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
+        }
+        
+        XCTAssertNil(retrievedLayer);
     };
 
     addRemoveGLLayer();
@@ -71,7 +88,7 @@
 }
 
 - (void)testReusingOpenGLLayer {
-    NSTimeInterval waitInterval = 0.01;
+    NSTimeInterval waitInterval = 0.02;
 
     MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
     [self.style insertLayer:layer atIndex:0];
diff --git a/platform/ios/ios.xcodeproj/project.pbxproj b/platform/ios/ios.xcodeproj/project.pbxproj
index 07fae5945c..7f0407ab11 100644
--- a/platform/ios/ios.xcodeproj/project.pbxproj
+++ b/platform/ios/ios.xcodeproj/project.pbxproj
@@ -27,7 +27,6 @@
 		16376B411FFDB4B40000563E /* main.m in Sources */ = {isa = PBXBuildFile; fileRef = 16376B401FFDB4B40000563E /* main.m */; };
 		16376B471FFDB92B0000563E /* one-liner.json in Resources */ = {isa = PBXBuildFile; fileRef = DA35D0871E1A6309007DED41 /* one-liner.json */; };
 		16376B491FFEED010000563E /* MGLMapViewLayoutTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 16376B481FFEED010000563E /* MGLMapViewLayoutTests.m */; };
-		165D0CE720005419009A3C66 /* Mapbox.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = DA8847D21CBAF91600AB86E3 /* Mapbox.framework */; };
 		170C437C2029D96F00863DF0 /* MGLHeatmapColorTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 170C43782028D49800863DF0 /* MGLHeatmapColorTests.mm */; };
 		170C437D2029D97900863DF0 /* MGLHeatmapStyleLayerTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 170C43792028D49800863DF0 /* MGLHeatmapStyleLayerTests.mm */; };
 		1753ED421E53CE6F00A9FD90 /* MGLConversion.h in Headers */ = {isa = PBXBuildFile; fileRef = 1753ED411E53CE6F00A9FD90 /* MGLConversion.h */; };
@@ -309,6 +308,8 @@
 		AC518E04201BB56100EBC820 /* MGLTelemetryConfig.m in Sources */ = {isa = PBXBuildFile; fileRef = AC518DFE201BB55A00EBC820 /* MGLTelemetryConfig.m */; };
 		CA55CD41202C16AA00CE7095 /* MGLCameraChangeReason.h in Headers */ = {isa = PBXBuildFile; fileRef = CA55CD3E202C16AA00CE7095 /* MGLCameraChangeReason.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		CA55CD42202C16AA00CE7095 /* MGLCameraChangeReason.h in Headers */ = {isa = PBXBuildFile; fileRef = CA55CD3E202C16AA00CE7095 /* MGLCameraChangeReason.h */; settings = {ATTRIBUTES = (Public, ); }; };
+		CAAD2BD62041EF05003881EC /* Mapbox.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = DA4A26961CB6E795000B7809 /* Mapbox.framework */; };
+		CAAD2BD72041EF05003881EC /* Mapbox.framework in Embed Frameworks */ = {isa = PBXBuildFile; fileRef = DA4A26961CB6E795000B7809 /* Mapbox.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; };
 		DA00FC8E1D5EEB0D009AABC8 /* MGLAttributionInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = DA00FC8C1D5EEB0D009AABC8 /* MGLAttributionInfo.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		DA00FC8F1D5EEB0D009AABC8 /* MGLAttributionInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = DA00FC8C1D5EEB0D009AABC8 /* MGLAttributionInfo.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		DA00FC901D5EEB0D009AABC8 /* MGLAttributionInfo.mm in Sources */ = {isa = PBXBuildFile; fileRef = DA00FC8D1D5EEB0D009AABC8 /* MGLAttributionInfo.mm */; };
@@ -622,6 +623,17 @@
 /* End PBXContainerItemProxy section */
 
 /* Begin PBXCopyFilesBuildPhase section */
+		CAAD2BD82041EF05003881EC /* Embed Frameworks */ = {
+			isa = PBXCopyFilesBuildPhase;
+			buildActionMask = 2147483647;
+			dstPath = "";
+			dstSubfolderSpec = 10;
+			files = (
+				CAAD2BD72041EF05003881EC /* Mapbox.framework in Embed Frameworks */,
+			);
+			name = "Embed Frameworks";
+			runOnlyForDeploymentPostprocessing = 0;
+		};
 		DA4A269A1CB6F5D3000B7809 /* Embed Frameworks */ = {
 			isa = PBXCopyFilesBuildPhase;
 			buildActionMask = 2147483647;
@@ -1121,7 +1133,6 @@
 			isa = PBXFrameworksBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
-				165D0CE720005419009A3C66 /* Mapbox.framework in Frameworks */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
@@ -1129,6 +1140,7 @@
 			isa = PBXFrameworksBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
+				CAAD2BD62041EF05003881EC /* Mapbox.framework in Frameworks */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
@@ -1908,7 +1920,6 @@
 				DA8848231CBAFA6200AB86E3 /* MGLOfflineStorage_Private.h in Headers */,
 				404326891D5B9B27007111BD /* MGLAnnotationContainerView_Private.h in Headers */,
 				CA55CD41202C16AA00CE7095 /* MGLCameraChangeReason.h in Headers */,
-				1FB7DAAF1F2A4DBD00410606 /* MGLVectorSource+MGLAdditions.h in Headers */,
 				DA88483B1CBAFB8500AB86E3 /* MGLCalloutView.h in Headers */,
 				35E0CFE61D3E501500188327 /* MGLStyle_Private.h in Headers */,
 				3510FFF01D6D9D8C00F413B2 /* NSExpression+MGLAdditions.h in Headers */,
@@ -2189,6 +2200,7 @@
 				16376B2B1FFDB4B40000563E /* Sources */,
 				16376B2C1FFDB4B40000563E /* Frameworks */,
 				16376B2D1FFDB4B40000563E /* Resources */,
+				CAAD2BD82041EF05003881EC /* Embed Frameworks */,
 			);
 			buildRules = (
 			);