[core, ios, macos] Pair retain/release of MGLOpenGLLayer in MGLStyle (for duration of style and underlying core layer), and a second pairing in the prepare and finish callbacks from the renderer.

7 files changed, 306 insertions, 24 deletions

diff --git a/include/mbgl/style/layers/custom_layer.hpp b/include/mbgl/style/layers/custom_layer.hpp
index bf3387f95b..0eb2f27852 100644
--- a/include/mbgl/style/layers/custom_layer.hpp
+++ b/include/mbgl/style/layers/custom_layer.hpp
@@ -2,6 +2,8 @@
 
 #include <mbgl/style/layer.hpp>
 
+#include <functional>
+
 namespace mbgl {
 namespace style {
 
@@ -44,7 +46,7 @@ using CustomLayerRenderFunction = void (*)(void* context, const CustomLayerRende
  * `CustomLayerInitializeFunction` will be called instead to prepare for a new render.
  *
  */
-using CustomLayerContextLostFunction = void (*)(void* context);
+using CustomLayerContextLostFunction = std::function<void(void*)>;
 
 /**
  * Destroy any GL state needed by the custom layer, and deallocate context, if necessary. This
diff --git a/platform/darwin/src/MGLOpenGLStyleLayer.mm b/platform/darwin/src/MGLOpenGLStyleLayer.mm
index 8933a77382..5d7beac040 100644
--- a/platform/darwin/src/MGLOpenGLStyleLayer.mm
+++ b/platform/darwin/src/MGLOpenGLStyleLayer.mm
@@ -16,6 +16,7 @@
  */
 void MGLPrepareCustomStyleLayer(void *context) {
     MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer *)context;
+    (__bridge_retained void *)layer;
     [layer didMoveToMapView:layer.style.mapView];
 }
 
@@ -101,7 +102,7 @@ void MGLFinishCustomStyleLayer(void *context) {
                                                             MGLPrepareCustomStyleLayer,
                                                             MGLDrawCustomStyleLayer,
                                                             MGLFinishCustomStyleLayer,
-                                                            (__bridge_retained void *)self);
+                                                            (__bridge void *) self);
     return self = [super initWithPendingLayer:std::move(layer)];
 }
 
@@ -116,7 +117,9 @@ void MGLFinishCustomStyleLayer(void *context) {
         [NSException raise:@"MGLLayerReuseException"
                     format:@"%@ cannot be added to more than one MGLStyle at a time.", self];
     }
+    _style.openGLLayers[self.identifier] = nil;
     _style = style;
+    _style.openGLLayers[self.identifier] = self;
 }
 
 - (void)addToStyle:(MGLStyle *)style belowLayer:(MGLStyleLayer *)otherLayer {
diff --git a/platform/darwin/src/MGLStyle.mm b/platform/darwin/src/MGLStyle.mm
index f6fc5533be..0106ea7601 100644
--- a/platform/darwin/src/MGLStyle.mm
+++ b/platform/darwin/src/MGLStyle.mm
@@ -83,6 +83,7 @@
 @property (nonatomic, readonly, weak) MGLMapView *mapView;
 @property (nonatomic, readonly) mbgl::style::Style *rawStyle;
 @property (readonly, copy, nullable) NSURL *URL;
+@property (nonatomic, readwrite, strong) NS_MUTABLE_DICTIONARY_OF(NSString *, MGLOpenGLStyleLayer *) *openGLLayers;
 @property (nonatomic) NS_MUTABLE_DICTIONARY_OF(NSString *, NS_DICTIONARY_OF(NSObject *, MGLTextLanguage *) *) *localizedLayersByIdentifier;
 
 @end
@@ -174,6 +175,7 @@ static NSURL *MGLStyleURL_trafficNight;
     if (self = [super init]) {
         _mapView = mapView;
         _rawStyle = rawStyle;
+        _openGLLayers = [NSMutableDictionary dictionary];
         _localizedLayersByIdentifier = [NSMutableDictionary dictionary];
     }
     return self;
diff --git a/platform/darwin/src/MGLStyle_Private.h b/platform/darwin/src/MGLStyle_Private.h
index 4cbe953a44..3597a0a0a6 100644
--- a/platform/darwin/src/MGLStyle_Private.h
+++ b/platform/darwin/src/MGLStyle_Private.h
@@ -25,7 +25,7 @@ namespace mbgl {
 @property (nonatomic, readonly) mbgl::style::Style *rawStyle;
 
 - (nullable NS_ARRAY_OF(MGLAttributionInfo *) *)attributionInfosWithFontSize:(CGFloat)fontSize linkColor:(nullable MGLColor *)linkColor;
-
+@property (nonatomic, readonly, strong) NS_MUTABLE_DICTIONARY_OF(NSString *, MGLOpenGLStyleLayer *) *openGLLayers;
 - (void)setStyleClasses:(NS_ARRAY_OF(NSString *) *)appliedClasses transitionDuration:(NSTimeInterval)transitionDuration;
 
 @end
diff --git a/platform/ios/Integration Tests/MBGLIntegrationTests.m b/platform/ios/Integration Tests/MBGLIntegrationTests.m
index deb362cd64..9b5281183a 100644
--- a/platform/ios/Integration Tests/MBGLIntegrationTests.m
+++ b/platform/ios/Integration Tests/MBGLIntegrationTests.m
@@ -1,6 +1,10 @@
 #import <XCTest/XCTest.h>
+#import <objc/message.h>
+#import <Mapbox/Mapbox.h>
+
+
+#define TRACE(message) NSLog(@"%@: %@", NSStringFromSelector(_cmd), message)
 
-@import Mapbox;
 
 @interface MBGLIntegrationTests : XCTestCase <MGLMapViewDelegate>
 
@@ -11,8 +15,11 @@
 
 @implementation MBGLIntegrationTests {
     XCTestExpectation *_styleLoadingExpectation;
+    XCTestExpectation *_renderFinishedExpectation;
 }
 
+#pragma mark - Setup/Teardown
+
 - (void)setUp {
     [super setUp];
 
@@ -32,6 +39,16 @@
     [window makeKeyAndVisible];
 }
 
+- (void)tearDown {
+    _styleLoadingExpectation = nil;
+    self.mapView = nil;
+    self.style = nil;
+
+    [super tearDown];
+}
+
+#pragma mark - MGLMapViewDelegate
+
 - (void)mapView:(MGLMapView *)mapView didFinishLoadingStyle:(MGLStyle *)style {
     XCTAssertNotNil(mapView.style);
     XCTAssertEqual(mapView.style, style);
@@ -39,30 +56,139 @@
     [_styleLoadingExpectation fulfill];
 }
 
-- (void)tearDown {
-    _styleLoadingExpectation = nil;
-    self.mapView = nil;
+- (void)mapViewDidFinishRenderingFrame:(MGLMapView *)mapView fullyRendered:(__unused BOOL)fullyRendered {
+    [_renderFinishedExpectation fulfill];
+    _renderFinishedExpectation = nil;
+}
 
-    [super tearDown];
+#pragma mark - Utilities
+
+- (void)waitForMapViewToBeRendered {
+    [self.mapView setNeedsDisplay];
+    _renderFinishedExpectation = [self expectationWithDescription:@"Map view should be rendered"];
+    [self waitForExpectations:@[_renderFinishedExpectation] timeout:1];
 }
 
 - (MGLStyle *)style {
     return self.mapView.style;
 }
 
+#pragma mark - Tests
+
+- (void)testStoringOpenGLLayerInCollections {
+    // If we change the meaning of equality for MGLStyleLayer we want to know about it - since we
+    // store layers in both a NSMutableSet and in a NSMapTable
+    MGLStyleLayer *layer1 = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+    MGLStyleLayer *layer2 = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+
+    XCTAssertNotEqual(layer1, layer2);          // Test diff pointers, in case behind the scenes it tries to return the same object.
+    XCTAssertNotEqualObjects(layer1, layer2);   // Although they have the same identifier, we DO NOT consider them equal.
+    NSMutableSet *set = [NSMutableSet set];
+    [set addObject:layer1];
+    [set addObject:layer2];
+    XCTAssert(set.count == 2);
+}
+
+// This test does not strictly need to be in this test file/target. Including here for convenience.
+- (void)testOpenGLLayerDoesNotLeakWhenCreatedAndDestroyedWithoutAddingToStyle {
+    MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+    __weak id weakLayer = layer;
+    layer = nil;
+
+    XCTAssertNil(weakLayer);
+}
+
+- (void)testAddingRemovingOpenGLLayerWithoutRendering {
+    XCTAssertNotNil(self.style);
+
+    void(^addRemoveGLLayer)(void) = ^{
+        __weak id weakLayer = nil;
+
+        @autoreleasepool {
+            MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+            [self.style insertLayer:layer atIndex:0];
+            weakLayer = layer;
+
+            // Nil the layer prior to remove to ensure it's being retained
+            layer = nil;
+            [self.style removeLayer:weakLayer];
+        }
+
+        XCTAssertNil(weakLayer);
+    };
+
+    addRemoveGLLayer();
+    addRemoveGLLayer();
+    addRemoveGLLayer();
+}
+
+- (void)testReusingOpenGLLayerIdentifier {
+    __weak MGLOpenGLStyleLayer *weakLayer2;
+
+    @autoreleasepool {
+        MGLOpenGLStyleLayer *layer1 = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+        [self.style insertLayer:layer1 atIndex:0];
+        [self waitForMapViewToBeRendered];
+        [self.style removeLayer:layer1];
+
+        MGLOpenGLStyleLayer *layer2 = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+        weakLayer2 = layer2;
+
+        XCTAssertNotNil(layer2);
+        XCTAssert(layer1 != layer2);
+
+        [self.style insertLayer:layer2 atIndex:0];
+        [self waitForMapViewToBeRendered];
+        [self.style removeLayer:layer2];
+
+        XCTAssertNil([layer1 style]);
+        XCTAssertNil([layer2 style]);
+    }
+
+    // At this point, layer2 (and layer1) should still be around, since the render process needs to
+    // keep a reference to them.
+    XCTAssertNotNil(weakLayer2);
+
+    // Let render loop run enough to release the layers
+    [self waitForMapViewToBeRendered];
+    XCTAssertNil(weakLayer2);
+}
+
 - (void)testAddingRemovingOpenGLLayer {
     XCTAssertNotNil(self.style);
 
     void(^addRemoveGLLayer)(void) = ^{
-        MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
-        [self.style insertLayer:layer atIndex:0];
-        layer = nil;
 
-        [[NSRunLoop currentRunLoop] runUntilDate:[NSDate date]];
+        __weak id retrievedLayer = nil;
+        
+        @autoreleasepool {
+            MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+            [self.style insertLayer:layer atIndex:0];
+
+            NSUInteger address1 = (NSUInteger)layer;
+            layer = nil;
+            
+            // We want to ensure the MGLOpenGLStyleLayer's raw Layer's Impl gets handed to a RenderLayer
+            // (which in turn has a raw context pointer back to the layer)
+            [self waitForMapViewToBeRendered];
+
+            retrievedLayer = [self.style layerWithIdentifier:@"gl-layer"];
+            NSUInteger address2 = (NSUInteger)retrievedLayer;
+
+            XCTAssertNotNil(retrievedLayer);
+
+            // These two should be the same object in the case of MGLOpenGLStyleLayer
+            XCTAssert(address1 == address2);
+
+            [self.style removeLayer:retrievedLayer];
+
+            // We need to run the runloop for a little while (2 trips through the renderer) to allow
+            // the managed layer to be released. This is because it takes 2 renders before the RenderLayer
+            // (and the associated Impl) are destroyed.
+            [self waitForMapViewToBeRendered];
+        }
 
-        id retrievedLayer = [self.style layerWithIdentifier:@"gl-layer"];
-        XCTAssertNotNil(retrievedLayer);
-        [self.style removeLayer:retrievedLayer];
+        XCTAssertNil(retrievedLayer);
     };
 
     addRemoveGLLayer();
@@ -71,33 +197,181 @@
 }
 
 - (void)testReusingOpenGLLayer {
-    NSTimeInterval waitInterval = 0.01;
+    NSTimeInterval waitInterval = 0.02;
 
+    NSLog(@"testReusingOpenGLLayer - init and insert layer");
     MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
     [self.style insertLayer:layer atIndex:0];
 
     [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
 
+    NSLog(@"testReusingOpenGLLayer - remove layer %p", layer);
     [self.style removeLayer:layer];
 
     [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
 
+    NSLog(@"testReusingOpenGLLayer - re-insert layer %p", layer);
+
     [self.style insertLayer:layer atIndex:0];
 
     [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
 
+    NSLog(@"testReusingOpenGLLayer - re-remove layer %p", layer);
+
     [self.style removeLayer:layer];
 
     [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
 }
 
-// This test does not strictly need to be in this test file/target. Including here for convenience.
-- (void)testOpenGLLayerDoesNotLeakWhenCreatedAndDestroyedWithoutAddingToStyle {
-    MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
-    __weak id weakLayer = layer;
-    layer = nil;
-    
+- (void)testOpenGLLayerDoesNotLeakWhenRemovedFromStyle {
+
+    //    MGLOpenGLStyleLayer *layer;
+    __weak id weakLayer;
+    @autoreleasepool {
+        MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+        weakLayer = layer;
+        [self.style insertLayer:layer atIndex:0];
+        layer = nil;
+
+        // Run the render loop, so the layer gets used/managed.
+        [self waitForMapViewToBeRendered];
+        //        [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]];
+
+        [self.style removeLayer:[self.style layerWithIdentifier:@"gl-layer"]];
+    }
+
+    MGLStyleLayer *layer2 = weakLayer;
+
+    XCTAssertNotNil(weakLayer);
+    [self waitForMapViewToBeRendered];
+
+    layer2 = nil;
+    XCTAssertNil(weakLayer);
+}
+
+- (void)testUserAddedLineLayerIsReleasedWhenStyleChanged {
+    __weak MGLStyleLayer* weakLayer1 = nil;
+    __weak MGLStyleLayer* weakLayer2 = nil;
+
+    __unsafe_unretained MGLStyleLayer* unsafeLayer1 = nil;
+    __unsafe_unretained MGLStyleLayer* unsafeLayer2 = nil;
+
+    // Add a line layer
+    @autoreleasepool {
+        NSString *filename = [[NSBundle mainBundle] pathForResource:@"polyline" ofType:@"geojson"];
+        NSData *shapeData = [NSData dataWithContentsOfFile:filename];
+        MGLShape *shape = [MGLShape shapeWithData:shapeData encoding:NSUTF8StringEncoding error:nil];
+        MGLSource *source1 = [[MGLShapeSource alloc] initWithIdentifier:@"polyline" shape:shape options:nil];
+
+        [self.style addSource:source1];
+        MGLStyleLayer *lineLayer1 = [[MGLLineStyleLayer alloc] initWithIdentifier:@"line-layer" source:source1];
+        [self.style insertLayer:lineLayer1 atIndex:0];
+
+        unsafeLayer1 = lineLayer1;
+        weakLayer1 = lineLayer1;
+    }
+    XCTAssertNil(weakLayer1);
+
+    // Look for the same layer by Id
+    @autoreleasepool {
+        MGLStyleLayer *lineLayer2 = [self.style layerWithIdentifier:@"line-layer"];
+        MGLSource *source2 = [self.style sourceWithIdentifier:@"polyline"];
+
+        XCTAssertNotNil(lineLayer2);
+        XCTAssertNotNil(source2);
+
+        unsafeLayer2 = lineLayer2;
+        weakLayer2 = lineLayer2;
+    }
+    XCTAssertNil(weakLayer2);
+
+    // Since we only add MGLOpenGLStyleLayers to our retain/release management set, the above layers
+    // will have been released, but we want to check that the second one was re-created. So just
+    // check pointer values
+    XCTAssert(unsafeLayer1);
+    XCTAssert(unsafeLayer2);
+    XCTAssert(unsafeLayer1 != unsafeLayer2);
+
+    // Swap style
+    NSURL *styleURL = [[NSBundle bundleForClass:[self class]] URLForResource:@"one-liner" withExtension:@"json"];
+    _styleLoadingExpectation = [self expectationWithDescription:@"Map view should finish loading style."];
+    [self.mapView setStyleURL:styleURL];
+    [self waitForExpectations:@[_styleLoadingExpectation] timeout:10];
+
+    // Asking the style for the layer should return nil
+    {
+        MGLStyleLayer *lineLayer = [self.style layerWithIdentifier:@"line-layer"];
+        MGLSource *lineSource = [self.style sourceWithIdentifier:@"polyline"];
+        XCTAssertNil(lineLayer);
+        XCTAssertNil(lineSource);
+    }
+}
+
+- (void)testOpenGLLayerDoesNotLeakWhenStyleChanged {
+
+    __weak MGLOpenGLStyleLayer *weakLayer;
+
+    @autoreleasepool {
+        {
+            MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+            weakLayer = layer;
+            [self.style insertLayer:layer atIndex:0];
+            layer = nil;
+        }
+    }
+
+    // Unlike other MGLStyleLayers, MGLOpenGLStyleLayers are managed for retain/release purposes
+    XCTAssertNotNil(weakLayer);
+
+    [self waitForMapViewToBeRendered];
+
+    // should be identical to the original
+    MGLStyleLayer *layer2 = [self.mapView.style layerWithIdentifier:@"gl-layer"];
+
+
+    NSURL *styleURL = [[NSBundle bundleForClass:[self class]] URLForResource:@"one-liner" withExtension:@"json"];
+    _styleLoadingExpectation = [self expectationWithDescription:@"Map view should finish loading style."];
+    [self.mapView setStyleURL:styleURL];
+    [self waitForExpectations:@[_styleLoadingExpectation] timeout:10];
+
+    // At this point the C++ CustomLayer will have been destroyed, and the rawLayer pointer has been NULLed
+    XCTAssert(weakLayer == layer2);
+    XCTAssertNotNil(weakLayer);
+
+    // Asking the style for the layer should return nil
+    MGLStyleLayer *layer3 = [self.mapView.style layerWithIdentifier:@"gl-layer"];
+    XCTAssertNil(layer3);
+}
+
+
+- (void)testOpenGLLayerDoesNotLeakWhenMapViewDeallocs {
+    __weak id weakLayer;
+ 
+    @autoreleasepool {
+
+        NSURL *styleURL = [[NSBundle bundleForClass:[self class]] URLForResource:@"one-liner" withExtension:@"json"];
+        MGLMapView *mapView2 = [[MGLMapView alloc] initWithFrame:UIScreen.mainScreen.bounds styleURL:styleURL];
+        mapView2.delegate = self;
+
+        XCTAssertNil(mapView2.style);
+
+        _styleLoadingExpectation = [self expectationWithDescription:@"Map view should finish loading style."];
+        [self waitForExpectationsWithTimeout:1 handler:nil];
+
+        MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"];
+        weakLayer = layer;
+        [mapView2.style insertLayer:layer atIndex:0];
+        layer = nil;
+
+        // Let the render process fire, to ensure a RenderLayer gets created with a raw pointer back
+        // to the layer (and it should be retained).
+        [self waitForMapViewToBeRendered];
+
+        // We don't wait for the layers to be released here, we'll just let the MGLMapView dealloc
+        // so everything should be cleaned up.
+    }
     XCTAssertNil(weakLayer);
 }
 
 @end
+
diff --git a/platform/ios/ios.xcodeproj/project.pbxproj b/platform/ios/ios.xcodeproj/project.pbxproj
index 07fae5945c..d1140f93ef 100644
--- a/platform/ios/ios.xcodeproj/project.pbxproj
+++ b/platform/ios/ios.xcodeproj/project.pbxproj
@@ -1908,7 +1908,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 */,
@@ -2341,6 +2340,7 @@
 					};
 					16376B2E1FFDB4B40000563E = {
 						CreatedOnToolsVersion = 9.2;
+						DevelopmentTeam = GJZR2MEM28;
 						ProvisioningStyle = Automatic;
 					};
 					DA1DC9491CB6C1C2006E619F = {
@@ -3072,6 +3072,7 @@
 				CLANG_WARN_UNGUARDED_AVAILABILITY = YES_AGGRESSIVE;
 				CODE_SIGN_IDENTITY = "iPhone Developer";
 				CODE_SIGN_STYLE = Automatic;
+				DEVELOPMENT_TEAM = GJZR2MEM28;
 				GCC_C_LANGUAGE_STANDARD = gnu11;
 				INFOPLIST_FILE = "Integration Test Harness/Info.plist";
 				IPHONEOS_DEPLOYMENT_TARGET = 11.2;
@@ -3097,6 +3098,7 @@
 				CLANG_WARN_UNGUARDED_AVAILABILITY = YES_AGGRESSIVE;
 				CODE_SIGN_IDENTITY = "iPhone Developer";
 				CODE_SIGN_STYLE = Automatic;
+				DEVELOPMENT_TEAM = GJZR2MEM28;
 				GCC_C_LANGUAGE_STANDARD = gnu11;
 				INFOPLIST_FILE = "Integration Test Harness/Info.plist";
 				IPHONEOS_DEPLOYMENT_TARGET = 11.2;
diff --git a/src/mbgl/style/layers/custom_layer.cpp b/src/mbgl/style/layers/custom_layer.cpp
index 854c771847..ed3ffb21b2 100644
--- a/src/mbgl/style/layers/custom_layer.cpp
+++ b/src/mbgl/style/layers/custom_layer.cpp
@@ -38,7 +38,6 @@ std::unique_ptr<Layer> CustomLayer::cloneRef(const std::string&) const {
 }
 
 // Visibility
-
 void CustomLayer::setVisibility(VisibilityType value) {
     if (value == getVisibility())
         return;