[ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound #8686

boundsj · 2017-04-07T17:15:43Z

In #8637 we stopped indexing into std::map to avoid silently creating bogus entries in collections like _annotationContextsByAnnotationTag. However, since the user annotation is effectively always represented with MGLAnnotationTagNotFound (that equals UINT32_MAX) and the user annotation is processed similarly to regular annotations in some cases, we opened the door to a crash.

Specifically, with #8637, attempts to lookup an annotation context for the user annotation's tag (MGLAnnotationTagNotFound) in order to update the callout view triggered the map's std::out_of_range exception.

This PR guards against this by first checking to see if the annotation tag involved in the context lookup for the callout view update is MGLAnnotationTagNotFound or not. It also adds several more similar defensive checks in other areas that attempt to obtain a context.

Methods that are now guarded:

handleSingleTapGesture:
updateCalloutView:
observeValueForKeyPath: (for coordinate)
visibleAnnotationsInRect:
selectedAnnotation:

Methods that already had this check in place:

updateCalloutView:
viewForAnnotation:
removeAnnotations:
imageOfAnnotationWithTag:

cc @friedbunny @1ec5 @fabian-guerra

boundsj · 2017-04-07T17:22:40Z

Will update this to include macOS and changelog entries next cc @1ec5

…ound

boundsj · 2017-04-07T17:51:12Z

For macOS, I added the guard to:

visibleAnnotationsInRect:
annotationWithTag:

These methods already had the guard:

selectedAnnotation:
imageOfAnnotationWithTag:

friedbunny · 2017-04-07T17:52:28Z

platform/ios/CHANGELOG.md

@@ -2,6 +2,10 @@

 Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.

+## 3.5.2
+
+* Fixed an issue that caused a crash when the user annotation was presenting a callout view and the map was moved. ([#8686](https://github.com/mapbox/mapbox-gl-native/pull/8686))


I think we tend to call it the “user location annotation”, to make clear that it’s not some annotation otherwise belonging to the user.

I made that change for the iOS changelog in 99eee0a. In that same commit, I also made the macOS changelog less specific since the possibility of a crash there is more theoretical since there is currently no user location annotation.

friedbunny

LGTM and works in practice, but I’ll let someone more familiar with the annotation code give the official 🍏.

1ec5 · 2017-04-07T20:49:34Z

platform/ios/src/MGLMapView.mm

-    if ( ! _annotationContextsByAnnotationTag.count(tag))
-    {
+    if ( ! _annotationContextsByAnnotationTag.count(tag) ||
+        tag == MGLAnnotationTagNotFound) {


Should this return userLocation?

Never mind. The code used to overload MGLAnnotationTagNotFound to mean the user location annotation tag. But in retrospect that's a pretty risky practice. This change moves us further away from that practice, forcing us to explicitly account for the user location annotation explicitly, which is a good thing.

1ec5 · 2017-04-07T21:04:40Z

platform/ios/src/MGLMapView.mm

@@ -3550,6 +3555,10 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
        auto end = std::remove_if(nearbyAnnotations.begin(), nearbyAnnotations.end(),
                                  [&](const MGLAnnotationTag annotationTag)
        {
+            if (annotationTag == MGLAnnotationTagNotFound) {


This is redundant to the check in -annotationWithTag:. Should we put an assertion in right above for this case? Is it legitimate for the user location annotation to be a nearby annotation?

Going to answer my own question here: -annotationTagsInRect: wouldn’t know about the user location annotation, so it should never return MGLAnnotationTagNotFound. I still think we should either rely on the assertion below or assert here about MGLAnnotationTagNotFound, in case some sort of internal consistency leads it to show up in nearbyAnnotations.

That makes sense. I'll remove this check to rely on the assertion below.

Also, this is the assertion we hit in debug builds if annotations are removed at almost the same time and just barely before the user tries to select them (part of the repro from #8163 (comment)).

The check below recently added in #8637 to make sure the annotation is not nil protects release builds from crashing at the call to std::map::at also just below.

1ec5

Looks good, but I think we should be less shy about asserting about some of these cases in debug builds.

1ec5 · 2017-04-07T21:11:09Z

platform/ios/src/MGLMapView.mm

@@ -3550,6 +3555,10 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
        auto end = std::remove_if(nearbyAnnotations.begin(), nearbyAnnotations.end(),
                                  [&](const MGLAnnotationTag annotationTag)
        {
+            if (annotationTag == MGLAnnotationTagNotFound) {


Going to answer my own question here: -annotationTagsInRect: wouldn’t know about the user location annotation, so it should never return MGLAnnotationTagNotFound. I still think we should either rely on the assertion below or assert here about MGLAnnotationTagNotFound, in case some sort of internal consistency leads it to show up in nearbyAnnotations.

1ec5 · 2017-04-07T21:14:25Z

platform/ios/src/MGLMapView.mm

-            [self updateCalloutView];
+                // Update the annotation’s backing geometry to match the annotation model object. Any associated annotation view is also moved by side effect. However, -updateAnnotationViews disables the view’s animation actions, because it can’t distinguish between moves due to the viewport changing and moves due to the annotation’s coordinate changing.
+                _mbglMap->updateAnnotation(annotationTag, mbgl::SymbolAnnotation { point, symbolName.UTF8String });
+                [self updateCalloutView];


This change is correct with respect to the user location annotation, because we rely on Core Location, not KVO, to tell us when the model object has changed.

…or_merge * release-ios-v3.5.0-android-v5.0.0: (31 commits) [qt] Renamed qt5 formula to qt macos-v0.4.1 [ios] Update podspecs and changelog for v3.5.2 [ios, macos] Guard against looking up annotation contexts MGLAnnotationTagNotFound (mapbox#8686) [ios] Update podspecs and changelog for v3.5.1 [ios, macos] Hardened std::map usage in MGLMapVIew [ios] replaced link to select a feature example (mapbox#8651) [ios, macos] Rename Data-driven styling guide (mapbox#8627) [ios] assertion to check if an annotationTag > 0 (mapbox#8588) [ios, macos] Preserve symlinks when zipping framework [ios] Silence incompatible type warning for callout view (mapbox#8608) Release android v5.0.2 (mapbox#8629) [macos] Removed MGLUserTrackingMode from jazzy ToC [macos] Fixed broken images in DDS guide [core] cache binary shaders on Android [core] Extract and de-templatize several Program static methods macos-v0.4.0 [ios, macos] Updated changelogs [ios] Replaced UIActionSheet, UIAlertView with UIAlertController [ios, macos] Copyedited data-driven styling guides ...

boundsj added annotations Annotations on iOS and macOS or markers on Android crash iOS Mapbox Maps SDK for iOS labels Apr 7, 2017

boundsj added this to the ios-v3.5.2 milestone Apr 7, 2017

boundsj self-assigned this Apr 7, 2017

boundsj changed the title ~~[ios] Guard against creating bogus context for MGLAnnotationTagNotFound~~ [ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound Apr 7, 2017

[ios, macos] Guard against getting contexts with MGLAnnotationTagNotF…

5c2a7bc

…ound

boundsj force-pushed the boundsj-annotation-not-found-guard branch from c4c0f29 to 5c2a7bc Compare April 7, 2017 17:48

boundsj requested review from 1ec5 and friedbunny April 7, 2017 17:48

friedbunny reviewed Apr 7, 2017

View reviewed changes

Make changelogs more and less specific

99eee0a

friedbunny reviewed Apr 7, 2017

View reviewed changes

1ec5 reviewed Apr 7, 2017

View reviewed changes

1ec5 approved these changes Apr 7, 2017

View reviewed changes

Allow assertions for inconsistent state that should not happen

de5cceb

1ec5 approved these changes Apr 7, 2017

View reviewed changes

boundsj merged commit 03d10bb into release-ios-v3.5.0-android-v5.0.0 Apr 7, 2017

boundsj deleted the boundsj-annotation-not-found-guard branch April 7, 2017 22:23

boundsj mentioned this pull request Apr 7, 2017

User location annotation callout crash in v3.5.1 #8674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound #8686

[ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound #8686

boundsj commented Apr 7, 2017

boundsj commented Apr 7, 2017

boundsj commented Apr 7, 2017

friedbunny Apr 7, 2017

boundsj Apr 7, 2017

friedbunny left a comment

1ec5 Apr 7, 2017

1ec5 Apr 7, 2017

boundsj Apr 7, 2017

1ec5 Apr 7, 2017

1ec5 Apr 7, 2017

boundsj Apr 7, 2017

boundsj Apr 7, 2017

1ec5 left a comment

1ec5 Apr 7, 2017

1ec5 Apr 7, 2017

[ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound #8686

[ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound #8686

Conversation

boundsj commented Apr 7, 2017

boundsj commented Apr 7, 2017

boundsj commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedbunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment