Fix CustomLayer context retain count #10765

asheemmamoowala · 2017-12-20T20:31:20Z

In core, with style diffing a RenderCustomLayer instance is reused if the new style has a CustomLayer with the same identifier as the previous style. This was not handled correctly in RenderCustomLayer. When the impl or context for the layer is changed, it should invoke the CustomLayerDeinitializeFunction callback.

Changes to core GL for asynchronous rendering changed when the mbgl::style::CustomLayer's CustomLayerDeinitializeFunction callback method is invoked. This resulted in the MGLOpenGLStyleLayer releasing itself early when removed from the owning style:

mapbox-gl-native/platform/darwin/src/MGLOpenGLStyleLayer.mm

Lines 114 to 121 in 920057c

    
           - (void)setStyle:(MGLStyle *)style { 
        
               if (_style && style) { 
        
                   [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;

When MGLFinishCustomStyleLayer is called on a subsequent frame, it is left holding on to a dangling pointer to the layer.

To remedy this, the layer retention needs to match the lifecycle required by the async renderer. Using __bridge_retain implicitly retains the bridged pointer passed to core. This should be the correct behavior since we are not using a weak pointer. When MGLFinishCustomStyleLayer takes back ownership of the pointer from core and uses __bridge_transfer.

Apple docs for Toll-Free Bridged Types

//cc @lilykaiser @jmkiley @jfirebaugh @1ec5

…ayer instead of a separate array.

1ec5

Just took a cursory glance at this PR; @akitchen, would you take a deeper look in my stead? Thanks!

1ec5 · 2017-12-20T20:51:47Z

platform/darwin/src/MGLStyle.mm

@@ -77,7 +77,6 @@ @interface MGLStyle()
 @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;


Looks like this dictionary wound up unused after #8626. The original purpose of this dictionary was to allow -layerWithIdentifier: to return a fully initialized MGLOpenGLStyleLayer object that still had its callback implementations intact. As long as that’s still working correctly, this change looks good.

@1ec5 verified

@1ec5 [MGLStlye layerFromMBGLLayer] already takes care of this for all layer types.

akitchen · 2017-12-20T21:11:29Z

@1ec5 yep - darwin changes look good to me; @asheemmamoowala and I were also looking at this together yesterday. I'm currently testing with a local test case I have set up.

akitchen · 2017-12-20T21:18:07Z

platform/darwin/src/MGLOpenGLStyleLayer.mm

    _style = style;
-    _style.openGLLayers[self.identifier] = self;


This is the only place where this dictionary was being used @1ec5 ; transferring the ownership is a better solution (and fixes the crash)

akitchen · 2017-12-20T21:58:08Z

I'm currently testing with a local test case I have set up.

Local test case / manual verification passed. I'm toying with some automated testing strategies for this but nothing that should block the merge.

jfirebaugh · 2017-12-20T23:05:19Z

platform/darwin/src/MGLOpenGLStyleLayer.mm

@@ -101,7 +101,7 @@ - (instancetype)initWithIdentifier:(NSString *)identifier {
                                                            MGLPrepareCustomStyleLayer,
                                                            MGLDrawCustomStyleLayer,
                                                            MGLFinishCustomStyleLayer,
-                                                            (__bridge void *)self);
+                                                            (__bridge_retained void *)self);


It's a corner case, but: this will leak a MGLOpenGLStyleLayer that's init'd but never added to a map.

😞 I knew I was missing something! Moved to : #10771

Revert this commit to see integration test fail.

Asheem Mamoowala added 2 commits December 19, 2017 18:58

[core] RenderCustomLayer should de-initialize old context

ea6ff4e

[iOS, macOS] Use toll-free bridging to retain/release MGLOpenGLStyleL…

2b2164f

…ayer instead of a separate array.

asheemmamoowala added Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Dec 20, 2017

asheemmamoowala requested review from jfirebaugh and 1ec5 December 20, 2017 20:31

1ec5 reviewed Dec 20, 2017

View reviewed changes

1ec5 requested a review from akitchen December 20, 2017 20:53

akitchen reviewed Dec 20, 2017

View reviewed changes

akitchen approved these changes Dec 20, 2017

View reviewed changes

asheemmamoowala merged commit 6a9eec8 into release-agua Dec 20, 2017

asheemmamoowala deleted the custom-layer-fix branch December 20, 2017 22:51

friedbunny added this to the ios-v3.7.2 milestone Dec 20, 2017

jfirebaugh reviewed Dec 20, 2017

View reviewed changes

asheemmamoowala mentioned this pull request Dec 21, 2017

MGLOpenGLStyleLayer leaks when not added to a map #10771

Closed

friedbunny mentioned this pull request Dec 21, 2017

MGLOpenGLStyleLayer is released prior to calling MGLFinishCustomStyleLayer #10755

Closed

akitchen pushed a commit that referenced this pull request Jan 5, 2018

Restores fix to MGLOpenGLStyleLayer memory-related crash (#10765)

cc7775a

Revert this commit to see integration test fail.

akitchen pushed a commit that referenced this pull request Jan 8, 2018

Restores fix to MGLOpenGLStyleLayer memory-related crash (#10765)

c439a97

Revert this commit to see integration test fail.

akitchen pushed a commit that referenced this pull request Jan 9, 2018

Restores fix to MGLOpenGLStyleLayer memory-related crash (#10765)

9c27380

Revert this commit to see integration test fail.

akitchen pushed a commit that referenced this pull request Jan 12, 2018

Restores fix to MGLOpenGLStyleLayer memory-related crash (#10765)

64f0a67

Revert this commit to see integration test fail.

akitchen pushed a commit that referenced this pull request Jan 19, 2018

Restores fix to MGLOpenGLStyleLayer memory-related crash (#10765)

654377e

Revert this commit to see integration test fail.

akitchen pushed a commit that referenced this pull request Jan 20, 2018

Restores fix to MGLOpenGLStyleLayer memory-related crash (#10765)

d0193c5

Revert this commit to see integration test fail.

jfirebaugh mentioned this pull request Feb 13, 2018

MGLOpenGLStyleLayer released when removed from style #11143

Closed

asheemmamoowala mentioned this pull request Mar 29, 2018

Use a host interface for CustomLayer instead of function pointers #11553

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CustomLayer context retain count #10765

Fix CustomLayer context retain count #10765

asheemmamoowala commented Dec 20, 2017

1ec5 left a comment

1ec5 Dec 20, 2017

akitchen Dec 20, 2017

asheemmamoowala Dec 20, 2017

akitchen commented Dec 20, 2017

akitchen Dec 20, 2017

akitchen commented Dec 20, 2017 •

edited

Loading

jfirebaugh Dec 20, 2017

asheemmamoowala Dec 21, 2017

	- (void)setStyle:(MGLStyle *)style {
	if (_style && style) {
	[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;

Fix CustomLayer context retain count #10765

Fix CustomLayer context retain count #10765

Conversation

asheemmamoowala commented Dec 20, 2017

1ec5 left a comment

Choose a reason for hiding this comment

1ec5 Dec 20, 2017

Choose a reason for hiding this comment

akitchen Dec 20, 2017

Choose a reason for hiding this comment

asheemmamoowala Dec 20, 2017

Choose a reason for hiding this comment

akitchen commented Dec 20, 2017

akitchen Dec 20, 2017

Choose a reason for hiding this comment

akitchen commented Dec 20, 2017 • edited Loading

jfirebaugh Dec 20, 2017

Choose a reason for hiding this comment

asheemmamoowala Dec 21, 2017

Choose a reason for hiding this comment

akitchen commented Dec 20, 2017 •

edited

Loading