Crash in mbgl::Buffer when showing an annotation backed by sprite

[boundsj]

Platform: iOS
Mapbox SDK version: master branch as of 9/27/2016

Steps to trigger behavior

1. Start iossim app
2. Tap and hold to add a single annotation backed by a sprite (annotation classic)
3. Pan back and forth a few times

Expected behavior

No crash.

Actual behavior

Crash with this trace

Additional notes

I'm able to reproduce this 100% of the time with the steps noted above on macOS Sierra, Xcode 8, and iPhone 6 simulator. So far I've been unable to reproduce on a device (I've only tried iPhone 7, iOS 10).

cc @incanus @1ec5 @jfirebaugh

[1ec5]

For searching purposes:

libc++abi.dylib: terminating with uncaught exception of type mbgl::gl::Error: glBufferData(target, pos, array, GL_STATIC_DRAW): Error GL_INVALID_OPERATION at /Users/boundsj/workspace/mbgl/src/mbgl/geometry/buffer.hpp:59
(lldb) bt
* thread #1: tid = 0x2a3a90, 0x000000010a1eddda libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
…
    frame #8: 0x0000000104d521cf Mapbox`__clang_call_terminate + 15
  * frame #9: 0x00000001050dd4ec Mapbox`mbgl::Buffer<16, 34962u, 8192, false>::bind(this=0x00007fff5af712f0)::'lambda'()::operator()() const::__MBGL_C_E::~__MBGL_C_E() + 60 at buffer.hpp:59
    frame #10: 0x00000001050dd4a5 Mapbox`mbgl::Buffer<16, 34962u, 8192, false>::bind(this=0x00007fff5af712f0)::'lambda'()::operator()() const::__MBGL_C_E::~__MBGL_C_E() + 21 at buffer.hpp:59
    frame #11: 0x00000001050dd463 Mapbox`mbgl::Buffer<16, 34962u, 8192, false>::bind(this=0x00007fff5af71340)::'lambda'()::operator()() const + 51 at buffer.hpp:59
    frame #12: 0x00000001050dd413 Mapbox`mbgl::Buffer<16, 34962u, 8192, false>::bind(this=0x00007fa34c5707a0, context=0x00007fa34b035738) + 371 at buffer.hpp:59
    frame #13: 0x00000001050dab2e Mapbox`mbgl::Buffer<16, 34962u, 8192, false>::upload(this=0x00007fa34c5707a0, context=0x00007fa34b035738) + 62 at buffer.hpp:80
    frame #14: 0x00000001050da969 Mapbox`mbgl::SymbolBucket::upload(this=0x00007fa34c570000, context=0x00007fa34b035738) + 137 at symbol_bucket.cpp:28
    frame #15: 0x00000001050c8218 Mapbox`mbgl::Painter::render(this=0x00007fa34b035600, style=0x00007fa34a511250, frame_=0x00007fff5af72110, annotationSpriteAtlas=0x00007fa34a40ef30) + 3064 at painter.cpp:120
    frame #16: 0x0000000105081320 Mapbox`mbgl::Map::Impl::render(this=0x00007fa34a60f8d0) + 1472 at map.cpp:263
    frame #17: 0x0000000105080afd Mapbox`mbgl::Map::render(this=0x000060000001fd80) + 445 at map.cpp:170

[boundsj]

Update

Actually, I can reproduce the crash pretty quickly on my device (iPhone 7, iOS 10), too.

[boundsj]

I should add that if I pan left and right slowly I usually don't see the crash. When I continuously flick the map left and right the crash often happens within a few seconds. I'm also noticing a lag when panning quickly that seems worse than I remember (although I'm running a debug build).

[boundsj]

This looks to be similar to or dupe of #6469

[jfirebaugh]

I added some debug logging and can observe the sequence of GL calls leading up to the crash:

1. glGenBuffers is called and returns a buffer with name 214
2. glBindBuffer(GL_ARRAY_BUFFER, 214)
3. glBufferData(GL_ARRAY_BUFFER, 64, 0x7fc93aaaca00, GL_STATIC_DRAW)
4. glGenBuffers is called and returns a buffer with name 215
5. glBufferData(GL_ARRAY_BUFFER, 12, 0x7fc93ab77400, GL_STATIC_DRAW)
6. glBindBuffer(GL_ARRAY_BUFFER, 214)
7. glDeleteBuffers(215)
8. glDeleteBuffers(214)
9. glGenBuffers is called and returns a buffer with name 214
10. glBufferData(GL_ARRAY_BUFFER, 64, 0x7fc93b118800, GL_STATIC_DRAW)

The final call causes GL_INVALID_OPERATION because the bound buffer is implicitly reset to 0 if you delete the bound buffer, and the bound buffer 214 was deleted. Furthermore, the call sequence from (5) onward looks nonsensical. (5) should be glBindBuffer(215), and (6) should be glBufferData(...). Generating a new buffer should always be immediately followed by binding, then uploading data for that buffer. Furthermore, why do we rebind 214 in (6)? That's already bound, and our GL state tracking is supposed to prevent unnecessary work. Finally, why are we not rebinding 214 before (10)?

[jfirebaugh]

I had a bug in my logging. The call sequence is fine, the bug is that our state tracking for buffers, introduced in #5620, does not take into account the fact that glDeleteBuffers can revert the binding to 0. Fix incoming.