Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MapboxCoreMaps Crashes #2052

Open
Ogerets opened this issue Nov 10, 2023 · 10 comments
Open

MapboxCoreMaps Crashes #2052

Ogerets opened this issue Nov 10, 2023 · 10 comments
Labels
bug 🪲 Something is broken!

Comments

@Ogerets
Copy link

Ogerets commented Nov 10, 2023

Environment

  • Xcode version: 14.3.1
  • iOS version: 17.0.3, 17.1.0, 16.3.1, 16.0.0
  • Devices affected: iPhone 13 Pro, iPhone 14, iPhone 13, iPhone 14 Pro Max, iPhone 11 etc.
  • Maps SDK Version: 10.16.1

Observed behavior and steps to reproduce

Crash from MapboxCoreMaps with message

Pure virtual function called!

pure-virtual-function-stack-trace.txt

We also see in crashlytics a lot of crashes from MapboxCoreMaps with messages

malloc: Heap corruption detected, free list is damaged at 0x280bf9200 *** Incorrect guard value: 10749975056

and

malloc: Incorrect checksum for freed object 0x116208c00: probably modified after being freed. Corrupt value: 0x1160c5600

with different stack traces. Example:
incorrect-checksum-stack-trace.txt

Expected behavior

no crashes

Notes / preliminary analysis

We migrated from Mapbox v6. At the time, around 10% of users, who updated or downloaded brand new version, experience some of these crashes.
Unfortunately, I was unable to reproduce it on any of my devices.

It looks like all the crashes are about accessing released objects memory somewhere inside MapboxCoreMaps

I will be grateful for any help here

@Ogerets Ogerets added the bug 🪲 Something is broken! label Nov 10, 2023
@persidskiy
Copy link
Contributor

Hi @Ogerets , thank you for the report!
I've created an internal ticket to investigate this https://mapbox.atlassian.net/browse/MAPSNAT-1591

@Ogerets
Copy link
Author

Ogerets commented Nov 17, 2023

@persidskiy Hi, I have an update on the issue
It turned out the issue appears when we update layers concurrently. Unfortunately, I couldn't find in the docs any info about thread safety of provided functions.

Could you provide some information in this regard, please?
We were not updating layers on the main thread, since for some of them the updateLayers functions hungs UI really badly (was not the case at Mapbox v6 though).

@Ogerets
Copy link
Author

Ogerets commented Nov 17, 2023

I did benchmarks of MapboxMaps.Style.updateLayer function for our style. The bottleneck appears to be the parsing of old layer properties on line 168 (see attached screenshot). It takes up to 200 ms for our FillExtrusionLayer.
It's a big deal on the main thread. It would be great to have the option to execute this on a background one.

Also, I am not aware of Mapbox v6 implementation of updating layers, but we didn't have this issue there with the same layers configuration on the main thread.

Screenshot 2023-11-17 at 15 07 56

@Ogerets
Copy link
Author

Ogerets commented Nov 22, 2023

@persidskiy Sorry for bothering you, but this issue is blocking us.
Is there anything else I can do to help investigate the issue?

@im029
Copy link

im029 commented Nov 28, 2023

@Ogerets Do you have the style or layer configs that can be used to replicate this?

@OdNairy
Copy link
Contributor

OdNairy commented Nov 28, 2023

We were not updating layers on the main thread

Working on the non-main thread is an undefined behavior for SDK.

It takes up to 200 ms for our FillExtrusionLayer.

@Ogerets the updateLayer is known to be slow. Unfortunately, this is our only solution with strong typed API for v10 and v11.
The good news is that you can update fields with another less convenient API. This one would skip all iOS SDK checks and calculations and report an update to the engine:

try mapView.mapboxMap.setLayerProperty(for: layer.id,
                                        property: "fill-extrusion-height",
                                        value: NSNumber(value: 10))

There are a few caveats:

  1. This type of API accepts only Objective-C objects (NSNumber, NSString, NSDictionary and NSArray) so you can't just pass an Int or Double
  2. Some types like UIColor need an extra conversion to string representation under-the-hood
  3. Codable CustomKeys enums are internal at the moment so you would need to copy-paste declarations

@Ogerets
Copy link
Author

Ogerets commented Nov 28, 2023

@OdNairy, thank you for the response

Do you have the style or layer configs that can be used to replicate this?

The layer is being created locally. I'll gather and send you a sample code of it and the tileset.

This is our only solution with strong typed API for v10

Shouldn't moving all expensive mappings to the non-main thread and leaving everything else on the main thread solve the issue? It looks like a quick and easy solution. Could be an alternative to the existing function.

You can update fields with another less convenient API

Thank you for the tip, I will look into it

@OdNairy
Copy link
Contributor

OdNairy commented Nov 29, 2023

Shouldn't moving all expensive mappings to the non-main thread and leaving everything else on the main thread solve the issue?

Sounds reasonable

Thank you for the tip, I will look into it

Another tip over the Objective-C objects – you can use a weird but working solution by converting swift objects (including Mapbox Maps ones) to the Data with JSONEncoder and convert them back with the JSONSerialization class.

@Ogerets
Copy link
Author

Ogerets commented Nov 30, 2023

Sounds reasonable

Tested a very crude implementation (see screenshot), and it solves the issue! Looks like a much better and easier approach than Obj-C API.
I wonder if the fetching old layer properties on line 169 could be moved to the global thread as well.

It could be implemented using modern concurrency or some Mapbox internal queue management (saw runtime logs about it). Is there a chance it could be done by the Mapbox Team in the near future?

Screenshot 2023-11-29 at 20 09 16

@Ogerets
Copy link
Author

Ogerets commented Nov 30, 2023

I'll gather and send you a sample code of it and the tileset.

Here is our tileset

Here's a stripped code we use for the layer which cause the issue
mapbox_issue.zip

The code is more for inspecting than compiling. I apologize we couldn't find time to make a full sample project reproducing the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something is broken!
Projects
None yet
Development

No branches or pull requests

4 participants