Random offlineRegionStatusDidChange unrecognized selector crash

[postmechanical]

After an offline pack successfully completes downloading the app randomly crashes on subsequent launches:

-[__NSTaggedDate offlineRegionStatusDidChange:]: unrecognized selector sent to instance 0xe00007fae0873c04

Thread 1Queue : com.apple.main-thread (serial)
#0  0x0000000110d0bdbb in objc_exception_throw ()
#1  0x000000011174d48d in -[NSObject(NSObject) doesNotRecognizeSelector:] ()
#2  0x000000011169a90a in ___forwarding___ ()
#3  0x000000011169a4b8 in __forwarding_prep_0___ ()
#4  0x000000010e2f4472 in ___ZN25MBGLOfflineRegionObserver13statusChangedEN4mbgl19OfflineRegionStatusE_block_invoke ()
#5  0x0000000111c50e5d in _dispatch_call_block_and_release ()
#6  0x0000000111c7149b in _dispatch_client_callout ()
#7  0x0000000111c592af in _dispatch_main_queue_callback_4CF ()
#8  0x00000001116a4d09 in __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ ()
#9  0x00000001116662c9 in __CFRunLoopRun ()
#10 0x0000000111665828 in CFRunLoopRunSpecific ()
#11 0x0000000112138ad2 in GSEventRunModal ()
#12 0x000000010f176610 in UIApplicationMain ()
#13 0x000000010aba89cf in main at app/main.m:14
#14 0x0000000111ca592d in start ()
Enqueued from com.apple.root.default-qos.overcommit (Thread 15)Queue : com.apple.root.default-qos.overcommit (serial)
#0  0x0000000111c5a106 in _dispatch_barrier_async_f_slow ()
#1  0x000000010e2f43e3 in MBGLOfflineRegionObserver::statusChanged(mbgl::OfflineRegionStatus) ()
#2  0x000000010e2e8556 in mbgl::OfflineDownload::setState(mbgl::OfflineRegionDownloadState) ()
#3  0x000000010e2d6eaa in mbgl::util::RunLoop::Invoker<auto mbgl::util::Thread<mbgl::DefaultFileSource::Impl>::bind<void (mbgl::DefaultFileSource::Impl::*)(long long, mbgl::OfflineRegionDownloadState)>(void (mbgl::DefaultFileSource::Impl::*)(long long, mbgl::OfflineRegionDownloadState))::'lambda'(void (mbgl::DefaultFileSource::Impl::*&&)(long long, mbgl::OfflineRegionDownloadState)), std::__1::tuple<long long, mbgl::OfflineRegionDownloadState> >::operator()() ()
#4  0x000000010e2cc218 in mbgl::util::RunLoop::process() ()
#5  0x000000010e3408fe in uv__async_event ()
#6  0x000000010e340ab6 in uv__async_io ()
#7  0x000000010e34d572 in uv__io_poll ()
#8  0x000000010e340f88 in uv_run ()
#9  0x000000010e2d2265 in void mbgl::util::Thread<mbgl::DefaultFileSource::Impl>::run<std::__1::tuple<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, unsigned long long&>, 0ul, 1ul>(mbgl::util::ThreadContext, std::__1::tuple<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, unsigned long long&>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) ()
#10 0x000000010e2d211b in std::__1::__thread_proxy<std::__1::tuple<mbgl::util::Thread<mbgl::DefaultFileSource::Impl>::Thread<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, unsigned long long&>(mbgl::util::ThreadContext const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&&&, unsigned long long&&&)::'lambda'()> >(void*, void*) ()
#11 0x0000000111fa9c13 in _pthread_body ()
#12 0x0000000111fa9b90 in _pthread_start ()
#13 0x0000000111fa7375 in thread_start ()

[1ec5]

Looks like OfflineRegionObserver has a dangling pointer to the MGLOfflinePack. The workaround seems to be to hold a strong reference to the offline pack somewhere. For example, if you have a table of offline packs in your UI, the table view controller should manage an array of offline packs, only removing them just before calling -removePackWithCompletionHandler:. See https://github.com/mapbox/mapbox-gl-native/blob/ios-v3.2.0-beta.2/platform/ios/app/MBXOfflinePacksTableViewController.m for the full example code.

[1ec5]

This is exactly the kind of scenario I'm trying to avoid in #4287.

[postmechanical]

@1ec5 This happens on first access of [MGLOfflineStorage sharedOfflineStorage] after app launch so my code has no references to any offline pack instances as that point.

[1ec5]

MBGLOfflineRegionObserver::statusChanged() is on the stack, which would indicate that an MGLOfflinePack has been created in response to either calling -[MGLOfflineStorage getPacksWithCompletionHandler:] or -[MGLOfflineStorage addPackForRegion:withContext:completionHandler:]. I’m not sure how the MBGLOfflineRegionObserver would be created otherwise.

[postmechanical]

@1ec5 I took another look and in fact getPacksWithCompletionHandler was being called in response to app activation. The docs need to be updated to clearly that offline packs passed to the completion handler need to be retained by the calling context.

Ultimately, it would be preferrable to have a different API on MGLOfflineStorage and MGLOfflinePack. Having to fetch all packs to fetch a specific pack is inefficient. The delegate protocol that is supposed to be used by both MGLOfflineStorage and app code is too fragile.

[1ec5]

The docs need to be updated to clearly that offline packs passed to the completion handler need to be retained by the calling context.

Updated. Thanks for the reminder.

Having to fetch all packs to fetch a specific pack is inefficient.

Good point. The iOS offline API started out mimicking NSURLSession and related classes to some extent, but it no longer clearly fits into that paradigm. MGLOfflinePack is just a lightweight wrapper around mbgl:OfflineRegion in the cross-platform code. Currently, if you call -getPacksWithCompletionHandler: twice in a row, you’ll get two different sets of MGLOfflinePack objects that wrap the same logical downloads.

The new notification-based design in #4287 (comment) will likely have MGLOfflineStorage maintain a master list of MGLOfflinePacks that allows synchronous access (adding, pausing, and resuming would still be asynchronous). I think we’ll also be able to maintain a one-to-one relationship between MGLOfflinePacks and mbgl::OfflineRegions that way.

[postmechanical]

@1ec5 Still crashing even though calling context now retains all offline packs from getPacksWithCompletionHandler:.

[1ec5]

Pretty sure this will be fixed through a combination of #4410 and #4482.

[1ec5]

Fixed in #4410 and #4482, which will be in v3.2.0-beta.3.