Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Adds tests and potential fixes for #10771 and #11143 #11291

Closed
wants to merge 5 commits into from
Closed

Adds tests and potential fixes for #10771 and #11143 #11291

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48918fa
Adds failing tests for #10771 and #11143
Feb 23, 2018
e4aaaa3
[ios] Potential fix for over retain/release issues with adding layers…
Feb 26, 2018
2a4d13b
[ios] Release only called if the layer is really being removed from t…
Feb 26, 2018
47e6a03
[ios] Add explicit dependency for integration tests (for CI)
Feb 26, 2018
fafedb6
[ios] Adding a few more test cases. testOpenGLLayerDoesNotLeakWhenSty…
Feb 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions platform/darwin/src/MGLOpenGLStyleLayer.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}

Expand Down Expand Up @@ -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];
}

Expand Down Expand Up @@ -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<mbgl::style::CustomLayer>(identifier.UTF8String,
MGLPrepareCustomStyleLayer,
MGLDrawCustomStyleLayer,
MGLFinishCustomStyleLayer,
(__bridge_retained void *)self);
(__bridge void*)self);
return self = [super initWithPendingLayer:std::move(layer)];
}

Expand Down
8 changes: 8 additions & 0 deletions platform/darwin/src/MGLStyleLayer.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> belowLayerId{otherLayer.identifier.UTF8String};
style.rawStyle->addLayer(std::move(_pendingLayer), belowLayerId);
Expand All @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm wondering - is this guaranteed to happen, always? Or do we need more of a dance to ensure this is released if the whole style is destroyed without an explicit call to -removeFromStyle:...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, when the style gets cleaned up. But we should add a test for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that - I think we need some more clean-up.

}
}

Expand Down
39 changes: 30 additions & 9 deletions platform/darwin/test/MGLStyleTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
66 changes: 55 additions & 11 deletions platform/ios/Integration Tests/MBGLIntegrationTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ - (void)mapView:(MGLMapView *)mapView didFinishLoadingStyle:(MGLStyle *)style {
- (void)tearDown {
_styleLoadingExpectation = nil;
self.mapView = nil;
self.style = nil;

[super tearDown];
}
Expand All @@ -53,25 +54,68 @@ - (MGLStyle *)style {
- (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]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we expect the layer to be released as of the next render, how about putting in an expectation that gets satisfied upon the next call to -[MGLMapViewDelegate mapViewDidFinishRenderingFrame:fullyRendered:]?

}

XCTAssertNil(retrievedLayer);
};

addRemoveGLLayer();
addRemoveGLLayer();
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);
}

@end
31 changes: 28 additions & 3 deletions platform/ios/ios.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
Expand Down Expand Up @@ -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 */; };
Expand Down Expand Up @@ -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 */;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1121,14 +1140,14 @@
isa = PBXFrameworksBuildPhase;
buildActionMask = 2147483647;
files = (
165D0CE720005419009A3C66 /* Mapbox.framework in Frameworks */,
);
runOnlyForDeploymentPostprocessing = 0;
};
16376B2C1FFDB4B40000563E /* Frameworks */ = {
isa = PBXFrameworksBuildPhase;
buildActionMask = 2147483647;
files = (
CAAD2BD62041EF05003881EC /* Mapbox.framework in Frameworks */,
);
runOnlyForDeploymentPostprocessing = 0;
};
Expand Down Expand Up @@ -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 */,
Expand Down Expand Up @@ -2175,6 +2193,7 @@
buildRules = (
);
dependencies = (
CAA55226204486D7004D1B8E /* PBXTargetDependency */,
165D0CE620005351009A3C66 /* PBXTargetDependency */,
);
name = integration;
Expand All @@ -2189,6 +2208,7 @@
16376B2B1FFDB4B40000563E /* Sources */,
16376B2C1FFDB4B40000563E /* Frameworks */,
16376B2D1FFDB4B40000563E /* Resources */,
CAAD2BD82041EF05003881EC /* Embed Frameworks */,
);
buildRules = (
);
Expand Down Expand Up @@ -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 */;
Expand Down