Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios] Address bad access exception in MGLAttributionInfo #13300

Merged
merged 2 commits into from
Nov 14, 2018
Merged

[ios] Address bad access exception in MGLAttributionInfo #13300

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c7eeff
[ios] move creation of attributed string to global queue]
Nov 6, 2018
137f734
[ios] check if on main queue
Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions platform/darwin/src/MGLAttributionInfo.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,15 @@ @implementation MGLAttributionInfo
NSData *htmlData = [styledHTML dataUsingEncoding:NSUTF8StringEncoding];

#if TARGET_OS_IPHONE
NSMutableAttributedString *attributedString = [[NSMutableAttributedString alloc] initWithData:htmlData
options:options
documentAttributes:nil
error:NULL];
__block NSMutableAttributedString *attributedString;

dispatch_sync(dispatch_get_main_queue() , ^{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this method is called as part of -[MGLTileSource attributionInfos], which is a public method that can be called at any time. Even internal uses of this method tend to occur in a for loop, since there are multiple sources to inspect at the same time.

If performance does turn out to be a problem, an alternative to blocking on the main thread here would be to do so further up the call stack, wherever it’s expected that the caller could be on a thread other than the main thread:

mapbox-gl-native/platform/darwin/src/MGLMapSnapshotter.mm

Line 213 in bb55bae

NSArray<MGLAttributionInfo *>* attributionInfo = [MGLMapSnapshotter generateAttributionInfos:attributions];

This would require documenting other callers, such as -[MGLTileSource attributionInfos], as needing to run on the main thread. There is precedent for such a requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to check whether it's being called on the main queue. Would it be better to call the NSMutableAttributedString initializer earlier than to fiddle with queues? @julianrex and I found that the exception did not occur when we created an NSMutableAttributedString in the initializer for a map view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julianrex and I found that the exception did not occur when we created an NSMutableAttributedString in the initializer for a map view.

We can't rely on that behavior as it's not documented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I'm just parading my iOS ignorance here, but isn't MGLMapView essentially required to be created and used on the main thread? And if that's the case then creating the attributed string in the initializer should be good, right?

If we can find a way to avoid "fiddling with queues" we should. It's not the queues themselves that are the problem -- but hidden/unexpected cross-thread blocking calls are trouble.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisLoer @jmkiley was referring to the fact that we can avoid the crash if we initialize any old NSMutableAttributedString on the main thread (it ends up calling WebKitInitialize using a dispatch_once - I suspect it's that initializer that needs to be on the main thread) - not the specific one.

// This initializer should be called from a global or main queue. https://developer.apple.com/documentation/foundation/nsattributedstring/1524613-initwithdata
attributedString = [[NSMutableAttributedString alloc] initWithData:htmlData
options:options
documentAttributes:nil
error:NULL];
});
#else
NSMutableAttributedString *attributedString = [[NSMutableAttributedString alloc] initWithHTML:htmlData
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does initWithHTML... have the same concern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing in the documentation: https://developer.apple.com/documentation/foundation/nsattributedstring/1535412-initwithhtml

options:options
Expand Down