Crash on render with view-backed annotations present — -[MGLMapView visibleAnnotationsInRect:]

[ollie-eman]

Platform: iOS
Mapbox SDK version: 3.4.1

Steps to trigger behaviour

Updated to the latest SDK version and get this error, it has effected over 1000 users in the past 7 days. Could you help me understand it so I can get a fix deployed?

Incident Identifier: D3B3C119-B99F-45D9-96DD-68BEADF88543
CrashReporter Key:   44069ad0adad7f6fe68b477dddc9285b987cc622
Hardware Model:      iPhone9,3
Process:             Snatch Production [7629]
Path:                /private/var/containers/Bundle/Application/9E753DDA-4C28-48DD-A30D-12A2C702B907/Snatch Production.app/Snatch Production
Identifier:          com.iPhone.Snatch
Version:             2 (1.2.4)
Code Type:           ARM-64 (Native)
Role:                Foreground
Parent Process:      launchd [1]
Coalition:           com.iPhone.Snatch [3379]


Date/Time:           2017-02-20 21:07:17.2052 +0000
Launch Time:         2017-02-20 19:36:04.1710 +0000
OS Version:          iPhone OS 10.3 (14E5239e)
Report Version:      104

Exception Type:  EXC_CRASH (SIGABRT)
Exception Codes: 0x0000000000000000, 0x0000000000000000
Exception Note:  EXC_CORPSE_NOTIFY
Triggered by Thread:  0

Last Exception Backtrace:
0   CoreFoundation                	0x18c5a2fb8 __exceptionPreprocess + 124 (NSException.m:165)
1   libobjc.A.dylib               	0x18b008538 objc_exception_throw + 56 (objc-exception.mm:521)
2   CoreFoundation                	0x18c484868 -[__NSArrayM insertObject:atIndex:] + 1372 (NSArray.m:654)
3   Mapbox                        	0x100dbf8c8 -[MGLMapView visibleAnnotationsInRect:] + 260 (MGLMapView.mm:2866)
4   Mapbox                        	0x100dc94bc -[MGLMapView updateAnnotationViews] + 408 (MGLMapView.mm:4728)
5   Mapbox                        	0x100dc8aa8 -[MGLMapView notifyMapChange:] + 796 (MGLMapView.mm:4681)
6   Mapbox                        	0x100dcc9ac MBGLView::notifyMapChange(mbgl::MapChange) + 44 (MGLMapView.mm:5173)
7   Mapbox                        	0x100e716d4 mbgl::Map::render(mbgl::View&) + 192
8   Mapbox                        	0x100db5da0 -[MGLMapView glkView:drawInRect:] + 68 (MGLMapView.mm:872)
9   GLKit                         	0x196d2f9c4 -[GLKView _display:] + 216 (GLKView.m:584)
10  Mapbox                        	0x100db65d0 -[MGLMapView updateFromDisplayLink] + 68 (MGLMapView.mm:1000)
11  QuartzCore                    	0x18f86b218 CA::Display::DisplayLinkItem::dispatch(unsigned long long) + 44 (CADisplay.mm:1881)
12  QuartzCore                    	0x18f86b0c8 CA::Display::DisplayLink::dispatch_items(unsigned long long, unsigned long long, unsigned long long) + 436 (CADisplay.mm:1347)
13  IOKit                         	0x18c810308 IODispatchCalloutFromCFMessage + 372 (IOKitLib.c:1205)
14  CoreFoundation                	0x18c539938 __CFMachPortPerform + 180 (CFMachPort.c:682)
15  CoreFoundation                	0x18c551abc __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 56 (CFRunLoop.c:1959)
16  CoreFoundation                	0x18c55125c __CFRunLoopDoSource1 + 436 (CFRunLoop.c:2078)
17  CoreFoundation                	0x18c54ed70 __CFRunLoopRun + 1752 (CFRunLoop.c:3028)
18  CoreFoundation                	0x18c47ed74 CFRunLoopRunSpecific + 424 (CFRunLoop.c:3113)
19  GraphicsServices              	0x18dee7074 GSEventRunModal + 100 (GSEvent.c:2245)
20  UIKit                         	0x1927db988 UIApplicationMain + 208 (UIApplication.m:4068)
21  Snatch Production             	0x1000ca21c main + 120 (AppDelegate.swift:21)
22  libdyld.dylib                 	0x18b48d59c start + 4

I've got quite a few more reports if you need more than one?

Thanks,
Ollie

[boundsj]

Sorry to hear you are having issues @ollie-eman. Can you please tell us what version of our iOS SDK you were using before you updated to 3.4.1? Also, if you can provide repro steps that would most likely help us resolve this issue faster.

A similar issue was reported in #6901 and #7170 but that seemed to be resolved by #6924 that landed in version 3.4.0 of our iOS SDK.

Before #6924, an action like panning the map while concurrently removing annotations that are being updated to track the pan gesture could cause this crash. We can add more guards around the NSArray addObject: call but it'd be really helpful to understand what your application is doing when this crash occurs.

[ollie-eman]

The version I was using before 3.4.1 was 3.3.7

I don't have any reproducible steps I'm afraid, from the crash reports it effects each user roughly 1.3 times.

The app is similar to Pokemon Go, the user is in the centre of the map and there are two available gestures. These are rotating the camera around the user and pinch to zoom and tilt the camera. When the user moves location I make changes to the current MGLMapCamera to reflect this.

The map shows annotations that represent parcels the user has to tap, these get updated regularly (every 10 seconds).

I use the following code to update the annotations on the map. The class DefaultPointAnnotation is a subclass of MGLPointAnnotation

    func processIncomingAnnotations(_ incomingAnnotations: [DefaultPointAnnotation], existingAnnotations: [DefaultPointAnnotation]) {
        
        var annotationsToKeep = [DefaultPointAnnotation]()
        
        // for each of the incoming annotations
        incomingAnnotations.forEach { incomingAnnotation in
            
            // if the annotation already exists on the map, keep it for now
            if existingAnnotations.contains(incomingAnnotation) {
                annotationsToKeep.append(incomingAnnotation)
            }
            else {
                // otherwise it's new, so add it
                mapView?.addAnnotation(incomingAnnotation)
            }
        }
        
        // for each existing annotation on the map
        existingAnnotations.forEach { existingAnnotation in
            
            // if it does not match one of the duplicates from the incoming, remove it
            if !annotationsToKeep.contains(existingAnnotation) {
                mapView?.removeAnnotation(existingAnnotation)
            }
        }
    }

[boundsj]

Thanks for the information @ollie-eman. We spent some time investigating and still cannot find a code path that would lead to the crash you are seeing. We will keep digging but if you can provide any more information that may help we would appreciate it. Some things that may be useful to know are:

• Are there any other places in your application where you are removing the annotation instances from the map and/or setting the annotation instances to nil (losing the reference to them)?
• How do you visualize the annotations? (i.e. do you use MGLAnnotationViews or GL backed annotations, or both?)

[1ec5]

How do you visualize the annotations? (i.e. do you use MGLAnnotationViews or GL backed annotations, or both?)

-updateAnnotationViews only calls -visibleAnnotationsInRect: if the delegate implements -mapView:viewForAnnotation:. So it’s likely that @ollie-eman’s application is using view-backed annotations at least, if not also GL point annotations.

[boundsj]

I'm leaving this issue open, but since there is no known repro yet I'm going to move this off of the 3.5.0 milestone.

[erichoracek]

We're experiencing this same crash as of 3.4.1. We were previously on 3.3.7. We use both view-backed and GL-backed annotations depending on the screen.

Exception Type:  SIGABRT
Exception Codes: #0 at 0x187201014
Crashed Thread:  0

Application Specific Information:
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[__NSArrayM insertObject:atIndex:]: object cannot be nil'

Last Exception Backtrace:
0   CoreFoundation                       0x0000000188206fd8 0x1880da000 + 1232856
1   libobjc.A.dylib                      0x0000000186c68538 0x186c60000 + 34104
2   CoreFoundation                       0x00000001880e8888 0x1880da000 + 59528
3   Mapbox                               0x000000010146f8c8 -[MGLMapView visibleAnnotationsInRect:] (MGLMapView.mm:2866)
4   Mapbox                               0x00000001014794bc -[MGLMapView updateAnnotationViews] (MGLMapView.mm:4728)
5   Mapbox                               0x0000000101478aa8 -[MGLMapView notifyMapChange:] (MGLMapView.mm:4681)
6   Mapbox                               0x000000010147c9ac MBGLView::notifyMapChange(mbgl::MapChange) (MGLMapView.mm:5173)
7   Mapbox                               0x00000001015216d4 mbgl::Map::render(mbgl::View&) + 188
8   Mapbox                               0x0000000101465da0 -[MGLMapView glkView:drawInRect:] (MGLMapView.mm:872)
9   GLKit                                0x00000001926489c4 0x192626000 + 141764
10  Mapbox                               0x00000001014665d0 -[MGLMapView updateFromDisplayLink] (MGLMapView.mm:1000)
11  QuartzCore                           0x000000018b416600 0x18b40c000 + 42496
12  QuartzCore                           0x000000018b4164b0 0x18b40c000 + 42160
13  IOKit                                0x0000000188473ebc 0x18846e000 + 24252
14  CoreFoundation                       0x000000018819d958 0x1880da000 + 801112
15  CoreFoundation                       0x00000001881b5adc 0x1880da000 + 899804
16  CoreFoundation                       0x00000001881b527c 0x1880da000 + 897660
17  CoreFoundation                       0x00000001881b2d90 0x1880da000 + 888208
18  CoreFoundation                       0x00000001880e2d94 0x1880da000 + 36244
19  GraphicsServices                     0x0000000189b4c074 0x189b40000 + 49268
20  UIKit                                0x000000018e39b44c 0x18e326000 + 480332
21  XXXXXXXXX                            0x00000001001e9b3c main (main.m:15)
22  libdyld.dylib                        0x00000001870f159c 0x1870ed000 + 17820

[1ec5]

Since -[MGLMapView visibleAnnotationsInRect:] uses mbgl::Map::queryPointAnnotations() under the hood, I suspect this is a duplicate of #8289, which was fixed in #8374. (/cc @ivovandongen)

@ollie-eman @erichoracek, can you check whether this crash still occurs after upgrading to iOS SDK v3.5.0? Thanks!

[Laeyoung]

I got same issue on ios-v3.5.0

[Laeyoung]

It usually occurred when a user rapidly repeat to use double-tap feature.

[fabian-guerra]

Thanks for the information @Laeyoung we investigated before the code path that lead to this crash but we weren't able to reproduce it, is there any code snipet or test app you can share with us? that help us reproduce this consistently.

[ollie-eman]

I'm still seeing this crash after upgrading to version 3.5.0. I don't have any 'double-tap' features enabled as described by @Laeyoung though. If it helps, it only seems to effect users running iOS 10. I have no reports from iOS 9 users.

[Laeyoung]

@fabian-guerra I tried to make a test app which has same issue, but I failed....

One thing I can sure is that it didn't occur when I remove all Mapview.revmoeAnnotations() function calls.

[boundsj]

Noting that a stopgap landed in #8513 for the iOS v3.5.1 patch release but we will move this issue to the iOS v3.6.0 release and continue to investigate the root cause.

[1ec5]

Here’s one possible way to get a similar crash; there may be others:

• Call -[MGLMapView selectAnnotation:animated:], passing in MGLMapView.userLocation.
• -selectAnnotation:animated: calls -annotationTagForAnnotation:, which will return MGLAnnotationTagNotFound in this case.
• -selectAnnotation:animated: then calls -positioningRectForCalloutForAnnotationWithTag:, which immediately subscripts _annotationContextsByAnnotationTag with MGLAnnotationTagNotFound, thereby creating a bogus entry for the key MGLAnnotationTagNotFound.
• Some other code needs to look up an annotation by its tag and incidentally pulls out the bogus entry from _annotationContextsByAnnotationTag.

#8513 papers over the crash by gracefully failing, but I think that just kicks the can down the road. We should supplement that fix with an assertion similar to this assertion in the accessibility code. That way we’ll at least get a more specific crash and the ability to rule out the MGLAnnotationTagNotFound case above.

/cc @fabian-guerra

[fabian-guerra]

@ollie-eman @Laeyoung since it has been difficult reproduce the code that lead to the crash, maybe it's better to try to reproduce the steps in the actual apps, is it possible to you share your apps (the Appstore link) + the device type & OS version?

[Laeyoung]

@fabian-guerra I reproduced this issue on my iPhone 5 (os v10.2). And my colleagues got same issues on their various iPhones.

https://itunes.apple.com/us/app/alleys-video-map-vr-travel/id1123458416?mt=8

[boundsj]

@fabian-guerra @1ec5 I'm able to reproduce this exact issue by removing all annotations that were added to the map at about the same time but just barely before tapping the map around some annotations and -[MGLMapView annotationTagAtPoint:persistingResults:] is called inside MGLMapView. Here is the repro:

note: you must run in release or comment out the assertion NSAssert(annotation, @"Unknown annotation found nearby tap"); in -[MGLMapView annotationTagAtPoint:persistingResults:] for this to work

• Annotations are added
• At about the same time but just before the user taps on the map to select an annotation, all annotations are removed
• _annotationContextsByAnnotationTag in MGLMapView is cleared
• User taps near some annotations
• The logic in -[MGLMapView annotationTagAtPoint:persistingResults:] queries mbgl for annotation tags near the tap point
• Even though _annotationContextsByAnnotationTag is cleared, mbgl is still not caught up and it returns one or more annotation tags that used to be near the tap point
• -[MGLMapView annotationTagAtPoint:persistingResults:] indexes into _annotationContextsByAnnotationTag for each annotation tag that came back from mbgl, creating bogus entries for each one
• Shortly after all of that, -[MGLMapView updateAnnotationViews] is called and the exception is encountered in -[MGLMapView visibleAnnotationsInRect:] because one of the previously added bogus annotation context entries is still considered by mbgl to exist and to be visible

The problem is that, when annotations are removed, Map::removeAnnotation() is asynchronous and we currently have no way to keep MGLMapView's notion of annotations up to date with mbgl. Ideally, we would not remove the map view entry until some sort of callback from mbgl confirmed that the annotation was removed.

Unfortunately, I think that the guard added in #8513 must stay for now.

[boundsj]

Also, I think that while it is true that #8163 (comment) could happen in theory, it is unlikely because of the extremely high value of MGLAnnotationTagNotFound. The repro described above in #8163 (comment) is more likely to happen because it involves actual annotation tags that were visible at one point.

[1ec5]

Your reproduction steps make a lot more sense than mine. 😄

-[MGLMapView annotationTagAtPoint:persistingResults:] indexes into _annotationContextsByAnnotationTag for each annotation tag that came back from mbgl, creating bogus entries for each one

This is also a bug; _annotationContextsByAnnotationTag should never have a bogus entry. We should supplement this assertion with a guard that returns true (filtering out the annotation) in the event of a nil annotation (indicating an annotation that no longer exists due to this race condition).

I think this crash also demonstrates that we should eliminate the use of std::map::operator[] to obtain entries from _annotationContextsByAnnotationTag. Using std::map::at() here, for example, gives us greater confidence that the std::map isn’t corrupted. An informative crash is still better than a latent bug.

Unfortunately, I think that the guard added in #8513 must stay for now.

👍 As long as defensive guards like that are accompanied by assertions, and as long as we eliminate the unchecked use of std::map::operator[], at least we have a better chance of diagnosing issues like this.

[1ec5]

I think this crash also demonstrates that we should eliminate the use of std::map::operator[] to obtain entries from _annotationContextsByAnnotationTag.

Implemented in #8637.

[boundsj]

Now that #8637, #8513, and #8588 have landed to stabilize release builds and will be included in v3.5.1, I think it is ok to close this.

#8655 tracks tail work to stabilize this situation in debug builds.