MGLMapSnapshotter iOS issue specific to Mapbox version 4.1+

[JustinGanzer]

Hello all,

I've recently come across an issue with the MGLMapSnapshotter in iOS.
What I intend to do with it is create a snapshot and nothing more.

In version 4.0 the following worked as I've expected:

//! I am somewhere on a dispatch queue !

let dg = DispatchGroup()
var image: UIImage?
var mapSnapshot: MGLMapSnapshot?
dg.enter()
DispatchQueue.main.async {
            let snapshotter = MGLMapSnapshotter(options: options)
            snapshotter.start{ (snapshot, error) in
                if error != nil {
                    err = error
                    print("Unable to create a map snapshot.")
                } else if let snapshot = snapshot {
                    image = snapshot.image
                    mapSnapshot = snapshot
                }
                dg.leave()
            }
        }

let timeout = dg.wait(timeout: .now() + 10.0)
//Do something with the image if there was no timeout

With an update to Mapbox 4.1 the above code does not work anymore. Instead the snappshotter.start callback is never called, even after multiple minutes of runtime.

I've got it working again by rewriting above code to the following:
(All I have done is moving the initialisation of the MapSnapShotter out of the Dispatch.main.async block)

//! I am somewhere on a dispatch queue !

let dg = DispatchGroup()
var image: UIImage?
var mapSnapshot: MGLMapSnapshot?
dg.enter()
//The difference is here:
let snapshotter = MGLMapSnapshotter(options: options)
DispatchQueue.main.async {
            //Snapshotter used to be initialised here
            snapshotter.start{ (snapshot, error) in
                if error != nil {
                    err = error
                    print("Unable to create a map snapshot.")
                } else if let snapshot = snapshot {
                    image = snapshot.image
                    mapSnapshot = snapshot
                }
                dg.leave()
            }
        }

let timeout = dg.wait(timeout: .now() + 10.0)
//Do something with the image if there was no timeout

Could someone explain as to why that is? Is this a bug?

However this results in a new problem that wasn't appearing in Mapbox 4.0 and that is
that now my app produces crashes. Not too often, but still.
Since Mapbox 4.1 with the included code change seen above, I have multiple crashlogs of other users that looks as follows:

Crashed: com.mapbox.mbgl.Map Snapshotter
0  Mapbox                         0x10567adc8 utrie2_enumForLeadSurrogate_58 + 35480
1  Mapbox                         0x10581c438 utrie2_enumForLeadSurrogate_58 + 1745160
2  Mapbox                         0x10583fc94 utrie2_enumForLeadSurrogate_58 + 1890660
3  Mapbox                         0x10574b570 utrie2_enumForLeadSurrogate_58 + 889408
4  Mapbox                         0x105753ca0 utrie2_enumForLeadSurrogate_58 + 924016
5  Mapbox                         0x1057519f8 utrie2_enumForLeadSurrogate_58 + 915144
6  Mapbox                         0x10574b228 utrie2_enumForLeadSurrogate_58 + 888568
7  Mapbox                         0x10573b718 utrie2_enumForLeadSurrogate_58 + 824296
8  Mapbox                         0x105676150 utrie2_enumForLeadSurrogate_58 + 15904
9  CoreFoundation                 0x18122f404 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24
10 CoreFoundation                 0x18122ec2c __CFRunLoopDoSources0 + 276
11 CoreFoundation                 0x18122c79c __CFRunLoopRun + 1204
12 CoreFoundation                 0x18114cda8 CFRunLoopRunSpecific + 552
13 CoreFoundation                 0x18119cb28 CFRunLoopRun + 116
14 Mapbox                         0x105677c58 utrie2_enumForLeadSurrogate_58 + 22824
15 Mapbox                         0x105677b34 utrie2_enumForLeadSurrogate_58 + 22532
16 libsystem_pthread.dylib        0x180ead220 _pthread_body + 272
17 libsystem_pthread.dylib        0x180ead110 _pthread_body + 290
18 libsystem_pthread.dylib        0x180eabb10 thread_start + 4

Steps to reproduce

1. Run code above.
2. First code snipped will run correctly on Mapbox 4.0
3. First code snipped will not run correctly on Mapbox 4.1
4. Distribute second code snipped with Mapbox 4.1 across a few devices. Produce new crashes

Expected behavior

Both code snippets should run on both Mapbox versions and produce no crashes.

Actual behavior

First code snipped does no longer run on Mapbox 4.1. Second code snipped produces crashes.

Configuration

Mapbox SDK versions:
Mapbox 4.1.1
iOS/macOS versions:
iOS 11.3 through 11.4
Device/simulator models:
All iphone models beyond iPhone 5s
Xcode version:
9.4.1

Edited on 07.07.2018 14:13 London Time -> Removed snapshotter = nil from code. Typo left from testing.

[julianrex]

Thanks for the report @JustinGanzer. Can you confirm that the crashes you see are on devices as opposed to simulators?

As for the less-than-useful callstack - we're tracking this issue over at #8463 (comment).

/cc @ChrisLoer

[ChrisLoer]

The probably-relevant change between 4.0 and 4.1 is this PR: #11831. Without really digging into the details of your example (or my rusty ARC knowledge), I suspect some of the "self capturing" we were previously doing extended the lifetime of the MGLMapSnapshotter long enough for your first example to work, but now the MGLMapSnapshotter is reaching the end of its life before the callback gets called, and its teardown aborts the callback.

In your second example, you have:

• Ownership of the snapshotter on a (non-main) dispatch queue
• Start call of the snapshotter on the main queue
• Completion callback of the snapshotter implicitly on the main queue, modifying objects owned on the dispatch queue

I haven't identified a specific problem, but is there a reason you need the extra complexity of going through the main dispatch queue? You could create the snapshotter on the dispatch queue, start it from that queue, and use the startWithQueue interface to make sure the completion callback runs on the same queue. I think that would simplify the concurrency a good bit.

[julianrex]

@JustinGanzer a couple of questions:

1. Can you clarify your declaration of MGLMapSnapshotter please? As it is the code above won't compile. It should look like:

   var snapshotter: MGLMapSnapshotter? = MGLMapSnapshotter(options: options)
   

   i.e. it should be an optional var so that it's captured by reference and can be set to nil in the callback.

2. At the top you state

   //! I am somewhere on a dispatch queue !

   Which queue is this? If I wrap your code above with a DispatchQueue.global().async { ... } then I see the callback being called.

[JustinGanzer]

First of all, thanks for the reply! New to Github, and appreciate the time you have taken for my issue.

@julianrex

"Can you confirm that the crashes you see are on devices as opposed to simulators?"

Yes. These crashes take place on users with iPhones, running versions of iOS 11 and up. Almost every user goes through this bit of code multiple times within their session and they have not produced crashes until the update to Mapbox 4.1. I am unable to reproduce this crash in my own development environment, either on devices or simulators, as the crash affects only about 1-2% of all sessions.

"Can you clarify your declaration of MGLMapSnapshotter please?"

I am sorry about the mistake. What you saw was a leftover from a piece of code I was experimenting with. I have removed the piece in the edited issue description.
The declaration is definitely let without setting it to nil at the end.

"Which queue is this? If I wrap your code above with a DispatchQueue.global().async { ... } then I see the callback being called."

I am too using DispatchQueue.global().async { ... }.
I've tried with global() and global(qos: .userInitiated) both won't call the callback, however:

Could you let me know which code block of my issue you were using, the first or second that is?
Because the second code block will run the callback where as the first one won't in Mapbox 4.1.1, but will in Mapbox 4.0.
Note: The only difference between the two being the time of initialisation of the snapshotter.
I have tried this multiple times now, while having said code run isolated with the same results.

@ChrisLoer

I haven't identified a specific problem, but is there a reason you need the extra complexity of going through the main dispatch queue?

Note that the following answer is only certain for simulators, as I will try with actual devices again soon, but I am almost certain the same problem was the case for devices:

Well yes. Running said code (exclusively to ensure there are no threading issues) and not dispatching the MGLSnapshotter's startmethod with the given callback to the main queue, results in crashes for every attempt. The startmethod will then run on my dispatch queue. I have tried with and without the with queue: parameter to no avail.

I have uploaded the crashlog in a textfile to reduce clutter:
CrashlogMainQueue.txt

@julianrex @ChrisLoer
Just in case, I want to share an example of the MGLMapSnapshotOptions that could and were generated by my application.
They are as follows:

//Camera
        let camera = MGLMapCamera()
        //Size
        let size = CGSize(width: 750.0, height: 508.5)
        //Coordinates
        let sw = CLLocationCoordinate2D(latitude: 52.309633285714284, longitude: 13.026126)
        let ne = CLLocationCoordinate2D(latitude: 52.519580428571423, longitude: 13.217471)
        //Bounds based on coordinates
        let bounds = MGLCoordinateBoundsMake(sw, ne)
        //Options
        let options = MGLMapSnapshotOptions(styleURL: URL(string: "mapbox:/Your/style/here")!, camera: camera, size: size)
        options.coordinateBounds = bounds

Hope I could clarify things a bit.

[julianrex]

Thanks for the great follow-up @JustinGanzer - I was able to create a test scenario that exhibits the behaviour you're seeing so that gives us a place to start. Hopefully we'll have an update soon.

[julianrex]

@JustinGanzer in your first code snippet the snapshotter is being deallocated before the snapshot completes (as @ChrisLoer states above), and the completion block isn't being called.

My comment above:

it should be an optional var so that it's captured by reference and can be set to nil in the callback.

is incorrect - apologies, I misread what you had written (and missed your typo comment)

However, you do need to ensure that snapshotter is alive for the duration of the snapshot process, by capturing it in the completion block. For example:

mapbox-gl-native/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterSwiftTests.swift

Lines 36 to 53 in 5e3478e

	DispatchQueue.main.async {
	let snapshotter = MGLMapSnapshotter(options: options)
	snapshotter.start(completionHandler: { (snapshot, error) in
	// // Without capturing snapshotter:
	// XCTAssertNil(snapshot)
	// XCTAssertNotNil(error)
	// Capture snapshotter
	dump(snapshotter)
	XCTAssertNotNil(snapshot)
	XCTAssertNil(error)
	dg.leave()
	})
	}

Without seeing the rest of the surrounding code in your second snippet, it's difficult to tell why that's working - though the assumption is that the snapshotter is being captured and is alive for the duration).

(In Objective-C it's a little more complicated: you'd need to make snapshotter a __block variable, and then set it to nil at the end of the completion block.)

[JustinGanzer]

@julianrex
Thanks for taking the time to test this special case. I've captured the snapshotter inside the callback and am seeing the correct result => The callback is being called.

My code now looks like this and works flawlessly:

        var image: UIImage?
        var mapSnapshot: MGLMapSnapshot?
        var err: Error?
        
        DispatchQueue.main.async {
            let snapshotter = MGLMapSnapshotter(options: options)
            snapshotter.start { (snapshot, error) in
                //Capturing the snapshotter inside the callback
                let capture = snapshotter
                print("Capture: \(capture)")
                if error != nil {
                    err = error
                    print("Unable to create a map snapshot.")
                } else if let snapshot = snapshot {
                    image = snapshot.image
                    mapSnapshot = snapshot
                }
            }
        }

Is this (manually capturing the snapshotter inside the callback) the intended behaviour of the MGLMapSnapshotter? I think your answer is going to be yes and that I have a generally wrong idea of how the scope regarding DispatchQueue's work.
If you look at the first code snippet I've posted, the MGLMapSnapshotter instance is captured inside the dispatch statement. I was under the impression that, having captured the variable inside the statement, it would be available until the end of said dispatch statement.

Without seeing the rest of the surrounding code in your second snippet, it's difficult to tell why that's working - though the assumption is that the snapshotter is being captured and is alive for the duration)

The answer probably is the let timeout = dg.wait(timeout: .now() + 10.0) inside the second code snippet. Since the function does not return until after 10 seconds or the callback being called, the second code snipped is working - that is until a snapshot takes longer than 10 seconds, which then results in the crashes I am experiencing. If you would still like to see the surrounding code, I'd be happy to provide it.

What i still fail to understand is why the snippet below will crash almost immediately.
Note: The difference is dispatching everything not to the main, but a global dispatch queue.

I feel like I am stretching the intentions of this github issue, but perhaps this might actually be a scenario where the snapshotter is released too early?

        var image: UIImage?
        var mapSnapshot: MGLMapSnapshot?
        var err: Error?
        
        DispatchQueue.global().async {
            let snapshotter = MGLMapSnapshotter(options: options)
            snapshotter.start { (snapshot, error) in
                //Capturing the snapshotter inside the callback
                let capture = snapshotter
                print("Capture: \(capture)")
                if error != nil {
                    err = error
                    print("Unable to create a map snapshot.")
                } else if let snapshot = snapshot {
                    image = snapshot.image
                    mapSnapshot = snapshot
                }
            }
        }

[Totolik]

Any progress on this issue? It seems you can no longer run Snapshotter in a background queue (e.g. global or global .userInitiated). There is a check for this in a code which was not there in iOS SDK v3.x... For some reason it now requires a queue that can do user interaction, I guess... It would be better if background running still is possible!

[julianrex]

@Totolik The snapshot still occurs on a background thread, but needs to be initiated from the main thread; the completion block will be called when the snapshot is complete (or has been cancelled).

Closing since the original issue has been fixed by #12355.

[Totolik]

@Totolik The snapshot still occurs on a background thread, but needs to be initiated from the main thread; the completion block will be called when the snapshot is complete (or has been cancelled).

Closing since the original issue has been fixed by #12355.

I already deployed 4.5.0 alpha and problem is indeed fixed there! Thank you!