Mapbox-gl-native: Top and bottom layout guide KVO causes crashes in some scenarios

Created on 8 Sep 2017  路  6Comments  路  Source: mapbox/mapbox-gl-native

Summary

Crashes have been reported that are related to KVO observation of the bounds property of topLayoutGuide and bottomLayoutGuide. So far, these crashes have only been observed when using a combination of Xcode 9 beta 6 (although the beta version of Xcode 9 may not matter) and testing on a device with iOS 10.x. The exact version of iOS may not matter although it does appear that the combination of Xcode 9 beta (iOS 11 SDK) and pre-iOS 11 on device may trigger this issue.

Details

The MGLMapView has several subviews that are drawn on top of the map (like a compass) that must be moved with respect to map view container's chrome. For example, if a navigation bar is initially hidden and then shown, the compass at the top of the map view should move down to stay under the bottom of the navigation bar and not be covered by it.

In https://github.com/mapbox/mapbox-gl-native/pull/7716 we removed code that had been used until that point to solve for updating the map view subviews with a constraint system. The constraints were replaced with a manual view layout approach that is invoked when the containing view controller's top and bottom layout guides bounds property changes. This is done by observing the layout guides with KVO and invoking setNeedsLayout on MGLMapView when the bounds of the layout guides is observed to have changed. Setting needs layout results in a call to the map view's implementation of layoutSubviews that, in turn, calls a method that updates the location of the relevant map view subviews based on the map view's contentInset that is based on the latest top and bottom layout guides sizes.

Using the layout guides is a cleaner approach, especially since it allows us to avoid manipulating the map view's containing view controller's view's constraints.

We did fix a related crasher in https://github.com/mapbox/mapbox-gl-native/pull/9109 that happened because it was now possible for layout guides that had previously been observed to be deallocated before observation could be removed..

The fix was to implement layout guide observation removal in -[MGLMapView willMoveToWindow:]. This appeared to have fixed the problem. However, using the combination of Xcode 9 beta and iOS 10.x, crashes like this have been reported again:

* Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'An instance 0x17f37f00 of class _UILayoutGuide was deallocated while key value observers were still registered with it. Current observation info: (
Context: 0x23cfa10, Property: 0x1901c690>
)'

Note that, in iOS 11, the top and bottom layout guides have been deprecated. From Xcode 9b1 release notes:

Interface Builder uses UIView.safeAreaLayoutGuide as a replacement for the deprecated Top and Bottom layout guides in UIViewController. To use the new safe area, select Safe Area Layout Guides in the File inspector for the view controller, and then add constraints between your content and the new safe area anchors. This prevents your content from being obscured by top and bottom bars, and by the overscan region on tvOS. Constraints to the safe area are converted to Top and Bottom when deploying to earlier versions of iOS. (29323293)

A good writeup of the new Safe Area Layout Guide is available here.

I've also observed that willMoveToWindow is called multiple times depending on the view hierarchy the map view is embedded in. This is especially true if container views like a UIPageViewController is used. It does seem like it may be possible for a race condition to occur where a second or third call to willMoveToWindow unexpectedly re-reregisters observers on a view controller's layout guides just before that view controller and its guides are deallocated. This may be relevant to at least one related report that an observer was attempted to be removed even though it had not apparently been registered previously:

[MGLMapView removeLayoutGuideObserversIfNeeded]
* Terminating app due to uncaught exception 'NSRangeException', reason: 'Cannot remove an observer <0x106283e00> for the key path "bounds" from <_UILayoutSpacer 0x1c41d0680> because it is not registered as an observer.'

Actions

  • We should determine if there are any work arounds we can use to make the current KVO based approach work

    • If we can keep the KVO approach, we should also update it to conditionally work with the new Safe Area Layout with iOS 11+

  • If we cannot guarantee the safety of the KVO approach, we should examine if (despite the drawback of manipulating things in the app space) it make sense to revert to something like the previous constraints based approach
  • We should continue to try and repro and confirm if this issue really is specific to Xcode 9 / iOS 10.x (although I've been unsuccessful so far and I'm out of ideas to try and repro this crash)

cc @frederoni @jmkiley @fabian-guerra @1ec5

crash iOS
Source
picture boundsj
👍1

Most helpful comment

I looked at the Info.plist inside the Mapbox.framework in both the built app I sideloaded onto the QA devices, and the one in the Pods folder of the app. The Pod's version said the bundle's version string was 3.6.3, but the build product was 3.6.2. Looks like I didn't do a "build clean" before creating the sideload.

Sorry for sending you on a wild goose chase. Or a snipe hunt. Or your metaphor of choice. Shaka, when the walls fell.

picture Curtis-Halbrook on 19 Sep 2017
😄4

All 6 comments

It seems that the KVO approach is unsafe. The KVO + manual layout approach was cleaner in several ways but I think that the reintroduction of constraints in https://github.com/mapbox/mapbox-gl-native/pull/9995 should work around the KVO related crashes we are seeing and is a reasonable approach given the limitations we are working with.

We should land https://github.com/mapbox/mapbox-gl-native/pull/9995 before shipping v3.6.3 cc @fabian-guerra

boundsj picture boundsj on 14 Sep 2017

This was fixed in https://github.com/mapbox/mapbox-gl-native/pull/9995 that was merged into release-ios-v3.6.0-android-v5.1.0 and will be in our iOS v3.6.3 release.

boundsj picture boundsj on 14 Sep 2017

We just had one of our QA's reproduce the issue still, running with Mapbox 3.6.3 Including relevant part of the stack trace:

Incident Identifier: 9E44F2E5-A355-4E2D-9BC5-EBFC18461871
CrashReporter Key:   3cd5da1c5ec75ceaaeb2b7625dff52eaf8ca2c61
Hardware Model:      iPhone8,1
Process:             TheWeather [1225]
Path:                /private/var/containers/Bundle/Application/E9C0962B-8041-4F15-9DD3-3E57A254B2A4/TheWeather.app/TheWeather
Identifier:          com.weather.TWC
Version:             dev-build (9.1.1)
Code Type:           ARM-64 (Native)
Role:                Foreground
Parent Process:      launchd [1]
Coalition:           com.weather.TWC [622]


Date/Time:           2017-09-18 14:18:19.9065 -0400
Launch Time:         2017-09-18 14:03:57.1214 -0400
OS Version:          iPhone OS 11.0 (15A372)
Baseband Version:    4.00.01
Report Version:      104

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

Application Specific Information:
abort() called

Filtered syslog:
None found

Last Exception Backtrace:
0   CoreFoundation                  0x183f3fd38 __exceptionPreprocess + 124
1   libobjc.A.dylib                 0x183454528 objc_exception_throw + 55
2   CoreFoundation                  0x183f3fc80 +[NSException raise:format:] + 115
3   Foundation                      0x184854b98 -[NSObject+ 207768 (NSKeyValueObserverRegistration) _removeObserver:forProperty:] + 495
4   Foundation                      0x184854688 -[NSObject+ 206472 (NSKeyValueObserverRegistration) removeObserver:forKeyPath:] + 91
5   Foundation                      0x1848545d0 -[NSObject+ 206288 (NSKeyValueObserverRegistration) removeObserver:forKeyPath:context:] + 159
6   Mapbox                          0x108bd1f18 -[MGLMapView removeLayoutGuideObserversIfNeeded] + 581400 (MGLMapView.mm:698)
7   Mapbox                          0x108bd1cb4 -[MGLMapView willMoveToWindow:] + 580788 (MGLMapView.mm:673)
8   UIKit                           0x18d349c58 -[UIView+ 44120 (Hierarchy) _willMoveToWindow:] + 839
9   UIKit                           0x18d34a144 __85-[UIView+ 45380 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:]_block_invoke + 87
10  UIKit                           0x18d34a028 -[UIView+ 45096 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:] + 451
11  UIKit                           0x18d34a160 __85-[UIView+ 45408 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:]_block_invoke + 115
12  UIKit                           0x18d34a028 -[UIView+ 45096 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:] + 451
13  UIKit                           0x18d34a160 __85-[UIView+ 45408 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:]_block_invoke + 115
14  UIKit                           0x18d34a028 -[UIView+ 45096 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:] + 451
15  UIKit                           0x18d34a160 __85-[UIView+ 45408 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:]_block_invoke + 115
16  UIKit                           0x18d34a028 -[UIView+ 45096 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:] + 451
17  UIKit                           0x18d34a160 __85-[UIView+ 45408 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:]_block_invoke + 115
18  UIKit                           0x18d34a028 -[UIView+ 45096 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:] + 451
19  UIKit                           0x18d34a160 __85-[UIView+ 45408 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:]_block_invoke + 115
20  UIKit                           0x18d34a028 -[UIView+ 45096 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:] + 451
21  UIKit                           0x18d34a160 __85-[UIView+ 45408 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:]_block_invoke + 115
22  UIKit                           0x18d34a028 -[UIView+ 45096 (Hierarchy) _makeSubtreePerformSelector:withObject:withObject:copySublayers:] + 451
23  UIKit                           0x18d62c7b0 __UIViewWillBeRemovedFromSuperview + 543
24  UIKit                           0x18d349674 -[UIView+ 42612 (Hierarchy) removeFromSuperview] + 99
25  UIKit                           0x18ddcdc44 -[UICollectionView _reuseCell:notifyDidEndDisplaying:] + 1251
26  CoreFoundation                  0x183e19b08 -[__NSArrayM enumerateObjectsWithOptions:usingBlock:] + 215
27  UIKit                           0x18ddc15d8 -[UICollectionView _resetPrefetchedCachedCells] + 235
28  UIKit                           0x18ddc14d4 -[UICollectionView _resetPrefetchState] + 27
29  UIKit                           0x18d4ba2ac -[UICollectionView _invalidateLayoutWithContext:] + 83
30  UIKit                           0x18d4b78b8 -[UICollectionViewLayout invalidateLayoutWithContext:] + 187
31  UIKit                           0x18d56ca4c -[UICollectionViewFlowLayout invalidateLayoutWithContext:] + 847
32  UIKit                           0x18dde674c -[UICollectionViewLayout _invalidateLayoutUsingContext:] + 75
33  UIKit                           0x18ddbc63c -[UICollectionView _invalidateLayoutIfNecessaryForReload] + 159
34  UIKit                           0x18d3f1140 -[UICollectionView reloadData] + 1055
35  WXFeedKit                       0x10bfeacdc 0x10bfe0000 + 44252
36  TheWeather                      0x104a02fd4 LocationViewController.stopRefreshing() + 3878868 (LocationViewController.swift:443)
37  TheWeather                      0x104a02ab8 LocationViewController.update(contextInfo:) + 3877560 (LocationViewController.swift:402)
38  TheWeather                      0x104a049fc protocol witness for WeatherUpdatable.update(contextInfo:) in conformance LocationViewController + 3885564 (LocationViewController.swift:0)
39  TheWeather                      0x104d3bf78 closure #1 in closure #1 in TWCAppDelegate.startupComplete(shouldWaitForAirlock:) + 7257976 (TWCAppDelegate.swift:561)
40  TheWeather                      0x1048587c0 thunk for @callee_owned () -> () + 2131904 (IOSWatchConnectivityDelegate.swift:0)
41  libdispatch.dylib               0x1838c5088 _dispatch_call_block_and_release + 23
42  libdispatch.dylib               0x1838c5048 _dispatch_client_callout + 15
43  libdispatch.dylib               0x1838d1b74 _dispatch_main_queue_callback_4CF$VARIANT$mp + 1015
44  CoreFoundation                  0x183ee7f20 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 11
45  CoreFoundation                  0x183ee5afc __CFRunLoopRun + 2011
46  CoreFoundation                  0x183e062d8 CFRunLoopRunSpecific + 435
47  GraphicsServices                0x185c97f84 GSEventRunModal + 99
48  UIKit                           0x18d3b2880 UIApplicationMain + 207
49  TheWeather                      0x1046dc7d4 main + 575444 (main.m:22)
50  libdyld.dylib                   0x18392a56c start + 3

Looking at your repo, the tag ios-3.6.3 does have the changes to remove KVO, but as you can see above we're still seeing it. Is it possible that you published the wrong compiled Mapbox.framework?

Curtis-Halbrook picture Curtis-Halbrook on 18 Sep 2017

Not able to reproduce.

The Mapbox.framework/Info.plist in the downloaded Mapbox pod has the expected value for the MGLCommitHash key (https://github.com/mapbox/mapbox-gl-native/commit/9613582417dd32c6e943eb23b3aad620a5404f1e). Also, dwarfdump of the dSYM file from the v3.6.3 pod does not find the problematic method that was removed (-[MGLMapView removeLayoutGuideObserversIfNeeded]) like it did for the dSYM in v3.6.2.

This seems like it might be related to a CocoaPods cache issue or the Mapbox version not actually updating somehow.

boundsj picture boundsj on 18 Sep 2017

I looked at the Info.plist inside the Mapbox.framework in both the built app I sideloaded onto the QA devices, and the one in the Pods folder of the app. The Pod's version said the bundle's version string was 3.6.3, but the build product was 3.6.2. Looks like I didn't do a "build clean" before creating the sideload.

Sorry for sending you on a wild goose chase. Or a snipe hunt. Or your metaphor of choice. Shaka, when the walls fell.

Curtis-Halbrook picture Curtis-Halbrook on 19 Sep 2017
😄4

No worries @Curtis-Halbrook! Thanks for the update.

boundsj picture boundsj on 19 Sep 2017
Was this page helpful?
0 / 5 - 0 ratings

Related issues

fousa picture fousa  路  3Comments
brunoabinader picture brunoabinader  路  4Comments
jmkiley picture jmkiley  路  3Comments
friedbunny picture friedbunny  路  4Comments
tobrun picture tobrun  路  3Comments