iOS - Removing and re-adding a MGL*Source causes a crash

[JesseCrocker]

Platform:
iOS

Mapbox SDK version:
ios-v3.4.0-beta.2

Steps to trigger behavior

1. Add a MGLRasterSource or MGLVectorSource to the current style
2. Remove the source
3. Add the source again

Expected behavior

Source is displayed, removed, displayed again

Actual behavior

Source is displayed, removed, then the app crashes when source is re-added.

Test Case

https://github.com/JesseCrocker/Mapbox-gl-native-testcases/blob/source-re-add-crash/MapboxTest/ViewController.m

Stack Trace

#0	0x000000010fabf823 in mbgl::style::Style::addSource(std::__1::unique_ptr<mbgl::style::Source, std::__1::default_delete<mbgl::style::Source> >) ()
#1	0x000000010f84c906 in mbgl::Map::addSource(std::__1::unique_ptr<mbgl::style::Source, std::__1::default_delete<mbgl::style::Source> >) ()
#2	0x000000010f5f2e1d in ::-[MGLRasterSource addToMapView:](MGLMapView *) at /Users/boundsj/mapbox-gl-native/platform/darwin/src/MGLRasterSource.mm:65
#3	0x000000010f62f19e in ::-[MGLStyle addSource:](MGLSource *) at /Users/boundsj/mapbox-gl-native/platform/darwin/src/MGLStyle.mm:209
#4	0x000000010f544a70 in __47-[ViewController removeAndReAddSource:toStyle:]_block_invoke_2 at /Users/jesse/projects/Mapbox-gl-native-testcases/MapboxTest/ViewController.m:67
#5	0x00000001141860cd in _dispatch_client_callout ()
#6	0x00000001141621c3 in _dispatch_continuation_pop ()
#7	0x00000001141771e2 in _dispatch_source_latch_and_call ()
#8	0x000000011416fd7d in _dispatch_source_invoke ()
#9	0x0000000114166b2e in _dispatch_main_queue_callback_4CF ()
#10	0x0000000110cd94f9 in __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ ()
#11	0x0000000110c9ef8d in __CFRunLoopRun ()
#12	0x0000000110c9e494 in CFRunLoopRunSpecific ()
#13	0x0000000118521a6f in GSEventRunModal ()
#14	0x0000000111138964 in UIApplicationMain ()
#15	0x000000010f54509f in main at /Users/jesse/projects/Mapbox-gl-native-testcases/MapboxTest/main.m:14
#16	0x00000001141d268d in start ()

Disassembly

Mapbox`::-[MGLRasterSource addToMapView:](MGLMapView *):
    0x10f5f2de1 <+0>:   pushq  %rbp
    0x10f5f2de2 <+1>:   movq   %rsp, %rbp
    0x10f5f2de5 <+4>:   pushq  %rbx
    0x10f5f2de6 <+5>:   pushq  %rax
    0x10f5f2de7 <+6>:   movq   %rdi, %rbx
    0x10f5f2dea <+9>:   movq   0x6d7917(%rip), %rsi      ; "mbglMap"
    0x10f5f2df1 <+16>:  movq   %rdx, %rdi
    0x10f5f2df4 <+19>:  callq  *0x6b8e1e(%rip)           ; (void *)0x000000011078cac0: objc_msgSend
    0x10f5f2dfa <+25>:  movq   0x6d9ee7(%rip), %rcx      ; MGLRasterSource._pendingSource
    0x10f5f2e01 <+32>:  movq   (%rbx,%rcx), %rdx
    0x10f5f2e05 <+36>:  movq   $0x0, (%rbx,%rcx)
    0x10f5f2e0d <+44>:  movq   %rdx, -0x10(%rbp)
    0x10f5f2e11 <+48>:  leaq   -0x10(%rbp), %rsi
    0x10f5f2e15 <+52>:  movq   %rax, %rdi
    0x10f5f2e18 <+55>:  callq  0x10f84c720               ; mbgl::Map::addSource(std::__1::unique_ptr<mbgl::style::Source, std::__1::default_delete<mbgl::style::Source> >)
->  0x10f5f2e1d <+60>:  movq   -0x10(%rbp), %rdi
    0x10f5f2e21 <+64>:  movq   $0x0, -0x10(%rbp)
    0x10f5f2e29 <+72>:  testq  %rdi, %rdi
    0x10f5f2e2c <+75>:  je     0x10f5f2e34               ; <+83> at MGLRasterSource.mm:66
    0x10f5f2e2e <+77>:  movq   (%rdi), %rax
    0x10f5f2e31 <+80>:  callq  *0x8(%rax)
    0x10f5f2e34 <+83>:  addq   $0x8, %rsp
    0x10f5f2e38 <+87>:  popq   %rbx
    0x10f5f2e39 <+88>:  popq   %rbp
    0x10f5f2e3a <+89>:  retq   
    0x10f5f2e3b <+90>:  movq   %rax, %rbx
    0x10f5f2e3e <+93>:  jmp    0x10f5f2e5a               ; <+121> at memory:2516
    0x10f5f2e40 <+95>:  movq   %rax, %rbx
    0x10f5f2e43 <+98>:  movq   -0x10(%rbp), %rdi
    0x10f5f2e47 <+102>: movq   $0x0, -0x10(%rbp)
    0x10f5f2e4f <+110>: testq  %rdi, %rdi
    0x10f5f2e52 <+113>: je     0x10f5f2e5a               ; <+121> at memory:2516
    0x10f5f2e54 <+115>: movq   (%rdi), %rax
    0x10f5f2e57 <+118>: callq  *0x8(%rax)
    0x10f5f2e5a <+121>: movq   %rbx, %rdi
    0x10f5f2e5d <+124>: callq  0x10fc1f2c0               ; symbol stub for: _Unwind_Resume

[boundsj]

Thanks @JesseCrocker. The MGLSource (and layer) classes are wrappers around C++ (mbgl) objects. We use move semantics to transfer ownership to the mbgl core layer from the iOS (or whatever) platform layer when a source is added. This means that operations like adding a source twice or adding it, removing it, then adding it again will indeed cause crashes. What's required still before the release is to better document this constraint in the API documentation in MGLStyle and with better (platform level) errors. If you want to read more this, I think this comment #6254 (comment) is a good place to start. You can see the implementations of that in #6904 and #6793.

One option for you may be to use -[MGLStyleLayer setVisible:] -- this allows you to hide and show layers associated with a source instead of entirely removing and adding a source. However, I just noticed that the common layer methods are currently broken because of #6904. I proposed a fix for the release branch in #6966.

[JesseCrocker]

Yeah, i understand the reason behind the crash, and I dont actually have a need to turn a layer off and back on at this time, I just thought it was odd that is was so easy to trigger a crash.

[boundsj]

@JesseCrocker you may also have noticed that the implementation of MGLGeoJSONSource is a bit different. If an instance of that source is removed and then one of its content properties is set, it'll create a new _pendingSource with std::make_unique. In that sense, it is currently more tolerant of application code adds and removes than the other sources.