diff --git a/platform/darwin/src/MGLOpenGLStyleLayer.mm b/platform/darwin/src/MGLOpenGLStyleLayer.mm index 8933a773820..9f957b7082b 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 @@ @implementation MGLOpenGLStyleLayer @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(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 6400b8fcbf5..45a44865254 100644 --- a/platform/darwin/src/MGLStyleLayer.mm +++ b/platform/darwin/src/MGLStyleLayer.mm @@ -38,6 +38,10 @@ - (void)addToStyle:(MGLStyle *)style belowLayer:(MGLStyleLayer *)otherLayer "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 belowLayerId{otherLayer.identifier.UTF8String}; style.rawStyle->addLayer(std::move(_pendingLayer), belowLayerId); @@ -50,6 +54,10 @@ - (void)removeFromStyle:(MGLStyle *)style { 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); } } diff --git a/platform/darwin/test/MGLStyleTests.mm b/platform/darwin/test/MGLStyleTests.mm index 8f610e338c7..5ab17dd62ed 100644 --- a/platform/darwin/test/MGLStyleTests.mm +++ b/platform/darwin/test/MGLStyleTests.mm @@ -359,20 +359,41 @@ - (void)testRemovingLayerBeforeAddingSameLayer { } - (void)testAddingLayerOfTypeABeforeRemovingLayerOfTypeBWithSameIdentifier { - MGLShapeSource *source = [[MGLShapeSource alloc] initWithIdentifier:@"shape-source-identifier" shape:nil options:nil]; - [self.style addSource:source]; - // Add a fill layer - MGLFillStyleLayer *fillLayer = [[MGLFillStyleLayer alloc] initWithIdentifier:@"some-identifier" source:source]; - [self.style addLayer:fillLayer]; + __weak MGLFillStyleLayer *weakFillLayer = nil; + __weak MGLLineStyleLayer *weakLineLayer = nil; - // Attempt to remove a line layer with the same identifier as the fill layer - MGLLineStyleLayer *lineLayer = [[MGLLineStyleLayer alloc] initWithIdentifier:fillLayer.identifier source:source]; - [self.style removeLayer:lineLayer]; + @autoreleasepool { + + MGLShapeSource *source = [[MGLShapeSource alloc] initWithIdentifier:@"shape-source-identifier" shape:nil options:nil]; + [self.style addSource:source]; + + // Add a fill layer + MGLFillStyleLayer *fillLayer = [[MGLFillStyleLayer alloc] initWithIdentifier:@"some-identifier" source:source]; + weakFillLayer = fillLayer; + + [self.style addLayer:fillLayer]; - XCTAssertTrue([[self.style layerWithIdentifier:fillLayer.identifier] isMemberOfClass:[MGLFillStyleLayer class]]); + // Attempt to remove a line layer with the same identifier as the fill layer + MGLLineStyleLayer *lineLayer = [[MGLLineStyleLayer alloc] initWithIdentifier:fillLayer.identifier source:source]; + weakLineLayer = lineLayer; + [self.style removeLayer:lineLayer]; + + // The above remove should fail, and the fill layer should still be there. + XCTAssertTrue([[self.style layerWithIdentifier:fillLayer.identifier] isMemberOfClass:[MGLFillStyleLayer class]]); + + // Finally remove the fill layer. + [self.style removeLayer:fillLayer]; + + fillLayer = nil; + lineLayer = nil; + } + + XCTAssertNil(weakFillLayer, @""); + XCTAssertNil(weakLineLayer, @""); } + - (NSString *)stringWithContentsOfStyleHeader { NSURL *styleHeaderURL = [[[NSBundle mgl_frameworkBundle].bundleURL URLByAppendingPathComponent:@"Headers" isDirectory:YES] diff --git a/platform/ios/Integration Tests/MBGLIntegrationTests.m b/platform/ios/Integration Tests/MBGLIntegrationTests.m index db6cc13930a..cb3bdc3df68 100644 --- a/platform/ios/Integration Tests/MBGLIntegrationTests.m +++ b/platform/ios/Integration Tests/MBGLIntegrationTests.m @@ -42,6 +42,7 @@ - (void)mapView:(MGLMapView *)mapView didFinishLoadingStyle:(MGLStyle *)style { - (void)tearDown { _styleLoadingExpectation = nil; self.mapView = nil; + self.style = nil; [super tearDown]; } @@ -53,16 +54,57 @@ - (MGLStyle *)style { - (void)testAddingRemovingOpenGLLayer { XCTAssertNotNil(self.style); + // Test fails with 0.1 (presumably because it's < one frame, ie. 1/60) + NSTimeInterval waitInterval = 0.02; + void(^addRemoveGLLayer)(void) = ^{ - MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; - [self.style insertLayer:layer atIndex:0]; - layer = nil; - [[NSRunLoop currentRunLoop] runUntilDate:[NSDate date]]; + 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(); + addRemoveGLLayer(); + addRemoveGLLayer(); +} + +- (void)testAddingRemovingOpenGLLayerWithoutRendering { + XCTAssertNotNil(self.style); - id retrievedLayer = [self.style layerWithIdentifier:@"gl-layer"]; - XCTAssertNotNil(retrievedLayer); - [self.style removeLayer:retrievedLayer]; + void(^addRemoveGLLayer)(void) = ^{ + MGLOpenGLStyleLayer *layer = nil; + __weak id weakLayer = nil; + + @autoreleasepool { + + layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + [self.style insertLayer:layer atIndex:0]; + weakLayer = layer; + layer = nil; + [self.style removeLayer:weakLayer]; + } + + XCTAssertNil(weakLayer); }; addRemoveGLLayer(); @@ -70,8 +112,103 @@ - (void)testAddingRemovingOpenGLLayer { addRemoveGLLayer(); } -//- (void)testOpenGLLayerDoesNotLeakWhenCreatedAndDestroyedWithoutAddingToStyle { -// XCTFail(@"Not yet implemented"); -//} +- (void)testReusingOpenGLLayer { + NSTimeInterval waitInterval = 0.02; + + MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + [self.style insertLayer:layer atIndex:0]; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + + [self.style removeLayer:layer]; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + + [self.style insertLayer:layer atIndex:0]; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + + [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; + + XCTAssertNil(weakLayer); +} + +- (void)testOpenGLLayerDoesNotLeakWhenRemovedFromStyle { + NSTimeInterval waitInterval = 0.02; + + MGLOpenGLStyleLayer *layer; + __weak id weakLayer; + @autoreleasepool { + layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + weakLayer = layer; + [self.style insertLayer:layer atIndex:0]; + layer = nil; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + + [self.style removeLayer:[self.style layerWithIdentifier:@"gl-layer"]]; + [self.mapView setNeedsDisplay]; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + } + XCTAssertNil(weakLayer); +} + +- (void)testOpenGLLayerDoesNotLeakWhenStyleChanged { + NSTimeInterval waitInterval = 0.02; + __weak id weakLayer; + + @autoreleasepool { + MGLOpenGLStyleLayer *layer = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + weakLayer = layer; + [self.style insertLayer:layer atIndex:0]; + layer = nil; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + + 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 waitForExpectationsWithTimeout:1 handler:nil]; + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + } + XCTAssertNil(weakLayer); +} + +- (void)testReusingOpenGLLayerIdentifier { + NSTimeInterval waitInterval = 0.02; + MGLOpenGLStyleLayer *layer1, *layer2; + @autoreleasepool { + layer1 = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + [self.style insertLayer:layer1 atIndex:0]; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + + [self.style removeLayer:layer1]; + + layer2 = [[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"gl-layer"]; + [self.style insertLayer:layer2 atIndex:0]; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + + [self.style removeLayer:layer2]; + + [[NSRunLoop currentRunLoop] runUntilDate:[[NSDate date] dateByAddingTimeInterval:waitInterval]]; + } + XCTAssertNotNil(layer1); + XCTAssertNotNil(layer2); + XCTAssertNil([layer1 style]); + XCTAssertNil([layer2 style]); +} @end diff --git a/platform/ios/ios.xcodeproj/project.pbxproj b/platform/ios/ios.xcodeproj/project.pbxproj index 07fae5945c0..af266543807 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 */; }; @@ -577,6 +578,13 @@ remoteGlobalIDString = DA8847D11CBAF91600AB86E3; remoteInfo = dynamic; }; + CAA55225204486D7004D1B8E /* PBXContainerItemProxy */ = { + isa = PBXContainerItemProxy; + containerPortal = DA1DC9421CB6C1C2006E619F /* Project object */; + proxyType = 1; + remoteGlobalIDString = 16376B2E1FFDB4B40000563E; + remoteInfo = "Integration Test Harness"; + }; DA25D5C71CCDA0C100607828 /* PBXContainerItemProxy */ = { isa = PBXContainerItemProxy; containerPortal = DA1DC9421CB6C1C2006E619F /* Project object */; @@ -622,6 +630,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 +1140,6 @@ isa = PBXFrameworksBuildPhase; buildActionMask = 2147483647; files = ( - 165D0CE720005419009A3C66 /* Mapbox.framework in Frameworks */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -1129,6 +1147,7 @@ isa = PBXFrameworksBuildPhase; buildActionMask = 2147483647; files = ( + CAAD2BD62041EF05003881EC /* Mapbox.framework in Frameworks */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -1908,7 +1927,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 */, @@ -2175,6 +2193,7 @@ buildRules = ( ); dependencies = ( + CAA55226204486D7004D1B8E /* PBXTargetDependency */, 165D0CE620005351009A3C66 /* PBXTargetDependency */, ); name = integration; @@ -2189,6 +2208,7 @@ 16376B2B1FFDB4B40000563E /* Sources */, 16376B2C1FFDB4B40000563E /* Frameworks */, 16376B2D1FFDB4B40000563E /* Resources */, + CAAD2BD82041EF05003881EC /* Embed Frameworks */, ); buildRules = ( ); @@ -2811,6 +2831,11 @@ target = DA8847D11CBAF91600AB86E3 /* dynamic */; targetProxy = 165D0CE520005351009A3C66 /* PBXContainerItemProxy */; }; + CAA55226204486D7004D1B8E /* PBXTargetDependency */ = { + isa = PBXTargetDependency; + target = 16376B2E1FFDB4B40000563E /* Integration Test Harness */; + targetProxy = CAA55225204486D7004D1B8E /* PBXContainerItemProxy */; + }; DA25D5C81CCDA0C100607828 /* PBXTargetDependency */ = { isa = PBXTargetDependency; target = DA25D5B81CCD9EDE00607828 /* settings */;