Mapbox-gl-native: Examine lifetime of _snapshotCallback in MGLMapSnapshotter startWithQueue:completionHandler:

[julianrex]

Platform: Darwin
Mapbox SDK version: iOS 4.0.0

Looking at

https://github.com/mapbox/mapbox-gl-native/blob/65a4ee2373d053ac5b8d179123fdc51b320a1bb7/platform/darwin/src/MGLMapSnapshotter.mm#L122-L151

it appears that the _snapshotCallback instance variable can be overwritten, before a previous snapshot can complete. This could potentially result in a NULL deference around line 149.

Let's examine whether _snapshotCallback needs to be an instance variable, and ensure we double check for a valid pointer before calling it.

If it's important it remains an instance variable, consider queueing the assignment (since _mbglMapSnapshotter->snapshot(...) can return before calling the _snapshotCallback).

(In addition, if strongSelf is nil, let's make sure to call completion.)

/cc @friedbunny @ChrisLoer

[ChrisLoer]

Note that MapSnapshotter::callback takes an ActorRef as an argument, and ActorRef is a weak pointer that's designed to handle the underlying Actor going away (after the Actor is torn down, any messages passed to the ActorRef are no-ops). ActorRef is copy-able, so you can create one on the stack up above and capture it in the block by value without having to maintain its lifetime separately (like you have to do with the Actor itself).

[ChrisLoer]

So I'm out of my depth on the iOS/grand-central-dispatch side, but since we're allowing the caller to pass in a queue to startWithQueue, don't we have to assume that the dispatch_async calls might be run on a separate thread from the thread used to create/manage the MGLMapSnapshotter? And if that's the case we couldn't safely use any of these pointers from within the async block.

[ChrisLoer]

This definitely looks broken to me, although I still don't know the context for how we expect the startWithQueue method to be used (on a quick glance, it looks like the only part of the iOS SDK where we allow external callers to pass in their own dispatch queue). But anything we're passing into an async block for running on a queue where we don't control the concurrency should either be captured by value or use the Immutable pattern.

@julianrex do you want to work on a fix for this together?

cc @ivovandongen @1ec5

[ChrisLoer]

Sorry, I kind of clobbered this issue with #11827 -- when I started writing that up I was thinking of it as a more limited problem, but I think it (and PR #11831) subsumes all of this.