MGLOfflineStorage should strongly own MGLOfflinePacks

[1ec5]

If you follow this example for creating an offline pack on iOS, you’ll run into a crash because the MGLOfflinePack is only held in a local variable. It is expected that the completion handler would store the pack in an ivar of the surrounding view controller, but this is still a poor experience for someone who follows the documentation to the letter.

MGLOfflineStorage should strongly own an MGLOfflinePack from the time it is added to the time it is removed, to prevent the object from being deleted via ARC.

/cc @boundsj @bsudekum @jfirebaugh

[jfirebaugh]

Will this flip the problem from dangling pointers to memory leaks? What if the MGLOfflinePack isn't removed? What about the packs from [[MGLOfflineStorage sharedOfflineStorage] getPacksWithCompletionHandler:]?

[1ec5]

These are good points. Maybe then what needs to happen is that MBGLOfflineRegionObserver should unregister itself as the mbgl::OfflineRegion's observer when it detects that the pack has gone away. Then someone following the help document as written will end up with code that will never trigger a call to the delegate methods, but at least it won't crash.

Still, we won't have an answer for situations where the application wants to follow the traditional delegate pattern. In that paradigm, the delegate doesn't need to separately hold strong references to offline packs but should still have access to the packs via the first parameter of the delegate methods.

[1ec5]

In other words, we've designed an API that makes it look like MGLOfflineStorage owns MGLOfflinePacks, but in reality they're owned by the class that uses MGLOfflineStorage, which is often an MGLOfflinePackDelegate in practice.

[1ec5]

Closing in favor of the safer fix in #4293.

[1ec5]

#4293 isn’t enough: if the surrounding class fails to capture a strong reference to the pack from within the completion handler, the pack’s underlying mbgl::OfflineRegion does begin downloading without crashing, but the delegate is never notified because the MGLOfflinePack instance itself is deallocated immediately after the completion handler finishes executing.

[1ec5]

@jfirebaugh is correct to be concerned about memory leaks, because currently only one object ever gets notified about progress updates and state changes for a given offline pack. There’s no way for MGLOfflineStorage to know what packs are active at a given time without preventing other classes from getting that information.

Fundamentally, this means we should be using NSNotificationCenter to track progress, not delegation. Along the lines of the original proposal, MGLOfflineStorage would serve as the clearinghouse for all progress updates for any pack. It would register and unregister the mbgl::OfflineRegionObserver for each pack and maintain an array of active packs (the required strong reference). Instead of registering as a delegate, client classes would add themselves as observers for MGLOfflinePackDownloadProgressNotification.

Expanding upon this model, we could also issue notifications for pack additions and removals that clients could use instead of the completion handlers. (Sometimes this is convenient.)

[1ec5]

Alternatively, we can address the memory leak concern more narrowly by having each MGLOfflinePack's OfflineRegionObserver call a private method on the shared MGLOfflineStorage object that removes the pack from the strongly referenced set of active packs.

[1ec5]

#4482 addresses all the issues raised above by having MGLOfflineStorage handle most of the steps we previously expected client code to handle, including owning a strong reference to each pack and broadcasting progress updates to potentially many objects via NSNotificationCenter.

Expanding upon this model, we could also issue notifications for pack additions and removals that clients could use instead of the completion handlers. (Sometimes this is convenient.)

This ends up being unnecessary because client code can register observers for KVO notifications on MGLOfflineStorage’s packs property.

[1ec5]

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